Bug 161515 – Web Inspector: Provide a way to open an inspector frontend for a remote target

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

[Joseph Pecoraro]

Summary:
Provide a way to open an inspector frontend for a remote target.

The Remote Web Inspector in Safari currently uses its own separate path for opening a frontend for a remote debuggable (JSContext, WebView). Implementing this in WebKit will have a number of advantages by sharing the implementation with the normal local Web Inspector. Unifying the implementations where possible will address a few subtle differences in their behavior, and making it much easier to modify this configuration.

[Joseph Pecoraro]

<rdar://problem/13182127>

[Joseph Pecoraro]

Created attachment 287696 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 287696 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebInspectorProxy.h:102:  The parameter name "page" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:88:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.cpp:90:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:129:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:130:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 287696 [details]
[PATCH] Proposed Fix

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

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:-116
> -    pageLevelMap().remove(m_inspectedPage);

Oh, I didn't mention in my changelog but this was a fix to the local inspector. This was wrong. It was using the inspected page instead of the inspector page so that is just plain wrong. Using new names trackInspectorPage and untrackInspectorPage this became more clear, and the new location is down below (didClose) before clearing m_inspectorPage.

[Joseph Pecoraro]

Seems I need a dummy RemoteWebInspectorProxy implementation for non-Mac ports used by generated code.
                                                                                            
/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/DerivedSources/WebKit2/RemoteWebInspectorProxyMessageReceiver.cpp:64:126: error: no member named 'sendMessageToBackend' in namespace 'Messages::RemoteWebInspectorProxy'
        IPC::handleMessage<Messages::RemoteWebInspectorProxy::SendMessageToBackend>(decoder, this, &RemoteWebInspectorProxy::sendMessageToBackend);

I'll be cleaning this up.

[Joseph Pecoraro]

Created attachment 287706 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 287715 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 287722 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 287729 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 287734 [details]
[PATCH] Proposed Fix

Still trying to get EFL building.

[Joseph Pecoraro]

Created attachment 287777 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 287777 [details]
[PATCH] Proposed Fix

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

(Taking a break for lunch)

> Source/WebCore/ChangeLog:11
> +        Now it may create a frontend for a javascript or web

'javascript' and 'web'

> Source/WebInspectorUI/UserInterface/Protocol/LoadInspectorBackendCommands.js:28
> +    if (!backendCommandsURL)

let backendCommandsURL = a || b

> Source/WebKit2/ChangeLog:12
> +        support and older version of the protocol. The Inspector frontend

'an'

> Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.h:61
> +- (void)inspectorViewControllerInspectorDidClose:(_WKRemoteWebInspectorViewController *)controller;

FrontendDidClose?

> Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:141
> +#endif // PLATFORM(MAC)

Nit: && WK_API_ENABLED in the comment

> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.cpp:147
> +{
> +    if (m_inspectorPage)
> +        return;
> +
> +    m_inspectorPage = platformCreateFrontendPageAndWindow();
> +
> +    trackInspectorPage(m_inspectorPage);
> +
> +    m_inspectorPage->process().addMessageReceiver(Messages::RemoteWebInspectorProxy::messageReceiverName(), m_inspectorPage->pageID(), *this);
> +}
> +
> +void RemoteWebInspectorProxy::closeFrontendPageAndWindow()
> +{
> +    if (!m_inspectorPage)
> +        return;
> +
> +    m_inspectorPage->process().removeMessageReceiver(Messages::RemoteWebInspectorProxy::messageReceiverName(), m_inspectorPage->pageID());
> +
> +    untrackInspectorPage(m_inspectorPage);
> +
> +    m_inspectorPage = nullptr;
> +
> +    platformCloseFrontendPageAndWindow();
> +}
> +

XX

> Source/WebKit2/UIProcess/WebInspectorUtilities.cpp:80
> +    WebProcessPool*& pool = inspectionLevel == 1 ? s_mainInspectorProcessPool : s_nestedInspectorProcessPool;

please put the conditional in parens to make easier to parse.

[Blaze Burg]

Comment on attachment 287777 [details]
[PATCH] Proposed Fix

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

r=me, this is very clean Joe! Great work.

> Source/WebKit2/ChangeLog:61
> +        Previously this was part of WebInspectorProxy and subclasses but can

This could be a separate patch but I don't care at this point =)

>> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.cpp:147
>> +
> 
> XX

disregard my placeholder.

> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:35
> +#if PLATFORM(MAC)

I am not sure there is any point to including the RemoteInspectorProxy stuff on non-Mac/iOS ports. But this is a guard for iOS right?

> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:106
> +    RemoteWebInspectorProxyClient *m_client { nullptr };

Nit: this is a C++ class, right?

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:522
> +

Nice catch.

> Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm:219
> +    [configuration setProcessPool: ::WebKit::wrapper(inspectorProcessPool(inspectorLevel))];

Maybe this should be inspectorProcessPoolForLevel ?

[Joseph Pecoraro]

Comment on attachment 287777 [details]
[PATCH] Proposed Fix

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

>> Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.h:61
>> +- (void)inspectorViewControllerInspectorDidClose:(_WKRemoteWebInspectorViewController *)controller;
> 
> FrontendDidClose?

I figured this is the InspectorViewController so if the Inspector closed that would be enough. I'll leave this as is but we can always change it later.

>> Source/WebKit2/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:141
>> +#endif // PLATFORM(MAC)
> 
> Nit: && WK_API_ENABLED in the comment

This seems to be a common pattern (all other COCOA headers with both in the #if just have this comment at the end), so I left it as is.

>> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:35
>> +#if PLATFORM(MAC)
> 
> I am not sure there is any point to including the RemoteInspectorProxy stuff on non-Mac/iOS ports. But this is a guard for iOS right?

I ended up including it to get things to compile. There may be a way to do this that I overlooked, but it is a very small unused object on ports that don't use it.

>> Source/WebKit2/UIProcess/RemoteWebInspectorProxy.h:106
>> +    RemoteWebInspectorProxyClient *m_client { nullptr };
> 
> Nit: this is a C++ class, right?

Yep! Moved the *.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/205369>

[Jonathan Bedard]

This patch caused a regression on the El Capitan CMake build:
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/8067

[Jonathan Bedard]

Never mind, fixed in https://trac.webkit.org/changeset/205375.

[Csaba Osztrogonác]

(In reply to comment #15)
> <https://trac.webkit.org/changeset/205369>

and the Apple Mac cmake build fixed by https://trac.webkit.org/changeset/205545

[Joseph Pecoraro]

(In reply to comment #18)
> (In reply to comment #15)
> > <https://trac.webkit.org/changeset/205369>
> 
> and the Apple Mac cmake build fixed by
> https://trac.webkit.org/changeset/205545

Doh, thanks!