Closed
Bug 768537
Opened 9 years ago
Closed 9 years ago
Update dictionary support to recent changes
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: Ms2ger, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
22.17 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
13.30 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
![]() |
Assignee | |
Comment 1•9 years ago
|
||
1) Enforce the requirement that dictionary arguments not followed by a required argument are optional. 2) Make dictionaries no longer be distinguishable from nullable types. 3) Disallow dictionaries or unions containing dictionaries inside a nullable type. 4) Force optional dictionaries to have a default value of null so that codegen doesn't have to worry about dealing with optional arguments that have no default value in the IDL but need to be treated as if they were null.
Attachment #636825 -
Flags: review?(justin.lebar+bug)
![]() |
Assignee | |
Comment 2•9 years ago
|
||
Attachment #636826 -
Flags: review?(peterv)
![]() |
Assignee | |
Updated•9 years ago
|
Whiteboard: [need review]
Reporter | ||
Comment 3•9 years ago
|
||
A test for a dictionary argument followed by a required argument?
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 636826 [details] [diff] [review] part 2. Allow dictionaries to be initialized with null or undefined, and treat them as dictionaries in which everything has its default value. Review of attachment 636826 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +4133,5 @@ > > return (string.Template( > "struct ${selfName} ${inheritance}{\n" > " ${selfName}() {}\n" > + " bool Init(JSContext* cx, JS::Value val);\n" const JS::Value&?
![]() |
Assignee | |
Comment 5•9 years ago
|
||
Attachment #636828 -
Flags: review?(justin.lebar+bug)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #636825 -
Attachment is obsolete: true
Attachment #636825 -
Flags: review?(justin.lebar+bug)
![]() |
Assignee | |
Comment 6•9 years ago
|
||
Attachment #636829 -
Flags: review?(peterv)
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #636826 -
Attachment is obsolete: true
Attachment #636826 -
Flags: review?(peterv)
Comment 7•9 years ago
|
||
>+ if self.type.isDictionary() and self.optional and not self.defaultValue:
>+ # Default optional dictionaries to null, for simplicity
>+ self.defaultValue = IDLNullValue(self.location)
Nit: Could you say "so the codegen doesn't have to"? Otherwise it's not clear whether this is a correctness-affecting simplification.
I'm sorry for taking so long to get to this and the other patches; the Barcelona work week was kind of nuts.
Updated•9 years ago
|
Attachment #636828 -
Flags: review?(justin.lebar+bug) → review+
![]() |
Assignee | |
Comment 8•9 years ago
|
||
> Nit: Could you say "so the codegen doesn't have to"?
Will do.
No problem on the lag; I was gone most of that time myself. ;)
Comment 9•9 years ago
|
||
Comment on attachment 636829 [details] [diff] [review] Part 2 with Value ref (probably better with moving GC). Review of attachment 636829 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +4207,5 @@ > "selfName": self.makeClassName(d), > "initParent": CGIndenter(CGGeneric(initParent)).define(), > "initMembers": "\n\n".join(memberInits), > + "idInit": CGIndenter(idinit).define(), > + "isWorker": toStringBool(self.workers) |not self.workers|? See also bug 773326, I wonder if we should change something to make it harder to get this wrong.
Attachment #636829 -
Flags: review?(peterv) → review+
![]() |
Assignee | |
Comment 10•9 years ago
|
||
> |not self.workers|? Er, yes. <sigh> > I wonder if we should change something to make it harder to get this wrong Make it an enum instead of a bool? :( Or fix things so we have only one exception mechanism. ;)
![]() |
Assignee | |
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cad1e553bea https://hg.mozilla.org/integration/mozilla-inbound/rev/ea5a243a60f1
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla17
Comment 12•9 years ago
|
||
Sorry, push backed out for compilation errors on all platforms: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9deb8edb5070 eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=13605808&tree=Mozilla-Inbound { In file included from ../../../../js/xpconnect/wrappers/XrayWrapper.cpp:22:0: ../../../dist/include/nsTSubstring.h:639:18: error: 'nsAString_internal::char_type* nsAString_internal::mData' is protected ../../../dist/include/mozilla/dom/BindingUtils.h:794:77: error: within this context ../../../dist/include/nsTSubstring.h:640:17: error: 'nsAString_internal::size_type nsAString_internal::mLength' is protected ../../../dist/include/mozilla/dom/BindingUtils.h:795:79: error: within this context ../../../dist/include/nsTSubstring.h:641:16: error: 'PRUint32 nsAString_internal::mFlags' is protected ../../../dist/include/mozilla/dom/BindingUtils.h:796:78: error: within this context make[6]: *** [XrayWrapper.o] Error 1 } https://hg.mozilla.org/integration/mozilla-inbound/rev/8212be806c67
Target Milestone: mozilla17 → ---
![]() |
Assignee | |
Comment 13•9 years ago
|
||
That was from bug 773519. Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/524dd4cc9a4f https://hg.mozilla.org/integration/mozilla-inbound/rev/6102678421c0
Target Milestone: --- → mozilla17
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/524dd4cc9a4f https://hg.mozilla.org/mozilla-central/rev/6102678421c0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•2 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•