Bug 166901 – Web Inspector: long press on New Tab Tab Item should show context menu with recently closed tabs that are still closed

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

[Blaze Burg]

Safari's New Tab Tab Item does this.

[Radar WebKit Bug Importer]

<rdar://problem/29952392>

[Devin Rousso]

Created attachment 298553 [details]
Patch

[Devin Rousso]

(In reply to comment #2)
> Created attachment 298553 [details]
> Patch

So this doesn't currently support long press, but I can add it.  I just wanted to know what constitutes a long press.  Is it just a certain timeframe (like 1s) or is it something more specific?

[Blaze Burg]

Comment on attachment 298553 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:80
> +        this._closedTabClasses = [];

Why not a set? It provides stable iteration order based on insertion order, and you don't need to de-dupe it yourself.

[Devin Rousso]

Comment on attachment 298553 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:80
>> +        this._closedTabClasses = [];
> 
> Why not a set? It provides stable iteration order based on insertion order, and you don't need to de-dupe it yourself.

For some reason I thought the JS Set was unordered.  Thanks for the clarification :D

[Devin Rousso]

Created attachment 298596 [details]
Patch

[Blaze Burg]

Comment on attachment 298596 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:77
> +        this._tabBar.addEventListener(WebInspector.TabBar.Event.NewTabContextmenu, this._handleNewTabContextmenu, this);

Is there any reason to track recent tabs in TabBrowser instead of TabBar? It seems unnecessary, maybe I'm missing something.

[Devin Rousso]

Comment on attachment 298596 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:77
>> +        this._tabBar.addEventListener(WebInspector.TabBar.Event.NewTabContextmenu, this._handleNewTabContextmenu, this);
> 
> Is there any reason to track recent tabs in TabBrowser instead of TabBar? It seems unnecessary, maybe I'm missing something.

Most of the other functions go through the TabBrowser, such as adding and removing the content views.  Also, the TabBrowser maintains a list of recent content views, so I figured that this was related.

[Blaze Burg]

Comment on attachment 298596 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:77
>>> +        this._tabBar.addEventListener(WebInspector.TabBar.Event.NewTabContextmenu, this._handleNewTabContextmenu, this);
>> 
>> Is there any reason to track recent tabs in TabBrowser instead of TabBar? It seems unnecessary, maybe I'm missing something.
> 
> Most of the other functions go through the TabBrowser, such as adding and removing the content views.  Also, the TabBrowser maintains a list of recent content views, so I figured that this was related.

Sure, but the TabBar doesn't know anything about content views, just TabBarItems. I think this can be implemented just in terms of TabBarItems. If not, then we'll have to indirect as you did. But I'd prefer to not do that unless necessary.

[Devin Rousso]

Comment on attachment 298596 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:77
>>>> +        this._tabBar.addEventListener(WebInspector.TabBar.Event.NewTabContextmenu, this._handleNewTabContextmenu, this);
>>> 
>>> Is there any reason to track recent tabs in TabBrowser instead of TabBar? It seems unnecessary, maybe I'm missing something.
>> 
>> Most of the other functions go through the TabBrowser, such as adding and removing the content views.  Also, the TabBrowser maintains a list of recent content views, so I figured that this was related.
> 
> Sure, but the TabBar doesn't know anything about content views, just TabBarItems. I think this can be implemented just in terms of TabBarItems. If not, then we'll have to indirect as you did. But I'd prefer to not do that unless necessary.

Either way I would need to use tabBarItem.representedObject, as that is how I can get the Type for the TabContentView class which is needed to open a tab of that type.  TabBar never deals with tabBarItem.representedObject, whereas TabBrowser does.  I think that it keeps consistency to have it handle the Set of closed tabs.  Additionally, the TabBar is an element wrapper with additional UI features (such as animating the tabs) while TabBrowser is more of a controller concept in that it holds the TabBar, sidebars, ContentView objects, and manages what is displayed at any time.  I think it makes more sense to keep this logic here, even though it is slightly uglier.

[Joseph Pecoraro]

Comment on attachment 298596 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:52
> +        this._newTabTabBarItem.element.addEventListener("contextmenu", this._handleNewTabContextmenu.bind(this));

Typo: "Contextmenu" => "ContextMenu"

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:753
> +    NewTabContextmenu: "tab-bar-new-tab-contextmenu",

Typo: "Contextmenu" => "ContextMenu"

>>>>> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:77
>>>>> +        this._tabBar.addEventListener(WebInspector.TabBar.Event.NewTabContextmenu, this._handleNewTabContextmenu, this);
>>>> 
>>>> Is there any reason to track recent tabs in TabBrowser instead of TabBar? It seems unnecessary, maybe I'm missing something.
>>> 
>>> Most of the other functions go through the TabBrowser, such as adding and removing the content views.  Also, the TabBrowser maintains a list of recent content views, so I figured that this was related.
>> 
>> Sure, but the TabBar doesn't know anything about content views, just TabBarItems. I think this can be implemented just in terms of TabBarItems. If not, then we'll have to indirect as you did. But I'd prefer to not do that unless necessary.
> 
> Either way I would need to use tabBarItem.representedObject, as that is how I can get the Type for the TabContentView class which is needed to open a tab of that type.  TabBar never deals with tabBarItem.representedObject, whereas TabBrowser does.  I think that it keeps consistency to have it handle the Set of closed tabs.  Additionally, the TabBar is an element wrapper with additional UI features (such as animating the tabs) while TabBrowser is more of a controller concept in that it holds the TabBar, sidebars, ContentView objects, and manages what is displayed at any time.  I think it makes more sense to keep this logic here, even though it is slightly uglier.

I agree with Brian that conceptually it makes sense for TabBar to have this logic. If we created a 2nd tab bar that had a NewTab button we would want the same behavior, without depending on a 2nd body. At the very least, the list of recently closed tabs could be known by TabBar and not require a controller.

That said, I think the logic of whether or not a tab is allowed (WebInspector.isNewTabWithTypeAllowed) is logic that should be separate of the TabBar (though it could be a delegate). So I'm fine with the current implementation because it does this in TabBrowser.

We can revisit this if we ever use TabBar in another place.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:290
> +        let tabClassesToDisplay = closedTabClasses.concat(allTabClasses.filter((tabClass) => !closedTabClasses.includes(tabClass) && WebInspector.isNewTabWithTypeAllowed(tabClass.Type) && !tabClass.isEphemeral()));

This one line is too complex to be one line. I suggest breaking it up:

    let closedTabClasses = ...;
    let allTabClasses = ...;
    let otherTabClassesToDisplay = allTabClasses.filter((tabClass) => {
        if (closedTabClasses.includes(tabClass))
            return false;
        if (tabClass.isEphemeral())
            return false;
        return WebInspector.isNewTabWithTypeAllowed(tabClass.Type);
    });

    let tabClassesToDisplay = closedTabClasses.concat(otherTabClassesToDisplay);

[Devin Rousso]

Created attachment 299533 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 299533 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:753
> +    NewTabContextMenu: "tab-bar-new-tab-contextmenu",

Nit: This could be context-menu on the right, but this is fine.

[WebKit Commit Bot]

Comment on attachment 299533 [details]
Patch

Clearing flags on attachment: 299533

Committed r211064: <http://trac.webkit.org/changeset/211064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.