Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Web storage #5560

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Refactor Web storage #5560

merged 2 commits into from
Jul 10, 2020

Conversation

annevk
Copy link
Member

@annevk annevk commented May 18, 2020

This makes use of the new primitives in the Storage Standard.

Closes #3210, closes #3283, closes #4650, and closes #5463.

Actually obtaining a storage bottle and dealing with storage bottle cloning to close #5498 still needs to be written down.

Some (removed) notes should maybe also move into Storage or elsewhere within HTML.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: …
    • Firefox: …
    • Safari: …

(See WHATWG Working Mode: Changes for more details.)


/browsers.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )
/webstorage.html ( diff )

@annevk annevk changed the base branch from annevk/storage-cleanup to master May 18, 2020 15:57
@annevk annevk force-pushed the annevk/refactor-web-storage branch from 93ac38d to 61ec86c Compare May 18, 2020 15:58
annevk added a commit to whatwg/storage that referenced this pull request May 20, 2020
This can be used by APIs that share keying logic with storage, such as BroadcastChannel and shared workers.

Also add and export create a storage shelf so HTML can more cleanly clone sessionStorage data as part of whatwg/html#5560.

Closes #92.
annevk added a commit to whatwg/storage that referenced this pull request May 20, 2020
"legacy-clone a browsing session storage shed" can be used by HTML to define creation of auxiliary browsing contexts, as part of whatwg/html#5560.

"obtain a storage key" can be used by APIs that share keying logic with storage, such as BroadcastChannel and shared workers. See whatwg/html#3054. It's potentially also useful for Indexed DB as discussed in w3c/IndexedDB#334.

Closes #92.
@annevk
Copy link
Member Author

annevk commented May 20, 2020

This now depends on whatwg/storage#93.

I'm happy with the normative aspects, except perhaps that there's not a clean way to encapsulate that modifying the map can fail and that it seems we only address that when setting. Though the latter is an existing issue.

I haven't really thought long about the non-normative parts. I think a fair bit could probably move to the Storage Standard, but I'm not sure to what extent.

@annevk annevk marked this pull request as ready for review May 20, 2020 12:32
@annevk annevk requested a review from domenic May 20, 2020 12:33
@annevk
Copy link
Member Author

annevk commented May 20, 2020

cc @whatwg/storage

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "lost" text consists of the following:

Each top-level browsing context has a unique set of session storage areas, one for each origin.

Sounds like this is moving to "browsing session" in some way, and generally handled by Storage? I'm OK removing this, as the domintro makes it vaguely clear that session storage is per-origin.

User agents should not expire data from a browsing context's session storage areas, but may do so when the user requests that such data be deleted, or when the UA detects that it has limited storage space, or for security reasons. User agents should always avoid deleting data while a script that could access that data is running. When a top-level browsing context is destroyed (and therefore permanently inaccessible to the user) the data stored in its session storage areas can be discarded with it, as the API described in this specification provides no way for that data to ever be subsequently retrieved.

User agents should expire data from the local storage areas only for security reasons or when requested to do so by the user. User agents should always avoid deleting data while a script that could access that data is running.

These should be moved to Storage somewhere, and maybe linked to from here.

The lifetime of a browsing context can be unrelated to the lifetime of the actual user agent process itself, as the user agent can support resuming sessions after a restart.

This seems important for our eventual definition of browsing session, hrm.

In the case of an iframe being moved to another Document, its nested browsing context is destroyed and a new one created.

This seems overly-detailed and I'm fine with it disappearing.

The localStorage attribute provides access to shared state. This specification does not define the interaction with other browsing contexts in a multiprocess user agent, and authors are encouraged to assume that there is no locking mechanism. A site could, for instance, try to read the value of a key, increment its value, then write it back out, using the new value as a unique identifier for the session; if the site does this twice in two different browser windows at the same time, it might end up using the same "unique" identifier for both sessions, with potentially disastrous effects.

I think this is worth keeping (and generalizing to include sessionStorage?) in some form. It's unique to localStorage and its sync nature, as opposed to IDB or similar.

Also worth noting, the whole "Disk space", "Privacy", and "Security" sections might be best moved to Storage. That could be done in a separate PR though.

