Closed Bug 963042 Opened 10 years ago Closed 10 years ago

Save state when docShells are swapped

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

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.
I would like to work on this bug.Can you please assign it to me?
(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
Can you please tell me how to start with this?
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 :)
Assignee: bhagya.ravikumar → nobody
Status: ASSIGNED → NEW
Mentor: ttaubert
Whiteboard: [good first bug][mentor=ttaubert][lang=js] → [good first bug][lang=js]
Can I be assigned to this bug?
Sure!
Assignee: nobody → andreaio.it
Status: NEW → ASSIGNED
Attachment #8469217 - Flags: review?(ttaubert)
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)
Attached patch Managed the SwapDocShells event (obsolete) — Splinter Review
Attachment #8470239 - Flags: review?(ttaubert)
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+
Attachment #8470837 - Flags: review?(ttaubert)
Attachment #8469217 - Attachment is obsolete: true
Attachment #8470239 - Attachment is obsolete: true
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+
Keywords: checkin-needed
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
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
Great, thank you
https://hg.mozilla.org/mozilla-central/rev/23ce5de257da
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.