[webkit-reviews] review granted: [Bug 224577] [Cocoa] re-enable test case WKInspectorDelegate.InspectorConfiguration : [Attachment 426457] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted BJ Burg <bburg at apple.com>'s
request for review:
Bug 224577: [Cocoa] re-enable test case
WKInspectorDelegate.InspectorConfiguration
https://bugs.webkit.org/show_bug.cgi?id=224577

Attachment 426457: Patch v1.0

https://bugs.webkit.org/attachment.cgi?id=426457&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 426457
  --> https://bugs.webkit.org/attachment.cgi?id=426457
Patch v1.0

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

r=me

This looks generally fine to me, but I'd recommend getting someone more
familiar with CSP (both the concept and the related WebKit logic) to look at it
as well.

BTW I was poking around some other CSP tests and I came across
`LegacySchemeRegistry::registerURLSchemeAsBypassingContentSecurityPolicy`. 
Would that also be a valid solution to this?  Or does the "Legacy" part suggest
that maybe we don't want to use this ��

Also, is it possible to write a layout/API test that confirms the CSP state in
Web Inspector so that we're not opening up any new CSP "holes"?

> Source/WebKit/ChangeLog:34
> +	   Drive-by, fix memory leak the right way.

Could you explain this more?

> Source/WebKit/ChangeLog:37
> +	   (WebKit::WebInspectorUIProxy::frontendLoaded): Call the client.

ditto (:34)

>
Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:51
> +    return _allowedURLSchemesForCSP.autorelease();

IIRC, `RetainPtr::autorelease` will `std::exchange` the underlying pointer.  Do
we want that here?  If so, can you rename this to
`releaseAllowedURLSchemesForCSP`?  If not, should this just be `.get()`
instead?  Or maybe `[_allowedURLSchemesForCSP copy]`?

>
Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:61
> +    NSMutableSet<NSURL *> *result = [[NSMutableSet alloc]
initWithCapacity:2];
> +    [result addObject:[NSURL
URLWithString:WebKit::WebInspectorUIProxy::inspectorPageURL()]];
> +    [result addObject:[NSURL
URLWithString:WebKit::WebInspectorUIProxy::inspectorTestPageURL()]];
> +    _mainResourceURLsForCSP = result;

Do we expect/allow these values to change?  If not, do we need to
recreate/reassign `_mainResourceURLsForCSP` each time, or can we make a lazy
getter that initializes it once?

>
Source/WebKit/UIProcess/Inspector/mac/WKInspectorResourceURLSchemeHandler.mm:11
2
> +	   NSMutableDictionary *headerFields = @{

Should we `adoptNS`?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:118
> +    RetainPtr<NSMutableSet<NSString *>> allowedURLSchemes =
adoptNS([NSMutableSet setWithObjects:WKInspectorResourceScheme, @"ws", @"wss",
nil]);

Are `ws` and `wss` actually used in the Web Inspector frontend?

> Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm:119
> +    for (auto pair : _configuration->_configuration->urlSchemeHandlers())

`auto&`