Closed
Bug 963042
Opened 10 years ago
Closed 10 years ago
Save state when docShells are swapped
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 34
People
(Reporter: ttaubert, Assigned: andreaio.it, Mentored)
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 3 obsolete files)
We already listen for the SwapDocShells event to update our internal caches. We also need to issue a delayed save state call to make sure the tab data is contained in the right tab.
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Bhagya from comment #1) > I would like to work on this bug.Can you please assign it to me? Great!
Assignee: nobody → bhagya.ravikumar
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•10 years ago
|
||
In SessionStore.jsm, we would need to add "SwapDocShells" to TAB_EVENTS. in SessionStore.handleEvent() we would then just need to add another case for "SwapDocShells", just like we already have for TabPinned and TabUnpinned. We basically want to ensure that saveStateDelayed() is called whenever docShells in a tabbrowser have been swapped. That's it :)
Reporter | ||
Updated•10 years ago
|
Assignee: bhagya.ravikumar → nobody
Reporter | ||
Updated•10 years ago
|
Status: ASSIGNED → NEW
Updated•10 years ago
|
Mentor: ttaubert
Whiteboard: [good first bug][mentor=ttaubert][lang=js] → [good first bug][lang=js]
Assignee | ||
Comment 5•10 years ago
|
||
Can I be assigned to this bug?
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8469217 -
Flags: review?(ttaubert)
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8469217 [details] [diff] [review] Added SwapDocShells in TAB_EVENTS and handled it Review of attachment 8469217 [details] [diff] [review]: ----------------------------------------------------------------- Andrea, first: thanks a lot for your patch! Second: I'm really sorry, I misguided you in my comment above. SwapDocShells is not actually a tab event but we send it to the <browser> element instead. This means we need to separately add an event listener for every tab's browser in onTabAdd() and remove it again in onTabRemove(). You can retrieve a tab's browser just like we do already in onTabRemove(). I'm looking forward to your next patch :) Sorry again!
Attachment #8469217 -
Flags: review?(ttaubert)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8470239 -
Flags: review?(ttaubert)
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8470239 [details] [diff] [review] Managed the SwapDocShells event Review of attachment 8470239 [details] [diff] [review]: ----------------------------------------------------------------- This looks almost good, thanks! ::: browser/components/sessionstore/SessionStore.jsm @@ +1264,5 @@ > * bool Do not save state if we're updating an existing tab > */ > onTabAdd: function ssi_onTabAdd(aWindow, aTab, aNoNotification) { > + let browser = aTab.linkedBrowser; > + browser.addEventListener("SwapDocShells", handleEvent); browser.addEventListener("SwapDocShells", this); @@ +1294,5 @@ > if (previousState == TAB_STATE_RESTORING) > this.restoreNextTab(); > } > > + browser.removeEventListener("SwapDocShells", handleEvent); browser.removeEventListener("SwapDocShells", this); And please put it further above, below "delete browser.__SS_data".
Attachment #8470239 -
Flags: review?(ttaubert) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8470837 -
Flags: review?(ttaubert)
Reporter | ||
Updated•10 years ago
|
Attachment #8469217 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Attachment #8470239 -
Attachment is obsolete: true
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8470837 [details] [diff] [review] Added event listening for SwapDocShells Review of attachment 8470837 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you! Can you please change the patch description to something like "Bug 963042 - Added event listener for SwapDocShells to ensure we save state when swapping docShells r=ttaubert", attach a new patch and set the "checkin-needed" keyword? Thanks!
Attachment #8470837 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8470837 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 14•10 years ago
|
||
patching file browser/components/sessionstore/SessionStore.jsm bad hunk #3 @@ -1275,17 +1278,17 @@ let SessionStoreInternal = { (17 17 18 17) patch failed, rejects left in working dir
Keywords: checkin-needed
Reporter | ||
Comment 15•10 years ago
|
||
That's really weird. The patch seems to be corrupt somehow but I can't tell what it is. And leaves lots of .rej files that have nothing to do with this patch. Anyway, I took the previous patch and adjusted the description before pushing: https://hg.mozilla.org/integration/fx-team/rev/23ce5de257da
Assignee | ||
Comment 16•10 years ago
|
||
Great, thank you
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/23ce5de257da
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•10 years ago
|
QA Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•