867142 - Remove browser.__SS_restoreState and use a WeakMap instead

Status: REOPENED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Let's use a WeakMap instead of tacking properties onto DOM elements!

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: remove browser.__SS_restoreState and use a WeakMap instead

Comment 2

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

Comment on attachment 743594 [details] [diff] [review]
remove browser.__SS_restoreState and use a WeakMap instead

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +235,5 @@
>  
>    checkPrivacyLevel: function ss_checkPrivacyLevel(aIsHTTPS, aUseDefaultPref) {
>      return SessionStoreInternal.checkPrivacyLevel(aIsHTTPS, aUseDefaultPref);
> +  },
> +

So that's a public method?
If so, it should be somewhat documented.

@@ +340,5 @@
>    _resume_session_once_on_shutdown: null,
>  
> +  // Keeps track of tab states when a tab is
> +  // restoring or waiting to be restored.
> +  _tabRestoreStates: new WeakMap(),

Shall we move this to an object, as the rest of __SS_stuff that we're putting in WeakMaps?

Side-note: at some point, we may wish to gather all these properties in an object |Metadata|, or something such.

@@ +2987,5 @@
>  
>        // keep the data around to prevent dataloss in case
>        // a tab gets closed before it's been properly restored
>        this._restoringTabsData.set(browser, tabData);
> +      this._tabRestoreStates.set(browser, TAB_STATE_NEEDS_RESTORE);

Since the getter is abstracted behind |isTabStateNeedsRestore|, the setter should probably be abstracted the same way.

@@ +3964,5 @@
>      return this._prefBranch.getIntPref(pref) < (aIsHTTPS ? PRIVACY_ENCRYPTED : PRIVACY_FULL);
>    },
>  
>    /**
> +   * Returns whether a given browser is currently restoring.

Nit: It would be nice to explain what "currently restoring" means.

::: browser/components/sessionstore/test/browser_636279.js
@@ -93,5 @@
> -    if (this.callback && aBrowser.__SS_restoreState == TAB_STATE_RESTORING &&
> -        aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
> -        aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK &&
> -        aStateFlags & Ci.nsIWebProgressListener.STATE_IS_WINDOW)
> -      this.callback.apply(null, countTabs());

So we're not counting tabs anymore?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: remove browser.__SS_restoreState and use a WeakMap instead

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> So that's a public method?
> If so, it should be somewhat documented.

Done.

> Shall we move this to an object, as the rest of __SS_stuff that we're
> putting in WeakMaps?

Of course.

> Side-note: at some point, we may wish to gather all these properties in an
> object |Metadata|, or something such.

Yes, a little more refactoring would surely be good after we got rid of all those tiny properties.

> Since the getter is abstracted behind |isTabStateNeedsRestore|, the setter
> should probably be abstracted the same way.

Good point.

> > +   * Returns whether a given browser is currently restoring.
> Nit: It would be nice to explain what "currently restoring" means.

Done.

> ::: browser/components/sessionstore/test/browser_636279.js
> > -      this.callback.apply(null, countTabs());
> So we're not counting tabs anymore?

We still are, I just removed lots of duplicated code that basically does the same as 'gProgressListener' we put in sessionstore/test/head.js quite some time ago.

Comment 4

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

Comment on attachment 748787 [details] [diff] [review]
remove browser.__SS_restoreState and use a WeakMap instead

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

Looks good, thanks.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4691,5 @@
> +// made visible (when restore_on_demand=true). If a tab is 'restoring' we wait
> +// for its actual page to load and will then restore form data etc. If has()
> +// returns false the tab has not been restored from previous data or it has
> +// already finished restoring and is thus now seen as a valid and complete tab.
> +let TabRestoreStates = {

It might be a good idea to add type-checks to ensure that we always call the methods of |TabRestoreStates| on browsers.

Probably a followup (mentored) bug, though.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Backed out:

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