[Webkit-unassigned] [Bug 88094] Web Inspector: Add a WebInspectorServer on Linux using the GSocket API for the GTK port

Sun Aug 26 13:13:55 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=88094

Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #157749|review?                     |review-
               Flag|                            |

--- Comment #11 from Gustavo Noronha (kov) <gns at gnome.org>  2012-08-26 13:13:56 PST ---
(From update of attachment 157749)
View in context: https://bugs.webkit.org/attachment.cgi?id=157749&action=review

Needs work still. I think I now understand better how starting and closing the server is supposed to work after reading more of the Qt code, so could make more useful suggestions.

> Source/WebCore/inspector/front-end/inspectorPageIndex.html:1
> +<!DOCTYPE html>

This is the same page qt uses as far as I understand, we should share it with qt somehow, either by moving it here (and thus removing qt's own copy) or by placing it somewhere under Source/WebKit2/UIProcess/InpsectorServer, let's not have a duplicate though.

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:116
> +    if (m_client) {

Make this an early return :

if (!m_client)
    return;

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:118
> +        // The client can close the handle, potentially removing the last reference.
> +        RefPtr<SocketStreamHandle> protect(this); 

My comment about this not being needed still stands =)

> Source/WebKit2/ChangeLog:14
> +        (for example using 192.168.124.130:2999 link).

I'd remove this parens, they are already implied by the rest of the description.

> Source/WebKit2/ChangeLog:16
> +        It is not possible to test the change automatically, so all testing has been done manually.

Qt has API tests, we can probably add a couple to our own tests, here: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/gtk/tests

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:87
> +    start();    

See bellow.

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.cpp:98
> +    if (m_clientMap.isEmpty()) // stop the server if no pages left, the server will be restarted if new page is registered.
> +        close();

See bellow (regarding start).

> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved. 

Doesn't look like a substation enough change to warrant this.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved. 

Ditto, but see bellow.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:94
> +void WebSocketServer::start()
> +{
> +    if (m_state == Listening)
> +        return;
> +
> +    platformGetIPAddress(m_bindAddress, m_port);
> +    listen(m_bindAddress, m_port);
> +}
> +

I don't think we should have something like this here. This kind of logic should go on the platform-specific implementation or be performed by the user agent. We should have public API in the future to start and close down the server, for now I think we should use Qt's approach of initializing the server as part of the WebContext initialization, using the variable, like you did. See: http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/qt/QtWebContext.cpp#L48

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved. 

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:43
> +#include <glib.h>

Should not need glib.h here from what I've seen.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:4
> + * Copyright (C) 2012 Samsung Electronics Ltd. All Rights Reserved.

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:129
> +    // To prevent double deletion in WebSocketServer::didCloseWebSocketServerConnection()
> +    if (!m_socket)
> +        return;

Is this triggered by something this patch does currently, like closing the socket when the last page is unregistered?

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:38
> +#include "FileSystem.h"
> +#include "WebInspectorProxy.h"
> +#include "WebPageProxy.h"
> +#include <WebCore/MIMETypeRegistry.h>
> +
> +#include <gio/gio.h>
> +#include <glib.h>
> +
> +#include <wtf/gobject/GOwnPtr.h>
> +#include <wtf/text/CString.h>
> +#include <wtf/text/StringBuilder.h>

There should be no empty lines here, and it looks like the sorting is not right (<g...> should go before <WebCore...>)

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:120
> +void getIPAddress(String& ipAddress)

I think we should always default to 127.0.0.1 if no explicit IP is given, opening up the server to a public IP would be quite bad, so I'd remove this function. If you need for your particular use-case you can add this kind of logic to the browser afterwards, or to a wrapper that starts the browser with the environment variable filled.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:147
> +void WebSocketServer::platformGetIPAddress(String& bindAddress, unsigned short& port)

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/qt/WebSocketServerQt.cpp:52
> +void WebSocketServer::platformGetIPAddress(String&, unsigned short&)
> +{
> +}
> +

Ditto, thus.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.