Closed Bug 907129 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: ttaubert, Assigned: smacleod)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

This is a follow-up of bug 904460, due to the conversation starting with bug 904460 comment #9.
I should probably add a few tests for the behavior with overwriteTabs=false and true.
Attachment #792752 - Flags: feedback?(smacleod)
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
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.
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
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.
Attachment #792752 - Flags: feedback?(smacleod) → feedback+
(Submitted too soon) Yeah, looks good to me. After some tests should be good to go.
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.
(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
Assignee: ttaubert → smacleod
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.
Yeah, this fell of my radar, I'm working on the test now though. Should be ready very soon.
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.
Attachment #792752 - Attachment is obsolete: true
Attachment #8344807 - Flags: review?(ttaubert)
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.
Attachment #8344807 - Flags: review?(ttaubert) → review+
(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.
Attachment #8344807 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/552e506ce21d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.