1449700 - refactor addTab/moveTabTo

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

After digging into bug 1446913, I see that addTab/moveTabTo are often combined in ways that can result in multiple TabMove events being fired.  It also leads to some hacky work-arounds in the webext code.  

This could be resolved by adding a position param to addTab, and passing a position in from those areas that use moveTabTo for positioning the tab.  The tab could be inserted where it is meant to be, rather than inserted then moved.  This would also provide us with a correct tab index upon creation.

Outside of webextensions, I see no areas that depend on a combination tab created/moved event sequence.  

I need to review TabMove listeners to see if any would break, but if they do, they are likely listening for the wrong event anyway (e.g. should listen for add/attach/etc), possibly for the same reason we attempt to work around this in webextensions.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1449700 refactor addTab/moveTabTo patterns,

Review commit: https://reviewboard.mozilla.org/r/232256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232256/

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

Looking for an initial feedback round on the approach of removing addTab/moveTabTo combinations.  I haven't finished reviewing all the places this happens yet, and am still considering pinTab.  This passes the couple of test files I've touched recently and am running a try to see where things may blow  up.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

seems reasonable

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232256/diff/1-2/

Comment 5

[Shane Caraveo (:mixedpuppy)]

Running this through try to be sure the latest changes didn't bust anything.

- Rather than sharing code between moveTabTo and addTab, addTab is doing its own calculations.  The way moveTabTo is handling that is a bit strange in the addTab case
- Added support to pin tabs during addTab.  This avoids another moveTabTo call.  Sending the TabPinned event in that case.
- fixed a pinTab/moveTab combo that resulted in a TabMove/TabPinned/TabMove event sequence.

I've used a few tests locally to test these changes and they have worked well for catching any problems.  Those include (but are not limited to):

browser/base/content/test/tabs/browser_pinnedTabs.js
browser/base/content/test/tabs/browser_new_tab_insert_position.js
browser/components/extensions/test/browser/browser_ext_tabs_create.js

Overall I think these changes are covered well by existing tests, given all the above broke during some iterations.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

Dao for the tabbrowser refactoring.  aswan for the webext changes.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

https://reviewboard.mozilla.org/r/232256/#review240710

I didn't look at the tabbrowser changes but the ext-tabs changes look great assuming all our tests still pass.  thanks!

::: commit-message-26d67:3
(Diff revision 2)
> +Bug 1449700 refactor addTab/moveTabTo patterns
> +
> +Calculate all positioning of the tab during addTab to avoid extrenuous events and to provide an accurate tab id during TabOpen.

typo: extraneous

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

https://reviewboard.mozilla.org/r/232256/#review244320

::: browser/base/content/tabbrowser.js:2268
(Diff revision 2)
>      this.tabContainer._unlockTabSizing();
>  
>      // When overflowing, new tabs are scrolled into view smoothly, which
>      // doesn't go well together with the width transition. So we skip the
>      // transition in that case.
>      let animate = !aSkipAnimation &&

Also need to disable the animation when the tab is added as a pinned one.

::: browser/base/content/tabbrowser.js:2294
(Diff revision 2)
>        if (aOwner)
>          t.owner = aOwner;
>  
> -      var position = this.tabs.length - 1;
> -      t._tPos = position;
> +      // Ensure we have an index if one was not provided. _insertTabAt
> +      // will do some additional validation.
> +      if (aIndex === undefined) {

if (typeof aIndex != "number") {

::: browser/base/content/tabbrowser.js:2311
(Diff revision 2)
> +          } else if (openerTab) {
> +            t.owner = openerTab;
> +            this._lastRelatedTabMap.set(openerTab, t);
> +          }
> +        } else {
> +          // This is intentionally past bounds, see the comment below/

Is the trailing / a typo?

::: browser/base/content/tabbrowser.js:2327
(Diff revision 2)
> +
> +      for (let i = 0; i < this.tabs.length; i++) {
> +        this.tabs[i]._tPos = i;
> +        this.tabs[i]._selected = false;
> +      }
> +      this.selectedTab._selected = true;

Why is this needed here?

::: browser/base/content/tabbrowser.js:2332
(Diff revision 2)
> +      this.selectedTab._selected = true;
> +
> +      if (aPinned) {
> +        this.tabContainer._unlockTabSizing();
> +        this.tabContainer._positionPinnedTabs();
> +        this.tabContainer._updateCloseButtons();

Can we somehow share this code with pinTab?

::: browser/base/content/tabbrowser.js:2499
(Diff revision 2)
> +    if (aPinned) {
> +      this.getBrowserForTab(t).messageManager.sendAsyncMessage("Browser:AppTab", { isAppTab: true });
> +
> +      let event = document.createEvent("Events");
> +      event.initEvent("TabPinned", true, false);
> +      t.dispatchEvent(event);

ditto

::: browser/base/content/tabbrowser.js:3495
(Diff revision 2)
>      let params = {
>        eventDetail: { adoptedTab: aTab },
>        preferredRemoteType: linkedBrowser.remoteType,
>        sameProcessAsFrameLoader: linkedBrowser.frameLoader,
> -      skipAnimation: true
> +      skipAnimation: true,
> +      index: aIndex

nit: add trailing comma

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232256/diff/2-3/

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

https://reviewboard.mozilla.org/r/232256/#review244320

> Why is this needed here?

If we insert rather than append, we need to update these.  I've modified it a little to avoid that if we're not inserting, and pushed that out to shared code (with moveTabTo)

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

https://reviewboard.mozilla.org/r/232256/#review245710

This looks very good, thank you. You may want to land this after 61 moved to beta unless you think it's unlikely enough that this will cause regressions that we'd rather not deal with on beta.

::: browser/base/content/tabbrowser.js:527
(Diff revision 3)
>    },
>  
> +  _updatePinnedTabs() {
> +    this.tabContainer._unlockTabSizing();
> +    this.tabContainer._positionPinnedTabs();
> +    this.tabContainer._updateCloseButtons();

_unlockTabSizing and _updateCloseButtons are specifically about unpinned tabs, so I'd prefer a clearer and more verbose name such as _updateTabBarForPinnedTabs.

::: browser/base/content/tabbrowser.js:1523
(Diff revision 3)
>          postData: aPostDatas[0],
>          userContextId: aUserContextId,
>          triggeringPrincipal: aTriggeringPrincipal,
>          bulkOrderedOpen: multiple,
> -      });
> +      };
> +      if (aNewIndex !== -1) {

nit: if (aNewIndex > -1) {

::: browser/base/content/tabbrowser.js:1542
(Diff revision 3)
>          postData: aPostDatas[i],
>          userContextId: aUserContextId,
>          triggeringPrincipal: aTriggeringPrincipal,
>          bulkOrderedOpen: true,
> -      });
> -      if (targetTabIndex !== -1)
> +      };
> +      if (targetTabIndex !== -1) {

ditto

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Dão Gottwald [::dao] from comment #11)

> This looks very good, thank you. You may want to land this after 61 moved to
> beta unless you think it's unlikely enough that this will cause regressions
> that we'd rather not deal with on beta.

agreed, I'd like this to have time to bake.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963351 [details]
Bug 1449700 refactor addTab/moveTabTo patterns,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232256/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/030d771ec8a3
refactor addTab/moveTabTo patterns, r=aswan,dao

Comment 15

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/030d771ec8a3