1007878 - Implement Open-Ended Dictionaries (MozMap)

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

One of the primary use-cases for |object|-typed arguments in WebIDL is for dictionaries whose keys are not from a pre-defined set. For example, the InstallTrigger API takes an argument like the following: {fooAddon: {/* details */ }, barAddon: { /* details */ }}. There's currently no way to represent this in WebIDL. And because of that, we can't represent the "details" part as a Dictionary (even though we know its fields), so we have to just go whole-hog |object|.

Apparently this is something that's been discussed in WebIDL for a while. But for now, Boris and I think it's pressing enough that we should implement something for internal use, which we're tentatively calling MozMap.

So in in the installTrigger case, we'd take a MozMap<InstallTriggerDetails>.

I'm hoping this will solve other use-cases as well. Jan-Ivar, would this be usable for the situation in bug 923904 comment 24?

Comment 1

[[:fabrice] Fabrice Desré]

\o/ that should also be useful for apps manifests where we have an icons property that looks like:

icons: {
 "128" : "/icon/128.png",
 "256" : "/icon/256.png"
}

Comment 2

[(no longer active)]

Botond also had to do some serious hackery because we did not support this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note to self: I probably want something like this:

  JS::AutoIdArray ids(cx, JS_Enumerate(cx, obj));

to get the equivalent of Object.keys.  Then JS_IdToValue, and then I can go ahead and stringify the values.  This will treat enumerable numeric props as keys too, but I think we actually want that here.

Comment 4

[(no longer active)]

What is the syntax we're considering here?

Comment 5

[Boris Zbarsky [:bzbarsky]]

MozMap<SomeOtherType>.  So

  void doSomething(MozMap<(DOMString or SomeDictionary)> arg);

for example.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Bobby Holley (:bholley) from comment #0)
> So in in the installTrigger case, we'd take a MozMap<InstallTriggerDetails>.
> 
> I'm hoping this will solve other use-cases as well. Jan-Ivar, would this be
> usable for the situation in bug 923904 comment 24?

Not entirely, because the dictionaries in the map would need to be polymorphic rather than of a single type, e.g. derivatives of RTCStats, a common base dictionary. http://dev.w3.org/2011/webrtc/editor/webrtc.html#derived-stats-dictionaries.

I assume MozMap<RTCStats> would not work for returning derivatives of RTCStats?

FWIW, right now we're solving it outside of WebIDL [1]. Since webidl getters are not available, I make the stats available as enumerable read-only properties directly on our content-facing object: https://bugzilla.mozilla.org/attachment.cgi?id=8350093&action=edit

---
[1] Confusingly, you'll also find a mildly deprecated non-spec MapClass-like attempt if you look in the webidl file, we support two ways right now.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add parsing of MozMap to the WebIDL parser

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a C++ type to represent MozMap

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add JS-to-C++ conversion for MozMap

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add C++-to-JS conversion for MozMap

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Implement callback codegen for MozMap

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: TestCodeGen bits for this patch queue

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: TestJSImplGenBinding.cpp

TestJSImplGen file; copying just the relevant bits is too much pain, but you can easily search for the function names you want to check over.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated so it builds on try

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2, with fixes to the includes/forward-decls

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8421543 [details] [diff] [review]
part 1.  Add parsing of MozMap to the WebIDL parser

Review of attachment 8421543 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +1711,5 @@
>  
> +class IDLMozMapType(IDLType):
> +    # XXXbz This is pretty similar to IDLSequenceType in various ways.
> +    # And maybe to IDLNullableType.  Should we have a superclass for
> +    # "type containing this other type"?

Probably not a bad idea, but we don't need to do it here.

@@ +1713,5 @@
> +    # XXXbz This is pretty similar to IDLSequenceType in various ways.
> +    # And maybe to IDLNullableType.  Should we have a superclass for
> +    # "type containing this other type"?
> +    def __init__(self, location, parameterType):
> +        assert not parameterType.isVoid()

Please add a test to ensure that this fails to parse.

@@ +1748,5 @@
> +        return self
> +
> +    def unroll(self):
> +        # We do not unroll our inner.  Just stop at ourselves.  That
> +        # lets us add hedears for both ourselves and our inner as

headers

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8421545 [details] [diff] [review]
part 3.  Add JS-to-C++ conversion for MozMap

Review of attachment 8421545 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +3823,5 @@
> +              binding_detail::FakeDependentString propName;
> +              if (!ConvertJSValueToString(cx, propNameValue, &propNameValue,
> +                                          eStringify, eStringify, propName)) {
> +                $*{exceptionCode}
> +              }

This is nit-picky, but why don't we just do

