Bug 183653 – Web Inspector: TabBar redesign: consolidate TabBar logic in controller classes

• Status: NEW
• Component: Web Inspector

[Matt Baker]

Summary:
Consolidate TabBar logic in controller classes. TabBar code is spread throughout Main.js, which complicates conditionally switching between the legacy and experimental UI.

This patch will add TabBarController and LegacyTabBarController. These will perform the supervisory TabBar duties currently in Main.js. This includes remembering open tabs, creating tabs of the correct type from the list of known tabs, and persisting the selection. WI.contentLoaded can check the value of WI.settings.experimentalEnableNewTabBar once, and create the correct controller.

[Radar WebKit Bug Importer]

<rdar://problem/38490488>

[Matt Baker]

Created attachment 337028 [details]
Patch

[Matt Baker]

Created attachment 337561 [details]
Patch

[EWS Watchlist]

Comment on attachment 337561 [details]
Patch

Attachment 337561 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7261089

New failing tests:
imported/w3c/web-platform-tests/workers/name-property.html

[EWS Watchlist]

Created attachment 337569 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Nikita Vasilyev]

Comment on attachment 337561 [details]
Patch

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

Looks good! Works as expected, too.

> Source/WebInspectorUI/ChangeLog:45
> +        (WI.TabBarController.isTabTypeAllowed):

This changelog item doesn't look right :/
This isn't a static method in the actual code.

> Source/WebInspectorUI/ChangeLog:56
> +        (WI.TabBarController._tryToRestorePendingTabs):

Neither this one.

> Source/WebInspectorUI/UserInterface/Base/Main.js:335
> +    this._tabBarController.initialize();

If initialize called only here, can we move all its code to constructor?

[Matt Baker]

Comment on attachment 337561 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:335
>> +    this._tabBarController.initialize();
> 
> If initialize called only here, can we move all its code to constructor?

I think there will come a time when separating construction from initialization will be beneficial, but now is not that time. :) I'll change this.

[Matt Baker]

Created attachment 337930 [details]
Patch

[Matt Baker]

(In reply to Matt Baker from comment #8)
> Created attachment 337930 [details]
> Patch

Manually fixed the spurious changelog entries. Not sure why prepare-Changelog would generate that.

[Matt Baker]

Created attachment 337934 [details]
Patch

[EWS Watchlist]

Comment on attachment 337934 [details]
Patch

Attachment 337934 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7310791

New failing tests:
animations/needs-layout.html

[EWS Watchlist]

Created attachment 337937 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Comment on attachment 337934 [details]
Patch

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

This is a really great start.  I have some general architectural comments inline, and I'd like to maybe think them through before we commit to any course of action.

I realize that the new tab bar is somewhat "experimental", but how long are we planning on keeping it around?  The functionality change is pretty minor (no New Tab tab, contextmenu open/close, etc.), so should we instead consider just dropping the old version?

> Source/WebInspectorUI/ChangeLog:32
> +        (WI.LegacyTabBarController.prototype.initialize):
> +        Initialization is kept separate from construction. This follows the
> +        precedent set by AppControllerBase.

I recognize that this was created then, but I don't really see the need for it.  I'd say we remove `initialize` until its really necessary (if ever).

> Source/WebInspectorUI/UserInterface/Base/Main.js:479
>  WI.showNewTabTab = function(options)

This is now only used by `WI.LegacyTabBarController`, so I think we should move it into that file as well.

> Source/WebInspectorUI/UserInterface/Base/Main.js:491
>  WI.createNewTabWithType = function(tabType, options = {})

Since `WI.NewTabContentView` has access to a `WI.TabBarController`, we should make this a member function of that instead of globally on `WI`.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2529
> +    this.tabBrowser.addTabForContentView(tabContentView, options);

I feel like instead of having the `WI.TabBarController` just deal with the `WI.TabBar`, we might want to create a sort of "UIController" that handles the TabBar and TabBrowser all in one, so we don't add delegates and "pollute" the `WI` namespace any further.

> Source/WebInspectorUI/UserInterface/Controllers/TabBarController.js:26
> +WI.TabBarController = class TabBarController extends WI.Object

I'm pretty sure this doesn't need to extend from `WI.Object`.  You don't have any event listeners on it.

> Source/WebInspectorUI/UserInterface/Controllers/TabBarController.js:40
> +        this._showTabAtIndexKeyboardShortcuts = [1, 2, 3, 4, 5, 6, 7, 8, 9].map((i) => new WI.KeyboardShortcut(WI.KeyboardShortcut.Modifier.CommandOrControl | WI.KeyboardShortcut.Modifier.Option, `${i}`, this._showTabAtIndex.bind(this, i)));

You could just make `_showTabAtIndex` inline here, so we can avoid the binding of `i`.

> Source/WebInspectorUI/UserInterface/Controllers/TabBarController.js:54
> +    knownTabClasses()

Is there a reason for making this a function instead of a getter?

> Source/WebInspectorUI/UserInterface/Controllers/TabBarController.js:61
> +        this._tabBar = new WI.TabBar(document.getElementById("tab-bar"), this);

A lot of other controllers seem to take in a their "view" as a constructor parameter, rather than creating it themselves.  I'm not sure what the pattern is here, but I think that "hiding" the `WI.TabBar` inside its own controller seems a bit odd, not to mention it makes some interactions with Main.js and `WI.TabBrowser` a bit more complex.  I'll defer to whatever pattern there already is (assuming it exists), but I think we should create the `WI.TabBar` inside Main.js and pass it into the controller.

> Source/WebInspectorUI/UserInterface/Controllers/TabBarController.js:73
> +        window.addEventListener("pagehide", this._pageHidden.bind(this));
> +        window.addEventListener("resize", this._windowResized.bind(this));

Adding these here instead of in Main.js is odd, especially since we already have a "resize" listener.

See above (61)

> Source/WebInspectorUI/UserInterface/Views/NewTabContentView.js:28
> +    constructor(tabBarController)

It seems weird to me that we pass a controller into a view, especially since we really only use it for getters, but I can't really think of a better way around it other than to just recreate the `WI.___` functions.  If you think this is cleaner than the "old" approach, then thats fine.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:28
> +    constructor(element, delegate)

It looks as though this `delegate` is only used for the contextmenu call.  Is there a way to rework it such that we don't need it?

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:91
> +        if (tabBarItem instanceof WI.PinnedTabBarItem)
> +            tabBarItem.element.addEventListener("mouseenter", this._handlePinnedTabMouseEnter.bind(this));

NIT: I realize that this shouldn't ever happen, but if we somehow add the same `WI.PinnedTabBarItem` to two different `WI.TabBar`, it'll be visible in both and will have two listeners for "mouseenter".  This is because we early return in `removeTabBarItem` if we're trying to remove a `WI.PinnedTabBarItem`.  It might be worth adding a check above that bails if `tabBarItem` is a `WI.PinnedTabBarItem` and already has a `parentTabBar`.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:788
> +        if (this._delegate && typeof this._delegate.tabBarPopulateContextMenu === "function") {

I personally think that the extra `typeof ___ === "function" is unnecessary, considering that we would really want an error to occur if it weren't a function (btw I am badly attempting to convey Joe's opinion as well, assuming I am remembering it correctly).

    if (this._delegate && this._delegate.tabBarPopulateContextMenu)

There shouldn't be a case where we have a property matching that name that isn't a function, so I'd rather it error out than just be "silent".

That being said, we can discuss this more if you'd rather not change this now.

[Blaze Burg]

Comment on attachment 337934 [details]
Patch

Marking r- per Devin's concerns.