Bug 216334 – [Cocoa] _WKInspectorDelegate should handle showing external resources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Blaze Burg]

.

[Blaze Burg]

Created attachment 408411 [details]
Patch

This patch depends on the blocking bug's patch. I'll re-spin with EWS when the other patch lands later tonight.

[Blaze Burg]

Created attachment 408532 [details]
Patch

[Blaze Burg]

Created attachment 408534 [details]
Patch

[Devin Rousso]

Comment on attachment 408534 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorDelegate.h:49
> +- (void)inspector:(_WKInspector *)inspector showURLExternally:(NSURL *)url;

How about just `openURL`?  The idea of "showing a URL" seems kinda weird IMO :/

I'm not sure what "external" means in this context either, as Web Inspector will attempt to open a new tab/window for any URL that it doesn't have a resource for (including those from the inspected domain).

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.h:72
> +        bool inspectorShowURLExternally: 1;

Style: missing space before `:`

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:232
> +    // The latter code path has known issues in Safari, but is better than simply doing nothing.

NIT: I'd move this right above the `loadRequest` so it's clear that that code can have issues, not this code

> Source/WebKit/UIProcess/Inspector/mac/WebInspectorProxyMac.mm:240
> +    URL convertedURL { url };

Is this needed?  I thought `URL` is able to implicitly convert to `NSURL *`.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:47
> +static RetainPtr<NSURL> URLToShow;

NIT: I'd call this `urlToShow`

[Blaze Burg]

Comment on attachment 408534 [details]
Patch

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

I'm going to rename showURLExternally to openURLExternally. Also, this patch forgot about InspectorFrontendHost.openInNewTab which is how most doc links are actually opened. I'll need to add a new API test which triggers this from the IFH code path somehow.

>> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorDelegate.h:49
>> +- (void)inspector:(_WKInspector *)inspector showURLExternally:(NSURL *)url;
> 
> How about just `openURL`?  The idea of "showing a URL" seems kinda weird IMO :/
> 
> I'm not sure what "external" means in this context either, as Web Inspector will attempt to open a new tab/window for any URL that it doesn't have a resource for (including those from the inspected domain).

externally means outside of the Inspector view/window and outside of the inspected view.

>> Source/WebKit/UIProcess/Inspector/mac/WebInspectorProxyMac.mm:240
>> +    URL convertedURL { url };
> 
> Is this needed?  I thought `URL` is able to implicitly convert to `NSURL *`.

It didn't work inline for some reason (that's what I tried first)

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorDelegate.mm:47
>> +static RetainPtr<NSURL> URLToShow;
> 
> NIT: I'd call this `urlToShow`

ok

[Blaze Burg]

Created attachment 408611 [details]
Patch

[Devin Rousso]

Comment on attachment 408611 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendClient.h:3
> + * Copyright (C) 2020 Apple Inc. All rights reserved.

What exactly is the "policy" for adding/updating copyright comments?  As I understand it, it only gets added/updated if major modifications are made to the file, but perhaps that's incorrect?

> Source/WebCore/inspector/InspectorFrontendClient.h:100
> +    WEBCORE_EXPORT virtual void openURLExternally(const String& url) = 0;

