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

[bugzilla-daemon at webkit.org]

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





--- Comment #41 from Anton Obzhirov <a.obzhirov at samsung.com>  2012-10-11 07:13:00 PST ---
(From update of attachment 167539)
View in context: https://bugs.webkit.org/attachment.cgi?id=167539&action=review

>> Source/WebKit2/ChangeLog:9
>> +        The server listens on port 2999 by default. Ip address of the server can be set
> 
> Ip -> IP

ok

>> Source/WebKit2/WebKit2.qrc:3
>> +    <file alias="inspectorPageIndex.html">UIProcess/InspectorServer/front-end/inspectorPageIndex.html</file>
> 
> This patch should also remove the now unused file at Source/WebKit2/qt/Resources/inspectorPageIndex.html

Yep, forgot to remove.

>> Source/WebKit2/UIProcess/InspectorServer/WebInspectorServer.h:64
>> +#if (PLATFORM(QT) || PLATFORM(GTK))
> 
> Outermost parentheses are not necessary.

ok

>>> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.h:40
>>> +#elif PLATFORM(GTK)
>> 
>> Shouldn't this be #elif USE(SOUP), instead? Then both GTK and EFL could benefit from it.
>> Also, I would move this entire block up, before the #if PLATFORM(QT), since it is related to includes and not namespaces/forward declarations.
> 
> I want to use 'WebSocketServer' using g_socket_service on efl port.
> Should I copy WebSocketServerGtk.cpp to WebSocketServerEfl.cpp? or make a shareable directory for gtk/efl?

Shareable folder should be better. Probably it should be WebSocketServerSoup.cpp in soup folder. I can change it in the next patch.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:44
>> +#ifndef LOG_DISABLED
> 
> I think this line should be "#if !LOG_DISABLED".

ok

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:46
>> +    GOwnPtr<gchar> addressStr(g_inet_address_to_string(g_inet_socket_address_get_address(G_INET_SOCKET_ADDRESS(socketAddress.get()))));
> 
> Abbreviation "Str" here, too. "addressString" would be fine.

ok

>>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:47
>>> +    LOG(Network, "New Connection from %s:%d.", addressStr.get(), g_inet_socket_address_get_port(G_INET_SOCKET_ADDRESS(socketAddress.get())));
>> 
>> Shouldn't the #ifndef LOG_DISABLED be protecting only this line instead of all the 3 ones above?
> 
> I guess the original code is fine in this case because socketAddress and addressStr are only used within LOG().

Yes, it doesn't make sense to move them out of the block.

>> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:68
>> +#endif
> 
> This #ifndef block is not necessary because LOG() becomes a no-op when LOG_DISABLED is true. See wtf/Assertions.h.

ok, will check

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:46
>> +    String envStr(g_getenv("WEBKIT_INSPECTOR_SERVER"));
> 
> We tend to avoid abbreviations like "env" or "Str" and spell the words fully.
> 
> Or, as an alternative, "serverAddress" or "serverSpecification" may sound better.

ok

>> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:55
>> +        if (result.size() == 2) {
> 
> I don't know how you intend to use WEBKIT_INSPECTOR_SERVER, but if users are going to fill it directly it might be good to give back some error message in cases where you couldn't parse the address/port or if you couldn't open the server.

ok, I will add LOG_ERROR message here.

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