[webkit-reviews] review denied: [Bug 206911] Web Inspector: safari app extension isolated worlds and injected files use the extension's identifier instead of it's name : [Attachment 389081] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 206911: Web Inspector: safari app extension isolated worlds and injected
files use the extension's identifier instead of it's name
https://bugs.webkit.org/show_bug.cgi?id=206911

Attachment 389081: Patch

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




--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 389081
  --> https://bugs.webkit.org/attachment.cgi?id=389081
Patch

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

Overall some great work. r- for bots. And I'd like InspectorClient to split
into InspectorDelegate + InspectorDelegate::InspectorClient. (Unless you can
get Brady or Alex to sign off on the correctness of the approach in this
patch.)

> Source/JavaScriptCore/ChangeLog:3
> +	   Web Inspector: safari app extension isolated worlds and injected
files use the extension's identifier instead of it's name

Nit: its name

> Source/JavaScriptCore/ChangeLog:12
> +	   debuggable rather than one per taret) and as such is not routed
through the `Target` agent.

Nit: target

> Source/WebInspectorUI/ChangeLog:33
> +	   should only be one per debuggable rather than one per taret) and as
such is not routed

Nit: target

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:41
> +	   // COMPATIBILITY (iOS 13.1): Browser did not exist yet.

Nit: 13.4

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:63
> +	       // COMPATIBILITY (iOS 13.1): Browser did not exist yet.

Nit: 13.4

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:75
> +	   return scheme && scheme.endsWith("-extension");

This seems kind of arbitrary, can the supported extension scheme syntax be
documented in the ChangeLog or something?

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:111
> +	      
console.assert(!this._extensionNameIdentifierMap.has(extensionId), `Browser
already exists with id ${extensionId}.`);

Nit: `Extension already exists..`

> Source/WebInspectorUI/UserInterface/Controllers/BrowserManager.js:121
> +	       console.assert(name);

Nit: needs assertion message, please include the extensionId as well.

> Source/WebInspectorUI/UserInterface/Protocol/MultiplexingBackendTarget.js:36
> +	   console.assert(Array.shallowEqual(Object.keys(this._agents),
["Browser", "Target"]));

Hah, nice! The design finally happened :P

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1552
> +    return
static_cast<WebKit::InspectorClient&>(_page->inspectorClient()).delegate().auto
release();

Hmm, this seems a little weird to rely on WebPageProxy to keep a reference to
the ObjC delegate. An approach like ResourceLoadDelegate / ResourceLoadClient
would be easier to keep track of, IMO. The code would look more like:

- (id <_WKResourceLoadDelegate>)_resourceLoadDelegate
{
    return _resourceLoadDelegate->delegate().autorelease();
}

- (void)_setResourceLoadDelegate:(id<_WKResourceLoadDelegate>)delegate
{
   
_page->setResourceLoadClient(_resourceLoadDelegate->createResourceLoadClient())
;
    _resourceLoadDelegate->setDelegate(delegate);
}

You can see the commit here: 
https://bugs.webkit.org/attachment.cgi?id=387046&action=review

This same pattern is followed for some other ObjC WKWebView delegates that plug
into WebPageProxy.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:156
> +- (void)_browserExtensionsEnabled:(NSDictionary<NSString *, NSString *>
*)extensionIDToName

Nit: extensionIDToNameMap

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:159
> +    transformed.reserveInitialCapacity([extensionIDToName count]);

Nit: extensionIDToName.count

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:171
> +	   transformed.addVoid(extensionID);

addVoid? ��

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorPrivate.h:35
> +- (void)_browserExtensionsEnabled:(NSDictionary<NSString *, NSString *>
*)extensionIDToName;

I find it a bit strange to avoid using NSUUID here, but it seems pretty common
across WebKit code to force NSUUID to NSString.

> Source/WebKit/UIProcess/Inspector/Agents/InspectorBrowserAgent.h:62
> +} // namespace WebCore

Nit: namespace WebKit

> Source/WebKit/UIProcess/Inspector/Cocoa/InspectorClient.h:64
> +} // WebKit

Nit: namespace WebKit

> Tools/TestWebKitAPI/Tests/WTF/HashSet.cpp:510
> +{

I'm not quite sure how this test can fail if reserveInitialCapacity was
incorrect. Is the initial capacity observable from calling .add, .remove, and
.size a lot?