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

Wed Aug 29 03:23:41 PDT 2012

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

--- Comment #12 from Anton Obzhirov <a.obzhirov at samsung.com>  2012-08-29 03:23:43 PST ---
(From update of attachment 157749)
View in context: https://bugs.webkit.org/attachment.cgi?id=157749&action=review

>> 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.

I think it should be in Source/WebCore/Inspector/front-end/inspector as it's part of front-end (kind of). I will remove qt's copy.

>> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:118
>> +        RefPtr<SocketStreamHandle> protect(this); 
> 
> My comment about this not being needed still stands =)

Forgot to remove :)

>> 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

OK, will check how it is done in QT port. Not entirely sure yet how to test it.

>> 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.

Is it about if change is less than 10 lines?

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:94
>> +
> 
> 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

Will undo my change here, hopefully there will be more generic approach in the future.

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:129
>> +        return;
> 
> Is this triggered by something this patch does currently, like closing the socket when the last page is unregistered?

No as far as I remember, will double check anyway.

>> 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.

That was one of your comments that it would be good to get IP address somehow :). Don't really need it, so will remove it anyway.

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