[Webkit-unassigned] [Bug 43988] Web Inspector: Remote Web Inspector support for QtWebKit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43988


Ariya Hidayat <ariya.hidayat at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #64599|review?                     |review-
               Flag|                            |




--- Comment #7 from Ariya Hidayat <ariya.hidayat at gmail.com>  2010-08-17 17:15:41 PST ---
(From update of attachment 64599)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.