[webkit-reviews] review denied: [Bug 216334] [Cocoa] _WKInspectorDelegate should handle showing external resources : [Attachment 408534] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied  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 #5 from Brian Burg <bburg 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

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