904460 - Pass _firstTabs 'parameter' as an argument to restoreWindow() rather than tacking it onto the state object

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We currently (roughly) do:

> this._initialState._firstTabs = true;
> this.restoreWindow(aWindow, this._initialState);

http://hg.mozilla.org/mozilla-central/file/3d20597e0a07/browser/components/sessionstore/src/SessionStore.jsm#l747

The _firstTabs property we're setting on the initial state here is actually a parameter that should be passed to restoreWindow(). There's no other code that makes use of it. We use _firstTabs to take care of a couple of things when we automatically restore a session and the user might have clicked a link in an external application that then launched Firefox.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Pass _firstTabs 'parameter' as an argument to restoreWindow() rather than tacking it onto the state object

This patch doesn't change behavior, it's just a cleanup. I seized the chance to make restoreWindow() take an "options" argument to make call sites a little more readable.

Comment 2

[tabmix.onemen]

This can brake some add-on functionality

Tabmix use it to add closed tab to a window for example when merging windows

it relay on this lines in restoreWindow
>    if (aOverwriteTabs || root._firstTabs) {
>      this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs || [];
>    }

i think it is a good idea to remove it from the state object
but add it as an option to setBrowserState and setWindowState

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Oh god, really?

We don't need an option for setBrowserState() as that calls restoreWindow() with aOverwrite=true anyway. If you want to overwrite the current set of closed tabs you should call setWindowState() with aOverwrite=true as well.

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 789418 [details] [diff] [review]
Pass _firstTabs 'parameter' as an argument to restoreWindow() rather than tacking it onto the state object

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

Looks good to me other than a single nit about comments.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2657,5 @@
> +   *        {overwriteTabs: true} to overwrite existing tabs w/ new ones
> +   *        {isFollowUp: true} if this is not the restoration of the 1st window
> +   *        {firstTabs: true} if this is the first non-private window we're
> +   *                          restoring in this session, that might open an
> +   *                          external link as well

While this comment is correct, I think it describes a single use case of the option, and not what it is actually used to do.

To me, firstTabs means "Make the restored tabs, the 'first tabs' in the window", so any other tabs will appear at the end. This is used in the case you mention is this comment, but might it be clearer to have the function of the option here, and put the description you have now at the callsite?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

So.. actually I think, it's the other way around. It's supposed to be set when we restore the first non-private window in this session.

That we then move existing tabs to the right and replace the list of closed tabs is a detail of how we restore the first non-private window, no?

Comment 6

[Steven MacLeod [:smacleod]]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> So.. actually I think, it's the other way around. It's supposed to be set
> when we restore the first non-private window in this session.
> 
> That we then move existing tabs to the right and replace the list of closed
> tabs is a detail of how we restore the first non-private window, no?

Yeah, you're right. The naming of "firstTabs" does make things a little ambiguous though, it's not too descriptive. Maybe we should take this opportunity to rename it to something better? "firstWindow" maybe?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

firstWindow sounds good to me.

https://hg.mozilla.org/integration/fx-team/rev/83998064b365

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/83998064b365

Comment 9

[tabmix.onemen]

(In reply to onemen.one from comment #2)
> This can brake some add-on functionality
> 
> Tabmix use it to add closed tab to a window for example when merging windows
> 
> it relay on this lines in restoreWindow
> >    if (aOverwriteTabs || root._firstTabs) {
> >      this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs || [];
> >    }
> 
> i think it is a good idea to remove it from the state object
> but add it as an option to setBrowserState and setWindowState

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Oh god, really?
> 
> We don't need an option for setBrowserState() as that calls restoreWindow()
> with aOverwrite=true anyway. If you want to overwrite the current set of
> closed tabs you should call setWindowState() with aOverwrite=true as well.

calling setWindowState with aOverwrite=true overwrite all window in a window.
as i mention above, I use it to merge window with tabs and closed tabs into existing window with tabs and closed tabs without overwriting existing tab

I can't do it anymore with the new code

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to onemen.one from comment #9)
> calling setWindowState with aOverwrite=true overwrite all window in a window.
> as i mention above, I use it to merge window with tabs and closed tabs into
> existing window with tabs and closed tabs without overwriting existing tab

I think a sensible thing to do here would be to merge the closed tab data as a default action. We can do something like:

if (overwriteTabs || firstWindow) {
  this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs || [];
} else {
  let closedTabs = winData._closedTabs || [];
  closedTabs = this._windows[aWindow.__SSi]._closedTabs.concat(closedTabs);
  this._windows[aWindow.__SSi]._closedTabs = closedTabs.slice(-this._max_tabs_undo);
}

Comment 11

[tabmix.onemen]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> I think a sensible thing to do here would be to merge the closed tab data as
> a default action. We can do something like:
> 
> if (overwriteTabs || firstWindow) {
>   this._windows[aWindow.__SSi]._closedTabs = winData._closedTabs || [];
> } else {
>   let closedTabs = winData._closedTabs || [];
>   closedTabs = this._windows[aWindow.__SSi]._closedTabs.concat(closedTabs);
>   this._windows[aWindow.__SSi]._closedTabs =
> closedTabs.slice(-this._max_tabs_undo);
> }

OK,
as long as we can retain old behavior when using overwrite=false

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to onemen.one from comment #11)
> OK,
> as long as we can retain old behavior when using overwrite=false

I don't understand. I was suggesting to merge closed tab data exactly when overwriteTabs=false. Because if we merge tabs we should also merge closed tab data.

Comment 13

[tabmix.onemen]

OK

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Filed follow-up bug 907129.