[webkit-reviews] review denied: [Bug 88094] Web Inspector: Add a WebInspectorServer on Linux using the GSocket API for the GTK port : [Attachment 157749] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Aug 26 13:13:53 PDT 2012


Gustavo Noronha (kov) <gns at gnome.org> has denied Anton Obzhirov
<a.obzhirov at samsung.com>'s request for review:
Bug 88094: Web Inspector: Add a WebInspectorServer on Linux using the GSocket
API for the GTK port
https://bugs.webkit.org/show_bug.cgi?id=88094

Attachment 157749: Patch
https://bugs.webkit.org/attachment.cgi?id=157749&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.c
pp#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.


More information about the webkit-reviews mailing list