Add page visibility observer support for Device Motion

Add page visibility observer support for Device Motion

Stop firing 'ondevicemotion' events when a tab goes invisible to optimize
battery life.

R=timvolodine@chromium.org
BUG=263411
TEST=fast/dom/DeviceMotion/page-visibility.html

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

[timvolodine, 2013-09-18 13:03:16]

thanks Christophe, looks good, just a couple of comments on the testing side..

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
File LayoutTests/fast/dom/DeviceMotion/page-visibility.html (right):

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:27: ++eventCount;
if (++eventCount == 1)
    setTimeout(function(){checkGotEventAndHidePage();}, 0);

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:31: {
debug("Received a device motion event");
++eventCount;

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:43:
generateDeviceMotionEvent();
remove

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:44:
window.addEventListener('devicemotion', deviceMotionListener);
I would put checkGotEventAndFinish here as callback and remove the line below.

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:55:
generateDeviceMotionEvent();
pls remove this line, not necessary to call generateDeviceMotionEvent.

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:62:
generateDeviceMotionEvent();
the name is somewhat misleading, it is actually more of setMockDeviceMotion,
which sets the data to return when addEventListener is called, so technically
addEventListener triggers an event callback.

https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:64:
setTimeout(checkGotEventAndHidePage, 100);
instead of using a timeout of 100ms you could use a "chained" approach by
removing this line and putting a zero-delay timeout inside the callback as
commented above.

[do-not-use, 2013-09-18 13:29:06]

On 2013/09/18 13:03:16, timvolodine wrote:
> thanks Christophe, looks good, just a couple of comments on the testing side..
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> File LayoutTests/fast/dom/DeviceMotion/page-visibility.html (right):
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> LayoutTests/fast/dom/DeviceMotion/page-visibility.html:27: ++eventCount;
> if (++eventCount == 1)
>     setTimeout(function(){checkGotEventAndHidePage();}, 0);
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> LayoutTests/fast/dom/DeviceMotion/page-visibility.html:31: {
> debug("Received a device motion event");
> ++eventCount;
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> LayoutTests/fast/dom/DeviceMotion/page-visibility.html:43:
> generateDeviceMotionEvent();
> remove
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> LayoutTests/fast/dom/DeviceMotion/page-visibility.html:44:
> window.addEventListener('devicemotion', deviceMotionListener);
> I would put checkGotEventAndFinish here as callback and remove the line below.
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> LayoutTests/fast/dom/DeviceMotion/page-visibility.html:55:
> generateDeviceMotionEvent();
> pls remove this line, not necessary to call generateDeviceMotionEvent.
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> LayoutTests/fast/dom/DeviceMotion/page-visibility.html:62:
> generateDeviceMotionEvent();
> the name is somewhat misleading, it is actually more of setMockDeviceMotion,
> which sets the data to return when addEventListener is called, so technically
> addEventListener triggers an event callback.
> 
>
https://codereview.chromium.org/23705010/diff/1/LayoutTests/fast/dom/DeviceMo...
> LayoutTests/fast/dom/DeviceMotion/page-visibility.html:64:
> setTimeout(checkGotEventAndHidePage, 100);
> instead of using a timeout of 100ms you could use a "chained" approach by
> removing this line and putting a zero-delay timeout inside the callback as
> commented above.

I improved the test as suggested.

[timvolodine, 2013-09-18 13:49:21]

https://codereview.chromium.org/23705010/diff/8001/LayoutTests/fast/dom/Devic...
File LayoutTests/fast/dom/DeviceMotion/page-visibility.html (right):

https://codereview.chromium.org/23705010/diff/8001/LayoutTests/fast/dom/Devic...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:33: function
testWithPageHidden()
this should be called via setTimeout 0, to make sure the
{remove,add}EventListener are immediately executed. As it is now this test will
probably pass even without the hidden page modification.

[do-not-use, 2013-09-18 14:04:39]

https://codereview.chromium.org/23705010/diff/8001/LayoutTests/fast/dom/Devic...
File LayoutTests/fast/dom/DeviceMotion/page-visibility.html (right):

https://codereview.chromium.org/23705010/diff/8001/LayoutTests/fast/dom/Devic...
LayoutTests/fast/dom/DeviceMotion/page-visibility.html:33: function
testWithPageHidden()
On 2013/09/18 13:49:21, timvolodine wrote:
> this should be called via setTimeout 0, to make sure the
> {remove,add}EventListener are immediately executed. As it is now this test
will
> probably pass even without the hidden page modification.

Done.

[timvolodine, 2013-09-18 14:10:57]

lgtm

[commit-bot: I haz the power, 2013-09-18 14:27:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/23705010/10001

[commit-bot: I haz the power, 2013-09-18 15:31:51]

Change committed as 157968