876238 - Convert DeviceAcceleration and DeviceRotationRate to WebIDL bindings

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

Try result: https://tbpl.mozilla.org/?tree=Try&rev=5002b9cb4930

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 754219 [details] [diff] [review]
patch

Review of attachment 754219 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/DeviceMotionEvent.webidl
@@ +15,5 @@
> +callback interface DeviceRotationRate {
> +  readonly attribute double? alpha;
> +  readonly attribute double? beta;
> +  readonly attribute double? gamma;
> +};

These look more like dictionaries...

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 754219 [details] [diff] [review]
patch

The spec is not updated for one year, so I had to rewrite |[Callback] interface| to |callback interface| anyway.
http://dev.w3.org/geo/api/spec-source-orientation.html#devicemotion
I'll rewrite these "callback" interfaces to dictionaries.

Comment 3

[Masatoshi Kimura [:emk]]

The parser complained that an argument or an attribute could not be a nullable dictionary. Is this a spec violation or a limitation of our implementation?

WebIDL.WebIDLError: error: An argument cannot be a nullable dictionary or nullab
le union containing a dictionary, DeviceMotionEvent.webidl line 25:49
                             DeviceAcceleration? acceleration,
                                                 ^

WebIDL.WebIDLError: error: An attribute cannot be of a dictionary type, DeviceMo
tionEvent.webidl line 30:11
  readonly attribute DeviceAcceleration? acceleration;
           ^

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 754219 [details] [diff] [review]
patch

Ah, I can just drop |?| because null will always be treated as an empty dictionary. But the parser still complained that an attribute could not be a dictionary.

WebIDL.WebIDLError: error: An attribute cannot be of a dictionary type, DeviceMo
tionEvent.webidl line 30:11
  readonly attribute DeviceAcceleration acceleration;
           ^

And definitely the WebIDL spec disallows using a dictionary type as an attribute.
http://dev.w3.org/2006/webapi/WebIDL/#idl-attributes
I'm reverting the dictionaries to callback interfaces for now.

Comment 5

[Cameron McCormack (:heycam)]

Attributes cannot have a dictionary type for the same reason they cannot be a sequence -- it would mean that every time you get the property corresponding to the attribute, a whole new object would be created.

The pattern that the spec is trying to use -- having an interface be used both as a callback interface for the JS author to pass in an object implementing it, and as a normal interface, describing the object that DeviceMotionEvent.acceleration returns -- is no longer supported in Web IDL.  An interface must either be a callback interface or a regular interface.

I think the spec just needs to define a dictionary for the input to the event interface constructor and the interface for acceleration and accelerationIncludingGravity separately.  Like:

  interface DeviceAcceleration {
    readonly attribute double? x;
    readonly attribute double? y;
    readonly attribute double? z;
  };

  interface DeviceRotationRate {
    readonly attribute double? alpha;
    readonly attribute double? beta;
    readonly attribute double? gamma;
  };

  [Constructor(DOMString type, optional DeviceMotionEventInit eventInitDict)]
  interface DeviceMotionEvent : Event {
    readonly attribute DeviceAcceleration? acceleration;
    readonly attribute DeviceAcceleration? accelerationIncludingGravity;
    readonly attribute DeviceRotationRate? rotationRate;
    readonly attribute double? interval;
  };

  dictionary DeviceAccelerationInit {
    double? x;
    double? y;
    double? z;
  };

  dictionary DeviceRotationRateInit {
    double? alpha;
    double? beta;
    double? gamma;
  };

  dictionary DeviceMotionEventInit {
    DeviceAccelerationInit? acceleration;
    DeviceAccelerationInit? accelerationIncludingGravity;
    DeviceRotationRate? rotationRate;
    double? interval;
  };

It's a bit repetitive, but we don't have a way to define matching dictionaries and interfaces at the moment.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 754219 [details] [diff] [review]
patch

Could you use null as default value in the dictionary. That would
simplify Constructor implementation a bit, since there isn't need to 
check whether the value was passed.

But I don't understand how does this work in case DeviceAcceleration or 
DeviceRotationRate is implemented in C++ (which is the normal case).
What keeps the wrapper alive for such objects for example?
I think heycam's suggestion would work well enough.

Comment 7

[Masatoshi Kimura [:emk]]

