[Device Sensors] Move Mojo communication to UI thread on Android

[Device Sensors] Move Mojo communication to UI thread on Android

In preparation for removing content::BrowserThread knowledge from
Device Sensors, this CL eliminates the need for SensorManagerAndroid to
live on multiple threads. Instead, Mojo communication for Device Sensors
occurs on the UI thread on Android, resulting in the following structure:

- On Android, all of the device sensors host code lives solely on the
UI thread.
- On all other platforms, all of the device sensors host code lives
solely on the IO thread.

This restructure will enable a straightforward followup that replaces
usage of content::BrowserThread with //base-level primitives in this code.

Note that device_sensor_browsertest.cc had to be updated to support this
change:
- Could no longer use WaitableEvents, as they blocked the UI thread while
  waiting for incoming Mojo communication, which on Android is now incoming
  on the UI thread :P.
- Instead, the tests spin up run loops and pass their QuitClosures into
  the FakeDataFetcher, which calls those QuitClosures on the UI thread when
  the appropriate events occur. In this way, the tests can wait for the
  various events (by Running the appropriate RunLoop) regardless of whether
  the Mojo communication (and FakeDataFetcher) live on the UI or IO thread.

BUG=612322
Committed: https://crrev.com/4489ef7781fd694d199e2c5a407f0614317de2d7
Cr-Commit-Position: refs/heads/master@{#432455}

[blundell, 2016-10-14 09:58:08]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 09:58:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 11:30:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 11:30:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[blundell, 2016-10-25 20:35:13]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 20:35:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blundell, 2016-10-25 20:38:07]

Description was changed from

==========
[Device Sensors] Move Mojo communication to UI thread on Android

In preparation for removing content::BrowserThread knowledge from
Device Sensors, this CL eliminates the need for SensorManagerAndroid to
live on multiple threads. Instead, Mojo communication for Device Sensors
occurs on the UI thread on Android, resulting in the following structure:

- On Android, all of the device sensors host code lives solely on the
UI thread.
- On all other platforms, all of the device sensors host code lives
solely on the IO thread.

This restructure will enable a straightforward followup that replaces
usage of content::BrowserThread with //base-level primitives in this code.

BUG=612322
==========

to

==========
[Device Sensors] Move Mojo communication to UI thread on Android

In preparation for removing content::BrowserThread knowledge from
Device Sensors, this CL eliminates the need for SensorManagerAndroid to
live on multiple threads. Instead, Mojo communication for Device Sensors
occurs on the UI thread on Android, resulting in the following structure:

- On Android, all of the device sensors host code lives solely on the
UI thread.
- On all other platforms, all of the device sensors host code lives
solely on the IO thread.

This restructure will enable a straightforward followup that replaces
usage of content::BrowserThread with //base-level primitives in this code.

Note that device_sensor_browsertest.cc had to be updated to support this
change:
- Could no longer use WaitableEvents, as they blocked the UI thread while
  waiting for incoming Mojo communication, which on Android is now incoming
  on the UI thread :P.
- Instead, the tests spin up run loops and pass their QuitClosures into
  the FakeDataFetcher, which calls those QuitClosures on the UI thread when
  the appropriate events occur. In this way, the tests can wait for the
  various events (by Running the appropriate RunLoop) regardless of whether
  the Mojo communication (and FakeDataFetcher) live on the UI or IO thread.

BUG=612322
==========

[commit-bot: I haz the power, 2016-10-25 22:04:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 22:04:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[blundell, 2016-10-25 22:05:49]

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

[blundell, 2016-10-27 23:38:52]

On 2016/10/25 22:05:50, blundell wrote:

ping :)

[timvolodine, 2016-10-31 15:51:45]

just what I had in mind, thanks!

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
File content/browser/device_sensors/device_sensor_browsertest.cc (right):

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
content/browser/device_sensors/device_sensor_browsertest.cc:375: base::RunLoop
motion_started_runloop;
would it be simpler to just declare run loops in the test fixture? then there
would be no need for creating loops and setting callbacks in every test (and
would look more like the waitable events)

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:75: return
base::Singleton<
maybe also DCHECK UI thread here

[blundell, 2016-11-09 14:20:36]

The CQ bit was checked by blundell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 14:20:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 15:06:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 15:06:20]

Dry run: This issue passed the CQ dry run.

[blundell, 2016-11-09 15:07:43]

Thanks for the review!

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
File content/browser/device_sensors/device_sensor_browsertest.cc (right):

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
content/browser/device_sensors/device_sensor_browsertest.cc:375: base::RunLoop
motion_started_runloop;
On 2016/10/31 15:51:44, timvolodine wrote:
> would it be simpler to just declare run loops in the test fixture? then there
> would be no need for creating loops and setting callbacks in every test (and
> would look more like the waitable events)

Good idea, done.

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/2416123003/diff/60001/content/browser/device_...
content/browser/device_sensors/sensor_manager_android.cc:75: return
base::Singleton<
On 2016/10/31 15:51:45, timvolodine wrote:
> maybe also DCHECK UI thread here

Done.

[timvolodine, 2016-11-10 16:18:21]

lgtm with 2 comments. thanks a lot :)

https://codereview.chromium.org/2416123003/diff/100001/content/browser/device...
File content/browser/device_sensors/device_sensor_browsertest.cc (right):

https://codereview.chromium.org/2416123003/diff/100001/content/browser/device...
content/browser/device_sensors/device_sensor_browsertest.cc:208: // Initialize
the RunLoops now that the main thread has been created.
This actually looks a bit more complicated than I thought.. Maybe it's possible
to put runloops inside the fetcher, which would make Set*Callback etc
unnecessary and probably simplify the code. This doesn't seem essential for this
patch however, so up to you to decide ;)

