Bug 234115 – Web Inspector: save and restore extension tab positions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

.

[Blaze Burg]

<rdar://85560636>

[Blaze Burg]

Created attachment 446791 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 446791 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:119
> +        let options = {

NIT: please inline this

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:204
> +        options.insertionIndex = this._insertionIndexForExtensionTab(tabContentView);

please inline this below the `...options`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:320
> +        let savedTabPositions = await WI.objectStores.general.get(WebInspectorExtensionController.extensionTabPositionsObjectStoreKey);

Aside: While this is fine, why can't we create a `WI.Setting` inside `WI.WebInspectorExtensionTabContentView` so that each extension tab keeps track of its own position?  IMO `WI.Setting` is way easier to deal with than `WI.ObjectStore` and should be preferred when the data being stored is simple.  I realize that the position info is a JSON object (and one that could have new keys added in the future), but I think it's maybe simple enough to not be that worried about the cost (especially since it's unlikely that a developer will have more than say a dozen extensions).

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:329
> +        let visibleTabs = WI.tabBrowser.tabBar.tabBarItems.filter((tabBarItem) => !tabBarItem.hidden && !(tabBarItem instanceof WI.PinnedTabBarItem));

Can we create a `WI.TabBar.prototype.get visibleTabBarItems` so that the logic dealing with `WI.PinnedTabBarItem` is kept within `WI.TabBar`?  Also, why does this need to care about `WI.PinnedTabBarItem` in the first place?

NIT: this should be named `visibleTabBarItems` since it's an array of `WI.TabBarItem`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:334
> +            let {anchorTabType, baseIndex, relativeIndex} = this._computeIndicesForExtensionTab(tabBarItem.representedObject, {recomputePositions: true});

We should move all of this logic inside the `Debouncer` so that we're not `_computeIndicesForExtensionTab` on every call.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:338
> +        // Save to the object store five seconds after the last change.

IMO this is an unnecessary comment just by reading the subsequent code

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:340
> +        if (!this._saveTabPositionsToObjectStoreTimeout) {
> +            this._saveTabPositionsToObjectStoreTimeout = new Debouncer(() => {

```
    this._saveTabPositionsDebouncer ||= new Debouncer(() => {
        ...
    });
    this._saveTabPositionsDebouncer.delayForTime(5000);
```

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:354
> +    _computeIndicesForExtensionTab(tabContentView, options = {})

Why not `{recomputePositions} = {}` instead of having to `options.recomputePositions` multiple times below?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:358
> +        let visibleTabs = WI.tabBrowser.tabBar.tabBarItems.filter((tabBarItem) => !tabBarItem.hidden && !(tabBarItem instanceof WI.PinnedTabBarItem));

ditto (:329)

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:359
> +        let baseIndex = 0;

Correct me if I'm wrong, but this makes me think that new extension tabs will be inserted right after the first tab.  That seems kinda odd.  Why aren't we adding them at the end of the tab bar?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:364
> +        for (let i = 0; i < visibleTabs.length; i++) {

`++i` instead of `i++`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:366
> +            // Stop looking once we've found the target tab.

this comment is unnecessary

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:371
> +            // Skip past extension tabs.
> +            if (!visibleTab.constructor.shouldSaveTab())

This code does not match the comment.  Should it be `visibleTab instanceof WI.WebInspectorExtensionTabContentView` instead?  Or maybe just drop the comment?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:374
> +            // We've found the next anchor tab.

ditto (:366)

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:379
> +            // If looking for savedPositions.anchorTabType, keep looking.

ditto (:366)

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:380
> +            else if (visibleTab.type !== anchorTabType)

Style: an `else` is not needed after an `if` that has a `continue`/`break`/`return`/etc.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:388
> +        // Adding `relativeIndex` to `baseIndex` should not insert the tab after a different anchor tab.

Can we come up with better names for these?  How about `anchorTabIndex` and `distanceFromAnchorTab`?  Also, is it actually really necessary to even have a `anchorTabIndex` given that we don't allow for more than one instance of any single `anchorTabType` (i.e. if you have an `anchorTabType` you could just search for the index of a tab that matches it)?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:389
> +        for (let j = 1; j < visibleTabs.length - baseIndex; j++) {

Why doesn't this start at `0`?  What if the `anchorTabIndex` (a.k.a. `baseIndex`) is `0`?  I think in that case the below logic would result in `distanceFromAnchorTab` (a.k.a. `relativeIndex`) also being `0` instead of `1`.

Style: `i` instead of `j`
Style: `++i` instead of `j++`

[Patrick Angle]

Comment on attachment 446791 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:123
> +        let options = {
> +            suppressAnimations: true,
> +            insertionIndex: this._insertionIndexForExtensionTab(tabContentView),
> +        };
> +        WI.tabBrowser.addTabForContentView(tabContentView, options);

NIT: I would just inline `options`.
```
WI.tabBrowser.addTabForContentView(tabContentView, {
    suppressAnimations: true,
    insertionIndex: this._insertionIndexForExtensionTab(tabContentView),
});
```

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:324
> +    async _saveExtensionTabPositions()

Am I missing a reason for this to be async?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:371
> +            // Skip past extension tabs.
> +            if (!visibleTab.constructor.shouldSaveTab())

If we truly want to check if this is an extension tab, shouldn't we check the type of visibleTab instead? Or do we really mean to skip based on `shouldSaveTab()` and update the comment instead?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:399
> +        // If the anchor tab is now hidden upon restoring, place the extension at the end.
> +        // This could happen if a smaller set of tabs are enabled for the inspection target.
> +        baseIndex = visibleTabs.length - 1;
> +        return {anchorTabType, baseIndex, relativeIndex};

Does this also mean that the position of extension tabs could be lost if the developer switches frequently between inspecting JS-only contexts, like ServiceWorkers, and web pages?

[Devin Rousso]

Comment on attachment 446791 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:399
>> +        return {anchorTabType, baseIndex, relativeIndex};
> 
> Does this also mean that the position of extension tabs could be lost if the developer switches frequently between inspecting JS-only contexts, like ServiceWorkers, and web pages?

Actually, speaking of, what do extension tabs even do when inspecting non-`WebPage` debuggables?  AFAIK the debuggable type isn't exposed to the extension in any way, so unless they do some injected JS testing (e.g. `globalThis.constructor.name === "ServiceWorkerGlobalScope") the extension could be wrongly assuming that it's dealing with a `WebPage`.

[Blaze Burg]

Comment on attachment 446791 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:123
>> +        WI.tabBrowser.addTabForContentView(tabContentView, options);
> 
> NIT: I would just inline `options`.
> ```
> WI.tabBrowser.addTabForContentView(tabContentView, {
>     suppressAnimations: true,
>     insertionIndex: this._insertionIndexForExtensionTab(tabContentView),
> });
> ```

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:204
>> +        options.insertionIndex = this._insertionIndexForExtensionTab(tabContentView);
> 
> please inline this below the `...options`

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:320
>> +        let savedTabPositions = await WI.objectStores.general.get(WebInspectorExtensionController.extensionTabPositionsObjectStoreKey);
> 
> Aside: While this is fine, why can't we create a `WI.Setting` inside `WI.WebInspectorExtensionTabContentView` so that each extension tab keeps track of its own position?  IMO `WI.Setting` is way easier to deal with than `WI.ObjectStore` and should be preferred when the data being stored is simple.  I realize that the position info is a JSON object (and one that could have new keys added in the future), but I think it's maybe simple enough to not be that worried about the cost (especially since it's unlikely that a developer will have more than say a dozen extensions).

I decided to use WI.ObjectStore because of the async API.

I don't think it makes sense for the tab itself to compute and store its position, because the controller is responsible for adding it to the tab bar.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:324
>> +    async _saveExtensionTabPositions()
> 
> Am I missing a reason for this to be async?

ooops

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:329
>> +        let visibleTabs = WI.tabBrowser.tabBar.tabBarItems.filter((tabBarItem) => !tabBarItem.hidden && !(tabBarItem instanceof WI.PinnedTabBarItem));
> 
> Can we create a `WI.TabBar.prototype.get visibleTabBarItems` so that the logic dealing with `WI.PinnedTabBarItem` is kept within `WI.TabBar`?  Also, why does this need to care about `WI.PinnedTabBarItem` in the first place?
> 
> NIT: this should be named `visibleTabBarItems` since it's an array of `WI.TabBarItem`

It is allowed for PinnedTabBarItem to have an empty representedObject, which is problematic later. So I filter these out.

I will rename and add the getter.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:338
>> +        // Save to the object store five seconds after the last change.
> 
> IMO this is an unnecessary comment just by reading the subsequent code

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:340
>> +            this._saveTabPositionsToObjectStoreTimeout = new Debouncer(() => {
> 
> ```
>     this._saveTabPositionsDebouncer ||= new Debouncer(() => {
>         ...
>     });
>     this._saveTabPositionsDebouncer.delayForTime(5000);
> ```

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:354
>> +    _computeIndicesForExtensionTab(tabContentView, options = {})
> 
> Why not `{recomputePositions} = {}` instead of having to `options.recomputePositions` multiple times below?

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:359
>> +        let baseIndex = 0;
> 
> Correct me if I'm wrong, but this makes me think that new extension tabs will be inserted right after the first tab.  That seems kinda odd.  Why aren't we adding them at the end of the tab bar?

They are added to the end, the value of baseIndex gets fixed up if we didn't find the anchorTabType.

>>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:371
>>> +            if (!visibleTab.constructor.shouldSaveTab())
>> 
>> This code does not match the comment.  Should it be `visibleTab instanceof WI.WebInspectorExtensionTabContentView` instead?  Or maybe just drop the comment?
> 
> If we truly want to check if this is an extension tab, shouldn't we check the type of visibleTab instead? Or do we really mean to skip based on `shouldSaveTab()` and update the comment instead?

Fixed

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:380
>> +            else if (visibleTab.type !== anchorTabType)
> 
> Style: an `else` is not needed after an `if` that has a `continue`/`break`/`return`/etc.

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:388
>> +        // Adding `relativeIndex` to `baseIndex` should not insert the tab after a different anchor tab.
> 
> Can we come up with better names for these?  How about `anchorTabIndex` and `distanceFromAnchorTab`?  Also, is it actually really necessary to even have a `anchorTabIndex` given that we don't allow for more than one instance of any single `anchorTabType` (i.e. if you have an `anchorTabType` you could just search for the index of a tab that matches it)?

Name suggestions are fine.

This is the place where the anchorTabType is resolved into a real index. I don't want multiple callsites to do the resolution. It's also valid for anchorTabType to be missing or null, so a simple indexOf will not do the right thing.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:389
>> +        for (let j = 1; j < visibleTabs.length - baseIndex; j++) {
> 
> Why doesn't this start at `0`?  What if the `anchorTabIndex` (a.k.a. `baseIndex`) is `0`?  I think in that case the below logic would result in `distanceFromAnchorTab` (a.k.a. `relativeIndex`) also being `0` instead of `1`.
> 
> Style: `i` instead of `j`
> Style: `++i` instead of `j++`

baseIndex=0 relativeIndex=0 means the tab is before any anchor tabs.

This is a lookahead (starting at i=1) because we know that i=0 is always a valid anchor position. I would rather subtract 1 from j rather than adding 1 to j to get a valid insertionIndex.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:399
>> +        return {anchorTabType, baseIndex, relativeIndex};
> 
> Does this also mean that the position of extension tabs could be lost if the developer switches frequently between inspecting JS-only contexts, like ServiceWorkers, and web pages?

Yes, if it's anchored to something like Audits, but I don't think this is a common use case. And extension tabs don't currently work with remote inspection, so it seems moot for now. When switching from webpage to JSContext inspection, It is possible for the anchorTabType to not be found, and in that case we put the extension at the very end and save it relative to a new anchorTabType that appears in both webpage and JSContext inspection.

[Blaze Burg]

Created attachment 446823 [details]
Patch v1.1

[Devin Rousso]

Comment on attachment 446791 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:329
>>> +        let visibleTabs = WI.tabBrowser.tabBar.tabBarItems.filter((tabBarItem) => !tabBarItem.hidden && !(tabBarItem instanceof WI.PinnedTabBarItem));
>> 
>> Can we create a `WI.TabBar.prototype.get visibleTabBarItems` so that the logic dealing with `WI.PinnedTabBarItem` is kept within `WI.TabBar`?  Also, why does this need to care about `WI.PinnedTabBarItem` in the first place?
>> 
>> NIT: this should be named `visibleTabBarItems` since it's an array of `WI.TabBarItem`
> 
> It is allowed for PinnedTabBarItem to have an empty representedObject, which is problematic later. So I filter these out.
> 
> I will rename and add the getter.

FYI all `WI.TabBarItem` are allowed to not provide a `representedObject`.  That is not something unique to `WI.PinnedTabBarItem`.  The only difference between `WI.PinnedTabBarItem` and `WI.GeneralTabBarItem` is whether it's always shown in the `WI.TabBar` or not.

[Patrick Angle]

Comment on attachment 446823 [details]
Patch v1.1

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:323
> +    async _saveExtensionTabPositions()

Still `async`?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:334
> +        for (let tabBarItem of WI.tabBrowser.tabBar.visibleTabBarItems) {
> +            if (!(tabBarItem.representedObject instanceof WI.WebInspectorExtensionTabContentView))
> +                continue;
> +
> +            let {anchorTabType, anchorTabIndex, distanceFromAnchorTab} = this._computeIndicesForExtensionTab(tabBarItem.representedObject, {recomputePositions: true});
> +            this._extensionTabPositions[tabBarItem.representedObject.savedTabPositionKey] = {anchorTabType, distanceFromAnchorTab};
> +        }

Following up on Devin's comment from the previous patch this should still probably be moved inside the Debouncer below so that we only do the work when we actually intend to save, not a few times leading up to actually saving.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:359
> +        for (let i = 0; i < visibleTabBarItems.length; i++) {

++i

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:381
> +        for (let j = 1; j < visibleTabBarItems.length - anchorTabIndex; j++) {

Can we use `i` instead since this isn't a nested loop? Also `++i`

[Devin Rousso]

Comment on attachment 446823 [details]
Patch v1.1

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

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

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:356
> +        let anchorTabType = !recomputePositions ? savedPositions.anchorTabType : null;
> +        let distanceFromAnchorTab = !recomputePositions ? savedPositions.distanceFromAnchorTab : 0;

Should these also check that `savedPosition` is valid?
```
    let savedPosition = this._extensionTabPositions[tabContentView.savedTabPositionKey];
    let anchorTabType = (recomputePositions && savedPosition.anchorTabType) || null;
    let distanceFromAnchorTab = (recomputePositions & savedPosition.distanceFromAnchorTab) || 0;
```

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:368
> +            if (recomputePositions) {
> +                anchorTabType = visibleTab.type;

Doesn't this mean that every extension tab will match to the leftmost non-extension tab when recomputing positions?  I would think that the anchor would be the closest tab before the extension tab.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:412
> +         return this._tabBarItems.filter((item) => !item.hidden && !(item instanceof WI.PinnedTabBarItem));

ditto (comment #8)

Also, I think you may want to use `this._tabBarItemsFromLeftToRight` (and if so please rename this to `visibleTabBarItemsFromLeftToRight`) 🤔

[Blaze Burg]

Comment on attachment 446823 [details]
Patch v1.1

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:323
>> +    async _saveExtensionTabPositions()
> 
> Still `async`?

Ok

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:334
>> +        }
> 
> Following up on Devin's comment from the previous patch this should still probably be moved inside the Debouncer below so that we only do the work when we actually intend to save, not a few times leading up to actually saving.

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:356
>> +        let distanceFromAnchorTab = !recomputePositions ? savedPositions.distanceFromAnchorTab : 0;
> 
> Should these also check that `savedPosition` is valid?
> ```
>     let savedPosition = this._extensionTabPositions[tabContentView.savedTabPositionKey];
>     let anchorTabType = (recomputePositions && savedPosition.anchorTabType) || null;
>     let distanceFromAnchorTab = (recomputePositions & savedPosition.distanceFromAnchorTab) || 0;
> ```

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:359
>> +        for (let i = 0; i < visibleTabBarItems.length; i++) {
> 
> ++i

OK

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:368
>> +                anchorTabType = visibleTab.type;
> 
> Doesn't this mean that every extension tab will match to the leftmost non-extension tab when recomputing positions?  I would think that the anchor would be the closest tab before the extension tab.

Initially yes, but it keeps getting updated with later anchorTabTypes as the search continues.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:381
>> +        for (let j = 1; j < visibleTabBarItems.length - anchorTabIndex; j++) {
> 
> Can we use `i` instead since this isn't a nested loop? Also `++i`

OK

>> Source/WebInspectorUI/UserInterface/Views/TabBar.js:412
>> +         return this._tabBarItems.filter((item) => !item.hidden && !(item instanceof WI.PinnedTabBarItem));
> 
> ditto (comment #8)
> 
> Also, I think you may want to use `this._tabBarItemsFromLeftToRight` (and if so please rename this to `visibleTabBarItemsFromLeftToRight`) 🤔

OK

[Blaze Burg]

Created attachment 446834 [details]
Patch v1.2

Addressed all outstanding feedback in v1.2

[Patrick Angle]

Comment on attachment 446834 [details]
Patch v1.2

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:338
> +        }, 5000);

NIT: Debouncer doesn't take a time parameter in its constructor. (Leftover from a timeout-based approach?)

[Devin Rousso]

Comment on attachment 446834 [details]
Patch v1.2

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

r=me, nice work!

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:360
> +            if (!representedObject)

Did you mean `visibleTab`?

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:370
> +                anchorTabType = visibleTab.type;

This assumes that the `representedObject` has a type.  It's probably not an issue, but you might wanna add a `|| null` just in case.

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:383
> +        for (let i = 1; i < visibleTabBarItems.length - anchorTabIndex; ++j) {

oops `++j` instead of `++i`

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:384
> +            if (visibleTabBarItems[anchorTabIndex + i].representedObject.constructor.shouldSaveTab()) {

This should check `representedObject` (and `constructor` and `shouldSaveTab` just in case the `representedObject` is not a `WI.TabContentView`).
```
    if (visibleTabBarItems[anchorTabIndex + i].representedObject?.constructor?.shouldSaveTab?.()) {
```

[Blaze Burg]

Comment on attachment 446834 [details]
Patch v1.2

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:338
>> +        }, 5000);
> 
> NIT: Debouncer doesn't take a time parameter in its constructor. (Leftover from a timeout-based approach?)

Haha, it used to be a Throttler.

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:384
>> +            if (visibleTabBarItems[anchorTabIndex + i].representedObject.constructor.shouldSaveTab()) {
> 
> This should check `representedObject` (and `constructor` and `shouldSaveTab` just in case the `representedObject` is not a `WI.TabContentView`).
> ```
>     if (visibleTabBarItems[anchorTabIndex + i].representedObject?.constructor?.shouldSaveTab?.()) {
> ```

> shouldSaveTab?.()

is that really the syntax? Yikes.. (-:

[Blaze Burg]

Created attachment 446845 [details]
Patch v1.3

[Blaze Burg]

Committed r286887 (245116@trunk): <https://commits.webkit.org/245116@trunk>