1003096 - Remove tab reordering feature implemented in bug 480148

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

browser_480148.js fails permanently with the patch from bug 715376 applied as with per-tab event queues it doesn't make sense anymore to reorder tabs before starting to restore them i.e. send async messages to them.

To unblock bug 715376 we should remove _setTabsRestoringOrder() and the test. With restore_on_demand=true being the default nowadays it doesn't seem relevant anymore anyway.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1003096-Remove-tab-reordering-feature-implemente.patch

Comment 2

[Steven MacLeod [:smacleod]]

Comment on attachment 8414410 [details] [diff] [review]
0001-Bug-1003096-Remove-tab-reordering-feature-implemente.patch

Review of attachment 8414410 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the doc changes.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2444,5 @@
>        this._setWindowStateReady(aWindow);
>        return;
>      }
>  
> +    // Select the selected tab.

I think something like "If provided, set the selected tab" might be clearer?

Also, the documentation for the "aSelectTab" argument is pretty inadequate. Can we take this opportunity to call out the fact that it's a 1-based index, and that a value of 0 means don't select a tab? Maybe something like:

* @param aSelectTab
*        Index of the tab to select. This is a 1-based index where "1" indicates
*        the first tab should be selected, and 0 indicates that the currently
*        selected tab will not be changed.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Thanks for the suggestions, I will steal and copy them.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/434ffe0d33b2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Backed out for tabview failures:

https://hg.mozilla.org/integration/fx-team/rev/83c3a7471051

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1003096-Make-browser_tabview_bug595601.js-wait-u.patch

This patch fixes browser_tabview_bug595601.js so that it actually waits until the test session has been restored before continuing. I think this should also fix bug 964009.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=54bd6b24f821

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8415173 [details] [diff] [review]
0002-Bug-1003096-Make-browser_tabview_bug595601.js-wait-u.patch

Review of attachment 8415173 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me

::: browser/components/tabview/test/browser_tabview_bug595601.js
@@ +47,5 @@
>  }
>  
>  function testRestoreWithHiddenTabs() {
> +  TabsProgressListener.setCallback(function (needsRestore, isRestoring) {
> +    if (4 < needsRestore)

Since we're moving it, might as well put braces around?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/f55a3989aee0
https://hg.mozilla.org/integration/fx-team/rev/e057952237dc

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f55a3989aee0
https://hg.mozilla.org/mozilla-central/rev/e057952237dc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fde20263e8c9
https://hg.mozilla.org/releases/mozilla-beta/rev/66518d59cdbd

*JUST* the bug 964009 test fix :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Sorry, got a little status flag happy there.