Bug 43988 – Web Inspector: Remote Web Inspector support for QtWebKit

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Jamey Hicks]

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).

[Jamey Hicks]

Created attachment 64367 [details]
screen shot showing Safari on right remotely inspecting QtTestBrowser on left

[Jamey Hicks]

This patch depends on the addition of a WebSocket to inspector.js to communicate with the inspector backend in the remote case.

[Ilya Tikhonovsky]

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.

[Kenneth Rohde Christiansen]

Hmm, this didn't seem to apply for some reason.

[Jamey Hicks]

Created attachment 64581 [details]
revised patch to apply against current webkit trunk

[Jamey Hicks]

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.

[Ariya Hidayat]

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?

[Jamey Hicks]

Created attachment 64874 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Jamey Hicks]

Created attachment 65565 [details]
Patch

[Early Warning System Bot]

Attachment 65565 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3806110

[Jamey Hicks]

Created attachment 65566 [details]
Patch

[Ilya Tikhonovsky]

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.

[Jamey Hicks]

(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.

[Jamey Hicks]

Created attachment 65715 [details]
Patch

[Ilya Tikhonovsky]

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.

[Ariya Hidayat]

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?

[Jamey Hicks]

Created attachment 66045 [details]
Patch

[Ilya Tikhonovsky]

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.

[Jamey Hicks]

Created attachment 67401 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Jamey Hicks]

Created attachment 67549 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Jamey Hicks]

Created attachment 68382 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 68382 [details]
Patch

Clearing flags on attachment: 68382

Committed r68056: <http://trac.webkit.org/changeset/68056>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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