WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
216334
[Cocoa] _WKInspectorDelegate should handle showing external resources
https://bugs.webkit.org/show_bug.cgi?id=216334
Summary
[Cocoa] _WKInspectorDelegate should handle showing external resources
Blaze Burg
Reported
2020-09-09 16:50:27 PDT
.
Attachments
Patch
(24.17 KB, patch)
2020-09-09 21:51 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Patch
(23.95 KB, patch)
2020-09-11 08:25 PDT
,
Blaze Burg
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(24.06 KB, patch)
2020-09-11 08:28 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Patch
(64.59 KB, patch)
2020-09-12 14:27 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
For GTK EWS
(64.43 KB, patch)
2020-09-21 12:17 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
For GTK EWS
(64.43 KB, patch)
2020-09-21 17:07 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Blaze Burg
Comment 1
2020-09-09 21:51:27 PDT
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
Comment 2
2020-09-11 08:25:13 PDT
Created
attachment 408532
[details]
Patch
Blaze Burg
Comment 3
2020-09-11 08:28:49 PDT
Created
attachment 408534
[details]
Patch
Devin Rousso
Comment 4
2020-09-11 10:59:51 PDT
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 5
2020-09-11 14:09:57 PDT
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
Comment 6
2020-09-12 14:27:59 PDT
Created
attachment 408611
[details]
Patch
Devin Rousso
Comment 7
2020-09-15 11:19:09 PDT
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
Comment 8
2020-09-16 16:51:13 PDT
<
rdar://problem/69024354
>
Blaze Burg
Comment 9
2020-09-21 11:55:37 PDT
(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
Comment 10
2020-09-21 11:57:18 PDT
I addressed the other feedback but forgot to comment, oops. Investigating GTK failures now.
Blaze Burg
Comment 11
2020-09-21 12:17:34 PDT
Created
attachment 409295
[details]
For GTK EWS
Blaze Burg
Comment 12
2020-09-21 17:07:46 PDT
Created
attachment 409332
[details]
For GTK EWS
EWS
Comment 13
2020-09-22 09:14:12 PDT
Committed
r267411
: <
https://trac.webkit.org/changeset/267411
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 409332
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug