Closed
Bug 907129
Opened 11 years ago
Closed 11 years ago
restoreWindow() should merge closed tabs data when overwriteTabs=false
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
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.
Reporter | ||
Comment 1•11 years ago
|
||
I should probably add a few tests for the behavior with overwriteTabs=false and true.
Attachment #792752 -
Flags: feedback?(smacleod)
Comment 2•11 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•11 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•11 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•11 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•11 years ago
|
Attachment #792752 -
Flags: feedback?(smacleod) → feedback+
Assignee | ||
Comment 6•11 years ago
|
||
(Submitted too soon) Yeah, looks good to me. After some tests should be good to go.
Reporter | ||
Comment 7•11 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•11 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
Comment 9•11 years ago
|
||
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•11 years ago
|
||
Yeah, this fell of my radar, I'm working on the test now though. Should be ready very soon.
Assignee | ||
Comment 11•11 years ago
|
||
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•11 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•11 years ago
|
||
(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•11 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 14•11 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
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•