Created attachment 64366 [details] patch to enable remote web inspector for Qt Adds Qt support for remote Web Inspector. Runs a web debug server on port 9222, as specified by property q_webDebugServerPort on QWebPage instances. All pages with that property set will be remotely inspectable. URL for remote inspection of first QWebPage is http://localhost:9222/devtools/page/1 (where 1 is the number of the QWebPage instance).
Created attachment 64367 [details] screen shot showing Safari on right remotely inspecting QtTestBrowser on left
This patch depends on the addition of a WebSocket to inspector.js to communicate with the inspector backend in the remote case.
Created attachment 64404 [details] the same patch just for checking on try bots. Old one was not processed because jamey.hicks is not a committer.
Hmm, this didn't seem to apply for some reason.
Created attachment 64581 [details] revised patch to apply against current webkit trunk
Created attachment 64599 [details] patch to enable remote web inspector for qtwebkit Previous patch had two problems: 1) remote web inspector was alway enabled. Now enabled only if property _q_webDebugServerPort is set on QWebPage instances. QtTestBrowser takes argument -remote-inspector-port to control this property. 2) Fixed a segv on close of the QWebView window when remote inspector is enabled.
Comment on attachment 64599 [details] patch to enable remote web inspector for qtwebkit 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?
Created attachment 64874 [details] Patch
Comment on attachment 64874 [details] Patch 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
Created attachment 65565 [details] Patch
Attachment 65565 [details] did not build on qt: Build output: http://queues.webkit.org/results/3806110
Created attachment 65566 [details] Patch
Comment on attachment 65566 [details] Patch WebKit/qt/WebCoreSupport/InspectorServerQt.h:62 + QMap<QWebPage*, InspectorClientQt*> m_inspectorClients; It is not clear for me why you use two maps. id to page and page to client. As I can see you can use simple map id to client. WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:266 + void InspectorClientQt::dispatch(const String &message) Usually we are using InspectorClient only for sending messages to inspector. The dispatch implementation should be in InspectorFrontendClient. WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:129 + } it is not clear for me why you need an explicit m_remoteInspector flag. I think it would be better to keep both abilities. 1) open local inspector 2) open remote inspector. WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:243 + if (m_remoteInspector + || (m_inspectedWebPage && m_inspectedWebPage->d->inspectorServerPort())) { + m_remoteInspector = true; + Q_ASSERT(m_inspectedWebPage->d->inspector->d->remoteFrontend); + + RemoteFrontendChannel* session = qobject_cast<RemoteFrontendChannel*>(m_inspectedWebPage->d->inspector->d->remoteFrontend); + if (session) + session->sendMessageToFrontend(message); + return true; + } I think this portion of code can be simplified a bit. 1) m_remoteFrontend is unnecessary 2) m_inspectedWebPage is always not null In that case you can just check the RemoteFrontendChannel and if it is not established then try to use local inspector.
(In reply to comment #13) > (From update of attachment 65566 [details]) > > WebKit/qt/WebCoreSupport/InspectorServerQt.h:62 > + QMap<QWebPage*, InspectorClientQt*> m_inspectorClients; > It is not clear for me why you use two maps. id to page and page to client. > As I can see you can use simple map id to client. Good point. > WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:266 > + void InspectorClientQt::dispatch(const String &message) > Usually we are using InspectorClient only for sending messages to inspector. > The dispatch implementation should be in InspectorFrontendClient. I'll clean this up. > > WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:129 > > I think this portion of code can be simplified a bit. > 1) m_remoteFrontend is unnecessary > 2) m_inspectedWebPage is always not null > In that case you can just check the RemoteFrontendChannel and if it is not established then try to use local inspector. You are right. The code evolved over time and I did not notice that m_remoteInspector was no longer necessary.
Created attachment 65715 [details] Patch
Comment on attachment 65715 [details] Patch WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:129 + if (m_inspectedWebPage->d->inspectorServerPort()) + return; do you mean that the local inspector should be blocked if the remote debugging port is specified. In chromium you still be able to invoke local inspector for a page even if the remote port was specified. But only one inspector per page at a time.
Comment on attachment 65715 [details] Patch First question: does everything still compile if we build without inspector support? WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:89 + sServer = new InspectorServerQt(); This is not deleted anywhere. To avoid intentional leak, make QCoreApplication::instance as the parent object so it gets deleted by the application on exit. WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:286 + m_tcpConnection = 0; Any reason to only nullify this and not delete it? Although it will be deleted by the owning server socket, it will stay around in memory until the server socket dies. WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:151 + QTcpSocket* m_tcpConnection = m_tcpServer->nextPendingConnection(); This is a local variable, don't use m_ prefix (to avoid confusion with variables of the same name in another class). WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:324 + size_t length = pos - 1; What happens if pos is 0 or 1?
Created attachment 66045 [details] Patch
Comment on attachment 66045 [details] Patch > WebKit/qt/WebCoreSupport/InspectorClientQt.cpp:231 > + Q_ASSERT(m_inspectedWebPage->d->inspector->d->remoteFrontend); Nit: This assert is not neccessary. Inspector's business logic looks good to me.
Created attachment 67401 [details] Patch
Comment on attachment 67401 [details] Patch 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.
Created attachment 67549 [details] Patch
Comment on attachment 67549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=67549&action=review > WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:67 > +/* > + * Parses and returns a WebSocket challenge number according to the method specified in the WebSocket protocol. > + * > + * The field contains numeric digits interspersed with spaces and > + * non-numeric digits. The protocol ignores the characters that are > + * neither digits nor spaces. The digits are concatenated and > + * interpreted as a long int. The result is this number divided by the > + * number of spaces. > + */ We don't add * per line. Look at our other comments. > WebKit/qt/WebCoreSupport/InspectorServerQt.cpp:75 > + nSpaces++; numSpaces?
Created attachment 68382 [details] Patch
Comment on attachment 68382 [details] Patch Clearing flags on attachment: 68382 Committed r68056: <http://trac.webkit.org/changeset/68056>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/68056 might have broken Qt Windows 32-bit Release The following changes are on the blame list: http://trac.webkit.org/changeset/68056 http://trac.webkit.org/changeset/68055