[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
bugzilla-daemon at webkit.org
Tue Aug 9 02:31:37 PDT 2011
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.
More information about the webkit-reviews
mailing list