905533 - Move SessionStore._updateCookies() to SessionCookies.update()

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Bug 894595 will need to update cookies after all the session history data has been collected asynchronously. We should move the cookie updating code to SessionCookies.jsm so that we don't have to somehow call into SessionStoreInternal.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Move SessionStore._updateCookies() to SessionCookies.update()

This mostly just moves code. I left _extractHostsForCookiesFromEntry() and _extractHostsForCookiesFromHostScheme() unchanged because I really didn't want to mess with that for now. We should clean them up later.

Comment 2

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

Comment on attachment 790657 [details] [diff] [review]
Move SessionStore._updateCookies() to SessionCookies.update()

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

A few nitpicks, but looks good.

::: browser/components/sessionstore/src/SessionCookies.jsm
@@ +45,5 @@
>     */
>    _initialized: false,
>  
>    /**
> +   * Updates cookies for given array of window states.

Nit: It might be useful to explain what "update" means.

@@ +83,5 @@
> +   *
> +   * @param window
> +   *        A window state object containing tabs with history entries.
> +   * @param checkPrivacy (bool)
> +   *        Whether to check the privacy level for each host.

Nit: @return (in particular, to relieve any ambiguity due to the fact that the function returns "a map" materialized by a general-purpose object)

@@ +130,5 @@
> +   * extract the base domain from a history entry and its children
> +   * @param aEntry
> +   *        the history entry, serialized
> +   * @param aHosts
> +   *        the hash that will be used to store hosts eg, { hostname: true }

That piece of documentation looks wrong.

@@ +137,5 @@
> +   * @param aIsPinned
> +   *        is the entry we're evaluating for a pinned tab; used only if
> +   *        aCheckPrivacy
> +   */
> +  _extractHostsFromEntry: function (aEntry, aHosts, aCheckPrivacy, aIsPinned) {

Nit: Naming conventions for arguments doesn't seem to match the rest of the file.

@@ +196,5 @@
> +  /**
> +   * Returns the list of active session cookies for a given host.
> +   */
> +  _getCookiesForHost: function (host) {
> +    this._ensureInitialized();

Not sure it's relevant, but you should now be able to move |this._ensureInitialized| somewhere else.

Comment 3

[Steven MacLeod [:smacleod]]

Comment on attachment 790657 [details] [diff] [review]
Move SessionStore._updateCookies() to SessionCookies.update()

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

After the comments David had, looks good to me.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> > +  _getCookiesForHost: function (host) {
> > +    this._ensureInitialized();
> 
> Not sure it's relevant, but you should now be able to move
> |this._ensureInitialized| somewhere else.

Good catch, thanks!

Will address all other review comments. Thanks for looking at this to both of you!

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/120af945491b