[webkit-reviews] review denied: [Bug 64297] Web Inspector: : [Attachment 103018] Patch to add Web Inspector feature to WebKit2 Qt 4.7 MiniBrowser.

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Genisim <genisim at yahoo.com>'s
request for review:
Bug 64297: Web Inspector:
https://bugs.webkit.org/show_bug.cgi?id=64297

Attachment 103018: Patch to add Web Inspector feature to WebKit2 Qt 4.7
MiniBrowser. 
https://bugs.webkit.org/attachment.cgi?id=103018&action=review

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


Nice idea, it would be nice to have the inspector back.
This should be implemented through private C++ apis though, not via the C APIs.
Talk with Alexis to see what his plans are for private APIs.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:54
> +    QDesktopWebView(WKContextRef, WKPageGroupRef);
> +    WKPageRef pageRef() const;
> +

This should really not be public. Do not forget this Class is in the public
API.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:85
> +    m_view->setWindowTitle(QString::fromAscii("Web Inspector -
")+QString::fromAscii(url.utf8().data()));

QString::fromAscii() is never a good idea in the library. The first should
probably be tr().
The second is definitely wrong. There is constructor for that.
You need to add spaces on both side of the + sign.

> Source/WebKit2/UIProcess/qt/WebInspectorProxyQt.cpp:105
> +    return ("qrc:/webkit/inspector/inspector.html");

Not sure Whey there are parentesis here :)