Test DeviceMotionEventPump to ensure events deliver at 60Hz

Test DeviceMotionEventPump to ensure events deliver at 60Hz

Add a unit test for DeviceMotionEventPump to ensure that events are not delivered to Blink at a rate faster than 60 Hz.

TEST=DeviceMotionEventPumpTest.PumpThrottlesEventRate
BUG=421691
Committed: https://crrev.com/d771a782774564197c9c4d3ddd07605e53397a12
Cr-Commit-Position: refs/heads/master@{#314574}

[jonross, 2015-02-03 21:55:23]

jonross@chromium.org changed reviewers:
+ timvolodine@chromium.org

[jonross, 2015-02-03 21:55:24]

Hi Tim,

I've written a unit test for the rate at which Device Motion events are pumped.

Since the pumps polls, a high speed fetcher would have no effect. By testing the
blink::WebDeviceMotionListener I avoid having to block on web pages.

Could you provide an owners review?

Thanks,
Jon

[timvolodine, 2015-02-04 13:30:06]

On 2015/02/03 21:55:24, jonross wrote:
> Hi Tim,
> 
> I've written a unit test for the rate at which Device Motion events are
pumped.
> 
> Since the pumps polls, a high speed fetcher would have no effect. By testing
the
> blink::WebDeviceMotionListener I avoid having to block on web pages.
> 
> Could you provide an owners review?
> 
> Thanks,
> Jon

yup, that should work as well thanks!

[timvolodine, 2015-02-04 13:30:18]

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
File content/renderer/device_sensors/device_motion_event_pump_unittest.cc
(right):

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:12:
#include "content/renderer/device_sensors/device_sensor_event_pump.h"
is this needed?

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:183:
EXPECT_EQ(60, base::Time::kMicrosecondsPerSecond /
should this better be EXPECT_GE?

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:184:
motion_pump()->pump_delay_microseconds());
indentation

[jonross, 2015-02-04 14:52:55]

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
File content/renderer/device_sensors/device_motion_event_pump_unittest.cc
(right):

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:12:
#include "content/renderer/device_sensors/device_sensor_event_pump.h"
On 2015/02/04 13:30:18, timvolodine wrote:
> is this needed?

Nope, forgot to remove that after testing

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:183:
EXPECT_EQ(60, base::Time::kMicrosecondsPerSecond /
On 2015/02/04 13:30:18, timvolodine wrote:
> should this better be EXPECT_GE?

Done.

https://codereview.chromium.org/870053009/diff/1/content/renderer/device_sens...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:184:
motion_pump()->pump_delay_microseconds());
On 2015/02/04 13:30:17, timvolodine wrote:
> indentation

Done.

[timvolodine, 2015-02-04 15:11:53]

lgtm (with nits)

https://codereview.chromium.org/870053009/diff/20001/content/renderer/device_...
File content/renderer/device_sensors/device_motion_event_pump_unittest.cc
(right):

https://codereview.chromium.org/870053009/diff/20001/content/renderer/device_...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:182: //
Confirm that the delay for pumping events is 60 Hz.
nit: -> confirm that the frequency for pumping events is not greater than 60Hz ?

https://codereview.chromium.org/870053009/diff/20001/content/renderer/device_...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:184:
motion_pump()->pump_delay_microseconds());
nit: this looks a bit confusing (as if it's an extra parameter), just put a 4
space indent here?

[jonross, 2015-02-04 16:57:23]

New patchsets have been uploaded after l-g-t-m from timvolodine@chromium.org

[jonross, 2015-02-04 16:57:51]

https://codereview.chromium.org/870053009/diff/20001/content/renderer/device_...
File content/renderer/device_sensors/device_motion_event_pump_unittest.cc
(right):

https://codereview.chromium.org/870053009/diff/20001/content/renderer/device_...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:182: //
Confirm that the delay for pumping events is 60 Hz.
On 2015/02/04 15:11:53, timvolodine wrote:
> nit: -> confirm that the frequency for pumping events is not greater than 60Hz
?

Done.

https://codereview.chromium.org/870053009/diff/20001/content/renderer/device_...
content/renderer/device_sensors/device_motion_event_pump_unittest.cc:184:
motion_pump()->pump_delay_microseconds());
On 2015/02/04 15:11:53, timvolodine wrote:
> nit: this looks a bit confusing (as if it's an extra parameter), just put a 4
> space indent here?

Yeah, auto-indent/cl format, are both giving weird indents.

Done.

[jonross, 2015-02-04 16:57:54]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2015-02-04 16:58:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870053009/40001

[commit-bot: I haz the power, 2015-02-04 17:31:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-04 17:32:53]

Patchset 3 (id:??) landed as
https://crrev.com/d771a782774564197c9c4d3ddd07605e53397a12
Cr-Commit-Position: refs/heads/master@{#314574}