[webkit-reviews] review granted: [Bug 237601] [GTK][WPE] Web Inspector: make it possible to use the remote inspector from other browsers : [Attachment 454114] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 237601: [GTK][WPE] Web Inspector: make it possible to use the remote
inspector from other browsers
https://bugs.webkit.org/show_bug.cgi?id=237601

Attachment 454114: Patch

https://bugs.webkit.org/attachment.cgi?id=454114&action=review




--- Comment #2 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 454114
  --> https://bugs.webkit.org/attachment.cgi?id=454114
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=454114&action=review

Cool.

> Source/WebKit/UIProcess/API/glib/WebKitInitialize.cpp:78
> +	   g_setenv("WEBKIT_INSPECTOR_SERVER", inspectorAddress.get(), TRUE);

Careful: this function is called inside std::call_once, which uses threads, so
it's too late to modify the environment.

IMO you are insufficiently fearful of setenv. It's not just a theoretical
problem: remember we have documented cases where this broke gnome-session
(https://bugzilla.gnome.org/show_bug.cgi?id=754951) and also
gnome-initial-setup (downstream fork).

> Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:50
> +	   g_warning("Failed to start remote inspector HTTP server on %s:%u:
invalid address\n", address, port);

Oops: should be no trailing \n here

Also: if address is an IPv6 address, then %s:%u will be an invalid URI: it
should be [%s]:%u in that case. I suppose that's an edge case that's probably
not worth handling manually, but there is a g_inet_address_to_string() you
might be able to use to avoid it.

> Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:56
> +	   g_warning("Failed to start remote inspector HTTP server on %s:%u:
%s\n", address, port, error->message);

Ditto

> Source/WebKit/UIProcess/Inspector/glib/RemoteInspectorHTTPServer.cpp:157
> +    g_signal_handlers_disconnect_by_data(webSocketConnection.get(), this);

Good.