Closed Bug 1003096 Opened 9 years ago Closed 9 years ago

Remove tab reordering feature implemented in bug 480148


(Firefox :: Session Restore, defect)

Not set



Firefox 32
Tracking Status
firefox32 --- fixed


(Reporter: ttaubert, Assigned: ttaubert)


(Blocks 1 open bug)



(2 files)

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]

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]

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+
Closed: 9 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.