https://codereview.chromium.org/2416123003/diff/100001/content/browser/device...
content/browser/device_sensors/device_sensor_browsertest.cc:365: // The test
page registers an event handlers for orientation/orientation
"orientation/orientation" -> "motion/orientation" (as it was originally)

[blundell, 2016-11-15 09:04:44]

Thanks, Tim!

https://codereview.chromium.org/2416123003/diff/100001/content/browser/device...
File content/browser/device_sensors/device_sensor_browsertest.cc (right):

https://codereview.chromium.org/2416123003/diff/100001/content/browser/device...
content/browser/device_sensors/device_sensor_browsertest.cc:208: // Initialize
the RunLoops now that the main thread has been created.
On 2016/11/10 16:18:21, timvolodine wrote:
> This actually looks a bit more complicated than I thought.. Maybe it's
possible
> to put runloops inside the fetcher, which would make Set*Callback etc
> unnecessary and probably simplify the code. This doesn't seem essential for
this
> patch however, so up to you to decide ;)

The problem with that approach is that the fetcher is created on the IO thread
on platforms other than Android, but the runloops need to be running on the UI
thread on all platforms. I think that the fact that the fetcher is created on
different threads on different platforms unavoidably adds complexity to this
test. I did spend a lot of time trying to figure out the best way to mitigate
that complexity, and what we've come up with here (thanks to your suggestion) is
the best approach that I've seen so far.

https://codereview.chromium.org/2416123003/diff/100001/content/browser/device...
content/browser/device_sensors/device_sensor_browsertest.cc:365: // The test
page registers an event handlers for orientation/orientation
On 2016/11/10 16:18:21, timvolodine wrote:
> "orientation/orientation" -> "motion/orientation" (as it was originally)

Whoops, done.

[blundell, 2016-11-15 09:07:06]

Oh right, we could just have the runloops live as pointers inside the fake data
fetcher the way they do now in the test fixture, which would indeed eliminate
the Set*Callback functions. I'm going to go ahead and land this CL as-is now
though :P.

[blundell, 2016-11-15 09:07:16]

The CQ bit was checked by blundell@chromium.org

[blundell, 2016-11-15 09:07:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2416123003/#ps140001
(title: "Response to review")

[commit-bot: I haz the power, 2016-11-15 09:07:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[blundell, 2016-11-15 09:09:43]

I'll look at doing that simplification in a followup patch.

[commit-bot: I haz the power, 2016-11-15 09:16:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 09:16:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[blundell, 2016-11-15 09:17:59]

blundell@chromium.org changed reviewers:
+ ben@chromium.org

[blundell, 2016-11-15 09:17:59]

Ben, can you review //content/browser/render_process_host_impl.cc as jam is out?
Thanks!

[Ben Goodger (Google), 2016-11-16 01:14:49]

lgtm

[blundell, 2016-11-16 10:04:27]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2016-11-16 10:04:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 11:00:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-16 11:02:52]

Description was changed from

==========
[Device Sensors] Move Mojo communication to UI thread on Android

In preparation for removing content::BrowserThread knowledge from
Device Sensors, this CL eliminates the need for SensorManagerAndroid to
live on multiple threads. Instead, Mojo communication for Device Sensors
occurs on the UI thread on Android, resulting in the following structure:

- On Android, all of the device sensors host code lives solely on the
UI thread.
- On all other platforms, all of the device sensors host code lives
solely on the IO thread.

This restructure will enable a straightforward followup that replaces
usage of content::BrowserThread with //base-level primitives in this code.

Note that device_sensor_browsertest.cc had to be updated to support this
change:
- Could no longer use WaitableEvents, as they blocked the UI thread while
  waiting for incoming Mojo communication, which on Android is now incoming
  on the UI thread :P.
- Instead, the tests spin up run loops and pass their QuitClosures into
  the FakeDataFetcher, which calls those QuitClosures on the UI thread when
  the appropriate events occur. In this way, the tests can wait for the
  various events (by Running the appropriate RunLoop) regardless of whether
  the Mojo communication (and FakeDataFetcher) live on the UI or IO thread.

BUG=612322
==========

to

==========
[Device Sensors] Move Mojo communication to UI thread on Android

In preparation for removing content::BrowserThread knowledge from
Device Sensors, this CL eliminates the need for SensorManagerAndroid to
live on multiple threads. Instead, Mojo communication for Device Sensors
occurs on the UI thread on Android, resulting in the following structure:

- On Android, all of the device sensors host code lives solely on the
UI thread.
- On all other platforms, all of the device sensors host code lives
solely on the IO thread.

This restructure will enable a straightforward followup that replaces
usage of content::BrowserThread with //base-level primitives in this code.

Note that device_sensor_browsertest.cc had to be updated to support this
change:
- Could no longer use WaitableEvents, as they blocked the UI thread while
  waiting for incoming Mojo communication, which on Android is now incoming
  on the UI thread :P.
- Instead, the tests spin up run loops and pass their QuitClosures into
  the FakeDataFetcher, which calls those QuitClosures on the UI thread when
  the appropriate events occur. In this way, the tests can wait for the
  various events (by Running the appropriate RunLoop) regardless of whether
  the Mojo communication (and FakeDataFetcher) live on the UI or IO thread.

BUG=612322

Committed: https://crrev.com/4489ef7781fd694d199e2c5a407f0614317de2d7
Cr-Commit-Position: refs/heads/master@{#432455}
==========

[commit-bot: I haz the power, 2016-11-16 11:02:53]

Patchset 8 (id:??) landed as
https://crrev.com/4489ef7781fd694d199e2c5a407f0614317de2d7
Cr-Commit-Position: refs/heads/master@{#432455}