Fix the issue with adding/removing event listeners for Device Motion/Orientation.

Fix the issue with adding/removing event listeners for Device Motion/Orientation.

Make sure to only stop updating Device Motion/Orientation when
there are no event listeners. Currently Device Motion/Orientation
stops updating upon first event listener removal which is
problematic in case multiple event listeners are registered.

BUG=364491
TEST=Layout test and also http://jsbin.com/motion.

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

[Inactive, 2014-04-17 14:38:19]

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
Source/modules/device_orientation/DeviceMotionController.cpp:101: &&
window->hasEventListeners(EventTypeNames::devicemotion)) {
How can this new condition ever be false?

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
Source/modules/device_orientation/DeviceMotionController.cpp:114: &&
!window->hasEventListeners(EventTypeNames::devicemotion)) {
Right, much better than my "counter" idea :)

[Inactive, 2014-04-17 14:39:32]

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceMotion/multiple-event-listeners.html (right):

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceMotion/multiple-event-listeners.html:6: <script
src="script-tests/multiple-event-listeners.js"></script>
I think it is frown upon to have the test in a separate js file these days. We
prefer to inline them in the html when possible.

[Inactive, 2014-04-17 14:43:17]

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
Source/modules/device_orientation/DeviceMotionController.cpp:101: &&
window->hasEventListeners(EventTypeNames::devicemotion)) {
On 2014/04/17 14:38:20, Chris Dumez wrote:
> How can this new condition ever be false?

Unless I am mistaken, when didAddEventListener() is called, we always have at
least 1 listener so window->hasEventListeners(EventTypeNames::devicemotion)
would always be true, right? What we would like to know if there is only 1
listener.

Admittedly, I don't know if it is an issue to call startUpdating() too many
times. The most important is that we fix the didRemoveEventListener() case so
that we don't call stopUpdating() prematurely.

[Peter Beverloo, 2014-04-17 14:48:17]

It would be great if you could remove the runtime flags for Device Motion and
Orientation! They're enabled by default everywhere and we've got the NULL events
as an availability detection mechanism.

Thanks for the fix, Tim! :-)

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceMotion/multiple-event-listeners.html (right):

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceMotion/multiple-event-listeners.html:6: <script
src="script-tests/multiple-event-listeners.js"></script>
On 2014/04/17 14:39:33, Chris Dumez wrote:
> I think it is frown upon to have the test in a separate js file these days. We
> prefer to inline them in the html when possible.

+1

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceMotion/script-tests/multiple-event-listeners.js
(right):

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceMotion/script-tests/multiple-event-listeners.js:5:
function setMockMotion(accelerationX, accelerationY, accelerationZ,
Can you try to increase focus of this test? It's very big, while the thing to
test for comes down to "make sure an event gets fired on remaining event
listeners after one gets removed". All the equal checks and NULL checks should
not be necessary.

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
Source/modules/device_orientation/DeviceMotionController.cpp:114: &&
!window->hasEventListeners(EventTypeNames::devicemotion)) {
nit: you could early return here.

[timvolodine, 2014-04-17 15:34:13]

thanks for the comments. I've also removed the runtime flag check.

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceMotion/multiple-event-listeners.html (right):

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceMotion/multiple-event-listeners.html:6: <script
src="script-tests/multiple-event-listeners.js"></script>
On 2014/04/17 14:39:33, Chris Dumez wrote:
> I think it is frown upon to have the test in a separate js file these days. We
> prefer to inline them in the html when possible.

Done.

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceMotion/multiple-event-listeners.html:6: <script
src="script-tests/multiple-event-listeners.js"></script>
On 2014/04/17 14:48:17, Peter Beverloo wrote:
> On 2014/04/17 14:39:33, Chris Dumez wrote:
> > I think it is frown upon to have the test in a separate js file these days.
We
> > prefer to inline them in the html when possible.
> 
> +1

Done.

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
File LayoutTests/fast/dom/DeviceMotion/script-tests/multiple-event-listeners.js
(right):

https://codereview.chromium.org/240993003/diff/20001/LayoutTests/fast/dom/Dev...
LayoutTests/fast/dom/DeviceMotion/script-tests/multiple-event-listeners.js:5:
function setMockMotion(accelerationX, accelerationY, accelerationZ,
On 2014/04/17 14:48:17, Peter Beverloo wrote:
> Can you try to increase focus of this test? It's very big, while the thing to
> test for comes down to "make sure an event gets fired on remaining event
> listeners after one gets removed". All the equal checks and NULL checks should
> not be necessary.

Done.

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
File Source/modules/device_orientation/DeviceMotionController.cpp (right):

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
Source/modules/device_orientation/DeviceMotionController.cpp:101: &&
window->hasEventListeners(EventTypeNames::devicemotion)) {
On 2014/04/17 14:43:17, Chris Dumez wrote:
> On 2014/04/17 14:38:20, Chris Dumez wrote:
> > How can this new condition ever be false?
> 
> Unless I am mistaken, when didAddEventListener() is called, we always have at
> least 1 listener so window->hasEventListeners(EventTypeNames::devicemotion)
> would always be true, right? What we would like to know if there is only 1
> listener.
> 
> Admittedly, I don't know if it is an issue to call startUpdating() too many
> times. The most important is that we fix the didRemoveEventListener() case so
> that we don't call stopUpdating() prematurely. 

I think you are right. Also it cannot hurt to call startUpdating() multiple
times (as it will be a noop after the first time). So removed this check here.
done.

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
Source/modules/device_orientation/DeviceMotionController.cpp:114: &&
!window->hasEventListeners(EventTypeNames::devicemotion)) {
On 2014/04/17 14:48:17, Peter Beverloo wrote:
> nit: you could early return here.

Done.

https://codereview.chromium.org/240993003/diff/20001/Source/modules/device_or...
Source/modules/device_orientation/DeviceMotionController.cpp:114: &&
!window->hasEventListeners(EventTypeNames::devicemotion)) {
On 2014/04/17 14:38:20, Chris Dumez wrote:
> Right, much better than my "counter" idea :)
;)

[Inactive, 2014-04-17 15:36:54]

lgtm but I am not an OWNER for this folder.

[timvolodine, 2014-04-22 19:59:47]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-04-22 20:00:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/240993003/60001

[commit-bot: I haz the power, 2014-04-22 20:07:56]

Change committed as 172165