Bug 112910 – Implement the DeviceAcceleration and DeviceRotationRate IDL objects

• Status: ASSIGNED
• Component: DOM

[Peter Beverloo]

Implement the DeviceAcceleration and DeviceRotationRate IDL objects

[Peter Beverloo]

Created attachment 194254 [details]
Patch

[EFL EWS Bot]

Comment on attachment 194254 [details]
Patch

Attachment 194254 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17228395

[Peter Beverloo]

Created attachment 194259 [details]
Patch

[Peter Beverloo]

(In reply to comment #2)
> (From update of attachment 194254 [details])
> Attachment 194254 [details] did not pass efl-ews (efl):
> Output: http://webkit-commit-queue.appspot.com/results/17228395

Fixed the build with ENABLE_DEVICE_ORIENTATION=0. This should probably also be depending on the patch in bug 112711, which addresses Sam's concerns in the nullable type addition to the JSC binding generator.

[Kentaro Hara]

Comment on attachment 194259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review

Basically the patch looks great (Let me take a detailed look tomorrow.)

> Source/WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:-133
> -static JSObject* createAccelerationObject(const DeviceMotionData::Acceleration* acceleration, ExecState* exec)
> -{
> -    JSObject* object = constructEmptyObject(exec);
> -    object->putDirect(exec->globalData(), Identifier(exec, "x"), acceleration->canProvideX() ? jsNumber(acceleration->x()) : jsNull());
> -    object->putDirect(exec->globalData(), Identifier(exec, "y"), acceleration->canProvideY() ? jsNumber(acceleration->y()) : jsNull());
> -    object->putDirect(exec->globalData(), Identifier(exec, "z"), acceleration->canProvideZ() ? jsNumber(acceleration->z()) : jsNull());
> -    return object;
> -}
> -
> -static JSObject* createRotationRateObject(const DeviceMotionData::RotationRate* rotationRate, ExecState* exec)
> -{
> -    JSObject* object = constructEmptyObject(exec);
> -    object->putDirect(exec->globalData(), Identifier(exec, "alpha"), rotationRate->canProvideAlpha() ? jsNumber(rotationRate->alpha()) : jsNull());
> -    object->putDirect(exec->globalData(), Identifier(exec, "beta"),  rotationRate->canProvideBeta()  ? jsNumber(rotationRate->beta())  : jsNull());
> -    object->putDirect(exec->globalData(), Identifier(exec, "gamma"), rotationRate->canProvideGamma() ? jsNumber(rotationRate->gamma()) : jsNull());
> -    return object;
> -}
> -

In your new code, where did this logic go?

Maybe did you remove the logic intentionally because there is no need to set those attributes onto the object? (It sounds reasonable to me, but I want to confirm your intention.)

> Source/WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:-57
> -static v8::Handle<v8::Value> createAccelerationObject(const DeviceMotionData::Acceleration* acceleration, v8::Isolate* isolate)
> -{
> -    v8::Local<v8::Object> object = v8::Object::New();
> -    object->Set(v8::String::NewSymbol("x"), acceleration->canProvideX() ? v8::Number::New(acceleration->x()) : v8Null(isolate));
> -    object->Set(v8::String::NewSymbol("y"), acceleration->canProvideY() ? v8::Number::New(acceleration->y()) : v8Null(isolate));
> -    object->Set(v8::String::NewSymbol("z"), acceleration->canProvideZ() ? v8::Number::New(acceleration->z()) : v8Null(isolate));
> -    return object;
> -}
> -
> -static v8::Handle<v8::Value> createRotationRateObject(const DeviceMotionData::RotationRate* rotationRate, v8::Isolate* isolate)
> -{
> -    v8::Local<v8::Object> object = v8::Object::New();
> -    object->Set(v8::String::NewSymbol("alpha"), rotationRate->canProvideAlpha() ? v8::Number::New(rotationRate->alpha()) : v8Null(isolate));
> -    object->Set(v8::String::NewSymbol("beta"),  rotationRate->canProvideBeta()  ? v8::Number::New(rotationRate->beta())  : v8Null(isolate));
> -    object->Set(v8::String::NewSymbol("gamma"), rotationRate->canProvideGamma() ? v8::Number::New(rotationRate->gamma()) : v8Null(isolate));
> -    return object;
> -}
> -

Ditto.

[Peter Beverloo]

This is now done lazily in the DeviceMotionEvent::{acceleration,accelerationIncludingGravity,rotationRate} methods.

[Kentaro Hara]

(In reply to comment #6)
> This is now done lazily in the DeviceMotionEvent::{acceleration,accelerationIncludingGravity,rotationRate} methods.

Where is your new code calling object->putDirect() or object->Set() ?

[Peter Beverloo]

(In reply to comment #7)
> (In reply to comment #6)
> > This is now done lazily in the DeviceMotionEvent::{acceleration,accelerationIncludingGravity,rotationRate} methods.
> 
> Where is your new code calling object->putDirect() or object->Set() ?

We don't need to do that anymore :-).

The Device Orientation specification specifies that these values should be stored in DeviceAcceleration (for DeviceMotionEvent.acceleration and .accelerationIncludingGravity) and DeviceRotationRate (for .rotationRate) objects.  This patch implements these objects and returns instances of them instead of manually creating an "anonymous" object, which we previously did.

[Kentaro Hara]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > This is now done lazily in the DeviceMotionEvent::{acceleration,accelerationIncludingGravity,rotationRate} methods.
> > 
> > Where is your new code calling object->putDirect() or object->Set() ?
> 
> We don't need to do that anymore :-).
> 
> The Device Orientation specification specifies that these values should be stored in DeviceAcceleration (for DeviceMotionEvent.acceleration and .accelerationIncludingGravity) and DeviceRotationRate (for .rotationRate) objects.  This patch implements these objects and returns instances of them instead of manually creating an "anonymous" object, which we previously did.

Makes sense! Thanks for the clarification.

[Kentaro Hara]

Comment on attachment 194259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review

Looks OK. Thanks for removing a bunch of custom bindings.

> Source/WebCore/dom/DeviceMotionEvent.cpp:69
> +    if (!m_acceleration.get())

Nit: .get() is not needed.

> Source/WebCore/dom/DeviceMotionEvent.cpp:80
> +    if (!m_accelerationIncludingGravity.get())

Ditto.

> Source/WebCore/dom/DeviceMotionEvent.cpp:91
> +    if (!m_rotationRate.get())

Ditto.

[Peter Beverloo]

Thank you!  I'll revise and land when bug 112711 is resolved.

[Steve Block]

Comment on attachment 194259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review

> Source/WebCore/dom/DeviceOrientationEvent.idl:32
> +    readonly attribute boolean? absolute;

The absolute property shouldn't be nullable, though it looks like this was wrong before your change too. Do we have a bug for that?

> Source/WebCore/dom/DeviceRotationRate.idl:28
> +    ImplementationLacksVTable

I think the new interfaces should specify 'OmitConstructor', as these types use 'NoInterfaceObject ' in the spec so shouldn't be constructable from JS.

[Adam Barth]

Comment on attachment 194259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review

This looks great, but I think there's a use-after-free issue here.

> Source/WebCore/dom/DeviceAcceleration.h:48
> +    const DeviceMotionData::Acceleration* m_acceleration;

What's the ownership relationship between these two objects?  DeviceAcceleration is RefCounted, so there's a risk that it might outlive m_acceleration.

> Source/WebCore/dom/DeviceAcceleration.idl:30
> +    readonly attribute double? x;

Nice!

> Source/WebCore/dom/DeviceRotationRate.h:48
> +    const DeviceMotionData::RotationRate* m_rotationRate;

Same problem here.

[Steve Block]

Comment on attachment 194259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review

>> Source/WebCore/dom/DeviceRotationRate.idl:28
>> +    ImplementationLacksVTable
> 
> I think the new interfaces should specify 'OmitConstructor', as these types use 'NoInterfaceObject ' in the spec so shouldn't be constructable from JS.

Actually, according to http://trac.webkit.org/wiki/WebKitIDL#OmitConstructor it looks OmitConstructor is deprecated, so I'm not sure what it does or if it's needed. Perhaps you should check with haraken?

[Kentaro Hara]

(In reply to comment #14)
> (From update of attachment 194259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review
> 
> >> Source/WebCore/dom/DeviceRotationRate.idl:28
> >> +    ImplementationLacksVTable
> > 
> > I think the new interfaces should specify 'OmitConstructor', as these types use 'NoInterfaceObject ' in the spec so shouldn't be constructable from JS.
> 
> Actually, according to http://trac.webkit.org/wiki/WebKitIDL#OmitConstructor it looks OmitConstructor is deprecated, so I'm not sure what it does or if it's needed. Perhaps you should check with haraken?

Correct me if I'm wrong, I'm not 100% sure:

In the current WebKit IDL, there is no IDL attribute for [NoInterfaceObject]. Alternately, whether an interface XXX is exposed to a window object or not is controlled (not by [NoInterfaceObject] but) by whether you write 'XXXConstructor XXX' in DOMWindow.idl or not.

I don't know what [OmitConstructor] does. [OmitConstructor] is used by JSC only. I tried to remove it a while ago but couldn't for some reason.

[Kentaro Hara]

(In reply to comment #15)
> > > I think the new interfaces should specify 'OmitConstructor', as these types use 'NoInterfaceObject ' in the spec so shouldn't be constructable from JS.

Note: [NoInterfaceObject] and [Constructor] are different things.

- [NoInterfaceObject] controls whether XXX is exposed to a window object or not (i.e. 'window.XXX' is undefined or not).

- [Constructor] controls whether XXX is constructable from JS or not (i.e. 'new XXX' works or not).

[Steve Block]

Thanks for the clarification. So in WebIDL, by default, interfaces are present on the window object (ie ![NoInterfaceObject]) but not constructable (ie ![Constructor]), right? And presumably if something specifies [NoInterfaceObject] in WebIDL, then it would make no sense to specify [Constructor] too.

As for whether you should use [OmitConstructor] here, I now have no idea.

[Kentaro Hara]

(In reply to comment #17)
> Thanks for the clarification. So in WebIDL, by default, interfaces are present on the window object (ie ![NoInterfaceObject]) but not constructable (ie ![Constructor]), right? And presumably if something specifies [NoInterfaceObject] in WebIDL, then it would make no sense to specify [Constructor] too.

Yes. The Web IDL spec says that [Constructor] must not be specified if [NoInterfaceObject] is specified. (http://www.w3.org/TR/WebIDL/#NoInterfaceObject)


> As for whether you should use [OmitConstructor] here, I now have no idea.

I think that the current patch is correct:

- [Constructor] shouldn't be specified (because the interface is not constructable)
- The interface should not be added to DOMWindow.idl (because the interface is speced as [NoInteraceObject])
- [OmitConstructor] is not needed (Actually I don't know what it is. It is used by JSC only and can be removed.)

[Peter Beverloo]

Created attachment 194572 [details]
Patch

[Peter Beverloo]

Thank you for the reviews!

(In reply to comment #10)
> (From update of attachment 194259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review
> 
> Looks OK. Thanks for removing a bunch of custom bindings.
> 
> > Source/WebCore/dom/DeviceMotionEvent.cpp:69
> > +    if (!m_acceleration.get())
> 
> Nit: .get() is not needed.
> 
> > Source/WebCore/dom/DeviceMotionEvent.cpp:80
> > +    if (!m_accelerationIncludingGravity.get())
> 
> Ditto.
> 
> > Source/WebCore/dom/DeviceMotionEvent.cpp:91
> > +    if (!m_rotationRate.get())
> 
> Ditto.

Done all of these, thanks.


(In reply to comment #13)
> (From update of attachment 194259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review
> 
> This looks great, but I think there's a use-after-free issue here.
> 
> > Source/WebCore/dom/DeviceAcceleration.h:48
> > +    const DeviceMotionData::Acceleration* m_acceleration;
> 
> What's the ownership relationship between these two objects?  DeviceAcceleration is RefCounted, so there's a risk that it might outlive m_acceleration.

Eh, you're right. Thanks for catching that! I've amended the patch so DeviceAcceleration and DeviceRotationRate maintain a reference instead.

> 
> > Source/WebCore/dom/DeviceAcceleration.idl:30
> > +    readonly attribute double? x;
> 
> Nice!
> 
> > Source/WebCore/dom/DeviceRotationRate.h:48
> > +    const DeviceMotionData::RotationRate* m_rotationRate;
> 
> Same problem here.

Done.

(In reply to comment #12)
> (From update of attachment 194259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review
> 
> > Source/WebCore/dom/DeviceOrientationEvent.idl:32
> > +    readonly attribute boolean? absolute;
> 
> The absolute property shouldn't be nullable, though it looks like this was wrong before your change too. Do we have a bug for that?

I asked Andrei about the same thing, and he mentioned it should be nullable in the case where the implementation cannot be certain.  I'll be sharing feedback on the specification later on with you as well.
 
> > Source/WebCore/dom/DeviceRotationRate.idl:28
> > +    ImplementationLacksVTable
> 
> I think the new interfaces should specify 'OmitConstructor', as these types use 'NoInterfaceObject ' in the spec so shouldn't be constructable from JS.

As discussed (thanks!) -- no action taken.


I need to modify the XCode project files before landing this, but don't have a Mac at hand to actually do this.  Is there a way to update these files without having XCode installed?

[Andrei Popescu]

(In reply to comment #12)
> (From update of attachment 194259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194259&action=review
> 
> > Source/WebCore/dom/DeviceOrientationEvent.idl:32
> > +    readonly attribute boolean? absolute;
> 
> The absolute property shouldn't be nullable, though it looks like this was wrong before your change too. Do we have a bug for that?
> 

Hey Steve, do you remember why it should not be nullable? Everything else in that interface is nullable. If the implementation runs on a device without a compass or accelerometer, what should be the absolute value? In the spec we say that, in such a case, the implementation should fire a DeviceOrienationEvent once with all properties set to null. But if absolute is not nullable...

[Steve Block]

> Hey Steve, do you remember why it should not be nullable?
I think we decided that if an implementation has rotation values, but isn't sure whether or not they're absolute, it should set 'absolute' to false. The only case when absolute can be null is if the implementation can't provide any values at. I think we just never updated the spec to reflect this. 
 
> I need to modify the XCode project files before landing this ...
You can edit the XCode project file by hand by pattern matching with entries for similar files - it's just slow!

[Andrei Popescu]

(In reply to comment #22)
> > Hey Steve, do you remember why it should not be nullable?
> I think we decided that if an implementation has rotation values, but isn't sure whether or not they're absolute, it should set 'absolute' to false. The only case when absolute can be null is if the implementation can't provide any values at. I think we just never updated the spec to reflect this. 
> 

Right, that's what I thought. But if there is a case where it can be null, shouldn't the IDL mark this attribute as nullable?

[Steve Block]

> But if there is a case where it can be null, shouldn't the IDL mark this attribute as nullable?
Correct. That's what I meant by needing to update the spec.

[Kentaro Hara]

Comment on attachment 194572 [details]
Patch

Looks good to me.

[Peter Beverloo]

Created attachment 196204 [details]
Patch for landing

[Peter Beverloo]

Comment on attachment 196204 [details]
Patch for landing

Will cq+ based on ews results.. Thanks Haraken-san!

[Early Warning System Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17383286

[Early Warning System Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17383290

[Build Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17409064

[WebKit Review Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17405107

[WebKit Review Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17376273

[Peter Beverloo (cr-android ews)]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17343666

[EFL EWS Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17320702

[Build Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17338669

[Build Bot]

Comment on attachment 196204 [details]
Patch for landing

Attachment 196204 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17240860