Closed Bug 867097 Opened 7 years ago Closed 6 years ago

Remove superfluous __SS_tabStillLoading property

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: ttaubert, Assigned: ttaubert)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: addon-compat, Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 687814
Blocks: 867118
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?
Attachment #743571 - Flags: review?(dteller) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/4a496e6b99af
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Blocks: 869900
Depends on: 873835
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/222621f74e96
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Rebased and removed leftover code that sets the now unused '_sessionhistory_max_entries' property. Carrying over r+.
Attachment #743571 - Attachment is obsolete: true
Attachment #754775 - Flags: review+
Target Milestone: Firefox 23 → ---
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.
Attachment #754775 - Attachment is obsolete: true
Attachment #8336706 - Flags: review?(dteller)
Summary: Remove re-use of __SS_data, and the superfluous __SS_tabStillLoading property → Remove superfluous __SS_tabStillLoading property
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!
Attachment #8336706 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/b285dc236bef
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.