Closed Bug 1007878 Opened 6 years ago Closed 6 years ago

Implement Open-Ended Dictionaries (MozMap)

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: bholley, Assigned: bzbarsky)

References

Details

Attachments

(8 files, 2 obsolete files)

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?
\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"
}
Blocks: 899322
Botond also had to do some serious hackery because we did not support this.
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.
What is the syntax we're considering here?
MozMap<SomeOtherType>.  So

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

for example.
(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.
Attachment #8421544 - Flags: review?(khuey)
Whiteboard: [need review]
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.
Attachment #8422207 - Flags: review?(khuey)
Attachment #8421544 - Attachment is obsolete: true
Attachment #8421544 - Flags: review?(khuey)
Attachment #8422207 - Attachment is obsolete: true
Attachment #8422207 - Flags: review?(khuey)
Blocks: 1011490
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
Attachment #8421543 - Flags: review?(khuey) → review+
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}
}
?
Attachment #8421545 - Flags: review?(khuey) → review+
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?
Attachment #8422798 - Flags: review?(khuey) → review+
> 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.
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.
Flags: needinfo?(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.
Attachment #8421543 - Flags: approval-mozilla-aurora?
Attachment #8421543 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
And https://hg.mozilla.org/releases/mozilla-aurora/rev/2671376afc4f to deal with the JSAPI changes since Aurora.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.