[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





--- Comment #66 from Genisim <genisim at yahoo.com>  2011-08-30 18:03:59 PST ---
(In reply to comment #65)
> (From update of attachment 105709 [details])
> 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);
> 
DONE
> > 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);
> 
DONE
> > 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();
> 
DONE
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:66
> > +    if (m_inspectorView)
> > +        m_inspectorView.clear();
> 
> No need for the null check here, clear() is always safe.
> 
DONE
> > 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.)
> 
NO CHANGES
> > Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:107
> > +    return ("qrc:/webkit/inspector/inspector.html");
> 
> No need for () around the string.
> 
DONE
> > Tools/ChangeLog:18
> > +        * MiniBrowser/qt/BrowserView.cpp:
> 
> A newline before this would be nice on the eyes.
> 
DONE
> > 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.
> 
DONE. If you look BrowserWindow.cpp and find same issues with:

  QAction* toggleFullScreen = windowMenu->addAction("Toggle FullScreen", this, SIGNAL(enteredFullScreenMode(bool)));

JUST REMEMBER it is not Genisim's code.

> > 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.
>
DONE partially (name changed). Logic did not moved to the onWebInspector... 
> > Tools/MiniBrowser/qt/BrowserWindow.h:87
> > +    QAction* enableDeveloperExtras;
> 
> Style, missing m_ prefix on member variable.

DONE

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