938093 - Swap TabStateCache contents when docShells are swapped

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We currently only swap existing tabs (that most likely have data in the tab state cache) with new ones. Therefore after swapping the tab state cache will not contain an entry for the non-blank docShell and we will collect data properly. We should handle the case correctly though.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-938093-Swap-TabStateCache-contents-when-docShell.patch

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 831453 [details] [diff] [review]
0003-Bug-938093-Swap-TabStateCache-contents-when-docShell.patch

Review of attachment 831453 [details] [diff] [review]:
-----------------------------------------------------------------

Why did you decide to go with aBrowser here?

::: browser/components/sessionstore/src/TabStateCache.jsm
@@ +98,5 @@
>  
>    /**
> +   * Swap cached data for two given browsers.
> +   *
> +   * @param {xul:browser} aBrowser The first of the two browsers that swapped

Doesn't the final text usually go on its own line?

@@ +234,5 @@
> +   *                                    swapped docShells.
> +   */
> +  onSwapDocShells: function(aBrowser, aOtherBrowser) {
> +    // Make sure that one or the other of these has cached data,
> +    // and let it be |browser|.

aBrowser

@@ +243,5 @@
> +      }
> +    }
> +
> +    // At this point, |browser| is guaranteed to have cached data,
> +    // although |otherBrowser| may not. Perform the swap.

Names also need update here.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #2)
> Why did you decide to go with aBrowser here?

You're right, that should be |browser| and |otherBrowser| here. I usually just stick with the surrounding conventions but we should rather improve incrementally towards what most of sessionstore looks like nowadays (or will be soon).

> >    /**
> > +   * Swap cached data for two given browsers.
> > +   *
> > +   * @param {xul:browser} aBrowser The first of the two browsers that swapped
> 
> Doesn't the final text usually go on its own line?

Yeah, that looks better.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/60c9e7d0d038

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/60c9e7d0d038