[Webkit-unassigned] [Bug 64297] [Qt][WK2] Add the Web Inspector to WebKit2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 29 22:27:03 PDT 2011


https://bugs.webkit.org/show_bug.cgi?id=64297


Noam Rosenthal <noam.rosenthal at nokia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105255|review?                     |review-
               Flag|                            |




--- Comment #54 from Noam Rosenthal <noam.rosenthal at nokia.com>  2011-08-29 22:27:02 PST ---
(From update of attachment 105255)
View in context: https://bugs.webkit.org/attachment.cgi?id=105255&action=review

> Source/WebKit2/ChangeLog:5
> +        Add WebInspector to WebKit2 MiniBrowser
> +        https://bugs.webkit.org/show_bug.cgi?id=64297
> +

Insufficient changelog.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:79
> +#if PLATFORM(QT)
> +    , m_view(0)
> +    , m_inspectorView(0)
> +#endif

OwnPtr would make this unnecessary.

> Source/WebKit2/UIProcess/WebInspectorProxy.h:185
> +#elif PLATFORM(QT)
> +    QSGView* m_view;
> +    QDesktopWebView* m_inspectorView;

OwnPtr

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:309
> +    WKPageGroupRef m_pageGroupRef = toAPI(toImpl(pageRef())->pageGroup());
> +    WKPreferencesRef m_preferences = toAPI(toImpl(m_pageGroupRef)->preferences());

What does the m_ stand for? Those are not members.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:45
> +    ASSERT(m_inspectorView);

This assert doesn't add anything.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:57
> +    ASSERT(m_view);

ditto.

> Tools/ChangeLog:5
> +        Add WebInspector to WebKit2 MiniBrowser
> +        https://bugs.webkit.org/show_bug.cgi?id=64297
> +

Insufficient changelog.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:119
> +    toggleWebInspector->connect(this, SIGNAL(enteredDeveloperExtrasMode(bool)), SLOT(setEnabled(bool)));
> +    toggleWebInspector->connect(this, SIGNAL(enteredWebInspectorMode(bool)), SLOT(setChecked(bool)));
> +    enableDeveloperExtras->connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(setEnabledInvert(bool)));
> +    connect(this, SIGNAL(enteredWebInspectorMode(bool)), this, SLOT(toggleWebInspectorMode(bool)));

Hard to understand from this code how this UI is going to behave. There has to be a cleaner way of doing this.
I'd be happy to help (on IRC / in person).

> Tools/MiniBrowser/qt/BrowserWindow.cpp:172
> +void BrowserWindow::setEnabledInvert(bool enable)
> +{
> +    enableDeveloperExtras->setEnabled(!enable);
> +}
> +

What does setEnabledInvert mean? It seems like logic that can be done in a cleaner way with better understanding of Qt desktop.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list