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

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: smacleod)

Tracking

Trunk
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
This is a follow-up of bug 904460, due to the conversation starting with bug 904460 comment #9.
(Reporter)

Comment 1

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

Comment 2

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

Comment 3

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

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

Comment 5

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

Updated

4 years ago
Attachment #792752 - Flags: feedback?(smacleod) → feedback+
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

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

Comment 10

3 years ago
Yeah, this fell of my radar, I'm working on the test now though. Should be ready very soon.
(Assignee)

Comment 11

3 years ago
Created attachment 8344807 [details] [diff] [review]
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.
Attachment #792752 - Attachment is obsolete: true
Attachment #8344807 - Flags: review?(ttaubert)
(Reporter)

Comment 12

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

Comment 13

3 years ago
Created attachment 8346107 [details] [diff] [review]
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.
Attachment #8344807 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Reporter)

Comment 14

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/552e506ce21d
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/552e506ce21d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29

Updated

3 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.