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.
Created attachment 454114 [details] Patch
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.
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?
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.
(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.
(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.
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)
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
Created attachment 454232 [details] Patch for landing Patch addressing review comments. Please do not cq+ this until Patrick or Devin approve the WebInspectorUI changes.
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.
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?
Committed r291093 (248255@trunk): <https://commits.webkit.org/248255@trunk>
<rdar://problem/90085144>