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

[bugzilla-daemon at webkit.org]

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 152986: Patch
https://bugs.webkit.org/attachment.cgi?id=152986&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152986&action=review


> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:109
> +    if (!m_readBuffer)
> +	   return;

This check seems unnecessary.

> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:121
> +    if (m_client) {
> +	   // The client can close the handle, potentially removing the last
reference.
> +	   RefPtr<SocketStreamHandle> protect(this); 
> +	   m_client->didOpenSocketStream(this);
> +    }

Protecting this should not be necessary here, since we are not doing anything
else after didOpenSocketStream's called.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:375
> +#if PLATFORM(GTK)
> +    // will read host address from environment variables
> +    if (m_pageGroup->preferences()->developerExtrasEnabled())
> +	   WebInspectorServer::shared().listen("", 2999);
> +    else
> +	   WebInspectorServer::shared().close();
> +#endif

This doesn't look right... WebSocketServer::platform{Listen,Close} should
already be called when appropriate, shouldn't they?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1747
> +#if PLATFORM(GTK)
> +    // will read host address from environment variables
> +    if (m_pageGroup->preferences()->developerExtrasEnabled())
> +	   WebInspectorServer::shared().listen("", 2999);
> +    else
> +	   WebInspectorServer::shared().close();
> +#endif

Ditto.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:38
> +    if (path == "/") {

So, as I understand it, if the remote side asks for / we want to give it the
default page, which is the inspector.html file, but we want any other resource
requested to be delivered as is. I don't understand what the meta refresh is
about; also, is there a reason why we are not delivering pagelist.json in a
special-case like qt?

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:50
> +	   if (!url)
> +	       url =
"http://trac.webkit.org/export/head/trunk/Source/WebCore/inspector/front-end/in
spector.html";

Hmm, no, we should use the locally installed file, not the one hosted in
webkit.org.

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:87
> +	   portValue = 2999;

Why 2999? Is it the same one used by the other ports?

> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:88
> +	   const char* envCStr = getenv("GTKWEBKIT_INSPECTOR_SERVER");

We use WEBKIT_ as our environment variables prefix.