Bug 219380 – [Cocoa] Web Inspector: add support for creating extension tabs in WebInspectorUI via _WKInspectorExtension

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Blaze Burg]

<rdar://69594496>

[Blaze Burg]

Created attachment 415098 [details]
Patch v1 (depends on 219378)

[Blaze Burg]

Created attachment 415099 [details]
Consolidated v1 (for EWS)

[Devin Rousso]

Comment on attachment 415098 [details]
Patch v1 (depends on 219378)

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

> Source/WebKit/ChangeLog:9
> +        WebInpectorUI interface. This can be used to implement browser.devtools.panels.create()

NIT: "UI interface" is redundant 😅

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:33
> +        // Map from extensionID -> WI.WebInspectorExtension.
>          this._extensionIDMap = new Map;

rather than add a comment, I'd rename this to `_extensionForExtensionIDMap`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:35
> +        // Map from extensionTabID -> WI.WebInspectorExtensionTabContentView.
> +        this._extensionTabsMap = new Map;

ditto (:32) as `_extensionTabContentViewForExtensionIDMap`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:37
> +        // Multimap from extensionID -> {extensionTabID, ...}.
> +        this._activeTabsForExtensionMap = new Multimap;

ditto (:32) as `_tabIDsforExtensionIDMap`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:41
> +    _makeNextExtensionTabID()

NIT: I don't think this needs to be a separate function.  I'd just inline it as `let extensionTabID = this._nextExtensionTabID++;`

