Save state when docShells are swapped

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: andreaio.it, Mentored)

Tracking

Trunk
Firefox 34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

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.

Comment 1

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

Comment 3

5 years ago
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]
Assignee

Comment 5

5 years ago
Can I be assigned to this bug?
Sure!
Assignee: nobody → andreaio.it
Status: NEW → ASSIGNED
Assignee

Comment 7

5 years ago
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)
Assignee

Comment 9

5 years ago
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+
Assignee

Comment 11

5 years ago
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+
Assignee

Updated

5 years ago
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
Assignee

Comment 16

5 years ago
Great, thank you
https://hg.mozilla.org/mozilla-central/rev/23ce5de257da
Status: ASSIGNED → RESOLVED
Closed: 5 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.