[Android] Move the android Device Sensors API to the UI thread.

[Android] Move the android Device Sensors API to the UI thread.

Move the Java-implementation of Device Motion/Orientation/Light APIs
to run on the UI thread. This is to avoid a blocking call from the
IO thread to the UI thread inside DeviceSensors.java. That call can
lead to deadlock and hence is removed in this patch.

BUG=455767
Committed: https://crrev.com/c166ffe71f201a40c6e7b237f76e0b3e63b41137
Cr-Commit-Position: refs/heads/master@{#316562}

[timvolodine, 2015-02-13 14:48:35]

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

[jdduke (slow), 2015-02-13 16:26:57]

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:179:
Java_DeviceSensors_stop(AttachCurrentThread(),
So will all Java calls now take place on the UI thread? Should we add more
DCHECKs? We could also use a ThreadChecker, but I guess that doesn't save any
boilerplate.

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:365:
base::Unretained(this),
What guarantees do we have about the lifetime of |this|? Do we need a weak
pointer?

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:371:
DeviceOrientationHardwareBuffer* buffer) {
Passing this buffer makes me a little nervous, how can we guarantee it stays
valid for the asynchronous call? Similar questions for the other sensor types.

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:421:
StopFetchingOrientationDataOnUI();
We should probably have a |DCHECK_CURRENTLY_ON(BrowserThread::UI);| here as
well?

[timvolodine, 2015-02-13 18:07:26]

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:179:
Java_DeviceSensors_stop(AttachCurrentThread(),
On 2015/02/13 16:26:57, jdduke wrote:
> So will all Java calls now take place on the UI thread? Should we add more
> DCHECKs? We could also use a ThreadChecker, but I guess that doesn't save any
> boilerplate.

the stop/starts will all be on UI thread. however now that you mention it the
getInstance in the constructor looks like is still on IO. hmm not sure if that's
a problem though.

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:365:
base::Unretained(this),
On 2015/02/13 16:26:57, jdduke wrote:
> What guarantees do we have about the lifetime of |this|? Do we need a weak
> pointer?

this is a singleton so will outlive callbacks. At some point we should switch to
non singletons and that way weak pointers would make more sense.

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:371:
DeviceOrientationHardwareBuffer* buffer) {
On 2015/02/13 16:26:57, jdduke wrote:
> Passing this buffer makes me a little nervous, how can we guarantee it stays
> valid for the asynchronous call? Similar questions for the other sensor types.

yeah it's a bit confusing, but these pointers are kept alive once created until
Shutdown() (which is checked below). Also stop/start happen on one thread (UI)
so don't think we should worry about this pointer being valid.

https://codereview.chromium.org/917253002/diff/20001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:421:
StopFetchingOrientationDataOnUI();
On 2015/02/13 16:26:57, jdduke wrote:
> We should probably have a |DCHECK_CURRENTLY_ON(BrowserThread::UI);| here as
> well?

Yes. Done.

[jdduke (slow), 2015-02-13 18:19:17]

lgtm, thanks! (though it might be worth getting another reviewer more familiar
with the code to review?)

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
File content/browser/device_sensors/data_fetcher_shared_memory_android.cc
(right):

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_android.cc:19: }
Wonder if it's worth validating that Shutdown() has been called previously? Not
sure, maybe that would be awkward to check.

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:202:
BrowserThread::PostTask(
Would it make any sense to check if we're already on the UI thread, and if so
call the implementation directly (and returning that result)? Would that make
testing any simpler?

[timvolodine, 2015-02-16 16:24:29]

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

[timvolodine, 2015-02-16 16:36:50]

thanks Jared,

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
File content/browser/device_sensors/data_fetcher_shared_memory_android.cc
(right):

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_android.cc:19: }
On 2015/02/13 18:19:17, jdduke wrote:
> Wonder if it's worth validating that Shutdown() has been called previously?
Not
> sure, maybe that would be awkward to check.

we have a DCHECK in the destructor of data_fetcher_shared_memory_base that the
number of started consumers is zero, so that should cover stopping of sensors.
checking here for shutdown would require adding an extra member while not really
necessary due to the above.

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
File content/browser/device_sensors/sensor_manager_android.cc (right):

https://codereview.chromium.org/917253002/diff/40001/content/browser/device_s...
content/browser/device_sensors/sensor_manager_android.cc:202:
BrowserThread::PostTask(
On 2015/02/13 18:19:17, jdduke wrote:
> Would it make any sense to check if we're already on the UI thread, and if so
> call the implementation directly (and returning that result)? Would that make
> testing any simpler?

sure I've added direct calls when on the UI thread. simplifies testing a bit as
well.

[timvolodine, 2015-02-16 16:38:01]

+ cc: mvanouwerkerk@ who may be familiar with some device sensor bits..

[jdduke (slow), 2015-02-16 17:28:13]

On 2015/02/16 16:38:01, timvolodine wrote:
> + cc: mvanouwerkerk@ who may be familiar with some device sensor bits..

Thanks, as a non-owner, lgtm.

[timvolodine, 2015-02-16 17:45:17]

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

[Michael van Ouwerkerk, 2015-02-16 18:42:08]

mvanouwerkerk@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2015-02-16 18:42:09]

lgtm

[timvolodine, 2015-02-17 10:49:10]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-02-17 10:50:19]

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

[commit-bot: I haz the power, 2015-02-17 10:55:02]

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

[commit-bot: I haz the power, 2015-02-17 10:55:03]

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

[timvolodine, 2015-02-17 11:06:57]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-02-17 11:07:28]

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

[commit-bot: I haz the power, 2015-02-17 11:12:27]

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

[commit-bot: I haz the power, 2015-02-17 11:12:29]

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

[timvolodine, 2015-02-17 11:25:02]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2015-02-17 11:25:52]

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

[commit-bot: I haz the power, 2015-02-17 11:38:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-17 11:38:41]

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