867097 - Remove superfluous __SS_tabStillLoading property

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

browser.__SS_data is used to retain a tab's data until it has fully been restored. When the tab has loaded and SSTabRestored fires, the property is deleted. Now, for some probably historical reason it was decided to be re-used to cache session history entries.

We should clean up and clarify the code by just removing the cache for now. It seems like browser.__SS_tabStillLoading was only introduced to tell the reason __SS_data exists apart: if stillLoading=true then it's the tab data until it's been restored. If stillLoading=false then it's the history entry cache.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 743571 [details] [diff] [review]
remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property

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

Could you take the opportunity to add a section documenting (some) of the _SS_* fields that session restore adds to DOM objects?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1256,2 @@
>      delete browser.__SS_formDataSaved;
>      delete browser.__SS_hostSchemeData;

Site-note: We should consider getting rid of all these semantically incorrect |delete foo.bar| and replace them with |foo.bar = undefined|.

::: browser/components/sessionstore/test/browser_579868.js
@@ -18,5 @@
>      // Undo pinning
>      gBrowser.unpinTab(tab1);
>  
> -    is(tab1.linkedBrowser.__SS_tabStillLoading, true,
> -       "_tabStillLoading should be true.");

Should we check tab1.linkedBrowser.__SS_data?

::: browser/components/sessionstore/test/browser_625257.js
@@ -38,5 @@
>    // Trigger a save state.
>    ss.getBrowserState();
>  
>    is(gBrowser.tabs[1], tab, "newly created tab should exist by now");
> -  ok(tab.linkedBrowser.__SS_data, "newly created tab should be in save state");

Now, we could even reverse the test, couldn't we?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> >      delete browser.__SS_formDataSaved;
> >      delete browser.__SS_hostSchemeData;
> 
> Site-note: We should consider getting rid of all these semantically
> incorrect |delete foo.bar| and replace them with |foo.bar = undefined|.

I agree, let's just slap anyone using 'delete' :)

> > -    is(tab1.linkedBrowser.__SS_tabStillLoading, true,
> > -       "_tabStillLoading should be true.");
> 
> Should we check tab1.linkedBrowser.__SS_data?

Yeah, good point.

> >    is(gBrowser.tabs[1], tab, "newly created tab should exist by now");
> > -  ok(tab.linkedBrowser.__SS_data, "newly created tab should be in save state");
> 
> Now, we could even reverse the test, couldn't we?

Technically, we could reverse the test because the tab should've stopped loading by now. OTOH this has nothing to do with the actual test and is probably quite confusing. I'm not even sure why the test originally checked that the __SS_data cache exists as this seems not really specific to the issue addressed.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/4a496e6b99af

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4a496e6b99af

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Backed out:

https://hg.mozilla.org/integration/fx-team/rev/222621f74e96

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Backed out from Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5a7dc83250b

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property

Rebased and removed leftover code that sets the now unused '_sessionhistory_max_entries' property. Carrying over r+.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-867097-Remove-superfluous-__SS_tabStillLoading-p.patch

Now that the __SS_data changes from the previous patch here have already landed I don't think there's much need to withhold this until Australis lands. Very few add-ons will be affected by removing __SS_tabStillLoading and most of them will still work afaict from reading the sources.

To re-iterate: browser.__SS_tabStillLoading was only introduced to tell the reason __SS_data exists apart: if stillLoading=true then it's the tab data until it's been restored. If stillLoading=false then it's the history entry cache.

Said history entry cache has been removed moons ago and thus nowadays (!!__SS_data) == (!!__SS_tabStillLoading). Easy to get rid of.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8336706 [details] [diff] [review]
0001-Bug-867097-Remove-superfluous-__SS_tabStillLoading-p.patch

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

Yeah, one __SS_* gone!

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/b285dc236bef