[webkit-reviews] review denied: [Bug 43988] Web Inspector: Remote Web Inspector support for QtWebKit : [Attachment 67401] Patch

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Jamey Hicks
<jamey.hicks at nokia.com>'s request for review:
Bug 43988: Web Inspector: Remote Web Inspector support for QtWebKit
https://bugs.webkit.org/show_bug.cgi?id=43988

Attachment 67401: Patch
https://bugs.webkit.org/attachment.cgi?id=67401&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67401&action=prettypatch

> WebKit/qt/Api/qwebinspector_p.h:40
> +    void attachRemoteFrontend(QObject* newRemoteFrontend);
so it actually substitutes it? attachAndReplaceFrontend?

> WebKit/qt/ChangeLog:15
> +	       http://localhost:9222/devtools/page/1
Couldn't http://localhost:9222/devtools redirect to page/1 or so automatically?


> WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:129
> +    // remote frontend was attached
Comments start with capital and end with dot.

> WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:44
> +static void setChallengeNumber(unsigned char* buf, uint32_t number)
Maybe add a comment on what it does on top

> WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:67
> +static quint32 getChallengeNumberFromField(QString field)
parseChallengeNumber?

> WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:88
> +    // sServer is deleted in unregisterClient() when the last client is
unregistered.
sServer? Badly named variable

> WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:144
> +	   sServer = 0;
Who is responsible for freeing this?

> WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:335
> +	   size_t length = pos - 1;
Is pos always > 0 here? Comment

> WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:344
> +	   m_data = m_data.mid(length + 2);
comment about the + 2

> WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:354
> +bool RemoteFrontendChannel::sendMessageToFrontend(const String &message)
& is placed wrongly

> WebKit/qt/WebCoreSupport/InspectorServerQt.h:50
> +    InspectorClientQt* inspectorClient(int pageNum);
inspectorClientForPageId or something similar?

> WebKit/qt/WebCoreSupport/InspectorServerQt.h:107
> +
This newline is not needed here.