[webkit-reviews] review denied: [Bug 64297] [Qt][WK2] Add the Web Inspector to WebKit2 : [Attachment 105255] Updated Web Inspector patch for Qt5 WebKit2. Patch modified according latest comments.

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Genisim
<genisim at yahoo.com>'s request for review:
Bug 64297: [Qt][WK2] Add the Web Inspector to WebKit2
https://bugs.webkit.org/show_bug.cgi?id=64297

Attachment 105255: Updated Web Inspector patch for Qt5 WebKit2. Patch modified
according latest comments.
https://bugs.webkit.org/attachment.cgi?id=105255&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
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.