if (!JS_GetPropertyById(...) ||
    !JS_IdToValue(...) ||
    !ConvertJSValueToString(...)) {
  $*{exceptionCode}
}
?

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8422798 [details] [diff] [review]
Part 2, with fixes to the includes/forward-decls

Review of attachment 8422798 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/BindingUtils.h
@@ +1983,5 @@
>  };
>  
> +// XXXbz It's not clear whether it's better to add a pldhash dependency here
> +// (for PLDHashOperator) or add a BindingUtils.h dependency (for
> +// SequenceTracer) to MozMap.h...

pldhash seems like it will change less frequently to me ...

@@ +2102,5 @@
> +      mMozMapType(eNullableMozMap)
> +  {
> +  }
> +
> + private:

to the left

@@ +2127,5 @@
> +  }
> +
> +  union {
> +    MozMap<T>* mMozMap;
> +    Nullable<MozMap<T> >* mNullableMozMap;

>> is legit now

::: dom/bindings/MozMap.h
@@ +37,5 @@
> +    : nsStringHashKey(aOther),
> +      mData(Move(aOther.mData))
> +  {
> +  }
> +  

nit: whitespace at EOL

::: xpcom/glue/nsTHashtable.h
@@ +384,5 @@
>    : mTable(mozilla::Move(aOther.mTable))
>  {
>    // aOther shouldn't touch mTable after this, because we've stolen the table's
>    // pointers but not overwitten them.
> +  MOZ_MAKE_MEM_UNDEFINED(&aOther.mTable, sizeof(aOther.mTable));

Why does this mistake not cause problems?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Probably not a bad idea, but we don't need to do it here.

Filed bug 1015318.

> Please add a test to ensure that this fails to parse.
...
> headers

Done.

> pldhash seems like it will change less frequently to me ...

Sure.  It was more about whether it's better to pull pldhash into every single binding file or to pull in whatever gunk BindingUtils.h pulls in into everything using MozMap...  It's hard to say.

>to the left
..
>>> is legit now
..
>nit: whitespace at EOL

Fixed.

> Why does this mistake not cause problems?

I am the first to instantiate this template (move ctor on a nsTHashtable), apparently.  Go me?

> This is nit-picky, but why don't we just do

Good idea.  Done.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ce90f049d620 for b2g build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=40303820&tree=Mozilla-Inbound

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And XP's broke too: https://tbpl.mozilla.org/php/getParsedLog.php?id=40305291&tree=Mozilla-Inbound

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ehsan figured out what happened here.

The issue is an attempt to mozilla::Move a Nullable<T>, where T has an explicit move ctor and a copy ctor that is MOZ_DELETE.  Nullable itself has no copy or move ctors declared.

On platforms that support MOZ_DELETE, this works out, because T has no copy ctor but has a move ctor, Nullable gets an autogenerated move ctor, and life is good.

On platforms that do not support MOZ_DELETE (apparently b2g and Windows), T has a copy ctor (albeit a private one), so Nullable<T> has a copy ctor, so it gets no autogenerated move ctor, so the mozilla::Move tries to invoke the copy ctor, which fails because the copy ctor of T is private.

Going to add explicit move and copy ctors to Nullable.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Addendum to part 2 to fix the build bustage

Comment 24

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/b3983ebf6c52
   https://hg.mozilla.org/integration/mozilla-inbound/rev/806de4974bd6
   https://hg.mozilla.org/integration/mozilla-inbound/rev/eb38ffafe17d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/76d959ab8241
   https://hg.mozilla.org/integration/mozilla-inbound/rev/be4250144a86

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8421543 [details] [diff] [review]
part 1.  Add parsing of MozMap to the WebIDL parser

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Needed for some other fixes we want to
   backport.
User impact if declined: Probably can't fix bug 924329 on Aurora 31.
Testing completed (on m-c, etc.): Passes tests.
Risk to taking this patch (and alternatives if risky): Very low risk, since
   nothing is using this yet.
String or IDL/UUID changes made by this patch:  None

This approval request applies to the full 5-patch queue.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3983ebf6c52
https://hg.mozilla.org/mozilla-central/rev/806de4974bd6
https://hg.mozilla.org/mozilla-central/rev/eb38ffafe17d
https://hg.mozilla.org/mozilla-central/rev/76d959ab8241
https://hg.mozilla.org/mozilla-central/rev/be4250144a86

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9458c2fa26a
https://hg.mozilla.org/releases/mozilla-aurora/rev/35de1beea9c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/340a07fc3173
https://hg.mozilla.org/releases/mozilla-aurora/rev/00bc4a5b1c49
https://hg.mozilla.org/releases/mozilla-aurora/rev/3966a40fe6bb

Comment 28

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/releases/mozilla-aurora/rev/2671376afc4f to deal with the JSAPI changes since Aurora.