[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 #72 from Alexis Menard <alexis.menard at openbossa.org>  2011-08-31 10:06:44 PST ---
(In reply to comment #66)
> (In reply to comment #65)
> > (From update of attachment 105709 [details] [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.

Code is not perfect otherwise we could call WebKit done. Things can be improved, let's make the new code the best we can.

> 
> > > 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.