Bug 73855

Summary: WebInspectorServer for WebKit2.
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Status: RESOLVED FIXED    
Severity: Normal CC: sam, seokju.kwon, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 73094    
Attachments:
Description Flags
Patch
none
Patch
none
Patch hausmann: review+

Description Jocelyn Turcotte 2011-12-05 12:37:44 PST
WebInspectorServer for WebKit2.
Comment 1 Jocelyn Turcotte 2011-12-05 12:38:46 PST
Created attachment 117921 [details]
Patch
Comment 2 Yael 2012-01-21 18:32:06 PST
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.
Comment 3 Jocelyn Turcotte 2012-03-29 11:27:52 PDT
Created attachment 134625 [details]
Patch

- Rebased
- Fixed the disconnection null connection bug
- Fixed another problem when reconnecting
Comment 4 Sam Weinig 2012-04-01 19:31:32 PDT
Can you explain what this is for, and why WebKit2 needs code for it?
Comment 5 Jocelyn Turcotte 2012-04-02 02:47:48 PDT
(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.
Comment 6 Kenneth Rohde Christiansen 2012-04-02 05:11:39 PDT
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?
Comment 7 Jocelyn Turcotte 2012-04-02 07:38:31 PDT
Created attachment 135099 [details]
Patch

- Addressed noted issues.
- Changed touched Nokia copyrights to 2012.
Comment 8 Simon Hausmann 2012-04-03 03:32:10 PDT
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.
Comment 9 Jocelyn Turcotte 2012-04-03 06:35:25 PDT
Committed r113027: <http://trac.webkit.org/changeset/113027>