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

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

({addon-compat})

Trunk
Firefox 26
addon-compat
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 789418 [details] [diff] [review]
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.
Attachment #789418 - Flags: review?(smacleod)

Comment 2

4 years ago
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
(Assignee)

Comment 3

4 years ago
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.
Keywords: addon-compat
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?
Attachment #789418 - Flags: review?(smacleod) → review+
(Assignee)

Comment 5

4 years ago
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?
(Assignee)

Updated

4 years ago
Flags: needinfo?(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?
Flags: needinfo?(smacleod)
(Assignee)

Comment 7

4 years ago
firstWindow sounds good to me.

https://hg.mozilla.org/integration/fx-team/rev/83998064b365
https://hg.mozilla.org/mozilla-central/rev/83998064b365
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26

Comment 9

4 years ago
(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
(Assignee)

Comment 10

4 years ago
(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

4 years ago
(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
(Assignee)

Comment 12

4 years ago
(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

4 years ago
OK
(Assignee)

Updated

4 years ago
Depends on: 907129
(Assignee)

Comment 14

4 years ago
Filed follow-up bug 907129.
You need to log in before you can comment on or make changes to this bug.