Re: [whatwg/storage] Clarify storage infrastructure (#86)

@inexorabletash commented on this pull request.

Mostly nitpicks.

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

> +the first time.
+
+<p>A <dfn>storage map</dfn> is a <a for=/>map</a> of <a>storage keys</a> to <a>storage units</a>. It
+is initially empty.
+
+<p>To <dfn>obtain a storage unit</dfn>, given a <a for=/>storage map</a> <var>map</var>, an
+<a>environment settings object</a> <var>environment</var>, and a <a>storage type</a>
+<var>type</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>key</var> be <var>environment</var>'s
+ <a for="environment settings object">origin</a>.
+
+ <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.

Do we need to define "disabled storage" somewhere?

>   <dd>Indexed DB, Cache API, service worker registrations, <code>localStorage</code>,
  <code>history.pushState()</code>, application caches, notifications, etc.
 </dl>
 
-<p>This standard primarily concerns itself with <dfn export id=site-storage>storage</dfn>.
+<p>This standard primarily concerns itself with storage.
+
+
+
+<h2 id=model>Model</h2>
+
+
+<h3 id=storage-units>Storage units</h3>
+
+<p>A <dfn>storage type</dfn> is "<code>more-durable</code>" or "<code>session</code>".

Further bikeshedding on the name: "perpetual" ?

> +
+
+<h3 id=storage-units>Storage units</h3>
+
+<p>A <dfn>storage type</dfn> is "<code>more-durable</code>" or "<code>session</code>".
+
+<p>A <dfn>storage key</dfn> is an <a for=/>origin</a>. [[HTML]]
+
+<p class=XXX>This is expected to change, see
+<a href="https://privacycg.github.io/storage-partitioning/">Client-Side Storage Partitioning</a>.
+
+<p>A <dfn id=site-storage-unit>storage unit</dfn> is the least granular unit of storage, other than
+the user agent. It holds a <dfn>bucket map</dfn>, which is a <a for=/>map</a> of
+<a for=/>strings</a> to <a>storage buckets</a>.
+
+<p class="note">For now "<code>default</code>" is the only <a for=map>key</a> that exists. See

Maybe clarify "that exists" to "that exists for bucket maps" ? Just because many keys/maps are defined nearby.

> + <a for="environment settings object">origin</a>.
+
+ <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.
+
+ <li>
+  <p>If <var>map</var>[<var>key</var>] does not <a for=map>exist</a>, then:
+
+  <ol>
+   <li><p>Let <var>unit</var> be a new <a>storage unit</a>.
+
+   <li><p>Set <var>unit</var>'s <a>bucket map</a>["<code>default</code>"] to the result of
+   <a>create a storage bucket</a> with <var>type</var>.
+
+   <li><p>Set <var>map</var>[<var>key</var>] to <var>unit</var>.

This may be clarified when addressing domenic's comments, but: as written, this would imply that the first "obtain" call with a given _type_ populates the bucket map with a unit of that type. This is fine, since a bucket map's contents should all be of the same type. But if that's the case, perhaps the bucket map should have a type, rather than it being always passed in.

> +<hr>
+
+<p>A <a for=/>user agent</a> holds a <dfn for="user agent">storage map</dfn>, which is a
+<a for=/>storage map</a>.
+
+<p>A browsing session holds a <dfn for="browsing session">storage map</dfn>, which is a
+<a for=/>storage map</a>.
+
+<p class=XXX>Browsing session is yet to be formally defined. For all intents and purposes it is a
+<a>top-level browsing context</a>, except that it survives the <a>top-level browsing context</a>
+being replaced due to a cross-origin opener policy.
+
+
+<h3 id=storage-endpoints>Storage endpoints</h3>
+
+<p>A <dfn export>storage endpoint</dfn> is a more-durable or session storage API that uses the

Do you want to quote/style "more-durable" and "session" here, for consistency?

> +<p>A <dfn>storage identifier</dfn> is an <a for=/>ASCII string</a>.
+
+<hr>
+
+<p>The <dfn>registered storage endpoints</dfn> are a <a for=/>set</a> of <a>storage endpoints</a>
+defined by the following table:
+<!-- Note, these will be exposed more generally through https://github.com/whatwg/storage/pull/69 so
+     please keep https://w3ctag.github.io/design-principles/#casing-rules in mind. -->
+
+<table>
+ <tr>
+  <th><a>Storage identifier</a>
+  <th><a>Storage types</a>
+ <tr>
+  <td>"<code>caches</code>"
+  <td>« "<code>more-durable</code>" »

It was discussed above but: since all of these sets have exactly one member, I'd appreciate a non-normative note calling out that this is intentional.

> + <tr>
+  <td>"<code>indexedDB</code>"
+  <td>« "<code>more-durable</code>" »
+ <tr>
+  <td>"<code>localStorage</code>"
+  <td>« "<code>more-durable</code>" »
+ <tr>
+  <td>"<code>serviceWorkerRegistrations</code>"
+  <td>« "<code>more-durable</code>" »
+ <tr>
+  <td>"<code>sessionStorage</code>"
+  <td>« "<code>session</code>" »
+</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>.

https://youtu.be/mZYu7DnsCF0?t=3


>  
-<p>A <a>bucket</a> is considered to be an atomic unit. Whenever a <a>bucket</a> is cleared by the
-user agent, it must be cleared in its entirety.
+<p>A <a>more-durable storage bucket</a> has a
+<dfn for="more-durable storage bucket" id=bucket-mode oldids=box-mode>mode</dfn>, which is
+"<code>best-effort</code>" or "<code>persistent</code>". It is initially "<code>best-effort</code>".
+
+<hr>

FWIW, this renders as a substantial amount of whitespace. I thought something was wrong with the spec, e.g. a missing image.

> +<hr>
+
+<p>A <dfn>session storage bucket</dfn> is a <a>storage bucket</a> for session storage APIs.
+
+<hr>
+
+<p>To <dfn>create a storage bucket</dfn>, given a <a>storage type</a> <var>type</var>, run these
+steps:
+
+<ol>
+ <li><p>Let <var>bucket</var> be null.
+
+ <li><p>If <var>type</var> is "<code>more-durable</code>", then set <var>bucket</var> to a new
+ <a>more-durable storage bucket</a>.
+
+ <li><p>Otherwise, if <var>type</var> is "<code>session</code>", set <var>bucket</var> to a new

nit: add "then" (I think we try and use if/then consistently?)

> +
+<hr>
+
+<p>To <dfn>create a storage bucket</dfn>, given a <a>storage type</a> <var>type</var>, run these
+steps:
+
+<ol>
+ <li><p>Let <var>bucket</var> be null.
+
+ <li><p>If <var>type</var> is "<code>more-durable</code>", then set <var>bucket</var> to a new
+ <a>more-durable storage bucket</a>.
+
+ <li><p>Otherwise, if <var>type</var> is "<code>session</code>", set <var>bucket</var> to a new
+ <a>session storage bucket</a>.
+
+ <li><p>Assert: <var>bucket</var> is a <a>storage bucket</a>.

Aside: this Assert surprised me, since it's not validating a precondition for the steps or some other algorithm, until I realized it's due to the "otherwise" case having an "if" and trying to ensure there's no new type snuck in to the enumeration? Or alternately write e.g.  Maybe a "Switch on..." would be clearer?

I've also written this as _Otherwise (type is session), set ..._, which is admittedly an implicit assertion tucked in there.

> +
+ <li><p>If <var>type</var> is "<code>more-durable</code>", then set <var>map</var> to the user
+ agent's <a for="user agent">storage map</a>.
+
+ <li>
+  <p>Otherwise, if <var>type</var> is "<code>session</code>", then set <var>map</var> to
+  <var>environment</var>'s <span class=XXX>browsing session</span>'s
+  <a for="browsing session">storage map</a>.
+
+  <p class="XXX">See
+  <a href="https://github.com/whatwg/html/issues/4782">whatwg/html issue #4782</a> and
+  <a href="https://github.com/whatwg/html/issues/5350">whatwg/html issue #5350</a> for defining
+  browsing session. It is roughly analogous to <a>top-level browsing context</a> except that it
+  cannot be replaced due to <code>Cross-Origin-Opener-Policy</code> or navigation.
+
+ <li><p>Assert: <var>map</var> is a <a for=/>storage map</a>.

Same aside as above.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/storage/pull/86#pullrequestreview-407700075