[webkit-reviews] review denied: [Bug 64297] [Qt][WK2] Add the Web Inspector to WebKit2 : [Attachment 105709] Patch to add Web Inspector to WebKit2 updated. using smart pointers OwnPtr, removed m_ prefix for local variable, added more info to ChangeLog's

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> 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 105709: Patch to add Web Inspector to WebKit2 updated. using smart
pointers OwnPtr, removed m_ prefix for local variable, added more info to
ChangeLog's
https://bugs.webkit.org/attachment.cgi?id=105709&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105709&action=review


> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:74
> +    void enableDeveloperExtras(bool enable);
> +    void toggleWebInspector(bool enable);

Inconsistent API. Let's call both of them either "enableBlahBlah" or
"toggleBlahBlah".
Or a more "WebKitty" option would be setBlahBlahEnabled(bool);

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:744
> +    WKPageGroupRef pageGroupRef = toAPI(toImpl(pageRef())->pageGroup());
> +    WKPreferencesRef preferences =
toAPI(toImpl(pageGroupRef)->preferences());
> +    toImpl(preferences)->setDeveloperExtrasEnabled(enable);

Pointless toAPI/toImpl churn.

m_webPageProxy->pageGroup()->preferences()->setDeveloperExtrasEnabled(enable);

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:753
> +    WKInspectorRef inspector = toAPI(toImpl(pageRef())->inspector());
> +    if (enable)
> +	   toImpl(inspector)->show();
> +    else
> +	   toImpl(inspector)->close();

Pointless toAPI/toImpl churn.

if (enable)
    m_webPageProxy->inspector()->show();
else
    m_webPageProxy->inspector()->close();

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:66
> +    if (m_inspectorView)
> +	   m_inspectorView.clear();

No need for the null check here, clear() is always safe.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:81
> +    m_view->setWindowTitle(QObject::tr("Web Inspector - ") +
QObject::tr(url.utf8().data()));

Why on earth do we need to translate the URL?
On a related note, I don't think it makes sense to translate the " - " part of
the string.
Either "Web Inspector" or "Web Inspector - %1" (so the translator is free to
localize the string any way she/he wants.)

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:107
> +    return ("qrc:/webkit/inspector/inspector.html");

No need for () around the string.

> Tools/ChangeLog:18
> +	   * MiniBrowser/qt/BrowserView.cpp:

A newline before this would be nice on the eyes.

> Tools/MiniBrowser/qt/BrowserWindow.cpp:107
> +    enableDeveloperExtras = toolsMenu->addAction("Enable Developer Extras",
this, SIGNAL(enteredDeveloperExtrasMode(bool)));

The QObject/member pair passed to addAction() is typically supposed to be a
receiver/slot, rather than a sender/signal.
The fact that it isn't makes the following code particularly hard to follow.

It would be much more readable if the addAction() call would connect to a
SLOT(onDeveloperExtrasModeChanged(bool)) (or some similar name.)
Same thing for the web inspector action.

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

This function needs a new name, badly.
To clarify, it is not obvious what a function called "setEnabledInvert" will do
if I call it.
In fact, it would be much nicer to fold this logic into the
onWebInspectorModeChanged() slot suggested above.

> Tools/MiniBrowser/qt/BrowserWindow.h:87
> +    QAction* enableDeveloperExtras;

Style, missing m_ prefix on member variable.