[webkit-reviews] review denied: [Bug 88094] Web Inspector: Add a WebInspectorServer on Linux using the GSocket API for the GTK port : [Attachment 163880] 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 163880: Patch
https://bugs.webkit.org/attachment.cgi?id=163880&action=review

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


OK, we're getting there. I'm worried about the changes to
WebSocketServerConnection, though.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:86
> +#if (!GLIB_CHECK_VERSION(2, 31, 0))
> +    g_thread_create(testServerMonitorThreadFunc, 0, FALSE, 0);
> +#else
> +    g_thread_new("TestServerMonitor", testServerMonitorThreadFunc, 0);

Do we really need to use a thread here? Wouldn't a glib timeout do?

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:96
> +    // Save current WEBKIT_INSPECTOR_SERVER variable to restore it after the
test is finished.
> +    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.

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

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:116
> +    if (!g_spawn_async_with_pipes(0, testServerArgv, 0,
static_cast<GSpawnFlags>(0), 0, 0,
> +				     &s_childProcessPid, 0, &childStdout, 0,
0)) {
> +	   close(childStdout);
> +	   return;
> +    }

I believe this should be an assert instead of a simple check; if the spawn
fails all bets are off.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:119
> +    // Start monitoring the test server (in a separate thread) to
> +    // ensure we don't block on the child process more than a timeout.

I don't understand why you'd block on the child process using  glib timeout on
the main thread.

> Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp:128
> +    GIOChannel* ioChannel = g_io_channel_unix_new(childStdout);
> +    if (g_io_channel_read_chars(ioChannel, msg, 2, 0, 0) ==
G_IO_STATUS_NORMAL) {
> +	   // Check whether the server sent a message saying it's ready
> +	   // and store the result globally, so the monitor can see it.
> +	   s_childIsReady = msg[0] == 'O' && msg[1] == 'K';
> +    }

Guess because you're blocking here? You should just need to g_io_add_watch here
instead.

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

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:84
>  {
> -    // If this ASSERT happens on any platform then their
SocketStreamHandle::send
> -    // followed by a SocketStreamHandle::close is not guarenteed to have
sent all
> -    // data. If this happens, we need to slightly change the design to
include a
> -    // SocketStreamHandleClient::didSend, handle it here, and add an
m_shutdownAfterSend
> -    // state on this WebSocketServerConnection.
> +    if (m_socket->bufferedAmount()) {
> +	   m_shutdownAfterSend = true;
> +	   return;
> +    }
> +
>      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.