Bug 73855 – WebInspectorServer for WebKit2.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

WebInspectorServer for WebKit2.

[Jocelyn Turcotte]

Created attachment 117921 [details]
Patch

[Yael]

Comment on attachment 117921 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117921&action=review

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:100
> +    ASSERT(connection);

You need to handle a NULL connection here. That will happen e.g. when the user closes the remote client.

[Jocelyn Turcotte]

Created attachment 134625 [details]
Patch

- Rebased
- Fixed the disconnection null connection bug
- Fixed another problem when reconnecting

[Sam Weinig]

Can you explain what this is for, and why WebKit2 needs code for it?

[Jocelyn Turcotte]

(In reply to comment #4)
> Can you explain what this is for, and why WebKit2 needs code for it?

This is an implementation of Chromium's remote debugging for WebKit2.
http://www.webkit.org/blog/1620/webkit-remote-debugging/

We're planning to primarily support remote debugging for the Qt port of WebKit2 instead of an in-process inspector, to make it easier to deploy on devices.
We'll check later if we need to also expose the in-process inspector in our API.

The code could be either owned by WebCore, WebKit2 or the Qt port of WebKit2. I can't say if WebKit2 needs it, but this code would be beneficial for Qt and other WebKit2 ports only need to compile the code and start the server if they want to use it.
See bug #51364 and bug #73094 to see where this came from.

[Kenneth Rohde Christiansen]

Comment on attachment 134625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134625&action=review

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:55
> +    static WebInspectorServer* staticServer = new WebInspectorServer;

s_server ?

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:144
> +    // Invalid page id.
> +    int pageId = pageIdFromRequestPath(path);
> +    if (!pageId)

Shouldnt that comment be on the next line

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:165
> +    // Ignore connections to a page that already have a remote inspector connected.
> +    if (m_connectionMap.contains(pageId)) {
> +        connection->shutdownNow();
> +        return;
> +    }

Shouldn't the user be notified in some way?

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:186
> +    // Connection has already shutdown.

shut down*

> Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:37
> +bool WebInspectorServer::platformResourceForPath(const String& path, Vector<char>& data, String& contentType)
> +{
> +    if (path == "/pagelist.json") {
> +        buildPageList(data, contentType);

Wouldn't the comments form the change log make sense here as well?

> Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:69
> +        builder.append("{ \"description\": \"");
> +        builder.append(webPage->pageTitle());
> +        builder.append("\", \"inspectorLocation\": \"");
> +        builder.append("/webkit/inspector/inspector.html?page=" + String::number(iter->first));

wouldn't adding the url make sense?

[Jocelyn Turcotte]

Created attachment 135099 [details]
Patch

- Addressed noted issues.
- Changed touched Nokia copyrights to 2012.

[Simon Hausmann]

Comment on attachment 135099 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135099&action=review

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:57
> +WebInspectorServer* WebInspectorServer::server()
> +{
> +    static WebInspectorServer* s_server = new WebInspectorServer;
> +    return s_server;
> +}

I think the more common pattern is a reference and a shared() method, like this:


WebProcess& WebProcess::shared()
{
    static WebProcess& process = *new WebProcess;
    return process;
}

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:106
> +    String path = request->url();

A comment would be good here explaining how what seems like a generic url is just the path that follows the "GET " of the HTTP request.

> Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:65
> +    ClientMap::iterator end = m_clientMap.end();
> +
> +    StringBuilder builder;
> +    builder.append("[ ");
> +    for (ClientMap::iterator iter = m_clientMap.begin(); iter != end; ++iter) {

I suggest to move the end variable declaration closer to the for loop.

[Jocelyn Turcotte]

Committed r113027: <http://trac.webkit.org/changeset/113027>