Closed Bug 876238 Opened 6 years ago Closed 6 years ago

Convert DeviceAcceleration and DeviceRotationRate to WebIDL bindings

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: emk, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch (obsolete) — Splinter Review
Try result: https://tbpl.mozilla.org/?tree=Try&rev=5002b9cb4930
Attachment #754219 - Flags: review?(bugs)
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 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.
Attachment #754219 - Flags: review?(bugs)
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 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.
Attachment #754219 - Flags: review?(bugs)
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 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.
Attachment #754219 - Flags: review?(bugs) → review-
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?
It looks like this stalled out in June.  bz, do you have some suggestions for the comment in the previous question?  Thanks.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Oh, and if there are actual web compat constraints here, then we better have a spec for this!
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.
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}
Attached patch patch v2 (obsolete) — Splinter Review
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.)
Attachment #754219 - Attachment is obsolete: true
Attachment #8346563 - Flags: review?(bugs)
(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}
Attached patch patch v2.1 (obsolete) — Splinter Review
Forgot to change test_interfaces.
Attachment #8346563 - Attachment is obsolete: true
Attachment #8346563 - Flags: review?(bugs)
Attachment #8346879 - Flags: review?(bugs)
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
Attachment #8346879 - Flags: review?(bugs) → review-
I'd rather simply remove XPCOM stuff.
That is probably ok
Attached patch patch v3Splinter Review
Killed nsIDOMDeviceMotionEvent.idl.
Attachment #8346879 - Attachment is obsolete: true
Attachment #8350018 - Flags: review?(bugs)
Attachment #8350018 - Flags: review?(bugs) → review+
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite+
A thought: should we add a nsDOMDeviceAcceleration constructor that takes a DeviceAccelerationInit?
https://hg.mozilla.org/mozilla-central/rev/a19c708f134b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 956494
Depends on: 996677
You need to log in before you can comment on or make changes to this bug.