IDL: Null values should be converted for non-nullable dictionary members

IDL: Null values should be converted for non-nullable dictionary members

The current implementation of the binding layer treats null values
as the same as undefined, but this behavior doesn't match what the
spec specified. According to the spec[1], when null is passed to
a dictionary member, we should do conversion unless the member is
nullable. For example, given a dictionary member and the type
of the member is DOMString, passing null should result in "null"
(of type string). This CL changes the binding's behavior to follow
the spec.

[1] http://heycam.github.io/webidl/#es-dictionary
BUG=321462
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186169

[bashi, 2014-11-28 05:14:16]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, jinho.bang@samsung.com, jl@opera.com

[bashi, 2014-11-28 05:14:16]

PTAL?

I'd like to change the behavior of IDL dictionary to conform to the spec. I'm
planning to use IDL dictionary for EventInits, and I think we should make this
change before expanding IDL dictionary use.

jinho.bang@, tests for addHitRegion() depend on the current behavior, but I
think we should update our expectations. It seems that other browsers don't
implement it so I can't check the behavior of other browsers. WDYT?

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

https://codereview.chromium.org/765673005/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:57: impl.{{member.resetter_name}}();
This is need to reset default values. Default values are set in impl's
constructor, but setting null for nullable member is valid and it should be
null, rather than the default value.

[haraken, 2014-11-28 05:50:25]

LGTM

https://codereview.chromium.org/765673005/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/765673005/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestDictionary.cpp:81: } else if
(block.HasCaught()) {

Not related to this CL, I'm a bit confused with these branches. Would it be
probably better to reorder the branches like this?

if (block.HasCaught()) {
  exceptionState.rethrowV8Exception();
  return;
}
if (value.IsEmpty() || value->IsUndefined()) {
  // Do nothing.
} else if (value->IsNull()) {
  impl.resetMember();
} else {
  impl.setMember();
}

You can address it in a follow-up CL.

[Jens Widell, 2014-11-28 06:36:18]

LGTM

https://codereview.chromium.org/765673005/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_impl.h (right):

https://codereview.chromium.org/765673005/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_impl.h:26: void {{member.resetter_name}}()
{ m_{{member.cpp_name}} = {{member.member_cpp_type}}(); }
We could have this only for nullable members, and call it
"set{{member.name}}ToNull()". I don't think it's really useful for members in
general, and would typically expect "reset" to reset to the default state, which
this doesn't quite.

[zino, 2014-11-28 07:09:12]

On 2014/11/28 05:14:16, bashi1 wrote:
> jinho.bang@, tests for addHitRegion() depend on the current behavior, but I
> think we should update our expectations. It seems that other browsers don't
> implement it so I can't check the behavior of other browsers. WDYT?
> 

lgtm

I've just confirmed the behavior on Firefox and you're correct!
We should update tests and expectations.

Thank you.

[bashi, 2014-11-28 10:52:49]

Thank you for reviews!

https://codereview.chromium.org/765673005/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_impl.h (right):

https://codereview.chromium.org/765673005/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_impl.h:26: void {{member.resetter_name}}()
{ m_{{member.cpp_name}} = {{member.member_cpp_type}}(); }
On 2014/11/28 06:36:18, Jens Widell wrote:
> We could have this only for nullable members, and call it
> "set{{member.name}}ToNull()". I don't think it's really useful for members in
> general, and would typically expect "reset" to reset to the default state,
which
> this doesn't quite.

Done.

https://codereview.chromium.org/765673005/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestDictionary.cpp (right):

https://codereview.chromium.org/765673005/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestDictionary.cpp:81: } else if
(block.HasCaught()) {
On 2014/11/28 05:50:24, haraken wrote:
> 
> Not related to this CL, I'm a bit confused with these branches. Would it be
> probably better to reorder the branches like this?
> 
> if (block.HasCaught()) {
>   exceptionState.rethrowV8Exception();
>   return;
> }
> if (value.IsEmpty() || value->IsUndefined()) {
>   // Do nothing.
> } else if (value->IsNull()) {
>   impl.resetMember();
> } else {
>   impl.setMember();
> }
> 
> You can address it in a follow-up CL.

Done.

https://codereview.chromium.org/765673005/diff/20001/LayoutTests/fast/canvas/...
File LayoutTests/fast/canvas/canvas-hit-regions-basic-test.html (left):

https://codereview.chromium.org/765673005/diff/20001/LayoutTests/fast/canvas/...
LayoutTests/fast/canvas/canvas-hit-regions-basic-test.html:80:
shouldThrow("context.addHitRegion({ id : null })");
Just removed "id : null" tests. Other tests should have enough coverage.

[bashi, 2014-11-28 11:09:45]

The CQ bit was checked by bashi@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/765673005/20001

[commit-bot: I haz the power, 2014-11-28 11:32:36]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186169

[pfeldman, 2014-11-28 11:45:45]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/768793002/ by pfeldman@chromium.org.

The reason for reverting is: Build failed:
http://build.chromium.org/p/chromium.webkit/builders/Linux%20GN/builds/13713/....