IDL: Enumerations support in dictionaries

IDL: Enumerations support in dictionaries

Enumerations are essentially the same as Strings, but those values
are restricted to specific values. When a value is invalid, we should
throw an exception as the spec requires[1].

[1] http://heycam.github.io/webidl/#es-enumeration

BUG=321462
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181729

[bashi, 2014-09-10 05:58:51]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org

[bashi, 2014-09-10 05:58:52]

Haraken-san, could you take a look when you have time?

[Jens Widell, 2014-09-10 06:20:15]

jl@opera.com changed reviewers:
+ jl@opera.com

[Jens Widell, 2014-09-10 06:20:15]

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:16: {{cpp_class}}*
{{v8_class}}::toImpl(v8::Isolate* isolate, v8::Handle<v8::Value> v8Value,
ExceptionState& exceptionState)
This sort of conflicts with where I was going with
https://codereview.chromium.org/559553003/ ultimately; to have all functions
that throw exceptions via an ExceptionsState& argument throw all their
exceptions that way.

This function effectively throws exceptions directly via V8 sometimes, since it
calls into V8 via DictionaryHelper. Any exceptions thrown that way are ignored
by this code, but would probably be caught by a v8::TryCatch in the caller, and
then propagated.

This does not block this patch, though. Just saying. :-)

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:27: if
({{member.enum_validation_expression}})
Rephrase this as

  if (!{{member.enum_validation_expression}}) {
    exceptionState.throwTypeError("member {{member.name}} ('" + string + "') is
not a valid enum value.");
    return NULL;
  }

and thus avoid duplicating the "impl->{{member.setter_name}}({{member.name}});"
line?

[bashi, 2014-09-10 07:21:04]

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:16: {{cpp_class}}*
{{v8_class}}::toImpl(v8::Isolate* isolate, v8::Handle<v8::Value> v8Value,
ExceptionState& exceptionState)
On 2014/09/10 06:20:15, Jens Widell wrote:
> This sort of conflicts with where I was going with
> https://codereview.chromium.org/559553003/ ultimately; to have all functions
> that throw exceptions via an ExceptionsState& argument throw all their
> exceptions that way.
> 
> This function effectively throws exceptions directly via V8 sometimes, since
it
> calls into V8 via DictionaryHelper. Any exceptions thrown that way are ignored
> by this code, but would probably be caught by a v8::TryCatch in the caller,
and
> then propagated.
> 
> This does not block this patch, though. Just saying. :-)

Thank you for the heads up. That's a fantastic plan! I'm also planning to
replace DictionaryHelper with a new class called PropertyBag, which will use
ExceptionStates to throw conversion exceptions.
https://codereview.chromium.org/534133002/

I'd like to land this at this moment, given that adding PropertyBag may take
time. But I'm willing to change the logic to follow your changes:)

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:27: if
({{member.enum_validation_expression}})
On 2014/09/10 06:20:15, Jens Widell wrote:
> Rephrase this as
> 
>   if (!{{member.enum_validation_expression}}) {
>     exceptionState.throwTypeError("member {{member.name}} ('" + string + "')
is
> not a valid enum value.");
>     return NULL;
>   }
> 
> and thus avoid duplicating the
"impl->{{member.setter_name}}({{member.name}});"
> line?

Great suggestion! Done.

[Jens Widell, 2014-09-10 08:31:11]

LGTM

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/561633003/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:16: {{cpp_class}}*
{{v8_class}}::toImpl(v8::Isolate* isolate, v8::Handle<v8::Value> v8Value,
ExceptionState& exceptionState)
> I'd like to land this at this moment, [...]

Yes, absolutely.

[haraken, 2014-09-10 08:37:55]

LGTM

https://codereview.chromium.org/561633003/diff/20001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestDictionary.cpp (right):

https://codereview.chromium.org/561633003/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestDictionary.cpp:37: return NULL;

return 0;

[bashi, 2014-09-10 08:45:43]

Thank you for review all!

https://codereview.chromium.org/561633003/diff/20001/Source/bindings/tests/re...
File Source/bindings/tests/results/V8TestDictionary.cpp (right):

https://codereview.chromium.org/561633003/diff/20001/Source/bindings/tests/re...
Source/bindings/tests/results/V8TestDictionary.cpp:37: return NULL;
On 2014/09/10 08:37:55, haraken wrote:
> 
> return 0;

Done.

[bashi, 2014-09-10 08:45:50]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-09-10 08:46:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bashi@chromium.org/561633003/40001

[commit-bot: I haz the power, 2014-09-10 09:47:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 09:47:19]

Failed to apply patch for Source/bindings/tests/results/TestDictionary.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/bindings/tests/results/TestDictionary.cpp
  |diff --git a/Source/bindings/tests/results/TestDictionary.cpp
b/Source/bindings/tests/results/TestDictionary.cpp
  |index
e4a15ed8adc5517a73ba5d5f32d07a480b49f5e3..491ecce954c97067497164b140326757ac8ab6a3
100644
  |--- a/Source/bindings/tests/results/TestDictionary.cpp
  |+++ b/Source/bindings/tests/results/TestDictionary.cpp
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/bindings/tests/results/TestDictionary.cpp
Index: Source/bindings/tests/results/TestDictionary.cpp
diff --git a/Source/bindings/tests/results/TestDictionary.cpp
b/Source/bindings/tests/results/TestDictionary.cpp
index
e4a15ed8adc5517a73ba5d5f32d07a480b49f5e3..491ecce954c97067497164b140326757ac8ab6a3
100644
--- a/Source/bindings/tests/results/TestDictionary.cpp
+++ b/Source/bindings/tests/results/TestDictionary.cpp
@@ -14,6 +14,7 @@ TestDictionary::TestDictionary()
 {
     setLongMember(1);
     setStringOrNullMember(String("default string value"));
+    setEnumMember(String("foo"));
 }
 
 void TestDictionary::trace(Visitor* visitor)

[bashi, 2014-09-10 11:09:02]

https://codereview.chromium.org/561633003/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/561633003/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_dictionary.py:110: if
(idl_type.impl_should_use_nullable_container or idl_type.is_enum or
idl_type.is_string_type):
Umm.. Need to wrap this line to pass presubmit.
check-webkit-style failed
  Source/bindings/scripts/v8_dictionary.py:111:  visually indented line with
same indent as next logical line  [pep8/E129] [5]

[bashi, 2014-09-10 11:16:11]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-09-10 11:17:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bashi@chromium.org/561633003/80001

[commit-bot: I haz the power, 2014-09-10 13:12:50]

Committed patchset #5 (id:80001) as 181729