Bug 237601 – [GTK][WPE] Web Inspector: make it possible to use the remote inspector from other browsers

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Carlos Garcia Campos]

This is no longer possible since we switched to the new remote inspector. We don't want to bring the legacy remote inspector back, but it should be possible to use a mixed approach, using a WebSocket for the inspector protocol communication, but still using the new remote inspector code.

[Carlos Garcia Campos]

Created attachment 454114 [details]
Patch

[Michael Catanzaro]

Comment on attachment 454114 [details]
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.

[Fujii Hironori]

This reminds me the previous discussion (Bug 172966).
Does the current WebInspectorUI work nicely with non-WebKit browsers?
If not, do you have a plan to change WebInspectorUI for non-WebKit?

[Carlos Garcia Campos]

Comment on attachment 454114 [details]
Patch

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

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

This can't be IPv6 at the moment. We plan to add support for IPv6 soon, though.

[Carlos Garcia Campos]

(In reply to Fujii Hironori from comment #3)
> This reminds me the previous discussion (Bug 172966).
> Does the current WebInspectorUI work nicely with non-WebKit browsers?
> If not, do you have a plan to change WebInspectorUI for non-WebKit?

No, it doesn't work with non-WebKit browsers. Note that being able to use Safari on MacOS to inspect WPE is already a huge improvement. We still plan to try to make it work at least in chrome, and at least the basic functionality.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #5)
> (In reply to Fujii Hironori from comment #3)
> > This reminds me the previous discussion (Bug 172966).
> > Does the current WebInspectorUI work nicely with non-WebKit browsers?
> > If not, do you have a plan to change WebInspectorUI for non-WebKit?
> 
> No, it doesn't work with non-WebKit browsers. Note that being able to use
> Safari on MacOS to inspect WPE is already a huge improvement. We still plan
> to try to make it work at least in chrome, and at least the basic
> functionality.

Making things work in Chromium is not much work, I have made a small patch
that seems to get the job done (I didn't do a lot of testing, there may be
still other changes needed, YMMV):

  https://o.perezdecastro.org/inspector-chromium.diff

The issue is that the inspector uses CSSStyleDeclaration.getPropertyCSSValue(),
which according to MDN is deprecated and implemented only in WebKit; the patch
linked above switches over to use Element.computedStyleMap() which the other
web engines implement (it's part of CSS Typed OM level 2).

In the case of Firefox, I have a small patch that adds a polyfill for
Element.scrollIntoViewIfNeeded() by using the “options” parameter to
Element.scrollIntoView() to achieve the same effect (this is part of
CSS OM View module): 

  https://o.perezdecastro.org/inspector-firefox.diff

While the patch above gets the inspector loaded in Firefox and many
things more or less working okay, there are still features that do not
work (I haven't looked at them yet) and in some places a few obvious
graphical glitches.

[Loïc Yhuel]

Comment on attachment 454114 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        started when the env var WEBKIT_INSPECTOR_HTTPS_SERVER is present, and the inspector server is started for the

WEBKIT_INSPECTOR_HTTP_SERVER (same below)

[Carlos Garcia Campos]

Comment on attachment 454114 [details]
Patch

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

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

You are right, it shouldn't be a problem after https://bugs.webkit.org/show_bug.cgi?id=237646

[Carlos Garcia Campos]

Created attachment 454232 [details]
Patch for landing

Patch addressing review comments. Please do not cq+ this until Patrick or Devin approve the WebInspectorUI changes.

[Joseph Pecoraro]

Comment on attachment 454232 [details]
Patch for landing

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

> Source/WebInspectorUI/UserInterface/Base/BrowserInspectorFrontendHost.js:1
> +if (!window.InspectorFrontendHost) {

I'd expect a copyright header in this file.

[Patrick Angle]

Comment on attachment 454232 [details]
Patch for landing

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

WebInspectorUI changes look fine to me - but please make sure to add a license header as joepeck mentioned.

> Source/WebInspectorUI/Scripts/copy-user-interface-resources.pl:188
> +my $shouldIncludeInspectorFrontendHost = defined $ENV{'INCLUDE_INSPECTOR_FRONTEND_HOST'} && ($ENV{'INCLUDE_INSPECTOR_FRONTEND_HOST'} eq 'YES');

Nit: Can we call this `$shouldIncludeBrowserInspectorFrontendHost` so its clearer what the setting actually does/relates to?

> Source/WebInspectorUI/UserInterface/Base/BrowserInspectorFrontendHost.js:213
> +            // FIXME: Create a Blob from the content, get an object URL, open it to trigger a download.

Should this throw an exception since call sites should be checking if `canSave` is true first anyways?

[Carlos Garcia Campos]

Committed r291093 (248255@trunk): <https://commits.webkit.org/248255@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/90085144>