[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