Restore DOMStringList so ancestorOrigins can use it #2192

annevk · 2016-12-17T17:12:33Z

This requires several follow-up changes:

To DOM, so it’s no longer marked obsolete.
To Indexed DB, to start making use of this definition and remove its
own version.
To Service Workers, which can no longer use “ancestor origins array”.

This requires several follow-up changes: * To DOM, so it’s no longer marked obsolete. * To Indexed DB, to start making use of this definition and remove its own version. * To Service Workers, which can no longer use “ancestor origins array”. Fixes #2179.

inexorabletash · 2016-12-18T05:21:59Z

I'll take care of Indexed DB (of course) assuming the PR above lands before I vanish for the holidays.

domenic · 2016-12-19T20:10:35Z

source

@@ -7643,6 +7643,48 @@ https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C%21DOCTYPE%20html%3E.
  </div>


+  <h4>The <code>DOMStringList</code> interface</h4>
+
+  <p>The <code>DOMStringList</code> interface is a non-fashionable-retro way of representing a list


nit: I think this should be "non-fashionable retro" or "non-fashionable, retro" :)

domenic · 2016-12-19T20:12:01Z

source

+  <p>Each <code>DOMStringList</code> object has an associated <span>list</span>.</p>
+
+  <p>The <span>supported property indices</span> for a <code>DOMStringList</code> object are the
+  numbers zero to the number of items in its associated list minus one. If its associated list <span


Use the size concept from infra

domenic · 2016-12-19T20:13:52Z

source

+  <p>The <dfn data-x="dom-DOMStringList-item"><code>item(<var>index</var>)</code></dfn> method, when
+  invoked, must return the <var>index</var>th item in this <code>DOMStringList</code> object's
+  associated list, or null if there are less than <var>index</var> plus one items in this
+  <code>DOMStringList</code> object's associated list.</p>


Use size I think

domenic · 2016-12-19T20:16:04Z

source

-  produce:</p>
+  data-x="concept-location-ancestor-origins-list">ancestor origins list</span> must be set to a
+  <code>DOMStringList</code> object whose associated list is the list of strings that the following
+  steps would produce:</p>

  <ol>


Maybe spruce this up to xref infra while you're here? I think "[list] of strings" x2 and [Append] would suffice. Maybe remove "as a new value".

domenic · 2016-12-19T20:17:34Z

source

+  readonly attribute unsigned long <span data-x="dom-DOMStringList-length">length</span>;
+  getter DOMString? <span data-x="dom-DOMStringList-item">item</span>(unsigned long index);
+  boolean <span data-x="dom-DOMStringList-contains">contains</span>(DOMString string);
+};</pre>


I think we still need a domintro for this (like IndexedDB has, and like HTMLCollection has). Even if new APIs should not use this, developers should still be told how it works, since it's the only way to get ancestor origins.

That makes a lot of sense.

inexorabletash · 2016-12-20T17:36:51Z

source

+   <dt><var>strings</var> . <code subdfn data-x="dom-DOMStringList-length">length</code></dt>
+   <dd><p>Returns the number of strings in <var>strings</var>.</p></dd>
+
+   <dt><var>strings</var> . <code subdfn data-x="dom-DOMStringList-item">item</code>(<var>index</var>)</dt>


Should we document strings[index] as well? (as I had in Indexed DB, and we do for DOMTokenList)

The reason I didn't is that getters are even more old-fashioned than DOMStringList and you don't need them to use the API.

Hmm, I think the getter is more modern than the item() function. Ideally we'd like people to stop using item() so that we could get rid of this API---at least, that was the original thinking. Anyway, consistency with DOMTokenList seems important.

I can remove it from DOMTokenList too. I don't think we should market these "legacy" (hopefully soon marked as such through IDL) features anywhere. E.g., consider strings["item"].

Indexes seems necessary for compat.

https://github.com/search?l=JavaScript&q=ancestororigins&type=Code&utf8=✓
https://github.com/search?l=JavaScript&q=objectstorenames+NOT+test&ref=searchresults&type=Code&utf8=✓