Why change the order?  Now it no longer matches the order of `InspectorFrontendHost` :(

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:124
> -        InspectorFrontendHost.openInNewTab(this._resource.url);
> +        InspectorFrontendHost.openURLExternally(this._resource.url);

Can we change this to call `WI.openURL` while you're at it?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:35
> +static NSString *snippetToOpenURLExternally(NSURL *url)

NIT: I'd add "JavaScript" into this name somewhere so that it's clear that this is meant to be evaluated in the frontend

> Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:41
> -    , m_alternativeTextUIController { makeUnique<AlternativeTextUIController>() }
> +    , m_alternativeTextUIController { makeUnique<WebCore::AlternativeTextUIController>() }

o_O

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.mm:100
> +    [delegate inspector:m_inspectorDelegate.m_inspector.get().get() openURLExternally:[NSURL URLWithString:url]];

Is it ever possible for the `url` to be relative?  If so, will that work with `[NSURL URLWithString:url]`?

> Source/WebKit/UIProcess/Inspector/WebInspectorProxy.cpp:719
> +void WebInspectorProxy::evaluateInFrontendForTesting(const String& expression)

There's already a `WebInspector::evaluateScriptForTest`.  Can we hook into that (i.e. add the message and send it here)?  If so, it'd be nice to share the name so it's clear to follow across processes.  If not, can we change the name enough so as to really clarify why not?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:236
> +    // Allow the delegate to handle it if possible, otherwise try to load in the inspected page.

NIT: I'd reword this comment (or split it into two comments) so that it doesn't read like the explanation comes after the thing that's being explained

e.g. `// Try to load the request in the inspected page if the delegate can't handle it.`

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.h:-156
> -    RefPtr<IPC::Connection> m_backendConnection;

ooooo neat!

[Radar WebKit Bug Importer]

<rdar://problem/69024354>

[Blaze Burg]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 408611 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408611&action=review
> 
> > Source/WebCore/inspector/InspectorFrontendClient.h:3
> > + * Copyright (C) 2020 Apple Inc. All rights reserved.
> 
> What exactly is the "policy" for adding/updating copyright comments?  As I
> understand it, it only gets added/updated if major modifications are made to
> the file, but perhaps that's incorrect?

I don't think we have a policy. In my opinion, adding a new method is enough change to update the copyright.

> > Source/WebCore/inspector/InspectorFrontendClient.h:100
> > +    WEBCORE_EXPORT virtual void openURLExternally(const String& url) = 0;
> 
> Why change the order?  Now it no longer matches the order of
> `InspectorFrontendHost` :(

Oh, oops.

> > Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:124
> > -        InspectorFrontendHost.openInNewTab(this._resource.url);
> > +        InspectorFrontendHost.openURLExternally(this._resource.url);
> 
> Can we change this to call `WI.openURL` while you're at it?
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKInspectorTesting.mm:35
> > +static NSString *snippetToOpenURLExternally(NSURL *url)
> 
> NIT: I'd add "JavaScript" into this name somewhere so that it's clear that
> this is meant to be evaluated in the frontend
> 
> > Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:41
> > -    , m_alternativeTextUIController { makeUnique<AlternativeTextUIController>() }
> > +    , m_alternativeTextUIController { makeUnique<WebCore::AlternativeTextUIController>() }
> 
> o_O
> 
> > Source/WebKit/UIProcess/Inspector/Cocoa/InspectorDelegate.mm:100
> > +    [delegate inspector:m_inspectorDelegate.m_inspector.get().get() openURLExternally:[NSURL URLWithString:url]];
> 
> Is it ever possible for the `url` to be relative?  If so, will that work
> with `[NSURL URLWithString:url]`

I don't expect relative URLs to be opened via this API.

> > Source/WebKit/UIProcess/Inspector/WebInspectorProxy.cpp:719
> > +void WebInspectorProxy::evaluateInFrontendForTesting(const String& expression)
> 
> There's already a `WebInspector::evaluateScriptForTest`.  Can we hook into
> that (i.e. add the message and send it here)?  If so, it'd be nice to share
> the name so it's clear to follow across processes.  If not, can we change
> the name enough so as to really clarify why not?

Currently, WebInspector::evaluateScriptForTest is a WebProcess thing sends the string to the backend to be evaluated via InspectorAgent. I'm not sure of the reasons to prefer one approach or the other. In any case this should be a followup.

> > Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:236
> > +    // Allow the delegate to handle it if possible, otherwise try to load in the inspected page.
> 
> NIT: I'd reword this comment (or split it into two comments) so that it
> doesn't read like the explanation comes after the thing that's being
> explained
> 
> e.g. `// Try to load the request in the inspected page if the delegate can't
> handle it.`
> 

OK

[Blaze Burg]

I addressed the other feedback but forgot to comment, oops.

Investigating GTK failures now.

[Blaze Burg]

Created attachment 409295 [details]
For GTK EWS

[Blaze Burg]

Created attachment 409332 [details]
For GTK EWS

[EWS]

Committed r267411: <https://trac.webkit.org/changeset/267411>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409332 [details].