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

[bugzilla-daemon at webkit.org]

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


Andreas Kling <kling at webkit.org> changed:

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




--- Comment #65 from Andreas Kling <kling at webkit.org>  2011-08-30 16:10:39 PST ---
(From update of attachment 105709)
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.

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