http://www.w3.org/TR/WebIDL/#dfn-dictionary We need to be able to specify default values for enum dictionary members. For an example of usage see titleDir and bodyDir in the notification API (http://dvcs.w3.org/hg/notifications/raw-file/tip/Overview.html#api)

So I looked into this. The parser part is pretty simple. But on the codegen end, we need to either add support for strings to convertIDLDefaultValueToJSVal or switch handling of default values to allow setting a C++ value directly instead of a jsval. Peter, which one do you think is better?

More up to date link: http://dev.w3.org/2006/webapi/WebIDL/#dfn-dictionary

(In reply to Boris Zbarsky (:bz) from comment #1) > switch handling of default values to allow > setting a C++ value directly instead of a jsval I think I like this one better.

With this patch, this IDL: void passOptionalLongWithDefault(optional long arg = 7); leads to this argument conversion code: int32_t arg0; if (0 < argc) { if (!ValueToPrimitive<int32_t>(cx, argv[0], &arg0)) { return false; } } else { arg0 = 7; } and similar for cases where the default value is null. And that's all the cases we support for now, pending the other patches in this bug.

With this patch, this IDL: void passEnumWithDefault(optional TestEnum arg = "a"); leads to this argument conversion code: TestEnum arg0; if (0 < argc) { { bool ok; int index = FindEnumStringIndex<true>(cx, argv[0], TestEnumValues::strings, "TestEnum", &ok); if (!ok) { return false; } MOZ_ASSERT(index >= 0); arg0 = static_cast<TestEnum>(index); } } else { arg0 = TestEnumValues::A; } and this IDL in a dictionary: TestEnum otherEnum = "b"; leads to this member conversion code: if (isNull) { found = false; } else if (!JS_HasPropertyById(cx, &val.toObject(), otherEnum_id, &found)) { return false; } if (found) { if (!JS_GetPropertyById(cx, &val.toObject(), otherEnum_id, &temp)) { return false; } } if (found) { { bool ok; int index = FindEnumStringIndex<true>(cx, temp, TestEnumValues::strings, "TestEnum", &ok); if (!ok) { return false; } MOZ_ASSERT(index >= 0); (this->otherEnum) = static_cast<TestEnum>(index); } } else { (this->otherEnum) = TestEnumValues::B; }

With this patch, this IDL: void passOptionalStringWithDefaultValue(optional DOMString arg = "abc"); leads to this argument conversion code: FakeDependentString arg0_holder; const NonNull<nsAString> arg0; if (0 < argc) { if (!ConvertJSValueToString(cx, argv[0], &argv[0], eStringify, eStringify, arg0_holder)) { return false; } } else { static const PRUnichar data[] = { 'a', 'b', 'c', 0 }; arg0_holder.SetData(data, ArrayLength(data) - 1); } const_cast<NonNull<nsAString>&>(arg0) = &arg0_holder; and this IDL in a dictionary: DOMString otherStr = "def"; leads to this member conversion code: if (isNull) { found = false; } else if (!JS_HasPropertyById(cx, &val.toObject(), otherStr_id, &found)) { return false; } if (found) { if (!JS_GetPropertyById(cx, &val.toObject(), otherStr_id, &temp)) { return false; } } { FakeDependentString str; if (found) { if (!ConvertJSValueToString(cx, temp, &temp, eStringify, eStringify, str)) { return false; } } else { static const PRUnichar data[] = { 'd', 'e', 'f', 0 }; str.SetData(data, ArrayLength(data) - 1); } (this->otherStr) = str; } also, this IDL: void passOptionalNullableStringWithDefaultValue(optional DOMString? arg = null); leads to this argument conversion code: FakeDependentString arg0_holder; const NonNull<nsAString> arg0; if (!ConvertJSValueToString(cx, (0 < argc) ? argv[0] : JSVAL_NULL, &argv[0], eNull, eNull, arg0_holder)) { return false; } const_cast<NonNull<nsAString>&>(arg0) = &arg0_holder;

(In reply to Boris Zbarsky (:bz) from comment #6) > also, this IDL: > > void passOptionalNullableStringWithDefaultValue(optional DOMString? arg = > null); > > leads to this argument conversion code: > > FakeDependentString arg0_holder; > const NonNull<nsAString> arg0; > if (!ConvertJSValueToString(cx, (0 < argc) ? argv[0] : JSVAL_NULL, > &argv[0], eNull, eNull, arg0_holder)) { No argc check for the pval argument?

Comment on attachment 643733 [details] [diff] [review] part 1. Rearrange default-value handling so we actually set C++ values directly instead of round-tripping through jsval. Review of attachment 643733 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +1459,5 @@ > argument with no default value. > > + invalidEnumValueFatal controls whether an invalid enum value conversion > + attempt will throw (if true) or simply return without doing anything (if > + false). Woops, sorry I didn't document it. @@ +2217,5 @@ > + IDLType.Tags.int64, IDLType.Tags.uint64, > + IDLType.Tags.float, IDLType.Tags.double]: > + defaultStr = defaultValue.value > + elif tag == IDLType.Tags.bool: > + defaultStr = "true" if defaultValue.value else "false" toBoolString or whatever it's called, maybe? @@ +2219,5 @@ > + defaultStr = defaultValue.value > + elif tag == IDLType.Tags.bool: > + defaultStr = "true" if defaultValue.value else "false" > + else: > + raise TypeError("Const value of unhandled type: " + value.type) Const value?

Comment on attachment 643734 [details] [diff] [review] part 2. Implement default values for WebIDL enums. Review of attachment 643734 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +2095,5 @@ > + "handleInvalidEnumValueCode" : handleInvalidEnumValueCode }) > + > + if defaultValue is not None: > + if isinstance(defaultValue, IDLNullValue): > + raise TypeError("Unexpected default value for enum") Don't we want to throw an error for all non-string defaults? ::: dom/bindings/parser/WebIDL.py @@ +1724,5 @@ > raise WebIDLError("Value %s is out of range for type %s." % > (self.value, type), [location]) > + elif self.type.isString() and type.isEnum(): > + # Just keep our string > + # Should we be checking whether this is a valid value for the enum? Would be nice, but I guess it's fine for now.

