[webkit-reviews] review denied: [Bug 64297] [Qt][WK2] Add the Web Inspector to WebKit2 : [Attachment 103333] Patch to add Web Inspector feature to WebKit2 Qt 4.7 MiniBrowser. (works with Qt 4.7, does not support Qt 5)

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin 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 103333: Patch to add Web Inspector feature to WebKit2 Qt 4.7
MiniBrowser. (works with Qt 4.7, does not support Qt 5)
https://bugs.webkit.org/attachment.cgi?id=103333&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103333&action=review


You should update to Qt 5 first, and look how to expose the inspector in the
cleanest way possible.

> ChangeLog:3
> +	   Add WebInspector to WebKit2 Qt4.7 (revision 91860) MiniBrowser

You should update the title. The task title is now "[Qt][WK2] Add the Web
Inspector to WebKit2"

> Source/WebKit.pro:12
> -    lessThan(QT_MAJOR_VERSION, 5) {
> +    lessThan(QT_MAJOR_VERSION, 4) {

Just no :)
WebKit2 trunk does not even build with Qt 4 anymore.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:47
> -
> +    

Junk change

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:94
> +    m_inspectorView = 0;

This is defined only for platform Qt, this will not compile on the other ports.


> Source/WebKit2/UIProcess/WebInspectorProxy.h:52
> +#include <WKView.h>
> +#include <QGraphicsView>
> +#include <QGraphicsScene>

You should use forward declaration.
Those are not the right types.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:35
> -    : q(q)
> +    : q(q ? q : new QDesktopWebView(contextRef, pageGroupRef))

This is a really bad idea. You create implicitely a view that nobody own.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:197
> +void QDesktopWebView::toggleWebInspector()
> +{
> +    WKPageGroupRef pageGroupRef = WKPageGetPageGroup(pageRef());
> +    WKPreferencesRef preferences = WKPageGroupGetPreferences(pageGroupRef);
> +    if (WKInspectorIsVisible(WKPageGetInspector(pageRef()))) { 
> +	   WKPreferencesSetDeveloperExtrasEnabled(preferences, false);
> +	   WKInspectorClose(WKPageGetInspector(pageRef()));
> +    } else {
> +	   WKPreferencesSetDeveloperExtrasEnabled(preferences, true);
> +	   WKInspectorShow(WKPageGetInspector(pageRef()));
> +    }
> +}

The interface should be in the view, but the implementation should not. This is
typically code for QtWebPageProxy.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:51
> +    void toggleWebInspector();

We do not want to expose that in QDesktopWebView.
The API should also be property based so it is accessible easily in QML2.