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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Apr 20 12:24:15 PDT 2021

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

Attachment 426457: Patch v1.0


--- 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


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
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)

> +    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]`?

> +    NSMutableSet<NSURL *> *result = [[NSMutableSet alloc]
> +    [result addObject:[NSURL
> +    [result addObject:[NSURL
> +    _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?

> +	   NSMutableDictionary *headerFields = @{

Should we `adoptNS`?

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

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())


More information about the webkit-reviews mailing list