[bindings] Eliminate custom bindings for DeviceOrientationEvent.

[bindings] Eliminate custom bindings for DeviceOrientationEvent.

Make the idl definitions Nullable to eliminate the custom bindings.

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

[vivekg, 2015-06-16 11:49:06]

vivekg@chromium.org changed reviewers:
+ vivekg@chromium.org

[vivekg, 2015-06-16 11:49:06]

haraken@, can you please take a look and let me know your feedback on the
approach?

This is a WIP patch which, once agreed upon the approach, would be divided into
small logical patches.

[vivekg, 2015-06-16 11:53:03]

https://codereview.chromium.org/1186823014/diff/1/Source/modules/device_orien...
File Source/modules/device_orientation/DeviceOrientationEvent.cpp (right):

https://codereview.chromium.org/1186823014/diff/1/Source/modules/device_orien...
Source/modules/device_orientation/DeviceOrientationEvent.cpp:54: m_orientation =
DeviceOrientationData::create(alpha, alpha ? *alpha : false, beta, beta ? *beta
: false, gamma, gamma ? *gamma : false, absolute, absolute ? *absolute : false);
There is a bit of typo here: false should be actually 0:

Will change this in the next CL once agreed.

Replace this line 

m_orientation = DeviceOrientationData::create(alpha, alpha ? *alpha : false,
beta, beta ? *beta : false, gamma, gamma ? *gamma : false, absolute, absolute ?
*absolute : false);   

with

m_orientation = DeviceOrientationData::create(alpha, alpha ? *alpha : 0, beta,
beta ? *beta : 0, gamma, gamma ? *gamma : 0, absolute, absolute ? *absolute :
false);

[haraken, 2015-06-16 16:07:46]

On 2015/06/16 11:53:03, vivekg_ wrote:
>
https://codereview.chromium.org/1186823014/diff/1/Source/modules/device_orien...
> File Source/modules/device_orientation/DeviceOrientationEvent.cpp (right):
> 
>
https://codereview.chromium.org/1186823014/diff/1/Source/modules/device_orien...
> Source/modules/device_orientation/DeviceOrientationEvent.cpp:54: m_orientation
=
> DeviceOrientationData::create(alpha, alpha ? *alpha : false, beta, beta ?
*beta
> : false, gamma, gamma ? *gamma : false, absolute, absolute ? *absolute :
false);
> There is a bit of typo here: false should be actually 0:
> 
> Will change this in the next CL once agreed.
> 
> Replace this line 
> 
> m_orientation = DeviceOrientationData::create(alpha, alpha ? *alpha : false,
> beta, beta ? *beta : false, gamma, gamma ? *gamma : false, absolute, absolute
?
> *absolute : false);   
> 
> with
> 
> m_orientation = DeviceOrientationData::create(alpha, alpha ? *alpha : 0, beta,
> beta ? *beta : 0, gamma, gamma ? *gamma : 0, absolute, absolute ? *absolute :
> false);

It's great that we can remove the custom bindings, but would there be any way to
avoid introducing [RawPrimitives]? For example, would it be helpful to introduce
[TreatUndefinedAs=False], [TreatNullAs=False] etc?

[jsbell, 2015-06-16 20:36:20]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-06-16 20:36:22]

drive-by: can we just eliminate this in favor of the constructor per spec?

http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...

Alternately, given that the properties are nullable, does undefined->null
mapping work if they are defined as nullable in the idl? If not, can we (as
haraken suggests) reintroduce e.g. [TreatUndefinedAs=Null] ?

[vivekg, 2015-06-17 14:22:04]

On 2015/06/16 at 20:36:22, jsbell wrote:
> drive-by: can we just eliminate this in favor of the constructor per spec?
> 
>
http://w3c.github.io/deviceorientation/spec-source-orientation.html#deviceori...
> 
> Alternately, given that the properties are nullable, does undefined->null
mapping work if they are defined as nullable in the idl? If not, can we (as
haraken suggests) reintroduce e.g. [TreatUndefinedAs=Null] ?

Done, PTAL.

[vivekg, 2015-06-17 14:23:46]

https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/IDLExte...
File Source/bindings/IDLExtendedAttributes.txt (right):

https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/IDLExte...
Source/bindings/IDLExtendedAttributes.txt:102: TreatUndefinedAs=NullString|Null
I should reorder this

TreatUndefinedAs=Null|NullString

https://codereview.chromium.org/1186823014/diff/20001/Source/modules/device_o...
File Source/modules/device_orientation/DeviceOrientationEvent.cpp (right):

