Closed Bug 1330699 Opened 3 years ago Closed 3 years ago

Implement the support for record<> in WebIDL Bindings

Categories

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

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: baku, Assigned: bzbarsky)

References

(Blocks 2 open bugs, )

Details

Attachments

(18 files, 19 obsolete files)

1.71 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.07 KB, text/plain
Details
4.65 KB, patch
Details | Diff | Splinter Review
15.72 KB, patch
qdot
: review+
Details | Diff | Splinter Review
5.12 KB, patch
qdot
: review+
Details | Diff | Splinter Review
1.10 KB, patch
qdot
: review+
Details | Diff | Splinter Review
1.26 KB, patch
qdot
: review+
Details | Diff | Splinter Review
5.57 KB, patch
qdot
: review+
Details | Diff | Splinter Review
3.22 KB, patch
qdot
: review+
Details | Diff | Splinter Review
5.76 KB, patch
qdot
: review+
Details | Diff | Splinter Review
101.23 KB, patch
qdot
: review+
Details | Diff | Splinter Review
106.84 KB, patch
qdot
: review+
Details | Diff | Splinter Review
4.73 KB, patch
qdot
: review+
Details | Diff | Splinter Review
8.99 KB, patch
qdot
: review+
Details | Diff | Splinter Review
5.72 KB, patch
qdot
: review+
Details | Diff | Splinter Review
15.23 KB, patch
qdot
: review+
Details | Diff | Splinter Review
56.12 KB, text/plain
Details
2.21 MB, text/plain
Details
No description provided.
Blocks: 1330678, 1330692
Blocks: 1331580
Blocks: 1336319
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8833486 - Flags: review?(kyle)
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.
Attachment #8833487 - Flags: review?(kyle)
The key type is unused so far.
Attachment #8833491 - Flags: review?(kyle)
Also renames all the test functions to mention "Record" instead of "MozMap".
Attachment #8833495 - Flags: review?(kyle)
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?
Attachment #8833482 - Flags: review?(nfroyd) → review+
> 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?
Attachment #8833495 - Attachment is obsolete: true
Attachment #8833495 - Flags: review?(kyle)
Attachment #8833572 - Flags: review?(kyle)
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...
Attachment #8833584 - Flags: review?(bugs)
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.
Attachment #8833485 - Flags: review?(bugs) → review+
Attachment #8833584 - Flags: review?(bugs)
> 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
Attachment #8834129 - Flags: review?(kyle)
Attachment #8833485 - Attachment is obsolete: true
Attachment #8833485 - Flags: review?(kyle)
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.
Attachment #8834132 - Flags: review?(kyle)
Attachment #8833486 - Attachment is obsolete: true
Attachment #8833486 - Flags: review?(kyle)
The key type is unused so far.
Attachment #8834142 - Flags: review?(kyle)
Attachment #8833491 - Attachment is obsolete: true
Attachment #8833491 - Flags: review?(kyle)
Also renames all the test functions to mention "Record" instead of "MozMap".
Attachment #8834143 - Flags: review?(kyle)
Attachment #8833572 - Attachment is obsolete: true
Attachment #8833572 - Flags: review?(kyle)
Attachment #8833540 - Flags: review?(annevk) → review+
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
Attachment #8833487 - Flags: review?(kyle)
Attachment #8833490 - Flags: review?(kyle)
Attachment #8833497 - Flags: review?(kyle)
Attachment #8833498 - Flags: review?(kyle)
Attachment #8833500 - Flags: review?(kyle)
Attachment #8833503 - Flags: review?(kyle)
Attachment #8834129 - Flags: review?(kyle)
Attachment #8834132 - Flags: review?(kyle)
Attachment #8834142 - Flags: review?(kyle)
Attachment #8834143 - Flags: review?(kyle)
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.
Attachment #8834129 - Attachment is obsolete: true
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.
Attachment #8837006 - Flags: review?(kyle)
Attachment #8833487 - Attachment is obsolete: true
Attachment #8833490 - Attachment is obsolete: true
The key type is unused so far.
Attachment #8837008 - Flags: review?(kyle)
Attachment #8834142 - Attachment is obsolete: true
Also renames all the test functions to mention "Record" instead of "MozMap".
Attachment #8837009 - Flags: review?(kyle)
Attachment #8834143 - Attachment is obsolete: true
Attachment #8833497 - Attachment is obsolete: true
Attachment #8833498 - Attachment is obsolete: true
Attachment #8833500 - Attachment is obsolete: true
Attachment #8833503 - Attachment is obsolete: true
Attachment #8833488 - Attachment is obsolete: true
Attachment #8834132 - Attachment is obsolete: true
Attachment #8833574 - Attachment is obsolete: true
Attachment #8833540 - Attachment is obsolete: true
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?
Attachment #8837001 - Flags: review?(kyle) → review+
> Should DataType be changed this to ValueType

Yes, done.
Attachment #8837002 - Flags: review?(kyle) → review+
Attachment #8837003 - Flags: review?(kyle) → review+
Attachment #8837004 - Flags: review?(kyle) → review+
Attachment #8837005 - Flags: review?(kyle) → review+
Attachment #8837006 - Flags: review?(kyle) → review+
Attachment #8837007 - Flags: review?(kyle) → review+
Attachment #8837008 - Flags: review?(kyle) → review+
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.
Attachment #8837009 - Flags: review?(kyle) → review+
> Just assuming the Data stuff in Record.h here will change

Yep, already done.
Attachment #8837010 - Flags: review?(kyle) → review+
Attachment #8837011 - Flags: review?(kyle) → review+
Attachment #8837012 - Flags: review?(kyle) → review+
Attachment #8837013 - Flags: review?(kyle) → review+
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
Depends on: 1366032
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.