Specific changes: 1) Null or undefined are valid values that lead to an empty MozMap. 2) MozMap is not distinguishable from nullable things. 3) MozMap has null as default value in arguments, but (unlike dictionaries) not in dictionary members. See also https://github.com/heycam/webidl/issues/76 and https://github.com/whatwg/fetch/issues/479 4) MozMap in trailing position must be optional. 5) A MozMap type cannot be used as the type of a constant; we already handled this for attributes.

The spec says to get all the property keys, then check each one for enumerability before doing the Get(). Our current code, before this change, asks for all the _enumerable_ keys instead, which is observably different when proxies are involved.

The key type is unused so far.

Also renames all the test functions to mention "Record" instead of "MozMap".

Comment on attachment 8833482 [details] [diff] [review] part 1. Add a ClearElementAt API to nsTArray Review of attachment 8833482 [details] [diff] [review]: ----------------------------------------------------------------- The shortest patches always get the most comments. r=me. I'm ambivalent about the comment, would like a better name, but not willing to go through a bunch of paint chips for this bikeshed, and strongly prefer the bounds-checked bit. ::: xpcom/ds/nsTArray.h @@ +1565,5 @@ > > + // Clear the element at the given index, and return a pointer to the cleared > + // element. This will destroy the existing element and default-construct a > + // new one, giving you a state much like what single-arg InsertElementAt(), or > + // no-arg AppendElement() does, but without changing the length of the array. Might be worth a note about how: array[idx] = T(); would accomplish the same thing, but T might not have the appropriate operator= methods for one reason or another. @@ +1566,5 @@ > + // Clear the element at the given index, and return a pointer to the cleared > + // element. This will destroy the existing element and default-construct a > + // new one, giving you a state much like what single-arg InsertElementAt(), or > + // no-arg AppendElement() does, but without changing the length of the array. > + elem_type* ClearElementAt(index_type aIndex) Maybe ReconstructElementAt? ClearElementAt sounds a bit like you're removing the element from the array, like std::vector::erase. @@ +1568,5 @@ > + // new one, giving you a state much like what single-arg InsertElementAt(), or > + // no-arg AppendElement() does, but without changing the length of the array. > + elem_type* ClearElementAt(index_type aIndex) > + { > + elem_type* elem = Elements() + aIndex; Can you do: elem_type* elem = &ElementAt(aIndex); so the access is bounds-checked?

> Might be worth a note about how: Done. > Maybe ReconstructElementAt? Done. > elem_type* elem = &ElementAt(aIndex); Done.

Comment on attachment 8833485 [details] [diff] [review] part 2. Change the MozMap API and data storage to more what we want record<> to look like >@@ -7281,26 +7280,23 @@ def wrapTypeIntoCurrentCompartment(type, > origType = type > if type.nullable(): > type = type.inner > value = "%s.Value()" % value > global mapWrapLevel > key = "mapName%d" % mapWrapLevel > mapWrapLevel += 1 > wrapElement = wrapTypeIntoCurrentCompartment(type.inner, >- "%s.Get(%sKeys[%sIndex])" % (value, key, key)) >+ "%sEntry.mValue" % key) > mapWrapLevel -= 1 > if not wrapElement: > return None > wrapCode = CGWrapper(CGIndenter(wrapElement), >- pre=(""" >- nsTArray<nsString> %sKeys; >- %s.GetKeys(%sKeys); >- for (uint32_t %sIndex = 0; %sIndex < %sKeys.Length(); ++%sIndex) { >- """ % (key, value, key, key, key, key, key)), >+ pre=("for (auto& %sEntry : %s.Entries()) {\n" % >+ (key, value)), > post="}\n") This particular piece is somehow hard to my eyes. Want to upload a small piece of generated code? > InternalHeaders::Fill(const MozMap<nsCString>& aInit, ErrorResult& aRv) > { >- nsTArray<nsString> keys; >- aInit.GetKeys(keys); >- for (uint32_t i = 0; i < keys.Length() && !aRv.Failed(); ++i) { >- Append(NS_ConvertUTF16toUTF8(keys[i]), aInit.Get(keys[i]), aRv); >+ for (auto& entry : aInit.Entries()) { >+ Append(NS_ConvertUTF16toUTF8(entry.mKey), entry.mValue, aRv); >+ if (aRv.Failed()) { >+ return; >+ } So with the new setup where MozMap is an array, that would allow multiple key-name pairs? Does that end up changing the behavior here?

I've pored over this some and fixed the things I found, but in case you want to see what the output codegen looks like in various situations...

> This particular piece is somehow hard to my eyes. Want to upload a small piece of generated code? It's not just hard on your eyes. It's super-ugly. I should rewrite it on top of fill()... I'll attach what the output looks like, both as of the part you're reviewing and with all patches applied. > So with the new setup where MozMap is an array, that would allow multiple key-name pairs? In theory, yes. In practice you can only produce that by using an object which claims to have the same property name multiple times, which is only doable with a proxy. And in any case, part 10 handles that case: if a property name is repeated, we only put it in the array once and keep the latest value we saw. I actually pushed for just allowing repeats in that edge case, but people weren't happy with that. ;) So in the end behavior does change somewhat for the proxy case, in that the exact set of operations (get list of property names, check which ones are enumerable, get property values) is in a different order and with a proxy you can do whatever you want at that point. For normal objects there is no change.

Hmm. Maybe I should rename those "mapName0Entry" bits too...

What does IsStringAnyRecord mean?

"StringAnyRecord" is the autogenerated type name for "record<DOMString, any>". Technically this is specced at https://heycam.github.io/webidl/#idl-record (search for "type name"). We use those type names in union accessors, for lack of something better... I'm not super happy with that. Maybe we should just use "Record" for the union accessors for a record, because it's not like you can have multiple records in a union...

Comment on attachment 8833485 [details] [diff] [review] part 2. Change the MozMap API and data storage to more what we want record<> to look like >@@ -7281,26 +7280,23 @@ def wrapTypeIntoCurrentCompartment(type, > origType = type > if type.nullable(): > type = type.inner > value = "%s.Value()" % value > global mapWrapLevel > key = "mapName%d" % mapWrapLevel > mapWrapLevel += 1 > wrapElement = wrapTypeIntoCurrentCompartment(type.inner, >- "%s.Get(%sKeys[%sIndex])" % (value, key, key)) >+ "%sEntry.mValue" % key) > mapWrapLevel -= 1 > if not wrapElement: > return None > wrapCode = CGWrapper(CGIndenter(wrapElement), >- pre=(""" >- nsTArray<nsString> %sKeys; >- %s.GetKeys(%sKeys); >- for (uint32_t %sIndex = 0; %sIndex < %sKeys.Length(); ++%sIndex) { >- """ % (key, value, key, key, key, key, key)), >+ pre=("for (auto& %sEntry : %s.Entries()) {\n" % >+ (key, value)), I think passing (key, value) is what makes this particularly hard for me to read, when value isn't actually the value mapped to the key, but value of the arg. Would it be possible to rename the variables a bit? Though, I don't have very good suggestions for the names.

> Would it be possible to rename the variables a bit? Hmm. So "value" comes from outside this entire block and is the "value" we're trying to wrap. But I could do something like this: if type.nullable(): mozMapRef = "%s.Value()" % value else mozMapRef = value entryRef = "mapEntry%d" % mapWrapLevel and replace |"%sEntry" % key| with |"%s" % entryRef| or so. How does that sound?

This will somewhat bitrot some of the other patches; I'll upload updated versions of those

Specific changes: 1) Null or undefined are valid values that lead to an empty MozMap. 2) MozMap is not distinguishable from nullable things. 3) MozMap has null as default value in arguments, but (unlike dictionaries) not in dictionary members. See also https://github.com/heycam/webidl/issues/76 and https://github.com/whatwg/fetch/issues/479 4) MozMap in trailing position must be optional. 5) A MozMap type cannot be used as the type of a constant; we already handled this for attributes.

The key type is unused so far.

Also renames all the test functions to mention "Record" instead of "MozMap".

Kyle, you should probably hold off on reviewing this until the spec folks get their act together. See https://github.com/whatwg/fetch/issues/479

Ok, saw discussion in #content. I've at least gotten up to date on issues surrounding the bug but hadn't started reviews yet, so just r? me after decisions are made.

The spec says to get all the property keys, then check each one for enumerability before doing the Get(). Our current code, before this change, asks for all the _enumerable_ keys instead, which is observably different when proxies are involved.

The key type is unused so far.

Also renames all the test functions to mention "Record" instead of "MozMap".

Comment on attachment 8837001 [details] [diff] [review] part 2. Change the MozMap API and data storage to more what we want record<> to look like Review of attachment 8837001 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/MozMap.h @@ +21,5 @@ > namespace mozilla { > namespace dom { > > namespace binding_detail { > +template<typename KeyType, typename DataType> not-very-important-nit: Should DataType be changed this to ValueType, just to be consistent?

> Should DataType be changed this to ValueType Yes, done.

Comment on attachment 8837009 [details] [diff] [review] part 10. Rename the MozMap C++ type to "record" and give it a template parameter for the key type Review of attachment 8837009 [details] [diff] [review]: ----------------------------------------------------------------- Just assuming the Data stuff in Record.h here will change with rebase on top of fixed patch 2.

> Just assuming the Data stuff in Record.h here will change Yep, already done.

Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/eca50d428320 part 1. Add a ClearElementAt API to nsTArray. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7450e715b5 part 2. Change the MozMap API and data storage to more what we want record<> to look like. r=qdot,smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/cd59aa1d7844 part 3. Fix up some minor issues with default value handling in codegen. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9bc7c6bdea part 4. Add more codegen tests for MozMap in dictionary r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/4b148b299f1a part 5. Disallow mozmap-typed constants. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/97d307213bf6 part 6. Add some tests for distinguishability of unions. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8ff160682e part 7. Change JS to MozMap conversion to more closely follow the record<> spec. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/52a24f98f12a part 8. Split up PrimitiveOrStringType into PrimitiveType and StringType in the Web IDL parser. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/60560ecf6ee3 part 9. Rename "MozMap" to "record" in our IDL parser and IDL files. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/794f653f1de6 part 10. Rename the MozMap C++ type to "record" and give it a template parameter for the key type. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/990c9e8d710e part 11. Add ConvertJSValueTo*String functions that just take a value and hand out a string, without extra complications. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/289f25464d09 part 12. Actually change the key type of a record, and its corresponding conversion behavior, depending on what the IDL says. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/85387004d587 part 13. Implement the spec provision for handling repeated keys in records by updating the existing value. r=qdot https://hg.mozilla.org/integration/mozilla-inbound/rev/01dd2928a1b7 part 14. Add some tests for the spec behavior. r=qdot

https://hg.mozilla.org/mozilla-central/rev/eca50d428320 https://hg.mozilla.org/mozilla-central/rev/0a7450e715b5 https://hg.mozilla.org/mozilla-central/rev/cd59aa1d7844 https://hg.mozilla.org/mozilla-central/rev/ce9bc7c6bdea https://hg.mozilla.org/mozilla-central/rev/4b148b299f1a https://hg.mozilla.org/mozilla-central/rev/97d307213bf6 https://hg.mozilla.org/mozilla-central/rev/1c8ff160682e https://hg.mozilla.org/mozilla-central/rev/52a24f98f12a https://hg.mozilla.org/mozilla-central/rev/60560ecf6ee3 https://hg.mozilla.org/mozilla-central/rev/794f653f1de6 https://hg.mozilla.org/mozilla-central/rev/990c9e8d710e https://hg.mozilla.org/mozilla-central/rev/289f25464d09 https://hg.mozilla.org/mozilla-central/rev/85387004d587 https://hg.mozilla.org/mozilla-central/rev/01dd2928a1b7