The point is, I think item() is more legacy than indexing.

I agree with @domenic - I think we want to steer developers towards the Array-like API to interact with this wart so we could maybe drop it, but even if we can't for compat we're not making developers learn a new way of interacting with an Array-like thing. (Making it iterable and adding includes() as an alias for contains() would also help.)

annevk · 2017-01-10T09:09:31Z

I think IDL getters are just plain bad, but I'll fix this.

annevk · 2017-01-10T09:10:09Z

I guess it's slightly less bad here because it's not names, just numbers.

This rewrites ancestorOrigins using DOMStringList instead of FrozenArray to USVString reflecting the changes in HTML. Related pull request: whatwg/html#2192. Related issue: #1051.

domenic

Looks good, needs tests

annevk · 2017-01-10T18:24:10Z

Isn't this just restoring the status quo? I guess I can look into tests at some point...

annevk · 2017-01-10T18:24:51Z

Probably not a priority for me if Firefox doesn't actually plan on shipping this.

zcorpan · 2017-01-10T23:36:42Z

I think this is OK to merge without tests as it's just moving from one spec to another. But tests would be good; filed web-platform-tests/wpt#4522

domenic · 2017-01-11T01:22:16Z

Part of the issue here I believe is that WebKit tried to follow the new web platform tests and ran into web compat problems, so it seems important to address that.

inexorabletash · 2017-01-12T18:27:07Z

source