source Outdated
keys were last added to the storage area.</p>
<dt><dfn data-x="concept-Storage-type">type</dfn>
<dd>"<code data-x="">local</code>" or "<code data-x="">session</code>".
</dl>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd just inline these into the preceding <p> since they're so short and simple.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the type between parenthesis or something? I quite like using dl for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking

has an associated map, which is a story proxy map, and type, which is either "local" or "session".

I think dl makes sense usually but it takes up a lot of vertical space for simple things like this, where the dds are only a few words. It's more useful when the dds are long explanations.

Maybe it would be better if we had a dl class="brief" which made each item take a single line instead of two lines.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
annevk added a commit to whatwg/infra that referenced this pull request May 29, 2020
annevk added a commit to whatwg/infra that referenced this pull request May 29, 2020
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting solid. Optional: I think the order of the whole section is a bit strange, with interface/getter/getter/interface. My preferred ordering would be localStorage/sessionStorage/Storage/StorageEvent, although putting localStorage before sessionStorage would also require some changing of the introduction, so sessionStorage/localStorage/Storage/StorageEvent is also fine.

source Outdated Show resolved Hide resolved
source Outdated
keys were last added to the storage area.</p>
<dt><dfn data-x="concept-Storage-type">type</dfn>
<dd>"<code data-x="">local</code>" or "<code data-x="">session</code>".
</dl>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking

has an associated map, which is a story proxy map, and type, which is either "local" or "session".

I think dl makes sense usually but it takes up a lot of vertical space for simple things like this, where the dds are only a few words. It's more useful when the dds are long explanations.

Maybe it would be better if we had a dl class="brief" which made each item take a single line instead of two lines.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
annevk added a commit that referenced this pull request Jun 2, 2020
Having a central point to acknowledge its non-existence will hopefully provide some clarity for #5560 and other changes.
annevk added a commit that referenced this pull request Jun 3, 2020
Having a central point to acknowledge its non-existence will hopefully provide some clarity for #5560 and other changes.
@annevk annevk force-pushed the annevk/refactor-web-storage branch from b36a62f to 319789d Compare June 5, 2020 12:25
@annevk
Copy link
Member Author

annevk commented Jun 5, 2020

whatwg/storage#93 and whatwg/storage#97 tackle the Storage side of this. I suspect moving the Privacy/Security sections is best done as a follow-up as it is getting rather unwieldy already.

annevk added a commit to whatwg/storage that referenced this pull request Jun 5, 2020
"legacy-clone a browsing session storage shed" can be used by HTML to define creation of auxiliary browsing contexts, as part of whatwg/html#5560.

"obtain a storage key" can be used by APIs that share keying logic with storage, such as BroadcastChannel and shared workers. See whatwg/html#3054. It's potentially also useful for Indexed DB as discussed in w3c/IndexedDB#334.

Also helps a bit with #95 by reorganizing and adding some more detail to how a user agent is supposed to manage storage.

Closes #92.
Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A couple nits about non-normative sections.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<code>Document</code>'s <span>origin</span> is an <span data-x="concept-origin-opaque">opaque
origin</span> or if the request violates a policy decision (e.g. if the user agent is
configured to not allow the page to persist data).</p>
<p>Returns the <code>Storage</code> object associated with that <var>window</var>'s origin's
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe update to something more correct than "origin's session storage area" ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to keep area here for now until we move the Privacy/Security sections.

@domenic
Copy link
Member

domenic commented Jun 9, 2020

This seems editorially reasonable but I continue to be pretty discontented with the removal of all the helpful text and replacing it by less helpful things in Storage, or in some cases by nothing. So I'd rather resolve whatwg/storage#95 first.

Use the new primitives in the Storage Standard.

Closes #3209, closes #3210, closes #3283, closes #4650, closes #5463, and closes #5498.
@annevk annevk force-pushed the annevk/refactor-web-storage branch from 9db9276 to d4855e3 Compare July 8, 2020 14:19
source Show resolved Hide resolved
@annevk annevk merged commit 09c8f6e into master Jul 10, 2020
@annevk annevk deleted the annevk/refactor-web-storage branch July 10, 2020 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants