952890 - Make sequence<> in WebIDL map to an iterable, not just something that quacks like an array

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See proposal at http://lists.w3.org/Archives/Public/public-script-coord/2013OctDec/0370.html

I have a prototype implementation of this, but it's ... very slow.  I'll file bugs blocking this for the JS engine bits we'll need to make this faster.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make WebIDL sequence JS to C++ conversions use for-of iteration, not length/index gets.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the IsArrayLike method, since it no longer matches any spec concept.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Reuben, the patches in this bug cause dom/contacts/tests/test_contacts_basics2.html to fail.  Specifically, this part of the test:

  684     ok(true, "Test setting array properties to scalar values")
  685     const FIELDS = ["email","url","adr","tel","impp"];
  686     createResult1 = new mozContact();
  687     for (var prop of FIELDS) {
  688       createResult1[prop] = {type: ["foo"]};
  689       ok(Array.isArray(createResult1[prop]), prop + " is array");
  690     }

Before this patch, doing 

  contact.email = { type: ["foo"] } 

just treats the right-hand side as a zero-length sequence; after this patch it throws an exception.  Is there a compat need to support the "treat it as a zero-length sequence" behavior here?

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is also blocked on bug 803106, because the idb tests assume (perfectly reasonably) that they can do db.transaction(db.objectStoreNames, {}) but db.objectStoreNames is a DOMStringList, which is not currently iterable, so it gets treated as a string, not a sequence.

Jonas, could we switch to using an actual array (possibly-frozen) for objectStoreNames?  I seem to recall you wanting to do something like that...

Comment 5

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky [:bz] from comment #3)
>   684     ok(true, "Test setting array properties to scalar values")

This should probably read "Test that after setting array properties to scalar values the property is not a non-Array".
 
> Before this patch, doing 
> 
>   contact.email = { type: ["foo"] } 
> 
> just treats the right-hand side as a zero-length sequence; after this patch
> it throws an exception.  Is there a compat need to support the "treat it as
> a zero-length sequence" behavior here?

The API was never specified to allow this type of stuff, and since we're already ignoring the RHS in this case today I don't think this has any compatibility problems.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So it's OK to just wrap those assignments in try/catch?

Comment 7

[Reuben Morais [:reuben]]

Yeah.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Jonas, could we switch to using an actual array (possibly-frozen) for
> objectStoreNames?  I seem to recall you wanting to do something like that...

Yes. This property falls into category B from https://www.w3.org/Bugs/Public/show_bug.cgi?id=23682

So I think we should return a frozen JS Array which contains a list of plain strings. We can return the same Array instance as long as the list remains unchanged. Whenever the list changes (happens only in response to calls to createObjectStore and deleteObjectStore) we start returning a new instance.

Same thing for IDBObjectStore.indexNames (changes in response to createIndex/deleteIndex)

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jonas Sicking (:sicking) from comment #8)
> So I think we should return a frozen JS Array which contains a list of plain
> strings. We can return the same Array instance as long as the list remains
> unchanged. Whenever the list changes (happens only in response to calls to
> createObjectStore and deleteObjectStore) we start returning a new instance.

That's exactly what the old code did, isn't it?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make WebIDL sequence JS to C++ conversions use for-of iteration, not length/index gets.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the IsArrayLike method, since it no longer matches any spec concept.

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b0dca8f792
https://hg.mozilla.org/integration/mozilla-inbound/rev/584e7a00ac79

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0b0dca8f792
https://hg.mozilla.org/mozilla-central/rev/584e7a00ac79