Implement PollingThread for Device Motion.

Implement PollingThread for Device Motion.

On some platforms data fetchers are poll-based (e.g. mac), so it requires a polling thread to fetch data.
This CL includes:
- implementation of a provider for device motion with a polling thread.
- unit test for provider
- make sure to call Stop* in fetcher destructors only when started.

BUG=135804
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214538

[bulach, 2013-07-24 14:33:27]

lgtm, nice! mostly nits below:

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
File content/browser/device_orientation/device_motion_provider.cc (right):

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:22:
PollingThread(const char* name);
nit: explicit

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:26:
DeviceMotionHardwareBuffer* buffer);
nit: left align with the open parens

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:30: void DoPoll();
nit: add a \n

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:32:
DataFetcherSharedMemory* fetcher_;
nit:
\n
DISALLOW_COPY_AND_ASSIGN(DeviceMotionProvider::PollingThread);

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:44:
data_fetcher_.reset(fetcher.release());
I think it's possible to:
data_fetcher_ = fetcher;

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:52:
data_fetcher_.reset();
reset would be called automatically, no?

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:72: if
(!is_stopped_or_stopping_)
would "is_started" be clearer?

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:98: new
PollingThread("Device Motion data polling thread"));
nit: perhaps just "DeviceMotion poller" would be enough?

https://codereview.chromium.org/19833005/diff/1/content/browser/device_orient...
content/browser/device_orientation/device_motion_provider.cc:132: // ----
Polling thread methods
nit: probably best to keep this higher up, closer to the declaration.

[timvolodine, 2013-07-25 16:46:41]

thanks for the comments!

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
File content/browser/device_orientation/device_motion_provider.cc (right):

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:22:
PollingThread(const char* name);
On 2013/07/24 14:33:27, bulach wrote:
> nit: explicit

Done.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:26:
DeviceMotionHardwareBuffer* buffer);
On 2013/07/24 14:33:27, bulach wrote:
> nit: left align with the open parens

Done.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:30: void DoPoll();
On 2013/07/24 14:33:27, bulach wrote:
> nit: add a \n

Done.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:32:
DataFetcherSharedMemory* fetcher_;
On 2013/07/24 14:33:27, bulach wrote:
> nit:
> \n
> DISALLOW_COPY_AND_ASSIGN(DeviceMotionProvider::PollingThread);

Done.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:44:
data_fetcher_.reset(fetcher.release());
On 2013/07/24 14:33:27, bulach wrote:
> I think it's possible to:
> data_fetcher_ = fetcher;

that doesn't seem to work because '=' is private apparently. Instead I've
changed this to data_fetcher_ = fetcher.Pass() to better reflect the change of
ownership.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:52:
data_fetcher_.reset();
On 2013/07/24 14:33:27, bulach wrote:
> reset would be called automatically, no?

yes, but this is a small kind of optimization to make sure data_fetcher is
destroyed asap and is not using CPU resources.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:72: if
(!is_stopped_or_stopping_)
On 2013/07/24 14:33:27, bulach wrote:
> would "is_started" be clearer?

Done.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:98: new
PollingThread("Device Motion data polling thread"));
On 2013/07/24 14:33:27, bulach wrote:
> nit: perhaps just "DeviceMotion poller" would be enough?

Done.

https://chromiumcodereview.appspot.com/19833005/diff/1/content/browser/device...
content/browser/device_orientation/device_motion_provider.cc:132: // ----
Polling thread methods
On 2013/07/24 14:33:27, bulach wrote:
> nit: probably best to keep this higher up, closer to the declaration.

Done.

[commit-bot: I haz the power, 2013-07-25 19:33:01]

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

[commit-bot: I haz the power, 2013-07-26 01:26:23]

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

[commit-bot: I haz the power, 2013-07-26 06:19:12]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[timvolodine, 2013-07-26 09:37:48]

Hi Antoine,
need owners approval for content_tests.gypi as well ;)

[piman, 2013-07-26 17:47:48]

LGTM+nits

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory.h:17: class
CONTENT_EXPORT DataFetcherSharedMemory {
This class is now used from multiple threads. Please document the thread safety
expectations, e.g. which methods and which fields are expected to be
called/accessed from which thread.

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory_android.cc
(right):

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:37:
started_ = false;
nit: indent is wrong.

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory_default.cc
(right):

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory_default.cc:13:
StopFetchingDeviceMotionData();
nit: indent

[timvolodine, 2013-07-29 12:06:25]

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory.h (right):

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory.h:17: class
CONTENT_EXPORT DataFetcherSharedMemory {
On 2013/07/26 17:47:48, piman wrote:
> This class is now used from multiple threads. Please document the thread
safety
> expectations, e.g. which methods and which fields are expected to be
> called/accessed from which thread.

Done.

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory_android.cc
(right):

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory_android.cc:37:
started_ = false;
On 2013/07/26 17:47:48, piman wrote:
> nit: indent is wrong.

Done.

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
File content/browser/device_orientation/data_fetcher_shared_memory_default.cc
(right):

https://chromiumcodereview.appspot.com/19833005/diff/12001/content/browser/de...
content/browser/device_orientation/data_fetcher_shared_memory_default.cc:13:
StopFetchingDeviceMotionData();
On 2013/07/26 17:47:48, piman wrote:
> nit: indent

Done.

[commit-bot: I haz the power, 2013-07-29 12:06:37]

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

[commit-bot: I haz the power, 2013-07-29 16:39:09]

Retried try job too often on win7_aura for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-30 12:17:48]

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

[commit-bot: I haz the power, 2013-07-31 03:27:30]

Change committed as 214538