[webkit-reviews] review denied: [Bug 43988] Web Inspector: Remote Web Inspector support for QtWebKit : [Attachment 64599] patch to enable remote web inspector for qtwebkit

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at gmail.com> 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 64599: patch to enable remote web inspector for qtwebkit
https://bugs.webkit.org/attachment.cgi?id=64599&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at gmail.com>
Overall looks good, with some minor issues (hence r-). I might add more
comments soon.

WebCore/WebCore.pro:2271
 +	../WebKit/qt/WebCoreSupport/WebDebugServerQt.cpp \
Does it have to be WebDebugServerQt? Can we use a name like InspectorServerQt?

WebKit/qt/Api/qwebinspector.cpp:211
 +  
This function is really confusing. It's not clear who owns newRemoteFrontend.
This is mostly looking like "please reparent this object" rather than "please
use this object as the remote front-end". Is the setParent part really
necessary.

WebKit/qt/Api/qwebpage.cpp:1301
 +	webDebugServer->listen(q->property("_q_webDebugServerPort").toInt());
Use spaces to indent, not tabs.

WebKit/qt/Api/qwebpage.cpp: 
 +  #include <QHttpRequestHeader>
Why is this one removed?

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:155
 +  
No need to add extra space

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:171
 +	// m_inspectedWebPage->d->inspectorController()->disconnectFrontend();
Is this TODO or FIXME? XXX does not really describe anything.
Also if disconnect is really necessary, then definitely it needs to be
implemented.

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:232
 +	    || (m_inspectedWebPage &&
m_inspectedWebPage->property("_q_webDebugServerPort").isValid())) {
This property access is repeated many times. Maybe it's worth to create a
function for that?

WebKit/qt/WebCoreSupport/WebDebugServerQt.cpp:40
 +  static bool debug = false;
We need to leave out all debugging.

WebKit/qt/WebCoreSupport/WebDebugServerQt.cpp:88
 +  
Will we leak here? Who deletes sServer?

WebKit/qt/WebCoreSupport/WebDebugServerQt.cpp:118
 +	    mTcpServer->setParent(0);
Why don't we delete mTcpServer as well?

WebKit/qt/WebCoreSupport/WebDebugServerQt.cpp:208
 +	      
Do we need all these qDebug?