[Webkit-unassigned] [Bug 43988] Web Inspector: Remote Web Inspector support for QtWebKit
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 17 17:15:42 PDT 2010
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.
More information about the webkit-unassigned
mailing list