Remove tab reordering feature implemented in bug 480148

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

Trunk
Firefox 32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 fixed)

Details

Attachments

(2 attachments)

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 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.
Attachment #8414410 - Flags: review?(smacleod) → review+
Thanks for the suggestions, I will steal and copy them.
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.
Attachment #8415173 - Flags: review?(smacleod)
Blocks: 964009
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?
Attachment #8415173 - Flags: review?(smacleod) → review+
https://hg.mozilla.org/mozilla-central/rev/f55a3989aee0
https://hg.mozilla.org/mozilla-central/rev/e057952237dc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Sorry, got a little status flag happy there.
You need to log in before you can comment on or make changes to this bug.