Bug 217783 – [Cocoa] Inspector Extensions: Add _WKInspectorExtension and related plumbing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Blaze Burg]

.

[Blaze Burg]

<rdar://problem/69968787>

[Blaze Burg]

Created attachment 411489 [details]
Patch v1

[Blaze Burg]

Created attachment 411493 [details]
Patch v1.1

[Devin Rousso]

Comment on attachment 411493 [details]
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
> +@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`

[Blaze Burg]

Created attachment 411601 [details]
Patch v1.2

[Blaze Burg]

Created attachment 411622 [details]
Patch v1.3

[Blaze Burg]

Created attachment 413110 [details]
Patch v2.0

This has been cleaned up and has API tests. I expect some dodgeball with EWS.

[Blaze Burg]

(In reply to Brian Burg from comment #7)
> Created attachment 413110 [details]
> Patch v2.0
> 
> This has been cleaned up and has API tests. I expect some dodgeball with EWS.

Test failures are likely due to missing files in Test.html, d'oh.

[Devin Rousso]

Comment on attachment 413110 [details]
Patch v2.0

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

r=me, nice work!

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:34
> +

missing `// Public`

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

please remove all `console` other than `console.assert` as they are not stripped from production builds

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:51
> +        if (!this._extensionIDMap.has(extensionID)) {

you could rework this to avoid the double (triple) lookup
```
    let extension = this._extensionIDMap.take(extensionID);
    if (!extension) {
        WI.reportInternalError("Unable to unregister extension with ID: ", extensionID);
        return WI.WebInspectorExtension.ErrorCode.InvalidRequest;
    }

    this.dispatchEventToListeners(WI.WebInspectorExtensionController.Event.ExtensionRemoved, {extension});
```

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:57
> +        let extension = this._extensionIDMap.get(extensionID);
> +        this._extensionIDMap.delete(extensionID);

`this._extensionIDMap.take(extensionID)`

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

ditto (:45)

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

this doesn't need to extend `WI.Object` if it's not dispatching any events

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

Style: missing comma

> Source/WebKit/Shared/InspectorExtensionTypes.cpp:35
> +WTF::String inspectorExtensionErrorToString(InspectorExtensionError error)

NIT: is the `WTF::` necessary?

> Source/WebKit/Shared/InspectorExtensionTypes.cpp:53
> +    return "InternalError";

`_s`

> Source/WebKit/Shared/InspectorExtensionTypes.h:35
> +using InspectorExtensionTabID = WTF::String;
> +using InspectorExtensionID = WTF::String;

NIT: I think the extension ID should go before the tab ID (both alphabetically and conceptually)

> Source/WebKit/Shared/InspectorExtensionTypes.h:38
> +    InternalError,

This doesn't appear to be used anywhere.  Can we defer adding it until it's actually needed?

> Source/WebKit/Shared/InspectorExtensionTypes.h:40
> +    InvalidState,

ditto (:38)

> Source/WebKit/Shared/InspectorExtensionTypes.h:43
> +    ResourceLoadFailed,

ditto (:38)

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:42
> +    , public CanMakeWeakPtr<WebInspectorUIExtensionControllerProxy> {

i dunno what WebKit style is, but I'd personally put the `{` on a newline (unindented) when there are multiple lines for inheritance

> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:60
> +    bool m_frontendLoaded { false };

NIT: can we move this below `m_frontendLoadedCallbackQueue` for (potentially) better packing?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:44
> +    Page* page = inspectorFrontend.frontendPage();
> +    ASSERT(page);

If we expect this to always be valid, can we use `Page&` instead?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:63
> +        if (resultString == "InternalError")

`_s` (below too)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:75
> +        // If the evaluation result is a string, the frontend returned an error string.
> +        // Anything else (falsy values, objects, arrays, DOM, etc.) is interpreted as success.

NIT: I'd move this right before the `if (result.isString()) {`

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:107
> +        completionHandler(true);

If you're only ever sending back `true`, then the `bool` is kinda pointless.  Could you `void(Optional<InspectorExtensionError>)` instead (i.e. `WTF::nullopt` is "no problem")?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:130
> +        completionHandler(true);

ditto (:107)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:49
> +    , public CanMakeWeakPtr<WebInspectorUIExtensionController> {

ditto (WebInspectorUIExtensionControllerProxy.h:42)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:64
> +    Optional<InspectorExtensionError> parseInspectorExtensionErrorFromResult(JSC::JSValue result);

forward declare `JSC::JSValue`?

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:66
> +    WeakPtr<WebCore::InspectorFrontendClient> m_inspectorFrontend;

NIT: shouldn't this really be called `m_inspectorFrontendClient` (or just `m_frontendClient`)?

[Blaze Burg]

Created attachment 413162 [details]
Patch v2.1

[Blaze Burg]

Comment on attachment 413110 [details]
Patch v2.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:45
>> +        console.log("Registered extension: ", extensionID, displayName);
> 
> please remove all `console` other than `console.assert` as they are not stripped from production builds

I'll revert this for now, but we should have a conversation about the intent for release logging. Diagnostic logging is not useful if only errors get logged.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:57
>> +        this._extensionIDMap.delete(extensionID);
> 
> `this._extensionIDMap.take(extensionID)`

Oh, yeah.

>> Source/WebKit/Shared/InspectorExtensionTypes.h:38
>> +    InternalError,
> 
> This doesn't appear to be used anywhere.  Can we defer adding it until it's actually needed?

It's used in the default case of the JSValue.toWTFString switch.

>> Source/WebKit/Shared/InspectorExtensionTypes.h:40
>> +    InvalidState,
> 
> ditto (:38)

ok

>> Source/WebKit/Shared/InspectorExtensionTypes.h:43
>> +    ResourceLoadFailed,
> 
> ditto (:38)

ok

>> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:42
>> +    , public CanMakeWeakPtr<WebInspectorUIExtensionControllerProxy> {
> 
> i dunno what WebKit style is, but I'd personally put the `{` on a newline (unindented) when there are multiple lines for inheritance

That's my preference as well but style bot doesn't agree. And I don't care enough to look into it.

>> Source/WebKit/UIProcess/Inspector/WebInspectorUIExtensionControllerProxy.h:60
>> +    bool m_frontendLoaded { false };
> 
> NIT: can we move this below `m_frontendLoadedCallbackQueue` for (potentially) better packing?

ok

>> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:44
>> +    ASSERT(page);
> 
> If we expect this to always be valid, can we use `Page&` instead?

We don't, because InspectorFrontendClientLocal stores a Page* instead of a Page&. This could probably be ironed out if InspectorFrontendClientLocal is indirectly owned by the Page.

>> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:107
>> +        completionHandler(true);
> 
> If you're only ever sending back `true`, then the `bool` is kinda pointless.  Could you `void(Optional<InspectorExtensionError>)` instead (i.e. `WTF::nullopt` is "no problem")?

I agree. The reason I did this is that every other command I can think of will have a return value, so I want all IPC to use Expected<T,E> as the return type.

In this case there is no return value, so  started with Expected<void, InspectorExtensionError>. This fails due to various problems in encoding/decoding. So a dummy boolean instead.

>> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:64
>> +    Optional<InspectorExtensionError> parseInspectorExtensionErrorFromResult(JSC::JSValue result);
> 
> forward declare `JSC::JSValue`?

ok

>> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.h:66
>> +    WeakPtr<WebCore::InspectorFrontendClient> m_inspectorFrontend;
> 
> NIT: shouldn't this really be called `m_inspectorFrontendClient` (or just `m_frontendClient`)?

OK

[Blaze Burg]

Created attachment 413172 [details]
Patch v2.2

[Blaze Burg]

Created attachment 413193 [details]
Patch v2.3

[Blaze Burg]

Created attachment 413353 [details]
Patch v2.4

[EWS]

Committed r269486: <https://trac.webkit.org/changeset/269486>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413353 [details].

[Truitt Savell]

Reverted r269486 for reason:

Caused 50+ timeouts on Mac Debug WK2

Committed r269524: <https://trac.webkit.org/changeset/269524>

[Blaze Burg]

Created attachment 413849 [details]
Patch v2.5 - for EWS

[EWS]

Committed r269701: <https://trac.webkit.org/changeset/269701>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413849 [details].

[Tim Horton]

Comment on attachment 413849 [details]
Patch v2.5 - for EWS

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

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:46
> +//    WTFReportBacktrace();

Oops, these landed.