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

Fri Sep 14 03:44:27 PDT 2012

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

--- Comment #28 from Anton Obzhirov <a.obzhirov at samsung.com>  2012-09-14 03:44:53 PST ---
(From update of attachment 163880)
View in context: https://bugs.webkit.org/attachment.cgi?id=163880&action=review

>> Source/WebCore/ChangeLog:13
>> +        * inspector/front-end/inspectorPageIndex.html: Added.
> 
> I don't think it belongs in WebCore, the inspector server is contained in WebKit2, so the resource should go there as well.
> Source/WebKit2/UIProcess/InspectorServer/front-end would feel like a better place to me, but that means that ports might have to include it in a different resource bundle.

OK, I will move it to Source/WebKit2/UIProcess/InspectorServer/front-end and update resource path if needed.

>> Source/WebKit2/config.h:122
>> +#define ENABLE_INSPECTOR_SERVER 1
> 
> #if PLATFORM(QT) || PLATFORM(GTK)

ok

>>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:86
>>> +    g_thread_new("TestServerMonitor", testServerMonitorThreadFunc, 0);
>> 
>> Do we really need to use a thread here? Wouldn't a glib timeout do?
> 
> I think the reasoning is the same as in bug #72589, see the discussion there.

Check as well implementation of TestWebkitAccessability.cpp /WebKit2/UIProcess/API/gtk/tests/TestWebKitAccessibility.cpp

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:96
>> +    s_inspectorServerEnv = g_getenv("WEBKIT_INSPECTOR_SERVER");
> 
> You shouldn't need to do this, setting the variable for your own process will not affect the parents.

yep, forget about this.

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:98
>> +    // Overwrite WEBKIT_INSPECTOR_SERVER variable with default value.
> 
> No need to add "what" comments when the line of code is already pretty explicit, I think you can cut back most of the comments.

there is no harm in additional comments I suppose plus I copied this code anyway from TestWebkitAccessability.cpp with all these comments :)

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:116
>> +    }
> 
> I believe this should be an assert instead of a simple check; if the spawn fails all bets are off.

ok

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:128
>> +    }
> 
> Guess because you're blocking here? You should just need to g_io_add_watch here instead.

ok will check

>> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:239
>> +    // debugging session was established correctly through web socket. It should be something like "Web Inspector - <Page URL>".
> 
> How about we assert g_str_has_prefix instead?

hm, probably you are right. What if some changes title format in the future?

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:84
>>      ASSERT(!m_socket->bufferedAmount());
> 
> This doesn't look right. The assert is now useless... it may make sense to drop it, but then we should drop it and implement what's descried in the comment, not just make it useless =). Might be useful to get the developer who wrote it (git blame should tell you) what they think about this.

yes, assert shouldn't be here any more. I implemented what was described in the comment but instead of adding new callback in SocketStreamHandleClient
I used existing didUpdateBufferedAmount which basically means didSend when bufferedAmount is 0.

>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:130
>> +        return;
> 
> Can you elaborate on this? If in any case didCloseWebSocketServerConnection isn't run, this would prevent the server from accepting a new connection if the previous connection wasn't cleaned-up properly.

I think if m_socket is 0 didCloseWebSocketServerConnection has been already called, I will attach call stack to show why double deletion happens.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:40
>> +bool WebInspectorServer::platformResourceForPath(const String& path, Vector<char>& data, String& contentType)
> 
> How often is this method called? I don't like all these synchronous API blocking the UI process, but the method is synchronous so I don't see any other solution.

Whenever inspector page on remote host sends resource request. Shouldn't happen often, didn't notice slow down because of that.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:58
>> +    GRefPtr<GFileInputStream> inputStream = adoptGRef(g_file_read(file.get(), NULL, 0));
> 
> Use 0 instead of NULL.

ok

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:66
>> +    contentType = GOwnPtr<gchar>(g_file_info_get_attribute_as_string(fileInfo.get(), G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE)).get();
> 
> You can use g_file_info_get_content_type() which returns a const char *, if what contentType expects is actually the mime type, you might need to call g_content_type_get_mime_type() to get the mime type from the content type.

g_file_info_get_content_type() returns NULL because G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE attribute is not set. I can use G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE
in attributes string but it will probably slow down getting file info a bit.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:44
>> +                                   WebSocketServer* server)
> 
> This should be a single line.

ok

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:47
>> +    GSocketAddress* sockaddr = g_socket_connection_get_remote_address(connection, NULL);
> 
> Use GRefPtr and adoptGRef. Use also 0 instead of NULL.

ok

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:45
>> +    String envStr(getenv("WEBKIT_INSPECTOR_SERVER"));
> 
> g_getenv?

ok

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:61
>> +        initialized = WebInspectorServer::shared().listen(bindAddress, port);
> 
> initialized is to ensure initInspectorServer() is executed only once, so it should set to true always, even if the env var is not present or the server fails to run.

ok

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