[webkit-reviews] review denied: [Bug 217783] [Cocoa] Inspector Extensions: Add _WKInspectorExtension and related plumbing : [Attachment 411493] Patch v1.1

Thu Oct 15 16:19:14 PDT 2020

Devin Rousso <drousso at apple.com> has denied Brian Burg <bburg at apple.com>'s
request for review:
Bug 217783: [Cocoa] Inspector Extensions: Add _WKInspectorExtension and related
plumbing
https://bugs.webkit.org/show_bug.cgi?id=217783

Attachment 411493: Patch v1.1

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

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

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

r-, as it doesn't compile :(

looks good overall tho!

> Source/WebInspectorUI/UserInterface/Controllers/WebExtensionController.js:26
> +WI.WebExtensionController = class WebExtensionController extends WI.Object

Can we put the word "Inspector" in here somewhere (e.g.
`WebInspectorExtensionController`)?  This would also more closely match the
backend `*InspectorExtension` object naming, making the relationship clearer. 
I think we should make it explicitly clear that this object "controls
extensions made to Web Inspector" as opposed to "controls web extensions"
(which really should be in `WI.BrowserManager`).

> Source/WebInspectorUI/UserInterface/Controllers/WebExtensionController.js:32
> +	   this._extensions = new Map;

NIT: how about `_extensionIDMap`?  `_extensions` sounds like an Array or Set :/

> Source/WebInspectorUI/UserInterface/Controllers/WebExtensionController.js:38
> +	       console.error("Unable to register extension, it's already
registered: ", extensionID, displayName);

Please avoid using any `console` functions other than `console.assert` as they
are not stripped from production builds.

Also, please use `WI.reportInternalException` instead of `console.error` so
that it is surfaced in engineering builds, thereby making it more likely that
engineers will report the issue and it can be fixed.

> Source/WebInspectorUI/UserInterface/Controllers/WebExtensionController.js:45
> +	   console.log("Registered extension: ", extensionID, displayName);

ditto (:38)

> Source/WebInspectorUI/UserInterface/Controllers/WebExtensionController.js:52
> +	       console.error("Unable to unregister extension with unknown ID:
", extensionID);

ditto (:38)

> Source/WebInspectorUI/UserInterface/Controllers/WebExtensionController.js:59
> +	   console.log("Unregistered extension: ", extensionID);

ditto (:38)

> Source/WebInspectorUI/UserInterface/Controllers/WebExtensionController.js:66
> +    ExtensionRemoved: "extension-removed"

Style: missing comma

> Source/WebInspectorUI/UserInterface/Models/WebExtension.js:26
> +WI.WebExtension = class WebExtension extends WI.Object

ditto (WebExtensionController.js:26)

Also, there's no need to `extend WI.Object` since this object doesn't dispatch
any events.  Though if you expect this to change in the future then it's fine
to keep :)

> Source/WebInspectorUI/UserInterface/Models/WebExtension.js:33
> +	   console.assert(typeof extensionID === "string");
> +	   console.assert(typeof displayName === "string");

NIT: I like to include the object being tested as an argument to
`console.assert` after the condition so that it's also logged, making it easier
to see the problem at a glance from the Console without trying to reproduce.
```
    console.assert(typeof extensionID === "string", extensionID);
    console.assert(typeof displayName === "string", displayName);
```

> Source/WebInspectorUI/UserInterface/Models/WebExtension.js:39
> +    // Public

Style: missing newline after

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:201
> +    registerExtension: function(extensionID, displayName)

Style: you can remove the `: function` in ES2015
```
    registerExtension(extensionID, displayName)
    {
	...
```

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:206
> +    unregisterExtension: function(extensionID)

Ditto (:201)

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:209
>      }

Style: missing comma

> Source/WebKit/UIProcess/API/APIInspectorExtension.cpp:49
> +InspectorExtension::~InspectorExtension()
> +{
> +}

`InspectorExtension::~InspectorExtension() = default;`

> Source/WebKit/UIProcess/API/APIInspectorExtension.h:49
> +    explicit InspectorExtension(const WTF::String& identifier,
WebKit::WebInspectorUIExtensionControllerProxy&);

NIT: I don't think `explicit` will actually do anything here.  AFAIU `explicit`
is used to prevent implicit conversions, but this is not a converting
constructor as it has multiple arguments.

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:207
> +- (void)registerExtensionWithID:(NSString *)extensionID
displayName:(NSString *)displayName completionHandler:(void(^)(NSError *,
_WKInspectorExtension *))completionHandler

