Remove browser.__SS_restoreState and use a WeakMap instead

REOPENED
Unassigned

Status

()

Firefox
Session Restore
REOPENED
4 years ago
3 years ago

People

(Reporter: ttaubert, Unassigned)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs, {addon-compat})

Trunk
Firefox 24
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Let's use a WeakMap instead of tacking properties onto DOM elements!
(Reporter)

Comment 1

4 years ago
Created attachment 743594 [details] [diff] [review]
remove browser.__SS_restoreState and use a WeakMap instead
Attachment #743594 - Flags: review?(dteller)
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?
Attachment #743594 - Flags: review?(dteller) → review+

Updated

4 years ago
Blocks: 869900
(Reporter)

Comment 3

4 years ago
Created attachment 748787 [details] [diff] [review]
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.
Attachment #743594 - Attachment is obsolete: true
Attachment #748787 - Flags: review?(dteller)
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.
Attachment #748787 - Flags: review?(dteller) → review+
(Reporter)

Comment 5

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/d1cd5199bf45
Blocks: 874381
Keywords: addon-compat
(Reporter)

Comment 6

4 years ago
https://hg.mozilla.org/mozilla-central/rev/d1cd5199bf45
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
(Reporter)

Comment 7

4 years ago
Backed out:

https://hg.mozilla.org/integration/fx-team/rev/24f843555fc4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 871246

Updated

3 years ago
Depends on: 960833
(Reporter)

Updated

3 years ago
Assignee: ttaubert → nobody
You need to log in before you can comment on or make changes to this bug.