Style: if you do keep this, it should go below after `// Private`.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:71
> +        let extensionTabIDsToRemove = this._activeTabsForExtensionMap.take(extensionID);
> +        for (let extensionTabID of extensionTabIDsToRemove) {
> +            let tabContentView = this._extensionTabsMap.take(extensionTabID);

I think we can combine `_activeTabsForExtensionMap` and `_extensionTabsMap` a single `MultiMap` of `[extensionID, [WI.WebInspectorExtensionTabContentView, ...]]` since `WI.TabContentView`.  You don't appear to be using the `identifier` of the tab for anything other than retrieving from this second map anyways.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:93
> +        return {extensionTabID};

Why create an object if there's only one key?

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.css:26
> +.content-view.web-inspector-extension-tab > iframe {

NIT: I'd add `.tab` to match what we do with the CSS for other tabs, so `.content-view.tab.web-inspector-extension`

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:31
> +            identifier: WebInspectorExtensionTabContentView.Type,

missing `WI.`

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:36
> +        super(tabInfo, {});

you can drop the `{}`

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:61
> +    get extensionTabID()

Style: you can inline this as `get extensionTabID() { return this._extensionTabID; }`

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:73
> +        iframeElement.src = this._sourceURL;

Are there any other attributes we need to set before we load the extension's code so that it can't reach out to the main Web Inspector context (i.e. something akin to XSS)?

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:77
> +WI.WebInspectorExtensionTabContentView.Type = "web-inspector-extension-tab";

I think this is going to have issues with `WI._openTabsSetting` as if there are multiple extension tabs then they'd all have the same `.Type`.  Speaking of, how does this deal with saving/restoring of tabs (or is that a planned followup)?

Also, we don't normally include `-tab`.

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

Should this be `private` since we have `create`?

> Source/WebKit/UIProcess/API/Cocoa/_WKInspectorExtensionInternal.h:52
> +- (instancetype)initWithIdentifier:(NSString *)extensionIdentifier forLocalInspector:(_WKInspector *)localInspector;
> +- (instancetype)initWithIdentifier:(NSString *)extensionIdentifier forRemoteInspector:(_WKRemoteWebInspectorViewController *)localInspector;

Can we have only one like `initWithIdentifier:controller:` that asks for the `WebInspectorUIExtensionControllerProxy&` instead?  I feel like local vs remote is an implementation detail that `_WKInspectorExtension` doesn't really need to know about.

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:157
> +    if (!result.has_value())

I don't think the `has_value` is needed as `operator bool` does the same thing

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:161
> +    if (!valueOrException.has_value())

ditto (:157)

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:165
> +    JSC::JSValue value = valueOrException.value();   
> +    return value.getObject();

NIT: I'd inline this as `valueOrException.value().getObject()`

> Source/WebKit/WebProcess/Inspector/WebInspectorUIExtensionController.cpp:180
> +    Vector<Ref<JSON::Value>> arguments {
> +        JSON::Value::create(extensionID),
> +        JSON::Value::create(tabName),
> +        JSON::Value::create(tabIconURL.string()),
> +        JSON::Value::create(sourceURL.string()),
> +    };

Aside: I'd love it if there was a convenience function for this :D (maybe a weekend project to have some fun with C++)

[Blaze Burg]

Created attachment 415489 [details]
Patch v2

In this iteration, I rebased and addressed Devin's comments.

[Blaze Burg]

Created attachment 415533 [details]
Patch v2.1 - for iOS EWS

[Blaze Burg]

Investigating EWS API test failures, failure message below:


Test suite failed
Failed
    TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        
        /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:136
        Value of: !(error)
          Actual: false
        Expected: true
        
        
        /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:152
        Value of: !(error)
          Actual: false
        Expected: true

[Blaze Burg]

(In reply to Brian Burg from comment #7)
> Investigating EWS API test failures, failure message below:
> 
> 
> Test suite failed
> Failed
>     TestWebKitAPI.WKInspectorExtensionHost.UnregisterExtension
>         _RegisterApplication(), FAILED TO establish the default connection
> to the WindowServer, _CGSDefaultConnection() is NULL.
>         _RegisterApplication(), FAILED TO establish the default connection
> to the WindowServer, _CGSDefaultConnection() is NULL.
>         
>        
> /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/
> TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:136
>         Value of: !(error)
>           Actual: false
>         Expected: true
>         
>         
>        
> /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/
> TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtensionHost.mm:152
>         Value of: !(error)
>           Actual: false
>         Expected: true

It's great when my test catches a problem. It's hilarious when I also forget to remove the intentional defect I used to ensure it was actually working >_<. New patch coming.

[Blaze Burg]

Created attachment 415691 [details]
Patch v2.2 - fix API test failure

[Devin Rousso]

Comment on attachment 415691 [details]
Patch v2.2 - fix API test failure

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

r=me as well :)

> Source/WebKit/ChangeLog:30
> +        Expose member m_remoteInspectorPorxy for use in the API:::InspectorExtension constructor.

extra ":"

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:85
> +        return {extensionTabID};

I'd add a comment here explaining that you need to use an object so that it's differentiable from `WI.WebInspectorExtension.ErrorCode.*` (which is a string).

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:218
> +    }

Style: missing comma

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:135
> +        return this.constructor.tabInfo();

I'd just make all the existing `static tabInfo` into instance functions instead of having this extra indirection.

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.css:36
> +    /* This is required for the iframe to expand if its intrinsic size
> +       is smaller than the tab content view. */

NIT: I'd rather have this be a longer line or have a `/*` and `*/` on each line.

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:48
> +        return Object.shallowCopy(this._tabInfo);

Why do we need to copy?  Is this to just match how existing `tabInfo` do it?

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:61
> +    get extensionTabID() { return this._extensionTabID; }

Style: we normally put single-line `get` at the beginning of `// Public`

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:74
> +WI.WebInspectorExtensionTabContentView.Type = "web-inspector-extension-tab";

you really should drop the `-tab` so that we don't have the redundant `.tab.web-inspector-extension-tab` :/

Also, I think this is going to have issues with `WI._openTabsSetting` as if there are multiple extension tabs then they'd all have the same `.Type`.  For now, I'd consider adding `static shouldSaveTab() { return false; }` to this class or skipping any `tabType === WI.WebInspectorExtensionTabContentView.Type` inside `WI._rememberOpenTabs`.  I'd also recommend filing a followup bug for making sure that the position of extension tabs are remembered across Web Inspector sessions.

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

we should make this `private` since we have a `create`

[Blaze Burg]

Committed r270606: <https://trac.webkit.org/changeset/270606>