Closed
Bug 1007878
Opened 11 years ago
Closed 11 years ago
Implement Open-Ended Dictionaries (MozMap)
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: bholley, Assigned: bzbarsky)
References
Details
Attachments
(8 files, 2 obsolete files)
(deleted),
patch
|
khuey
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•11 years ago
|
Blocks: 924329, SH-bindings
Comment 1•11 years ago
|
||
\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
Comment 2•11 years ago
|
||
Botond also had to do some serious hackery because we did not support this.
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
What is the syntax we're considering here?
Assignee | ||
Comment 5•11 years ago
|
||
MozMap<SomeOtherType>. So
void doSomething(MozMap<(DOMString or SomeDictionary)> arg);
for example.
Comment 6•11 years ago
|
||
(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.
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8421543 -
Flags: review?(khuey)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8421544 -
Flags: review?(khuey)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8421545 -
Flags: review?(khuey)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8421546 -
Flags: review?(khuey)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8421547 -
Flags: review?(khuey)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [need review]
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8422207 -
Flags: review?(khuey)
Assignee | ||
Updated•11 years ago
|
Attachment #8421544 -
Attachment is obsolete: true
Attachment #8421544 -
Flags: review?(khuey)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8422798 -
Flags: review?(khuey)
Assignee | ||
Updated•11 years ago
|
Attachment #8422207 -
Attachment is obsolete: true
Attachment #8422207 -
Flags: review?(khuey)
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+
Attachment #8421546 -
Flags: review?(khuey) → review+
Attachment #8421547 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 19•11 years ago
|
||
> 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.
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
Flags: needinfo?(bzbarsky)
And XP's broke too: https://tbpl.mozilla.org/php/getParsedLog.php?id=40305291&tree=Mozilla-Inbound
Assignee | ||
Comment 22•11 years ago
|
||
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)
Assignee | ||
Comment 23•11 years ago
|
||
Assignee | ||
Comment 24•11 years ago
|
||
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
Flags: in-testsuite+
Whiteboard: [need review]
Assignee | ||
Comment 25•11 years ago
|
||
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?
Comment 26•11 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•11 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Updated•11 years ago
|
Attachment #8421543 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 27•11 years ago
|
||
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
Assignee | ||
Comment 28•11 years ago
|
||
And https://hg.mozilla.org/releases/mozilla-aurora/rev/2671376afc4f to deal with the JSAPI changes since Aurora.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•