Remove content::BrowserThread knowledge from Device Sensors

Remove content::BrowserThread knowledge from Device Sensors

In preparation for decoupling the Device Sensors implementation from
//content, this CL removes knowledge of content::BrowserThread from that
code. It does so by explicitly initializing relevant classes on the UI
thread so that they can save the TaskRunner of that thread in an ivar
and later post to it when necessary.

BUG=612322

[blundell, 2016-10-11 13:28:04]

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

[commit-bot: I haz the power, 2016-10-11 13:28:12]

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-11 13:51:30]

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

[commit-bot: I haz the power, 2016-10-11 13:51:31]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[blundell, 2016-10-11 15:01:12]

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

[commit-bot: I haz the power, 2016-10-11 15:01:24]

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-11 16:02:15]

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

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

Dry run: This issue passed the CQ dry run.

[blundell, 2016-10-11 17:06:00]

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

[timvolodine, 2016-10-12 16:28:58]

https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
File content/browser/device_sensors/device_sensor_service.cc (right):

https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
content/browser/device_sensors/device_sensor_service.cc:109: void
DeviceSensorService::InitOnIOThread() {
I am slightly confused by the two Init methods, with the first only doing
something for android, while the second seems a bit 'hacky'. Is there a way to
limit the change to android (and drop the InitOnIOThread/UIThread)?

A few ideas: maybe by either putting the ifdef block in InitOnUIThread in
browser_main_loop.cc directly.. or maybe even better to let the mojo IPC arrive
on UI thread for android and set the taskrunner there since most android calls
happen on UI anyway. That way we avoid the overhead of constructing
sensor_manager_android and its java counterpart on each startup. wdyt?

[blundell, 2016-10-13 14:09:54]

On 2016/10/12 16:28:58, timvolodine wrote:
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> File content/browser/device_sensors/device_sensor_service.cc (right):
> 
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> content/browser/device_sensors/device_sensor_service.cc:109: void
> DeviceSensorService::InitOnIOThread() {
> I am slightly confused by the two Init methods, with the first only doing
> something for android, while the second seems a bit 'hacky'. Is there a way to
> limit the change to android (and drop the InitOnIOThread/UIThread)?
>

The reason for the two init methods is that the DeviceSensorService singleton
assumed that it was created on the IO thread (cf. its thread_checker_ variable).

 
> A few ideas: maybe by either putting the ifdef block in InitOnUIThread in
> browser_main_loop.cc directly.. or maybe even better to let the mojo IPC
arrive
> on UI thread for android and set the taskrunner there since most android calls
> happen on UI anyway. That way we avoid the overhead of constructing
> sensor_manager_android and its java counterpart on each startup. wdyt?

I think that we could indeed do the latter. The tradeoff then is that we'll have
DeviceSensorService and DeviceSensorHost living on the UI thread on Android and
the IO thread on other platforms. Is that overall less confusing in your
opinion?

We could also do the former, in which case I would drop the "InitOnUIThread()"
from DeviceSensorService entirely and just have browser_main_loop.cc directly
call SensorManagerAndroid::InitOnUIThread() on Android. This does still have the
property of creating SensorManagerAndroid on startup though, as you point out.

Out of curiosity, why do these objects need to live on the IO thread on
platforms other than Android?

[blundell, 2016-10-13 14:12:37]

On 2016/10/13 14:09:54, blundell wrote:
> On 2016/10/12 16:28:58, timvolodine wrote:
> >
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> > File content/browser/device_sensors/device_sensor_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> > content/browser/device_sensors/device_sensor_service.cc:109: void
> > DeviceSensorService::InitOnIOThread() {
> > I am slightly confused by the two Init methods, with the first only doing
> > something for android, while the second seems a bit 'hacky'. Is there a way
to
> > limit the change to android (and drop the InitOnIOThread/UIThread)?
> >
> 
> The reason for the two init methods is that the DeviceSensorService singleton
> assumed that it was created on the IO thread (cf. its thread_checker_
variable).
> 
>  
> > A few ideas: maybe by either putting the ifdef block in InitOnUIThread in
> > browser_main_loop.cc directly.. or maybe even better to let the mojo IPC
> arrive
> > on UI thread for android and set the taskrunner there since most android
calls
> > happen on UI anyway. That way we avoid the overhead of constructing
> > sensor_manager_android and its java counterpart on each startup. wdyt?
> 
> I think that we could indeed do the latter. The tradeoff then is that we'll
have
> DeviceSensorService and DeviceSensorHost living on the UI thread on Android
and
> the IO thread on other platforms. Is that overall less confusing in your
> opinion?

(apologies if this came off as hostile, it's an honest question as I can't
decide which is "better" ;).

> 
> We could also do the former, in which case I would drop the "InitOnUIThread()"
> from DeviceSensorService entirely and just have browser_main_loop.cc directly
> call SensorManagerAndroid::InitOnUIThread() on Android. This does still have
the
> property of creating SensorManagerAndroid on startup though, as you point out.
> 
> Out of curiosity, why do these objects need to live on the IO thread on
> platforms other than Android?

[blundell, 2016-10-13 15:21:50]

Patchset #4 (id:60001) has been deleted

[blundell, 2016-10-13 15:21:56]

Patchset #3 (id:40001) has been deleted

[timvolodine, 2016-10-13 16:14:56]

On 2016/10/13 14:12:37, blundell wrote:
> On 2016/10/13 14:09:54, blundell wrote:
> > On 2016/10/12 16:28:58, timvolodine wrote:
> > >
> >
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> > > File content/browser/device_sensors/device_sensor_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> > > content/browser/device_sensors/device_sensor_service.cc:109: void
> > > DeviceSensorService::InitOnIOThread() {
> > > I am slightly confused by the two Init methods, with the first only doing
> > > something for android, while the second seems a bit 'hacky'. Is there a
way
> to
> > > limit the change to android (and drop the InitOnIOThread/UIThread)?
> > >
> > 
> > The reason for the two init methods is that the DeviceSensorService
singleton
> > assumed that it was created on the IO thread (cf. its thread_checker_
> variable).
> > 
> >  
> > > A few ideas: maybe by either putting the ifdef block in InitOnUIThread in
> > > browser_main_loop.cc directly.. or maybe even better to let the mojo IPC
> > arrive
> > > on UI thread for android and set the taskrunner there since most android
> calls
> > > happen on UI anyway. That way we avoid the overhead of constructing
> > > sensor_manager_android and its java counterpart on each startup. wdyt?
> > 
> > I think that we could indeed do the latter. The tradeoff then is that we'll
> have
> > DeviceSensorService and DeviceSensorHost living on the UI thread on Android
> and
> > the IO thread on other platforms. Is that overall less confusing in your
> > opinion?
> 
> (apologies if this came off as hostile, it's an honest question as I can't
> decide which is "better" ;).

not at all, all valid questions ;)

I think I prefer the option with the mojo host on UI thread (for Android), this
has  a number of benefits:
- it's local (in the sense we stay in device_sensors/ and don't touch startup
etc)
- probably easier to change if we decide to put it on a different thread later
- does not 'pollute' startup, objects are created on demand
- we do similar things for some IPCs in other places
- probably requires less methods..

> 
> > 
> > We could also do the former, in which case I would drop the
"InitOnUIThread()"
> > from DeviceSensorService entirely and just have browser_main_loop.cc
directly
> > call SensorManagerAndroid::InitOnUIThread() on Android. This does still have
> the
> > property of creating SensorManagerAndroid on startup though, as you point
out.
> > 
> > Out of curiosity, why do these objects need to live on the IO thread on
> > platforms other than Android?

I guess they could live on UI but if I remember correctly the original thinking
was to unload the UI thread as much as possible..

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

On 2016/10/13 16:14:56, timvolodine wrote:
> On 2016/10/13 14:12:37, blundell wrote:
> > On 2016/10/13 14:09:54, blundell wrote:
> > > On 2016/10/12 16:28:58, timvolodine wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> > > > File content/browser/device_sensors/device_sensor_service.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2410123002/diff/20001/content/browser/device_...
> > > > content/browser/device_sensors/device_sensor_service.cc:109: void
> > > > DeviceSensorService::InitOnIOThread() {
> > > > I am slightly confused by the two Init methods, with the first only
doing
> > > > something for android, while the second seems a bit 'hacky'. Is there a
> way
> > to
> > > > limit the change to android (and drop the InitOnIOThread/UIThread)?
> > > >
> > > 
> > > The reason for the two init methods is that the DeviceSensorService
> singleton
> > > assumed that it was created on the IO thread (cf. its thread_checker_
> > variable).
> > > 
> > >  
> > > > A few ideas: maybe by either putting the ifdef block in InitOnUIThread
in
> > > > browser_main_loop.cc directly.. or maybe even better to let the mojo IPC
> > > arrive
> > > > on UI thread for android and set the taskrunner there since most android
> > calls
> > > > happen on UI anyway. That way we avoid the overhead of constructing
> > > > sensor_manager_android and its java counterpart on each startup. wdyt?
> > > 
> > > I think that we could indeed do the latter. The tradeoff then is that
we'll
> > have
> > > DeviceSensorService and DeviceSensorHost living on the UI thread on
Android
> > and
> > > the IO thread on other platforms. Is that overall less confusing in your
> > > opinion?
> > 
> > (apologies if this came off as hostile, it's an honest question as I can't
> > decide which is "better" ;).
> 
> not at all, all valid questions ;)
> 
> I think I prefer the option with the mojo host on UI thread (for Android),
this
> has  a number of benefits:
> - it's local (in the sense we stay in device_sensors/ and don't touch startup
> etc)
> - probably easier to change if we decide to put it on a different thread later
> - does not 'pollute' startup, objects are created on demand
> - we do similar things for some IPCs in other places
> - probably requires less methods..

Agree with all of this. Will create that patch.

> 
> > 
> > > 
> > > We could also do the former, in which case I would drop the
> "InitOnUIThread()"
> > > from DeviceSensorService entirely and just have browser_main_loop.cc
> directly
> > > call SensorManagerAndroid::InitOnUIThread() on Android. This does still
have
> > the
> > > property of creating SensorManagerAndroid on startup though, as you point
> out.
> > > 
> > > Out of curiosity, why do these objects need to live on the IO thread on
> > > platforms other than Android?
> 
> I guess they could live on UI but if I remember correctly the original
thinking
> was to unload the UI thread as much as possible..

[blundell, 2016-10-24 21:38:56]

Patchset #4 (id:100001) has been deleted

[blundell, 2016-10-24 21:40:49]

Patchset #4 (id:120001) has been deleted

[blundell, 2016-10-24 21:42:12]

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

[commit-bot: I haz the power, 2016-10-24 21:42:47]

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-25 00:34:10]

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

[commit-bot: I haz the power, 2016-10-25 00:34:11]

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

[blundell, 2016-11-09 14:43:08]

Description was changed from

==========
Remove content::BrowserThread knowledge from Device Sensors

In preparation for decoupling the Device Sensors implementation from
//content, this CL removes knowledge of content::BrowserThread from that
code. It does so by explicitly initializing relevant classes on the UI
thread so that they can save the TaskRunner of that thread in an ivar
and later post to it when necessary.

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

to

==========
Remove content::BrowserThread knowledge from Device Sensors

In preparation for decoupling the Device Sensors implementation from
//content, this CL removes knowledge of content::BrowserThread from that
code. It does so by explicitly initializing relevant classes on the UI
thread so that they can save the TaskRunner of that thread in an ivar
and later post to it when necessary.

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