Masatoshi Kimura [:emk] Assignee
	Description • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 1 • 11 years ago

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...

	Masatoshi Kimura [:emk] Assignee
	Comment 2 • 11 years ago

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.

	Masatoshi Kimura [:emk] Assignee
	Comment 3 • 11 years ago

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;
           ^

	Masatoshi Kimura [:emk] Assignee
	Comment 4 • 11 years ago

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.

	Cameron McCormack (:heycam)
	Comment 5 • 11 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 11 years ago

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.

	Masatoshi Kimura [:emk] Assignee
	Comment 7 • 11 years ago

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?

	Andrew McCreight [:mccr8]
	Comment 8 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 11 • 11 years ago

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.

	Masatoshi Kimura [:emk] Assignee
	Comment 12 • 11 years ago

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}

	Masatoshi Kimura [:emk] Assignee
	Comment 13 • 11 years ago

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.)

	Masatoshi Kimura [:emk] Assignee
	Comment 14 • 11 years ago

(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}

	Masatoshi Kimura [:emk] Assignee
	Comment 15 • 11 years ago

Forgot to change test_interfaces.

	Masatoshi Kimura [:emk] Assignee
	Comment 16 • 11 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 18 • 11 years ago

I'd rather simply remove XPCOM stuff.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 11 years ago

That is probably ok

	Masatoshi Kimura [:emk] Assignee
	Comment 20 • 11 years ago

Killed nsIDOMDeviceMotionEvent.idl.

	Masatoshi Kimura [:emk] Assignee
	Comment 21 • 11 years ago

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

	Masatoshi Kimura [:emk] Assignee
	Comment 22 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 23 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 24 • 11 years ago

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