[webkit-reviews] review granted: [Bug 216334] [Cocoa] _WKInspectorDelegate should handle showing external resources : [Attachment 408534] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 11 10:59:51 PDT 2020
Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 216334: [Cocoa] _WKInspectorDelegate should handle showing external
resources
https://bugs.webkit.org/show_bug.cgi?id=216334
Attachment 408534: Patch
https://bugs.webkit.org/attachment.cgi?id=408534&action=review
--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 408534
--> https://bugs.webkit.org/attachment.cgi?id=408534
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`
More information about the webkit-reviews
mailing list