Bug 73855 - WebInspectorServer for WebKit2.
Summary: WebInspectorServer for WebKit2.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks: 73094
  Show dependency treegraph
 
Reported: 2011-12-05 12:37 PST by Jocelyn Turcotte
Modified: 2012-04-03 06:35 PDT (History)
4 users (show)

See Also:


Attachments
Patch (19.82 KB, patch)
2011-12-05 12:38 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (20.04 KB, patch)
2012-03-29 11:27 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (20.61 KB, patch)
2012-04-02 07:38 PDT, Jocelyn Turcotte
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>