+  <pre class="idl">[Exposed=(Window,Worker)]
+interface <dfn>DOMStringList</dfn> {
+  readonly attribute unsigned long <span data-x="dom-DOMStringList-length">length</span>;
+  getter DOMString? <span data-x="dom-DOMStringList-item">item</span>(unsigned long index);


IndexedDB has separate getter and item() to handle the difference in behavior when an index out of range is specified (getter returns undefined, item() returns null).

Maybe that's automagically handled by WebIDL.

That's handled by saying what the supported property indices are.

But WebIDL says:

If an indexed property getter was specified using an operation with an identifier, then the value returned when indexing the object with a given supported property index is the value that would be returned by invoking the operation, passing the index as its only argument.

But that's not true given the behavior in Chrome, where dsl[999] returns undefined while dsl.item(999) returns null.

(Unless, as usual, I'm missing something.)

Ignoring that issue, the determine the value of an indexed property in this patch is irrelevant; that's only needed if the operation did not have an identifier:

If the operation used to declare the indexed property getter did not have an identifier, then the interface definition must be accompanied by a description of how to determine the value of an indexed property for a given index.

(BTW, thanks to everyone for being nitpicky along with me! I can't wait to tear this out of IndexedDB)

999 is not a supported property index

What @domenic said. I know it is confusing. ^_^

Thanks, I'm parsing it correctly now. Yes, null vs. undefined falls out; if not a supported property index, item() produces null (per prose) and indexing with [] does a normal property lookup and produces undefined. Thanks for bearing with me.

I still believe the prose for determine the value of an indexed property is unnecessary as that clause should not apply.

Yeah, I think that paragraph can be dropped. https://dom.spec.whatwg.org/#interface-nodelist doesn't have "determine the value of an indexed property".

inexorabletash · 2017-01-12T23:33:27Z

Can whoever merges this hit the merge button for web-platform-tests/wpt#4534 too?

(could do it now but might as well coordinate)

The getter has an identifier ("item"), so that is invoked per https://heycam.github.io/webidl/#idl-indexed-properties

See whatwg/html#2192. Fixes #4522.

Per whatwg/html#2192 it's now in HTML.

* Use DOMStringList for ancestorOrigins This reflects the change in HTML that restored ancestor origins list. This change uses the ancestor origins list to create a frozen array for windowClient.ancestorOrigins. Related pull request: whatwg/html#2192. Fixes #1051.

This was referenced Dec 17, 2016

DOMStringList has been removed from DOM w3c/IndexedDB#28

Closed

Point to HTML for DOMStringList whatwg/dom#385

Merged

transaction should either take DOMStringList or DOMStringList needs to be iterable w3c/IndexedDB#85

Closed

domenic requested changes Dec 19, 2016

View reviewed changes

address review feedback

5f6b2f0

inexorabletash reviewed Dec 20, 2016

View reviewed changes

add indexed getter to domintro

542cfcd

annevk mentioned this pull request Jan 10, 2017

"ancestor origins array" is going away w3c/ServiceWorker#1051

Closed

jungkees mentioned this pull request Jan 10, 2017

Use DOMStringList for ancestorOrigins w3c/ServiceWorker#1052

Merged

zcorpan approved these changes Jan 10, 2017

View reviewed changes

domenic added the needs tests Moving the issue forward requires someone to write tests label Jan 10, 2017

domenic approved these changes Jan 10, 2017

View reviewed changes

zcorpan mentioned this pull request Jan 10, 2017

Add tests for DOMStringList and ancestorOrigins web-platform-tests/wpt#4522

Closed

inexorabletash reviewed Jan 12, 2017

View reviewed changes

inexorabletash mentioned this pull request Jan 12, 2017

Add tests for DOMStringList web-platform-tests/wpt#4534

Merged

zcorpan removed the needs tests Moving the issue forward requires someone to write tests label Jan 12, 2017

Remove text that does not apply

c46dc09

The getter has an identifier ("item"), so that is invoked per https://heycam.github.io/webidl/#idl-indexed-properties

zcorpan pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 13, 2017

Add tests for DOMStringList

b94e060

See whatwg/html#2192. Fixes #4522.

zcorpan merged commit a830aef into master Jan 13, 2017

zcorpan deleted the annevk/DOMStringList branch January 13, 2017 00:55

inexorabletash mentioned this pull request Jan 13, 2017

DOMStringList should have [Exposed=(Window,Worker)] w3c/IndexedDB#118

Closed

annevk pushed a commit to whatwg/dom that referenced this pull request Jan 13, 2017

Point to HTML for DOMStringList

3fde24e

Per whatwg/html#2192 it's now in HTML.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore DOMStringList so ancestorOrigins can use it #2192

Restore DOMStringList so ancestorOrigins can use it #2192

annevk commented Dec 17, 2016

inexorabletash commented Dec 18, 2016

domenic Dec 19, 2016

domenic Dec 19, 2016

domenic Dec 19, 2016

domenic Dec 19, 2016

domenic Dec 19, 2016

annevk Dec 20, 2016

inexorabletash Dec 20, 2016

annevk Dec 20, 2016

domenic Dec 20, 2016

annevk Dec 21, 2016

zcorpan Dec 21, 2016

domenic Dec 21, 2016

inexorabletash Dec 21, 2016

annevk commented Jan 10, 2017

annevk commented Jan 10, 2017

domenic left a comment

annevk commented Jan 10, 2017

annevk commented Jan 10, 2017

zcorpan commented Jan 10, 2017

domenic commented Jan 11, 2017

inexorabletash Jan 12, 2017

zcorpan Jan 12, 2017

inexorabletash Jan 12, 2017

domenic Jan 12, 2017

zcorpan Jan 12, 2017

inexorabletash Jan 12, 2017

zcorpan Jan 13, 2017

inexorabletash commented Jan 12, 2017

Restore DOMStringList so ancestorOrigins can use it #2192

Restore DOMStringList so ancestorOrigins can use it #2192

Conversation

annevk commented Dec 17, 2016

inexorabletash commented Dec 18, 2016

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

annevk commented Jan 10, 2017

annevk commented Jan 10, 2017

domenic left a comment

Choose a reason for hiding this comment

annevk commented Jan 10, 2017

annevk commented Jan 10, 2017

zcorpan commented Jan 10, 2017

domenic commented Jan 11, 2017

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

inexorabletash commented Jan 12, 2017