Bug 231181 – Web Inspector: add TabBar context menu support for WI.WebInspectorExtensionTabContentView

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

.

[Blaze Burg]

<rdar://74698241>

[Blaze Burg]

Created attachment 440244 [details]
Patch v1.0

[Blaze Burg]

This patch depends on the fix for 230758 (extension iframe should not reload on hide).

[Patrick Angle]

Comment on attachment 440244 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:679
> +    // Returning a null content view will effectively prevent automatic restoration of this tab type.
> +    // Inspector Extension tabs must be created via WebInspectorExtensionController.createTabForExtension().
> +    if (tabClass === WI.WebInspectorExtensionTabContentView)
> +        return null;

Can we assert this?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:70
> +            WI.tabBrowser.closeTabForContentView(tabContentView, {suppressAnimations});

NIT: This can be {supressAnimations: true} instead of a const to keep the value at the call site here.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:90
> +        WI.tabBrowser.addTabForContentView(tabContentView, {suppressAnimations});

Ditto :70

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:209
> +        let didAddSeparator = false;
> +        for (let tabContentView of this._extensionTabContentViewForExtensionTabIDMap.values()) {
> +            let shouldIncludeTab = tabContentView.isHidden || !tabContentView.tabBarItem.parentTabBar;
> +            if (!shouldIncludeTab)
> +                continue;
> +
> +            if (!didAddSeparator) {
> +                didAddSeparator = true;
> +                contextMenu.appendSeparator();
> +            }

This could also be reworked like I suggested below on :220-225, although you'd have to filter the values, which would make this (worst case) O(2n).

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:213
> +                this.showExtensionTab(tabContentView.extensionTabID, {suppressAnimations});

Ditto :70
Also, why suppress these animations, but not the animations for `addContextMenuItemsForAllExtensionTabs`?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:225
> +        let didAddSeparator = false;
> +        for (let tabContentView of this._extensionTabContentViewForExtensionTabIDMap.values()) {
> +            if (!didAddSeparator) {
> +                didAddSeparator = true;
> +                contextMenu.appendSeparator();
> +            }

Might be easier to follow if we just early return if there are no tab content views to list, thus eliminating the need to keep track of if we added a separator yet. 
```
let tabContentViews = this._extensionTabContentViewForExtensionTabIDMap.values();
if (!tabContentViews.length)
    return;

contextMenu.appendSeperator()

for(let tabContentView of tabContentViews( {
...

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:233
> +                    this.showExtensionTab(tabContentView.extensionTabID);
> +                else
> +                    this.hideExtensionTab(tabContentView.extensionTabID);

See :213

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:354
> +        // Return true if the ContentView is closed, or if shouldNotRemoveFromDOMWhenHidden() is true and the ContentView has been hidden.

Most of this comment is redundant with the code, and the detail of `shouldNotRemoveFromDOMWhenHidden` seems like a loose contract at this point, and not necessarily a guarantee going forward.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:355
> +        return this._isClosed || this._isHidden;

The changelog says `Add a flag so that clients can determine when the content view is hidden but not closed.`, but this checks if the content view is hidden or closed. Call sites to isHidden should probably also just check `isClosed` as needed, instead of overloading the getter for this property.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:361
> +        this.element.classList.toggle("hidden-simulating-dom-detached", this._isHidden);

I know I suggested this CSS class name a few days ago, but I'm less thrilled with it now that it isn't so directly tied to the `shouldNotRemoveFromDOMWhenHidden` property.

[Devin Rousso]

Comment on attachment 440244 [details]
Patch v1.0

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

> Source/WebInspectorUI/ChangeLog:22
> +        List WebInspectorExtensionTabContentView as a known tab class. But do not allow
> +        this tab class to be instantiated directly. Extension tabs are intended to be
> +        created using WebInspectorExtensionController.createTabForExtension().

Do we save whether the user has removed the tab or not anywhere?  As a user I wouldn't want an extension tab that I'd removed in a previous Web Inspector session to suddenly show up again the next time (and every time after that) I inspect something.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:192
> +        tabContentView.hidden = true;
> +        WI.tabBrowser.closeTabForContentView(tabContentView, options);

This confuses me a bit.  If the developer has removed an extension tab from the tab bar, why are we keeping the tab alive?  I'd think that removing the tab from the tab bar would throw it away entirely such that adding it back to the tab bar would basically recreate it from scratch?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:207
> +            if (!didAddSeparator) {
> +                didAddSeparator = true;

`WI.ContextMenu` will ignore leading or sequential separators so you shouldn't need to worry about this in those cases.  Is there some other reason why you are doing this?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:223
> +            if (!didAddSeparator) {
> +                didAddSeparator = true;

ditto (:206)

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:352
> +    get isHidden()

Style: this should just be `get hidden` to match the variable name of `set hidden`

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:354
>> +        // Return true if the ContentView is closed, or if shouldNotRemoveFromDOMWhenHidden() is true and the ContentView has been hidden.
> 
> Most of this comment is redundant with the code, and the detail of `shouldNotRemoveFromDOMWhenHidden` seems like a loose contract at this point, and not necessarily a guarantee going forward.

FWIW the builtin DOM `hidden` also doesn't really have anything to do with whether something is attached to the DOM

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:429
> +            return;

Can we flip this and have this be an early-return instead?
```
    if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden() && contentView.hidden)
        return;
```

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:92
> +    static isTabAllowed()

Style: this (and `shouldSaveTab`) should be moved to a `// Static` section above

[Patrick Angle]

Comment on attachment 440244 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:202
> +            let shouldIncludeTab = tabContentView.isHidden || !tabContentView.tabBarItem.parentTabBar;

`isHidden` doesn't seem right here - If I understand correctly, `isHidden` is true when the extension is not the active tab, but here we should just be listing tabs that aren't in the tab bar at all. I think `isClosed` would be more correct?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:227
> +            let checked = !tabContentView.isHidden || !!tabContentView.tabBarItem.parentTabBar;

Ditto :202

[Blaze Burg]

Created attachment 440517 [details]
Patch v1.0.1 (rebased)

[Blaze Burg]

Created attachment 440542 [details]
Patch v1.1

[Patrick Angle]

Comment on attachment 440542 [details]
Patch v1.1

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

LGTM, provisional r+

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:220
> +            let disabled = false;

NIT: `const`

> Source/WebInspectorUI/UserInterface/Views/ContentView.css:2
> + * Copyright (C) 2013â2021 Apple Inc. All rights reserved.

Encoding issue?

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:2
> + * Copyright (C) 2013â2021 Apple Inc. All rights reserved.

Ditto ContentView.css:2

[Blaze Burg]

Comment on attachment 440244 [details]
Patch v1.0

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

>> Source/WebInspectorUI/ChangeLog:22
>> +        created using WebInspectorExtensionController.createTabForExtension().
> 
> Do we save whether the user has removed the tab or not anywhere?  As a user I wouldn't want an extension tab that I'd removed in a previous Web Inspector session to suddenly show up again the next time (and every time after that) I inspect something.

No, the fact that they were hidden is not saved. I think it would be even more confusing to continue hiding extension tabs if inspector has been closed and reopened. Let's live on this and see.

>> Source/WebInspectorUI/UserInterface/Base/Main.js:679
>> +        return null;
> 
> Can we assert this?

Yes

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:192
>> +        WI.tabBrowser.closeTabForContentView(tabContentView, options);
> 
> This confuses me a bit.  If the developer has removed an extension tab from the tab bar, why are we keeping the tab alive?  I'd think that removing the tab from the tab bar would throw it away entirely such that adding it back to the tab bar would basically recreate it from scratch?

Aside from reusing the arguments and praying.. we don't have a way to recreate the extension tab. Moreover the web extension's script contexts (background page, devtools background page) would not have a reference to the recreated extension tab, and thus can't communicate with it. It would just look broken to a user.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:202
>> +            let shouldIncludeTab = tabContentView.isHidden || !tabContentView.tabBarItem.parentTabBar;
> 
> `isHidden` doesn't seem right here - If I understand correctly, `isHidden` is true when the extension is not the active tab, but here we should just be listing tabs that aren't in the tab bar at all. I think `isClosed` would be more correct?

I renamed this to isVisible and now these conditions read much more clearly.

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:352
>> +    get isHidden()
> 
> Style: this should just be `get hidden` to match the variable name of `set hidden`

I renamed it to isVisible and inverted the logic at all use sites to preserve behavior. This naming matches isClosed just above–I'd prefer to extend that naming scheme for getters.

[Devin Rousso]

Comment on attachment 440542 [details]
Patch v1.1

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:220
>> +            let disabled = false;
> 
> NIT: `const`

it actually should just be removed as it's not needed

`appendCheckboxItem` assumes `false` for `disabled` :)

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:354
> +        return !this._isClosed && this._isVisible;

It still feels kinda weird to consult `_isClosed` :/

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:357
> +    set visible(value)

Style: this should either be `set isVisible` or the getter should be changed to `get visible`

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:360
> +        this.element.classList.toggle("content-view-not-visible", !this._isVisible);

NIT: I'd drop the "content-view" as that's already evident since this object always has a `.content-view` class on it anyways

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:428
> +        this.removeSubview(contentView);

I think this may cause assertions depending on how `_disassociateFromContentView` is called (e.g. `replaceContentView`, `closeContentView`, etc.)

[Blaze Burg]

Created attachment 440691 [details]
Patch v1.2.1 (for landing)

[EWS]

Committed r283859 (242737@main): <https://commits.webkit.org/242737@main>

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

[Blaze Burg]

Committed r283921 (242788@main): <https://commits.webkit.org/242788@main>