[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


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #163880|review?                     |review-
               Flag|                            |




--- Comment #24 from Gustavo Noronha (kov) <gns at gnome.org>  2012-09-13 09:48:02 PST ---
(From update of attachment 163880)
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.

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