Add domintro blocks for “Web storage” interfaces #3179

sideshowbarker · 2017-10-31T04:16:29Z

This change adds domintro blocks in the “Web storage” section and adds
markup to cause more implementor-specific parts of that section of the
spec to be supressed or reworded in the developer edition.

/infrastructure.html ( diff )
/webstorage.html ( diff )

domenic

Awesome stuff; thank you! I think there's a few opportunities to make this better; let me know what you think. I'm also happy to put in some work directly if you want to move on from this; just let me know.

I realize some of my comments are about our difference of opinion on what kind of stuff is interesting in the developer edition. As I've said before, I'm happy to defer to your judgment, since you're the one doing the work. So these comments are just for your consideration. My Twitter poll seemed to indicate web developers find this stuff useful at least when they understand it, in case that helps.

domenic · 2017-11-07T19:24:06Z

source

+  <dl class="domintro">
+
+   <dt><var>localStorage</var> . <code subdfn data-x="dom-Storage-length">length</code></dt>
+   <dt><var>sessionStorage</var> . <code data-x="">length</code></dt>


I think these can be data-x="dom-Storage-length" but just omit the subdfn for the second one.

domenic · 2017-11-07T19:26:33Z

source

+  <dl class="domintro">
+
+   <dt><var>localStorage</var> . <code subdfn data-x="dom-Storage-length">length</code></dt>
+   <dt><var>sessionStorage</var> . <code data-x="">length</code></dt>


What do you think of using storage instead of two identical lines, one for localStorage and one for sessionStorage? I'm on the fence. I like writing the code similar to what web developers would write, but on the other hand this is supposed to be more API documentation, and from that perspective documenting "the Storage interface" using a generic variable seems better. Also, you're using <var>, not linking to the localStorage/sessionStorage properties, so an actual variable name like storage seems a bit better.