Should there be `nonnull` and `_Nullable`, or is that only needed in the `.h`?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspector.mm:223
> +- (void)unregisterExtension:(_WKInspectorExtension *)extension
completionHandler:(void(^)(NSError *))completionHandler

ditto (:207)

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorDelegate.h:46
> +- (void)inspector:(_WKInspector *)inspector didShowTab:(NSString *)tabID
forExtension:(_WKInspectorExtension *)inspectorExtension;
> +- (void)inspector:(_WKInspector *)inspector didHideTab:(NSString *)tabID
forExtension:(_WKInspectorExtension *)inspectorExtension;

Are these needed for this patch?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:33
> +- (instancetype)init NS_UNAVAILABLE;

NIT: what about `+new`?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtension.h:35
> + at property (readonly, nonatomic) NSString *extensionID;

Will this cause a problem for non-`ENABLE(INSPECTOR_EXTENSIONS)`?

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:183
> +- (void)registerExtensionWithID:(NSString *)extensionID
displayName:(NSString *)displayName completionHandler:(void(^)(NSError *
_Nullable, _WKInspectorExtension * _Nullable))completionHandler

ditto (_WKInspector.mm:207)

> Source/WebKit/UIProcess/API/Cocoa/_WKRemoteWebInspectorViewController.mm:199
> +- (void)unregisterExtension:(_WKInspectorExtension *)extension
completionHandler:(void(^)(NSError * _Nullable))completionHandler

ditto (_WKInspector.mm:207)

>
Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:49
> +    auto callbacks = std::exchange(m_queuedFrontendActions, { });
> +    for (auto& callback : callbacks)
> +	   callback();

Are we sure we want to do this in `~WebInspectorUIExtensionControllerProxy`? 
Doesn't this happen when inside `closeFrontendPageAndWindow`/`closeWindow`?  If
anything I'd expect this to happen before `m_inspectorPage = nullptr;` (unless
we know that every `callback` null-checks `m_inspectedPage` before trying to
access it).

>
Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:68
> +    auto callbacks = std::exchange(m_queuedFrontendActions, { });

ditto (:47)

>
Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:77
> +    whenFrontendHasLoaded([weakThis = makeWeakPtr(this), extensionID,
displayName, completionHandler = WTFMove(completionHandler)] () mutable {

Why `mutable`?

>
Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.cpp:89
> +    whenFrontendHasLoaded([weakThis = makeWeakPtr(this), extensionID,
completionHandler = WTFMove(completionHandler)] () mutable {

ditto (:77)

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:62
> +    // Used to queue actions such as registering extensions that happen
early on.
> +    // There's no point sending these before the frontend is fully loaded.
> +    Vector<Function<void()>> m_queuedFrontendActions;

How about `m_frontendLoadedCallbackQueue`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.h:50
> +class WebInspectorUI : public RefCounted<WebInspectorUI>

Style: I think we should put the `: public RefCounted<WebInspectorUI>` on a
separate line too

NIT: while you're at it, can we make this class `final`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.h:53
> +    , public WebCore::InspectorFrontendClient {

(I'd personally also put the `{` on a separate line too, but I dunno if that's
WebKit style)

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.h:56
> +    ~WebInspectorUI();

I feel like this should be `virtual`, but I could be wrong :|

> Source/WebKit/WebProcess/Inspector/WebInspectorUI.h:159
> +    WebPage& frontendPage() const { return m_page; }

Why is this needed?

IMO `inspectorPage` is a better name, but I could go either way

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:56
> +    Vector<Ref<JSON::Value>> arguments { JSON::Value::create(extensionID),
JSON::Value::create(displayName) };

NIT: I'd just inline this

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:57
> +   
m_inspector->frontendAPIDispatcher().dispatchCommand("registerExtension",
WTFMove(arguments));

`"registerExtension"_s`

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:59
> +    // FIXME: use return value of the evaluation to determine if we
succeeded or failed.

Should we make a bug for this?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:67
> +    Vector<Ref<JSON::Value>> arguments { JSON::Value::create(extensionID) };

ditto (:56)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:68
> +   
m_inspector->frontendAPIDispatcher().dispatchCommand("unregisterExtension",
WTFMove(arguments));

`"unregisterExtension"_s`