When I define initDeviceMotionEvent as follows, parser complained about nullable dictionaries in an operation argument:
  void initDeviceMotionEvent(DOMString type,
                             boolean canBubble,
                             boolean cancelable,
                             DeviceAccelerationInit? acceleration,
                             DeviceAccelerationInit? accelerationIncludingGravity,
                             DeviceRotationRateInit? rotationRate,
                             double? interval);
But if I remove "?", test_bug662678.html will fail and it will be incompatible with WebKit/Blink.

Although initDeviceMotionEvent is an Mozilla (and WebKit/Blink) extension, nullable dictionary members threw a similar error:
dictionary DeviceMotionEventInit : EventInit {
  DeviceAccelerationInit? acceleration;
  DeviceAccelerationInit? accelerationIncludingGravity;
  DeviceRotationRateInit? rotationRate;
  double? interval = null;
};

According to bug 837339, nullable dictionaries as an operation argument or a dictionary member are intentionally disallowed. How can I solve this problem?

Comment 8

[Andrew McCreight [:mccr8]]

It looks like this stalled out in June.  bz, do you have some suggestions for the comment in the previous question?  Thanks.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Nullable dictionaries as in parameters are nonsense because the spec explicitly defines that null and {} behave identically when treated as a dictionary.

test_bug662678.html is explicitly testing that passing null and passing {} lead to different behavior, which they very naturally do in the XPConnect bindings because XPConnect knows nothing about dictionaries.

If there are web compat constraints here, in the sense that web sites depend on the difference in behavior between {} and null, we should just use 'any' in the IDL and then in the C++ check for null vs object and manually init a dictionary from the value if it's an object.  And cry about yet another sucky API we have inflicted on the web.

If there are no web compat constraints here (e.g. if pages never pass null in practice), then we should just implement as a dictionary and fix the test.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and if there are actual web compat constraints here, then we better have a spec for this!

Comment 11

[Masatoshi Kimura [:emk]]

Hm, the actual Chrome behavior didn't match what the spec is saying.
Chrome didn't differentiate passing null and passing {}. Both sets null value to DeviceMotionEvent members.

Comment 12

[Masatoshi Kimura [:emk]]

Chrome's behavior is very strange...
DeviceAccelerationInit      => DeviceAcceleration
null                        => null
{}                          => null
{x: null, y: null, z: null} => null
{x: null, y: null, z: 0}    => {x: 0, y: 0, z: 0}
{x: 0, y: 0, z: 0}          => {x: 0, y: 0, z: 0}

Comment 13

[Masatoshi Kimura [:emk]]

Attachment: patch v2

The behavior was completely different between IE11 and Chrome. The compatibility issue would not be so big in such edge cases (missing arguments, empty dictionary, null, etc.)

Comment 14

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> {x: null, y: null, z: 0}    => {x: 0, y: 0, z: 0}

Sorry, it was:
{x: null, y: null, z: 0}    => {x: null, y: null, z: 0}

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: patch v2.1

Forgot to change test_interfaces.

Comment 16

[Masatoshi Kimura [:emk]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=62730dc411a7

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8346879 [details] [diff] [review]
patch v2.1

> nsDOMDeviceMotionEvent::GetInterval(double *aInterval)
> {
>   NS_ENSURE_ARG_POINTER(aInterval);
> 
>-  *aInterval = Interval();
>+  *aInterval = GetInterval().Value();
This asserts if GetInterval() is null


> nsDOMDeviceAcceleration::GetX(double *aX)
> {
>   NS_ENSURE_ARG_POINTER(aX);
>-  *aX = mX;
>+  *aX = mX.Value();
Similar case here.
And elsewhere.

Need to check whether something is null and return 0 for xpcom stuff, I guess

Comment 18

[Masatoshi Kimura [:emk]]

I'd rather simply remove XPCOM stuff.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

That is probably ok

Comment 20

[Masatoshi Kimura [:emk]]

Attachment: patch v3

Killed nsIDOMDeviceMotionEvent.idl.

Comment 21

[Masatoshi Kimura [:emk]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c12bb5722218

Comment 22

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a19c708f134b

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

A thought: should we add a nsDOMDeviceAcceleration constructor that takes a DeviceAccelerationInit?

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a19c708f134b