https://codereview.chromium.org/1186823014/diff/20001/Source/modules/device_o...
Source/modules/device_orientation/DeviceOrientationEvent.cpp:54: m_orientation =
DeviceOrientationData::create(alpha, alpha ? *alpha : 0, beta, beta ? *beta : 0,
gamma, gamma ? *gamma : 0, absolute, absolute ? *absolute : false);
I will split this patch into two and will fix this ugly looking
DeviceOrientationData::Create(...) in the later patch.

[haraken, 2015-06-17 16:01:42]

https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/scripts...
File Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:228: is_undefined_to_null =
idl_type.is_primitive_type and \

\ is not needed.

https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/scripts...
Source/bindings/scripts/v8_methods.py:229:
has_extended_attribute_value(argument, "Default", "Undefined") and \

Do we need this condition? I mean, what should happen if JS passes undefined for
an DOM attribute that has [TreatUndefinedAs=Null] but doesn't have
[Default=Undefined]?

https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/tests/r...
File Source/bindings/tests/results/core/V8TestObject.cpp (right):

https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/tests/r...
Source/bindings/tests/results/core/V8TestObject.cpp:10095:
impl->voidMethodTreadUndefinedAsNull(firstArg, secondArg, optionalDoubleArg);

Would you help me understand why we need to pass a pointer to the C++
implementation? How are the fact that the DOM attribute/method has
[TreatUndefinedAs=Null] and the fact that we have to pass in a pointer related
with each other?

[vivekg, 2015-06-17 17:32:46]

On 2015/06/17 at 16:01:42, haraken wrote:
>
https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/scripts...
> Source/bindings/scripts/v8_methods.py:229:
has_extended_attribute_value(argument, "Default", "Undefined") and \
> 
> Do we need this condition? I mean, what should happen if JS passes undefined
for an DOM attribute that has [TreatUndefinedAs=Null] but doesn't have
[Default=Undefined]?
> 

Yeah makes sense. I will remove the check.


>
https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/tests/r...
> File Source/bindings/tests/results/core/V8TestObject.cpp (right):
> 
>
https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/tests/r...
> Source/bindings/tests/results/core/V8TestObject.cpp:10095:
impl->voidMethodTreadUndefinedAsNull(firstArg, secondArg, optionalDoubleArg);
> 
> Would you help me understand why we need to pass a pointer to the C++
implementation? How are the fact that the DOM attribute/method has
[TreatUndefinedAs=Null] and the fact that we have to pass in a pointer related
with each other?

This is being used in DeviceOrientationEvent in which the existing binding code
would convert the |undefined| double to 0. 
This situation makes it harder to determine whether this was actually sent by
the JS or something else.
If we have the ability to pass the pointer values, then the callee can easily
determine that this was indeed not sent by the user and appropriate action can
be taken.

[haraken, 2015-06-17 17:59:30]

On 2015/06/17 17:32:46, vivekg_ wrote:
> On 2015/06/17 at 16:01:42, haraken wrote:
> >
>
https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/scripts...
> > Source/bindings/scripts/v8_methods.py:229:
> has_extended_attribute_value(argument, "Default", "Undefined") and \
> > 
> > Do we need this condition? I mean, what should happen if JS passes undefined
> for an DOM attribute that has [TreatUndefinedAs=Null] but doesn't have
> [Default=Undefined]?
> > 
> 
> Yeah makes sense. I will remove the check.
> 
> 
> >
>
https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/tests/r...
> > File Source/bindings/tests/results/core/V8TestObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1186823014/diff/20001/Source/bindings/tests/r...
> > Source/bindings/tests/results/core/V8TestObject.cpp:10095:
> impl->voidMethodTreadUndefinedAsNull(firstArg, secondArg, optionalDoubleArg);
> > 
> > Would you help me understand why we need to pass a pointer to the C++
> implementation? How are the fact that the DOM attribute/method has
> [TreatUndefinedAs=Null] and the fact that we have to pass in a pointer related
> with each other?
> 
> This is being used in DeviceOrientationEvent in which the existing binding
code
> would convert the |undefined| double to 0. 
> This situation makes it harder to determine whether this was actually sent by
> the JS or something else.
> If we have the ability to pass the pointer values, then the callee can easily
> determine that this was indeed not sent by the user and appropriate action can
> be taken.

Ah, I understand... but that is a nasty way to distinguish the two things. Would
it be possible to update the DeviceOrientationEvent code so that it doesn't
depend on whether the pointer value is nullptr or not?

[vivekg, 2015-06-18 12:45:35]

Simply making this Nullable works as pointed out by jsbell@ 

With this we don't need any special handling from the bindings code generator.
PTAL, thanks.

[haraken, 2015-06-18 16:02:55]

This looks much nicer! LGTM.

[vivekg, 2015-06-18 16:12:51]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-06-18 16:13:08]

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

[commit-bot: I haz the power, 2015-06-18 16:36:00]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197367