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

Clarify storage infrastructure #86

Merged
merged 22 commits into from
May 15, 2020
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 30, 2020

  • At least two implementers are interested (and none opposed):
    • Chrome
    • Firefox
  • Tests are written and can be reviewed and commented upon at:
    • N/A (or maybe for sessionStorage implications?)
  • Implementation bugs are filed:
    • N/A

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


Preview | Diff

This was referenced Apr 30, 2020
@annevk annevk force-pushed the annevk/storage-infrastructure branch from 0e2a03b to f3ca585 Compare May 2, 2020 12:21
@annevk annevk force-pushed the annevk/storage-infrastructure branch from f3ca585 to fd84f61 Compare May 2, 2020 12:39
@annevk
Copy link
Member Author

annevk commented May 2, 2020

I think this now defines sufficient infrastructure to define storage and session storage APIs. It does not:

  1. Define a replacement operation that is needed for Add API to allow origin to purge all storage #4 and Clear-Site-Data.
  2. Change the keying away from origin (though it does acknowledge and isolate the problem to a single point).

A storage API would do something like:

Let map be the result of running obtain a storage bucket area map with environment, "storage", and "localStorage".

It can then use that map as it pleases, for instance in response to API calls.

(Writing this down I realize we should probably expose a separate "obtain a storage bucket area map" and a "obtain a session storage bucket area map" so APIs do not have to pass a storage type.)

It should also provide a solid basis for any kind of multiple storage bucket API though in the absence of one looks a bit complicated.

If this looks agreeable I would like, in order:

  1. Merge this.
  2. Work on defining the replacement operation and get that merged.
  3. Work on a PR for localStorage/sessionStorage to make use of this.
  4. Continue work on storage partitioning for improvements to the storage key.

I could wait with 1 until 2 is further along, but I would greatly appreciate detailed feedback on this PR first.

@annevk annevk marked this pull request as ready for review May 2, 2020 13:11
@asutherland
Copy link
Collaborator

I understand this to be the hierarchy:

  1. Storage Map
  2. Storage Unit
  3. Storage Bucket
  4. (Storage) Area

While the following sounds very silly, my brain has a lot of trouble with there being no inherent containment relationship among the terms in use and with 3 of the 4 being generic. What about something like:

  1. Storage Shed
  2. Storage Shelf
  3. Storage Bucket
  4. Storage Bottle

@annevk
Copy link
Member Author

annevk commented May 4, 2020

Note that now it's clearly prefixed, "storage box" would work as well. Nobody commented on that suggestion from #51 (comment).

@pwnall
Copy link
Contributor

pwnall commented May 4, 2020

I'd like to express mild support for keeping bucket. It wasn't the first word that came to my mind, but it has grown on me.

Storage box is a bit of a mouthful. I worry that developers everywhere (web apps, browsers) will end up with variables named box instead of storageBox. I'd definitely use the word "boxes" when discussing this concept with my colleagues.

For better or worse, "bucket" is not used in other parts of the web platform (as far as I know). I think this will make everyone's (web developers, browser vendors) life easier when searching in code / docs. I expect that this concept will be a big deal, and deserves a unique name.

@annevk
Copy link
Member Author

annevk commented May 4, 2020

Fair. Any thoughts on @asutherland's suggested names? (And the remainder of the PR? 😊)

@annevk
Copy link
Member Author

annevk commented May 4, 2020

cc @youennf

storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Show resolved Hide resolved
storage.bs Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member Author

annevk commented May 7, 2020

I plan to address the remainder of @domenic's comments tomorrow as well as fully adopt @asutherland's fancy naming scheme. If you all have comments beyond those now would be a great time to make them. Thanks everyone!

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.

Mostly nitpicks.

I think there will be enough churn after domenic's comments are addressed that another read through will be needed.

storage.bs Outdated

<li><p>If <var>key</var> is an <a>opaque origin</a>, then return failure.

<li><p>If the user has disabled storage, then return failure.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to define "disabled storage" somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe as a follow-up? I'm not entirely sure what that would look like, but happy for someone to take a stab at it.

storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated
</table>

<p class="note no-backref">Standards can use these <a>storage identifiers</a> with
<a>obtain a more-durable storage bottle map</a> and <a>obtain a session storage bottle map</a>.
Copy link
Member

Choose a reason for hiding this comment

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

storage.bs Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member Author

annevk commented May 8, 2020

I did the refactoring in batches, but reviewing https://whatpr.org/storage/86.html#model is probably easier. Note that apart from the Model section this also improved various aspects of the Storage Standard itself, by making it much more clear what exactly various aspects are talking about and when things might fail and such.

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! (one maybe nit)

storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved
storage.bs Outdated Show resolved Hide resolved

<p class="XXX">This allows for the <a for="storage proxy map">backing map</a> to be replaced. This
is needed for <a href="https://github.com/whatwg/storage/issues/4">issue #4</a> and potentially the
<a href="https://privacycg.github.io/storage-access/">Storage Access API</a>.
Copy link
Member

Choose a reason for hiding this comment

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

I guess maybe it'll be used as part of the future infrastructure, but right now the "proxy map reference set" is append-only, and never read from.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also share this concern, although it seems like that's a future-work follow-up on the "replace" algorithm proposed at the bottom of #18 (comment) ?

I'm assuming that's what's going to happen, in which case we might as well wait, as it seems likely to be non-trivial.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you need to know who have a proxy map in order to let their proxies point elsewhere when storage is replaced.

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'm a bit surprised by this feedback though as I thought from #18 that we agreed on this approach.

Copy link
Collaborator

@asutherland asutherland left a comment

Choose a reason for hiding this comment

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

Multiple buckets, here we come! Thank you!


<p class="XXX">This allows for the <a for="storage proxy map">backing map</a> to be replaced. This
is needed for <a href="https://github.com/whatwg/storage/issues/4">issue #4</a> and potentially the
<a href="https://privacycg.github.io/storage-access/">Storage Access API</a>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also share this concern, although it seems like that's a future-work follow-up on the "replace" algorithm proposed at the bottom of #18 (comment) ?

I'm assuming that's what's going to happen, in which case we might as well wait, as it seems likely to be non-trivial.

@annevk annevk requested a review from domenic May 13, 2020 12:43
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.

Generally looks good. I think all the support infra for stuff that hasn't landed yet is a bit confusing as a reader, but I'm happy to leave decisions there to editor discretion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants