Last Comment Bug 768537 - Update dictionary support to recent changes
: Update dictionary support to recent changes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-06-26 10:22 PDT by :Ms2ger
Modified: 2012-07-18 05:55 PDT (History)
3 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Update parser support for dictionaries to spec changes. are several parts here: (18.49 KB, patch)
2012-06-26 12:10 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags 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. (13.29 KB, patch)
2012-06-26 12:12 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Part 1 now with test for required dictionary followed by another required arg. (22.17 KB, patch)
2012-06-26 12:25 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
justin.lebar+bug: review+
Details | Diff | Review
Part 2 with Value ref (probably better with moving GC). (13.30 KB, patch)
2012-06-26 12:28 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
peterv: review+
Details | Diff | Review

Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-26 12:10:48 PDT
Created attachment 636825 [details] [diff] [review]
part 1.  Update parser support for dictionaries to spec changes.   are several parts here:

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.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-26 12:12:30 PDT
Created 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.
Comment 3 :Ms2ger 2012-06-26 12:14:07 PDT
A test for a dictionary argument followed by a required argument?
Comment 4 :Ms2ger 2012-06-26 12:16:55 PDT
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&?
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-26 12:25:20 PDT
Created attachment 636828 [details] [diff] [review]
Part 1 now with test for required dictionary followed by another required arg.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-26 12:28:01 PDT
Created attachment 636829 [details] [diff] [review]
Part 2 with Value ref (probably better with moving GC).
Comment 7 Justin Lebar (not reading bugmail) 2012-07-02 07:38:29 PDT
>+        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.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-06 10:51:24 PDT
> 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 Peter Van der Beken [:peterv] 2012-07-17 04:41:36 PDT
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.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-07-17 08:59:53 PDT
> |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.  ;)
Comment 12 Ed Morley [:emorley] 2012-07-17 09:52:10 PDT
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

Note You need to log in before you can comment on or make changes to this bug.