Last Comment Bug 771636 - Support enum value as default dictionary member in WebIDL.
: Support enum value as default dictionary member in WebIDL.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 779573
Blocks: ParisBindings 759622
  Show dependency treegraph
 
Reported: 2012-07-06 13:19 PDT by William Chen [:wchen]
Modified: 2013-05-28 08:49 PDT (History)
4 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Rearrange default-value handling so we actually set C++ values directly instead of round-tripping through jsval. (32.86 KB, patch)
2012-07-18 23:09 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 2. Implement default values for WebIDL enums. (7.52 KB, patch)
2012-07-18 23:13 PDT, Boris Zbarsky [:bz]
khuey: review+
Details | Diff | Splinter Review
part 3. Implement default values for WebIDL strings. (6.99 KB, patch)
2012-07-18 23:20 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 1. Rearrange default-value handling so we actually set C++ values directly instead of round-tripping through jsval. (32.09 KB, patch)
2012-07-19 09:26 PDT, Boris Zbarsky [:bz]
khuey: review+
Details | Diff | Splinter Review
part 3. Implement default values for WebIDL strings. (7.25 KB, patch)
2012-07-19 10:07 PDT, Boris Zbarsky [:bz]
khuey: review+
Details | Diff | Splinter Review

Description William Chen [:wchen] 2012-07-06 13:19:37 PDT
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)
Comment 1 Boris Zbarsky [:bz] 2012-07-06 14:13:22 PDT
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?
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-07-07 03:11:09 PDT
More up to date link: http://dev.w3.org/2006/webapi/WebIDL/#dfn-dictionary
Comment 3 Peter Van der Beken [:peterv] 2012-07-10 14:12:51 PDT
(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.
Comment 4 Boris Zbarsky [:bz] 2012-07-18 23:09:42 PDT
Created attachment 643733 [details] [diff] [review]
part 1.  Rearrange default-value handling so we actually set C++ values directly instead of round-tripping through jsval.

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.
Comment 5 Boris Zbarsky [:bz] 2012-07-18 23:13:46 PDT
Created attachment 643734 [details] [diff] [review]
part 2.  Implement default values for WebIDL enums.

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;
  }
Comment 6 Boris Zbarsky [:bz] 2012-07-18 23:20:41 PDT
Created attachment 643735 [details] [diff] [review]
part 3.  Implement default values for WebIDL strings.

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;
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-07-19 03:02:16 PDT
(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 8 :Ms2ger (⌚ UTC+1/+2) 2012-07-19 05:40:34 PDT
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 9 :Ms2ger (⌚ UTC+1/+2) 2012-07-19 05:42:06 PDT
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.
Comment 10 Boris Zbarsky [:bz] 2012-07-19 06:38:37 PDT
> 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.
Comment 11 Boris Zbarsky [:bz] 2012-07-19 09:26:30 PDT
Created attachment 643886 [details] [diff] [review]
part 1.  Rearrange default-value handling so we actually set C++ values directly instead of round-tripping through jsval.
Comment 12 Boris Zbarsky [:bz] 2012-07-19 10:07:56 PDT
Created attachment 643900 [details] [diff] [review]
part 3.  Implement default values for WebIDL strings.

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 13 Boris Zbarsky [:bz] 2012-07-25 11:32:16 PDT
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 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-30 15:36:04 PDT
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 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-30 15:56:06 PDT
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 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-30 15:59:31 PDT
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?
Comment 17 Boris Zbarsky [:bz] 2012-07-30 20:31:59 PDT
> 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.

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