We could even add an intro dl along the lines of `storage = window . localStorage / storage = window . sessionStorage / Retrieves the appropriate storage object?" Maybe that's just weird though.

<dt><var>localStorage</var> . <code subdfn data-x="dom-Storage-length">length</code></dt> <dt><var>sessionStorage</var> . <code data-x="">length</code></dt>

What do you think of using storage instead of two identical lines, one for localStorage and one for sessionStorage?

I thought about that initially but to me at least that seems potentially even more confusing.

So as an alternative I pushed a change that does this instead:

<dt><code>Storage</code> . <code subdfn data-x="dom-Storage-length">length</code></dt>

…which has the advantage of being unambiguous at least — though I realize it’s not the way we do it for other domintro section. But the thing is, whatever we do here is anyway gonna necessarily be different from any of the other domintro sections.

That's no good; Storage.length is undefined (there is no static length property).

<dt><code>Storage</code> . <code subdfn data-x="dom-Storage-length">length</code></dt>

That's no good; Storage.length is undefined (there is no static length property).

Yeah, hadn’t really thought that through… so, just changed to using <var>storage</var>

domenic · 2017-11-07T19:26:53Z

source

+   <dt><var>sessionStorage</var> . <code data-x="">key</code> ( <var>n</var> )</dt>
+   <dd>
+    <p>Returns the name of the <var>n</var>th key in the list, or null if <var>n</var> is greater
+    than or equal to the number of key/value pairs in the object</p>


Missing period

domenic · 2017-11-07T19:29:06Z

source

+    the given <var>key</var> and with its value set to <var>value</var>.</p>
+
+    <p>If the given <var>key</var> <em>does</em> exist in the list, and its value is not
+    equal to <var>value</var>, then updates its value to <var>value</var>.</p>


Hmm, does this breakdown into checks/does not exist/does exist add something? It seems like we could just change these three paragraphs into something more like

Sets the value of the pair identified by key to value, creating a new key/value pair if none existed for key previously.

seems like we could just change these three paragraphs into something more like

Sets the value of the pair identified by key to value, creating a new key/value pair if none existed for key previously.

Thanks — yep, that’s much better, so I changed it to that

domenic · 2017-11-07T19:31:01Z

source

@@ -98211,6 +98211,62 @@ interface <dfn>Storage</dfn> {
  implementing the <code>Storage</code> interface can all be associated with the same list of
  key/value pairs simultaneously.</p>

+  <dl class="domintro">


All of these operations might be clearer if phrased using https://infra.spec.whatwg.org/#maps, possibly including linking to them?

Probably that should be a follow-up for both the dev-facing docs and the normative definitions, instead of being done here.

All of these operations might be clearer if phrased using https://infra.spec.whatwg.org/#maps, possibly including linking to them?

yeah, seems so

Probably that should be a follow-up for both the dev-facing docs and the normative definitions, instead of being done here.

Raised #3252

domenic · 2017-11-07T19:36:38Z

source

@@ -98259,6 +98314,8 @@ interface <dfn>Storage</dfn> {
  list associated with the object to be emptied of all key/value pairs, if there are any. If there
  are none, then the method must do nothing.</p>

+  </div>
+
  <p class="note">When the <code data-x="dom-Storage-setItem">setItem()</code>, <code


This note refers to stuff that got cut from the dev edition. I think we should either keep the paragraph about event firing in the localStorage/sessionStorage sections, or add discussion of the events fired to the domintro and hide this note in the dev edition.

OK, restored the event-firing requirements to the dev edition

domenic · 2017-11-07T19:38:08Z

source

@@ -98383,6 +98446,8 @@ interface <dfn>WindowLocalStorage</dfn> {
  <code>Window</code> object's <code data-x="dom-localStorage">localStorage</code> attribute's
  <code>Storage</code> object is associated with the same storage area, other than <var>x</var>, <span>send a storage notification</span>.

+  </div>
+
  <p class="warning">The <code data-x="dom-localStorage">localStorage</code> attribute provides


Should we add a domintro for window.localStorage to let developers know that sometimes accessing it throws? I'm OK either way.

Should we add a domintro for window.localStorage to let developers know that sometimes accessing it throws?

yes, added

domenic · 2017-11-07T19:39:24Z

source

+   <dt><var>event</var> . <code subdfn data-x="dom-StorageEvent-oldValue">oldValue</code></dt>
+
+   <dd>
+     <p>Returns the old value of the key being changed.</p>


We should refer to "the entry being changed"; the key is not being changed. Maybe the above should even be "the key of the entry whose value is being changed".

<p>Returns the old value of the key being changed.</p>

Went with “storage item” rather than “entry” — because we don’t use the term “entry” anywhere else in this section, and the section intro says:

Each Storage object provides access to a list of key/value pairs, which are sometimes called items.

domenic · 2017-11-07T19:40:18Z

source

+  </dl>
+
+  <div w-nodev>
+
  <p>The <dfn><code data-x="dom-StorageEvent-key">key</code></dfn> attribute must return the value


Maybe all the normative parts should remove the "It represents"? Then we can just collapse it all into a single "The X, Y, ..., and Z attributes must return the values they are initialized to."

<p>The <dfn><code data-x="dom-StorageEvent-key">key</code></dfn> attribute must return the value

Maybe all the normative parts should remove the "It represents"? Then we can just collapse it all into a single "The X, Y, ..., and Z attributes must return the values they are initialized to."

Yes. Made it so.

domenic · 2017-11-07T19:42:33Z

source

@@ -98610,6 +98715,8 @@ dictionary <dfn>StorageEventInit</dfn> : <span>EventInit</span> {
  <p>To this end, user agents should ensure that when deleting data, it is promptly deleted from the
  underlying storage.</p>


I think the disk space and privacy sections would be valuable or interesting for developers. Cutting "Implementation risks" makes sense for me. Maybe "sensitivity of data" is not interesting either.

OK, restored the Disk Space and Privacy sections to the dev edition

sideshowbarker · 2017-11-23T11:23:22Z

OK, I think I’ve responded to all the review comments, so this should now be ready for (re)review.

This change adds domintro blocks in the “Web storage” section and adds markup to cause more implementor-specific parts of that section of the spec to be supressed in the developer edition.

sideshowbarker · 2017-11-23T19:55:38Z

Made one more change: 7932bad takes the non-normative text about the storage event and marks it up with a subdfn for “send a storage notification” so that the existing hyperlinked references to that will also be hyperlinked in the dev edition.

To enable that to work, also added additional w-nohtml wording at the reference points, to make the statements informative rather than normative: In the dev edition they become “user agents send a storage notification” (statement of fact) rather than just “send a storage notification” (imperative).

TimothyGu · 2017-11-27T02:24:05Z

source

+    than or equal to the number of key/value pairs in the object.</p>
+   <dd>
+
+   <dt><code>Storage</code> . <code subdfn data-x="dom-Storage-getItem">getItem</code> ( <var>key</var> )</dt>


It would be valuable to have the getter/setter/deleter equivalent (e.g.

value = storage[key]
storage[key] = value
delete storage[key]

) in the domintro box alongside the functional forms too, similar to HTMLOptionsCollection. Also, the use of Storage instead of storage may be slightly misleading as a static member of the Storage class.

It would be valuable to have the getter/setter/deleter equivalent (e.g.

value = storage[key]
storage[key] = value
delete storage[key]

) in the domintro box alongside the functional forms too

Good idea — now added

Also, the use of Storage instead of storage may be slightly misleading as a static member of the Storage class.

Yes, changed the markup to just use <var>storage</var>

domenic

Made some last-minute tweaks, but LGTM. Thanks for your patience here; it's been a particularly busy time :). And I know I still owe you on #3173 too.

If you like my tweaks, feel free to squash and merge, with an appropriate commit message.

Of note, I also removed some bogus spec text that seemed to imply you could return null for window.sessionStorage. That contradicted the IDL (the attribute was not nullable) and also the sentence after it which said that such a storage area must always exist.

TimothyGu

LGTM.

sideshowbarker added the dev edition label Oct 31, 2017

sideshowbarker force-pushed the sideshowbarker/web-storage-section-add-domintro branch from 5323e66 to 9f7e8fe Compare October 31, 2017 04:23

domenic reviewed Nov 7, 2017

View reviewed changes

annevk added the topic: storage label Nov 8, 2017

sideshowbarker mentioned this pull request Nov 23, 2017

Rewrite spec text for the Storage interface operations to use ordered maps #3252

Closed

sideshowbarker force-pushed the sideshowbarker/web-storage-section-add-domintro branch from e982316 to d9d17d0 Compare November 23, 2017 11:21

sideshowbarker added 10 commits November 23, 2017 20:23

Add domintro blocks for “Web storage” interfaces

5dc34f5

This change adds domintro blocks in the “Web storage” section and adds markup to cause more implementor-specific parts of that section of the spec to be supressed in the developer edition.

Use <code>Storage</code> instead of <var>s

0c1067a

Add missing period

a97cb5c

Streamline the domintro for Storage.setItem

774aa64

Restore event-firing requirements to dev edition

3d64cce

Restore data-expiration paragraph to dev edition

7c36915

Include Disk Space, Privacy section in dev edition

44cfce5

Add domintro for window.localStorage

ecd5178

Reword in terms of “storage item” being changed

269ac43

Streamline StorageEvent attributes normative text

0de841a

sideshowbarker force-pushed the sideshowbarker/web-storage-section-add-domintro branch from d9d17d0 to 0de841a Compare November 23, 2017 11:28

Add dev-edition “send a storage notification” dfn

7932bad

sideshowbarker force-pushed the sideshowbarker/web-storage-section-add-domintro branch from 10322e8 to 7932bad Compare November 23, 2017 20:04

TimothyGu reviewed Nov 27, 2017

View reviewed changes

sideshowbarker and others added 2 commits November 29, 2017 13:52

Respond to review comments

f477a83

Some last domintro tweaks

13cde45

domenic approved these changes Dec 1, 2017

View reviewed changes

TimothyGu approved these changes Dec 1, 2017

View reviewed changes

domenic merged commit a390122 into master Dec 12, 2017

domenic deleted the sideshowbarker/web-storage-section-add-domintro branch December 12, 2017 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add domintro blocks for “Web storage” interfaces #3179

Add domintro blocks for “Web storage” interfaces #3179

sideshowbarker commented Oct 31, 2017 •

edited by pr-preview bot

Loading

domenic left a comment

domenic Nov 7, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

domenic Nov 23, 2017

sideshowbarker Nov 29, 2017

domenic Nov 7, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

domenic Nov 7, 2017

sideshowbarker Nov 23, 2017

sideshowbarker commented Nov 23, 2017

sideshowbarker commented Nov 23, 2017 •

edited

Loading

TimothyGu Nov 27, 2017

sideshowbarker Nov 29, 2017

domenic left a comment

TimothyGu left a comment

		@@ -98610,6 +98715,8 @@ dictionary <dfn>StorageEventInit</dfn> : <span>EventInit</span> {
		<p>To this end, user agents should ensure that when deleting data, it is promptly deleted from the
		underlying storage.</p>

Add domintro blocks for “Web storage” interfaces #3179

Add domintro blocks for “Web storage” interfaces #3179

Conversation

sideshowbarker commented Oct 31, 2017 • edited by pr-preview bot Loading

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sideshowbarker commented Nov 23, 2017

sideshowbarker commented Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

sideshowbarker commented Oct 31, 2017 •

edited by pr-preview bot

Loading

sideshowbarker commented Nov 23, 2017 •

edited

Loading