1287479 - Lazy-browser-tabs: modify TabState.jsm to deal with lazy-browser tabs

Status: RESOLVED INVALID

(Firefox :: Session Restore, enhancement, P2)

Description

[u462496]

Support bug of bug 906076.

Methods in TabState.jsm will need to deal with lazy browser tabs.

Comment 1

[u462496]

Attachment: Patch

bug 1167923 has already dealt with modifications in TabStateCache.jsm to deal with either tab or browser.

Comment 2

[u462496]

Attachment: Patch V2

Changed tab.hasLinkedBrowser to tab.hasBrowser to conform to bug 1287456.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8774370 [details] [diff] [review]
Patch V2

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

Nice, small patch. r=me with comments addressed.

::: browser/components/sessionstore/TabState.jsm
@@ +115,5 @@
>      }
>      // If there is a userTypedValue set, then either the user has typed something
>      // in the URL bar, or a new tab was opened with a URI to load.
>      // If so, we also track whether we were still in the process of loading something.
> +    if (browser && !("userTypedValue" in tabData) && browser.userTypedValue) {

Please change this to
```js
if (!("userTypedValue" in tabData) && browser && browser.userTypedValue) {
```
...to keep this somewhat easier to read.

@@ +148,5 @@
>     *        The tab data belonging to the given |tab|.
>     * @param options (object)
>     *        {includePrivateData: true} to always include private data
>     */
> +  copyFromCache(browserOrTab, tabData, options = {}) {

You'll need to update the documentation of this method as well. Please check out TabStateCache.jsm for an example (which you authored ;-) ).

Comment 4

[u462496]

Attachment: Patch V3

Modifications per Mike's comments

Comment 5

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> @@ +148,5 @@
> >     *        The tab data belonging to the given |tab|.
> >     * @param options (object)
> >     *        {includePrivateData: true} to always include private data
> >     */
> > +  copyFromCache(browserOrTab, tabData, options = {}) {
> 
> You'll need to update the documentation of this method as well. Please check
> out TabStateCache.jsm for an example (which you authored ;-) ).

Arrrggggg ! :-)

Thanks for the review, Mike :-)

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775113 [details] [diff] [review]
Patch V3

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

::: browser/components/sessionstore/TabState.jsm
@@ +102,5 @@
>        tabData.extData = tab.__SS_extdata;
>      }
>      // Copy data from the tab state cache only if the tab has fully finished
>      // restoring. We don't want to overwrite data contained in __SS_data.
> +    this.copyFromCache(tab, tabData, options);

For minimum chance of side-effects, you could do s/tab/browser || tab/ here.

Comment 7

[u462496]

Attachment: Patch V4

Is this what you meant?

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775134 [details] [diff] [review]
Patch V4

Yup!

Comment 9

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Comment on attachment 8775134 [details] [diff] [review]
> Patch V4
> 
> Yup!

As I look at it more closely though, I don't see how it can make a difference.  The only thing done with `browserOrTab` in `copyFromCache` is it gets passed to `TabStateCache`.  `TabStateCache` uses permanentKey, which will always be the same for the tab and the browser (if it exists), and will always exist on the tab.

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Allasso Travesser from comment #9)
> As I look at it more closely though, I don't see how it can make a
> difference.  The only thing done with `browserOrTab` in `copyFromCache` is
> it gets passed to `TabStateCache`.  `TabStateCache` uses permanentKey, which
> will always be the same for the tab and the browser (if it exists), and will
> always exist on the tab.

True. It's more of a cosmetic change than anything else, really.

Comment 11

[u462496]

(In reply to Mike de Boer [:mikedeboer] from comment #10) 
> True. It's more of a cosmetic change than anything else, really.

I've reverted to the previous patch.

Comment 12

[u462496]

Comment on attachment 8775113 [details] [diff] [review]
Patch V3

Bug 1287330 is taking a turn which will probably make this patch obsolete.

Comment 13

[u462496]

This bug no longer applies due to the change in lazy browser strategy in bug 1287330.  Marking as RESOLVED:INVALID.