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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 25 04:37:20 PDT 2010


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 64874: Patch
https://bugs.webkit.org/attachment.cgi?id=64874&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
WebKit/qt/Api/qwebinspector.cpp:199
 +  /*! \internal */
In Qt we normally don't write this on one line.

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:235
 +	    || (m_inspectedWebPage &&
m_inspectedWebPage->d->inspectorServerPort())) {
no reason to put this on two lines. lines below are wider :-)

WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:238
 +		|| !m_inspectedWebPage->d
do you really need to check if it has the private? Seems more like an assert to
me

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:151
 +	    new HttpRequestHandler(mTcpConnection, this);
put this on the same line

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:162
 +	mEndOfHeaders = false;	  
We use m_ not just m, please fix.

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:189
 +  
please remove this newline

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:257
 +	    int rc = file.open(QIODevice::ReadOnly);
rc meaning what?

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:255
 +	    QString path = QLatin1String(":") + mPath;
Wouldn't using .arg() be more effecient?

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:262
 +	    int code = file.exists() ? 200 : 404;
Dont we have defines for these numbers in webcore?

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:311
 +  
strange and useless newline inside beginning of if(

WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:329
 +	    mData = mData.mid(len+2);
would need spaces around +

WebKit/qt/WebCoreSupport/InspectorServerQt.h:23
 +  class QTcpServer;
These should not be defined above the includes

WebKit/qt/WebCoreSupport/InspectorServerQt.h:40
 +	    public:
strange indentation of public:

WebKit/qt/WebCoreSupport/InspectorServerQt.h:46
 +	void registerPage(QWebPage *page, InspectorClientQt* client);
* should be left aligned

WebKit/qt/WebCoreSupport/InspectorServerQt.h:47
 +	void unregisterPage(QWebPage *page);
same here

WebKit/qt/WebCoreSupport/InspectorServerQt.h:62
 +	private slots:
wrong indentation! should be above the private fields

WebKit/qt/WebCoreSupport/InspectorServerQt.h:63
 +	void gotNewConnection();
receivedNewConnection? establishedNewConnection... check what is used for
QSocket and friends

WebKit/qt/WebCoreSupport/InspectorServerQt.h:69
 +	    public:
wrong indentation

WebKit/qt/WebCoreSupport/InspectorServerQt.h:81
 +	    public:
again!

WebKit/qt/WebCoreSupport/InspectorServerQt.h:86
 +	virtual int webSocketSend(const char *payload, size_t len);
length

WebKitTools/QtTestBrowser/launcherwindow.cpp:97
 +  
no need for two newlines, one will do


More information about the webkit-reviews mailing list