907129 - restoreWindow() should merge closed tabs data when overwriteTabs=false

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

This is a follow-up of bug 904460, due to the conversation starting with bug 904460 comment #9.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Merge closed tabs data when restoreWindow() is called with overwriteTabs=false

I should probably add a few tests for the behavior with overwriteTabs=false and true.

Comment 2

[tabmix.onemen]

What if I want to add the merged closed tabs before the curren closed tabs?
Maybe it will be simpler to add option to setWindowState to replace closed tabs even with overwrite=false

That give the caller control where and how to add tabs and closed tabs

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

I think we shouldn't over-engineer this. I am open to discuss what the default behavior should be but I don't see the point in offering a lot of flexibility here and changing the API to be more complex.

Comment 4

[tabmix.onemen]

I don't think this is over complex, after all we have this option until recently by using _firsttab=true

If you don't want to add this option, then your proposed patch is ok

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

This really wasn't an option, especially not an official one. It was an internal flag that could unfortunately be abused due to an improper implementation.

Comment 6

[Steven MacLeod [:smacleod]]

(Submitted too soon) Yeah, looks good to me. After some tests should be good to go.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Steven, would you mind taking over and writing a small test for the new behavior? I'm a little swamped right now and could really use some help with this. I will probably not get to work on it anytime soon.

Comment 8

[Steven MacLeod [:smacleod]]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> Steven, would you mind taking over and writing a small test for the new
> behavior? I'm a little swamped right now and could really use some help with
> this. I will probably not get to work on it anytime soon.

Sure

Comment 9

[Michael Kraft [:morac]]

Anything happening with this?  With bug 904460 dropping in Firefox 26, it's already too late to get this in there, but it looks like nothing is happening with this bug.

Currently as of Firefox 26, Session Manager will no longer be able to prevent the closed tab list from being overwritten when restoring a session at browser start up.  Without this fix, closed tabs will be lost.

Comment 10

[Steven MacLeod [:smacleod]]

Yeah, this fell of my radar, I'm working on the test now though. Should be ready very soon.

Comment 11

[Steven MacLeod [:smacleod]]

Attachment: Patch - Merge closed tabs data when restoreWindow() is called with overwriteTabs=false

Added a test for the closed tab merging, and updated the patch to throw away oldest tabs when needed and treat the restore closed tabs list as newer than the current one.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8344807 [details] [diff] [review]
Patch - Merge closed tabs data when restoreWindow() is called with overwriteTabs=false

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

r=me, thanks!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +2486,5 @@
> +      // If we merge tabs, we also want to merge closed tabs data. We'll assume
> +      // the restored tabs were closed more recently and append the current list
> +      // of closed tabs to the new one...
> +      let newClosedTabsData =
> +        (winData._closedTabs || []).concat(this._windows[aWindow.__SSi]._closedTabs);

Nit: I feel like we should put "winData._closedTabs || []" and "this._windows[aWindow.__SSi]" in variables outside the branches.

::: browser/components/sessionstore/test/browser_merge_closed_tabs.js
@@ +34,5 @@
> +
> +  const maxTabsUndo = 4;
> +  gPrefService.setIntPref("browser.sessionstore.max_tabs_undo", maxTabsUndo);
> +
> +  function verifyClosedData(win, initialState, restoredState, maxTabsUndo) {

Nit: looks like the last argument doesn't actually need to be specified. You actually don't need to specify any of these arguments... maybe just inline the function?

@@ +36,5 @@
> +  gPrefService.setIntPref("browser.sessionstore.max_tabs_undo", maxTabsUndo);
> +
> +  function verifyClosedData(win, initialState, restoredState, maxTabsUndo) {
> +    let iClosed = initialState.windows[0]._closedTabs
> +    let rClosed = restoredState.windows[0]._closedTabs

Nit: missing semicolons.

Comment 13

[Steven MacLeod [:smacleod]]

Attachment: Patch - Merge closed tabs data when restoreWindow() is called with overwriteTabs=false v2

(In reply to Tim Taubert [:ttaubert] from comment #12)
> Nit: I feel like we should put "winData._closedTabs || []" and
> "this._windows[aWindow.__SSi]" in variables outside the branches.

I moved "winData._closedTabs || []" out but kept the other in, since "this._windowspaWindow._SSi]" is littered throughout the function, so if we're going to change it we should use it throughout. I preferred to just keep this change smaller.


> Nit: looks like the last argument doesn't actually need to be specified. You
> actually don't need to specify any of these arguments... maybe just inline
> the function?

Yeah, I was planning to have multiple test cases, but I think we're fine with just the one so I've inlined it like you suggested.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/552e506ce21d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/552e506ce21d