> No argc check for the pval argument? Nice catch! Will fix. > toBoolString or whatever it's called, maybe? Can do. > Const value? Hmm. It was copy/pasted from a place where it made sense. Will fix. > Don't we want to throw an error for all non-string defaults? Either way. Worst case, the c++ won't compile.

Actually, I just changed around string default value stuff so that we don't have to do the trimorph thing at all. Now the null string case looks like this: FakeDependentString arg0_holder; const NonNull<nsAString> arg0; if (0 < argc) { if (!ConvertJSValueToString(cx, argv[0], &argv[0], eNull, eNull, arg0_holder)) { return false; } } else { arg0_holder.SetNull(); } const_cast<NonNull<nsAString>&>(arg0) = &arg0_holder;

Comment on attachment 643734 [details] [diff] [review] part 2. Implement default values for WebIDL enums. Let's have a Kyle-Peter race to who reviews first. ;)

Comment on attachment 643886 [details] [diff] [review] part 1. Rearrange default-value handling so we actually set C++ values directly instead of round-tripping through jsval. Review of attachment 643886 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +1498,5 @@ > + # purposes of what we need to be declared as. > + assert(defaultValue is None or not isOptional) > + > + # Also, we should not have a defaultValue if we know we're an object > + assert(defaultValue is None or not isDefinitelyObject) Maybe it's just me, but it seems to make more sense here if the 'or' is reversed. @@ +1524,5 @@ > + "}" % > + CGIndenter(CGGeneric(setDefault)).define())).define() > + > + # A helper function for handling null default values. Much like > + # handleDefault, but checks that the default value is null. "the default value, if it exists, is null" is clearer, I think. @@ +2217,5 @@ > + if tag in [IDLType.Tags.int8, IDLType.Tags.uint8, > + IDLType.Tags.int16, IDLType.Tags.uint16, > + IDLType.Tags.int32, IDLType.Tags.uint32, > + IDLType.Tags.int64, IDLType.Tags.uint64, > + IDLType.Tags.float, IDLType.Tags.double]: Can we define this set of tags as a constant somewhere (numberTags or something?)

Comment on attachment 643734 [details] [diff] [review] part 2. Implement default values for WebIDL enums. Review of attachment 643734 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +2095,5 @@ > + "handleInvalidEnumValueCode" : handleInvalidEnumValueCode }) > + > + if defaultValue is not None: > + if isinstance(defaultValue, IDLNullValue): > + raise TypeError("Unexpected default value for enum") Shouldn't we throw in coerceToType for this case? Are we calling coerceToType on default values? ::: dom/bindings/parser/WebIDL.py @@ +1724,5 @@ > raise WebIDLError("Value %s is out of range for type %s." % > (self.value, type), [location]) > + elif self.type.isString() and type.isEnum(): > + # Just keep our string > + # Should we be checking whether this is a valid value for the enum? Yes. I'd like this to throw a sane error, rather than fail at compiletime or runtime.

Comment on attachment 643900 [details] [diff] [review] part 3. Implement default values for WebIDL strings. Review of attachment 643900 [details] [diff] [review]: ----------------------------------------------------------------- I think all the validation here should probably be in coerceToType ... ::: dom/bindings/Codegen.py @@ +2052,5 @@ > + if isinstance(defaultValue, IDLNullValue): > + if not type.nullable(): > + raise TypeError("Null default value for non-nullable " > + "string") > + return handleDefault(conversionCode, two spaces between return and handleDefault?

> Maybe it's just me, but it seems to make more sense here if the 'or' is reversed. > "the default value, if it exists, is null" is clearer, I think. OK. Fixed. > Can we define this set of tags as a constant somewhere (numberTags or something?) Yes, done. > Are we calling coerceToType on default values? Yes, we are, in IDLArgument.complete. I can convert the things it's already supposed to have checked into asserts. > Yes. I'd like this to throw a sane error, rather than fail at compiletime or runtime. Alright. Hooked that up with a snazzy: if self.value not in type.inner.values(): check. Now it says something like: error: 'd' is not a valid default value for enum Enum, <unknown> line 8:35 void foo(optional Enum e = "d"); ^ <unknown> line 2:10 enum Enum { (<unknown> because it's test code parsing from a string with no filename). > two spaces between return and handleDefault? Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a71640c1bb75 https://hg.mozilla.org/integration/mozilla-inbound/rev/d0456e27ab40 https://hg.mozilla.org/integration/mozilla-inbound/rev/06c2dd7e702d

https://hg.mozilla.org/mozilla-central/rev/a71640c1bb75 https://hg.mozilla.org/mozilla-central/rev/d0456e27ab40 https://hg.mozilla.org/mozilla-central/rev/06c2dd7e702d