Port BatteryMonitor into Device Service

Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920, 680936
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.

Review-Url: https://codereview.chromium.org/2643713002
Cr-Commit-Position: refs/heads/master@{#459089}
Committed: https://chromium.googlesource.com/chromium/src/+/663fd6d75da6a49f89485181bc7ce91463b39f91

[blundell, 2017-01-23 12:59:50]

Description was changed from

==========
In-progress port of BatteryMonitor into Device Service

NOT FOR REVIEW, NOT FOR SUBMISSION
==========

to

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920
==========

[blundell, 2017-01-23 13:01:02]

Description was changed from

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920
==========

to

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.
==========

[blundell, 2017-01-23 13:01:32]

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

[commit-bot: I haz the power, 2017-01-23 13:02:10]

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, 2017-01-23 13:03:12]

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

[commit-bot: I haz the power, 2017-01-23 13:03:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[blundell, 2017-01-23 13:06:48]

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

[commit-bot: I haz the power, 2017-01-23 13:07:05]

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, 2017-01-23 13:08:21]

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

[commit-bot: I haz the power, 2017-01-23 13:08:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[blundell, 2017-01-23 15:31:26]

Description was changed from

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.
==========

to

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.
==========

[blundell, 2017-01-23 15:34:03]

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

[commit-bot: I haz the power, 2017-01-23 15:34: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, 2017-01-23 15:35:05]

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

[commit-bot: I haz the power, 2017-01-23 15:35:06]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[blundell, 2017-01-23 16:20:34]

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

[commit-bot: I haz the power, 2017-01-23 16:20: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, 2017-01-23 16:21:53]

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

[commit-bot: I haz the power, 2017-01-23 16:21:54]

Dry run: 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...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[blundell, 2017-01-24 09:30:38]

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

[commit-bot: I haz the power, 2017-01-24 09:30: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, 2017-01-24 11:04:49]

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

[commit-bot: I haz the power, 2017-01-24 11:04:51]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-01-24 11:33:52]

Description was changed from

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.
==========

to

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920, 680936
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.
==========

[blundell, 2017-01-24 11:35:42]

blundell@chromium.org changed reviewers:
+ rockot@chromium.org, sammc@chromium.org, timvolodine@chromium.org

[blundell, 2017-01-24 11:35:43]

rockot@: whole CL
timvolodine@: for //content/browser/battery_status OWNERS, the question that I
left for you re: BatteryStatusService::Shutdown() and as much of the rest of the
CL as you'd like to look at :)
sammc@: for the Java interface registration in the Device Service

Thanks!

https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1372:
device::BatteryStatusService::GetInstance()->Shutdown();
As I wrote in the CL description, the move of this call to the Device Service
destructor has the effect of having this code execute earlier in this method, at
line 1259 (ServiceManagerContext's destructor calls
ServiceManagerConnection::DestroyForProcess(), which destroys the
ServiceManagerConnectionImpl that the Device Service was added to, which causes
the Device Service to be destroyed). From looking at
BatteryStatusService::Shutdown(), I don't see any problem with this move. Tim,
do you?

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
File
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java
(right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java:24:
BatteryMonitor.MANAGER, new BatteryMonitorFactory(applicationContext));
I originally thought I would generalize this code into a library available to
all services. However, there's so little code here that attempting to generalize
it seems like it would result in more or less the same amount of code
per-service to register that particular service's Java interfaces.

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
File services/device/device_service.h (right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
services/device/device_service.h:70: base::android::ScopedJavaGlobalRef<jobject>
java_interface_registry_;
AFAICT, there is nothing holding a reference to the created InterfaceRegistry on
the Java side, which is why I'm doing it here. However, my Java-fu is weak, and
I don't know if there is magic going on under the covers in the Java bindings
code.

[Ken Rockot(use gerrit already), 2017-01-24 21:15:34]

This is really great to see. LGTM all around.

[Sam McNally, 2017-01-25 05:53:06]

https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD.gn
File services/device/BUILD.gn (right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD....
services/device/BUILD.gn:40: deps -= [ "//device/battery" ]
if (is_android) {
  deps += [ ":device_service_jni_headers" ]
} else {
  deps += [ "//device/battery" ]
}

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
File
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java
(right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java:19:
static InterfaceRegistry createInterfaceRegistryForContext(
Return void.

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
services/device/device_service.cc:52: InitializeJavaInterfaceProviderIfNeeded();
I'd prefer this as a getter for |java_interface_provider_| that initializes if
necessary.

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
services/device/device_service.cc:54: registry->AddInterface<BatteryMonitor>(
The <BatteryMonitor> is unnecessary.

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
File services/device/device_service.h (right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
services/device/device_service.h:70: base::android::ScopedJavaGlobalRef<jobject>
java_interface_registry_;
On 2017/01/24 11:35:43, blundell wrote:
> AFAICT, there is nothing holding a reference to the created InterfaceRegistry
on
> the Java side, which is why I'm doing it here. However, my Java-fu is weak,
and
> I don't know if there is magic going on under the covers in the Java bindings
> code.

The bindings code will manage its lifetime.

The C++ side of WatcherImpl keeps a reference to it while it's watching it. That
chains Connector -> RouterImpl -> generated bindings (I think) -> mojo interface
impl.

[blundell, 2017-01-25 11:02:26]

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

[commit-bot: I haz the power, 2017-01-25 11:02:39]

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, 2017-01-25 11:04:37]

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

[commit-bot: I haz the power, 2017-01-25 11:04:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[blundell, 2017-01-25 11:11:22]

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

[commit-bot: I haz the power, 2017-01-25 11:11:34]

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, 2017-01-25 12:57:27]

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

[commit-bot: I haz the power, 2017-01-25 12:57:28]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-01-25 15:00:35]

Thanks! sammc@, PTAL.

https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD.gn
File services/device/BUILD.gn (right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/BUILD....
services/device/BUILD.gn:40: deps -= [ "//device/battery" ]
On 2017/01/25 05:53:06, Sam McNally wrote:
> if (is_android) {
>   deps += [ ":device_service_jni_headers" ]
> } else {
>   deps += [ "//device/battery" ]
> }

Done.

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
File
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java
(right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/25 05:53:06, Sam McNally wrote:
> 2017

Done.

https://codereview.chromium.org/2643713002/diff/120001/services/device/androi...
services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java:19:
static InterfaceRegistry createInterfaceRegistryForContext(
On 2017/01/25 05:53:06, Sam McNally wrote:
> Return void.

Done.

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
services/device/device_service.cc:52: InitializeJavaInterfaceProviderIfNeeded();
On 2017/01/25 05:53:06, Sam McNally wrote:
> I'd prefer this as a getter for |java_interface_provider_| that initializes if
> necessary.

Done.

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
services/device/device_service.cc:54: registry->AddInterface<BatteryMonitor>(
On 2017/01/25 05:53:06, Sam McNally wrote:
> The <BatteryMonitor> is unnecessary.

Done.

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
File services/device/device_service.h (right):

https://codereview.chromium.org/2643713002/diff/120001/services/device/device...
services/device/device_service.h:70: base::android::ScopedJavaGlobalRef<jobject>
java_interface_registry_;
On 2017/01/25 05:53:06, Sam McNally wrote:
> On 2017/01/24 11:35:43, blundell wrote:
> > AFAICT, there is nothing holding a reference to the created
InterfaceRegistry
> on
> > the Java side, which is why I'm doing it here. However, my Java-fu is weak,
> and
> > I don't know if there is magic going on under the covers in the Java
bindings
> > code.
> 
> The bindings code will manage its lifetime.
> 
> The C++ side of WatcherImpl keeps a reference to it while it's watching it.
That
> chains Connector -> RouterImpl -> generated bindings (I think) -> mojo
interface
> impl.

Awesome, thanks! Done.

[timvolodine, 2017-01-25 18:54:40]

maybe I am missing something, is the description wrt battery monitor mojo
correct? it says it's being hosted in //content/browser/ while apparently it
lives in device/battery/?

https://codereview.chromium.org/2643713002/diff/180001/content/browser/batter...
File content/browser/battery_status/battery_monitor_integration_browsertest.cc
(left):

https://codereview.chromium.org/2643713002/diff/180001/content/browser/batter...
content/browser/battery_status/battery_monitor_integration_browsertest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
do we need some similar test in the new framework?

https://codereview.chromium.org/2643713002/diff/180001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2643713002/diff/180001/services/device/device...
services/device/device_service.cc:42: DeviceService::~DeviceService() {
when does this happen?

https://codereview.chromium.org/2643713002/diff/180001/services/device/manife...
File services/device/manifest.json (right):

https://codereview.chromium.org/2643713002/diff/180001/services/device/manife...
services/device/manifest.json:7: "device:battery_monitor":
["device::BatteryMonitor" ],
nit: space before "

[blundell, 2017-01-26 08:42:09]

Thanks for the review, Tim!

With respect to the CL description: The BatteryMonitor *code* lives in
//device/battery, but the Mojo interface is *hosted* in
render_process_host_impl.cc (i.e., it's render_process_host_impl.cc that exposes
that interface to the renderer).

https://codereview.chromium.org/2643713002/diff/180001/content/browser/batter...
File content/browser/battery_status/battery_monitor_integration_browsertest.cc
(left):

https://codereview.chromium.org/2643713002/diff/180001/content/browser/batter...
content/browser/battery_status/battery_monitor_integration_browsertest.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2017/01/25 18:54:39, timvolodine wrote:
> do we need some similar test in the new framework?

I don't think so: These tests inject a dummy BatteryMonitor implementation and
test that this dummy implementation is correctly exposed to the renderer. The
functionality being tested is already covered by the BatteryMonitorImpl
browsertests, which remain.

https://codereview.chromium.org/2643713002/diff/180001/services/device/device...
File services/device/device_service.cc (right):

https://codereview.chromium.org/2643713002/diff/180001/services/device/device...
services/device/device_service.cc:42: DeviceService::~DeviceService() {
On 2017/01/25 18:54:39, timvolodine wrote:
> when does this happen?

Please see the comment that I left on browser_main_loop.cc in PS#7, which
explains this issue and has a question for you about whether the behavioral
change is OK.

[blundell, 2017-01-26 08:44:59]

https://codereview.chromium.org/2643713002/diff/180001/services/device/manife...
File services/device/manifest.json (right):

https://codereview.chromium.org/2643713002/diff/180001/services/device/manife...
services/device/manifest.json:7: "device:battery_monitor":
["device::BatteryMonitor" ],
On 2017/01/25 18:54:40, timvolodine wrote:
> nit: space before "

Done.

[Sam McNally, 2017-01-26 23:43:07]

lgtm

[blundell, 2017-01-27 12:52:19]

Tim: friendly ping.

[timvolodine, 2017-01-27 18:16:27]

https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
File content/browser/browser_main_loop.cc (left):

https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
content/browser/browser_main_loop.cc:1372:
device::BatteryStatusService::GetInstance()->Shutdown();
On 2017/01/24 11:35:43, blundell wrote:
> As I wrote in the CL description, the move of this call to the Device Service
> destructor has the effect of having this code execute earlier in this method,
at
> line 1259 (ServiceManagerContext's destructor calls
> ServiceManagerConnection::DestroyForProcess(), which destroys the
> ServiceManagerConnectionImpl that the Device Service was added to, which
causes
> the Device Service to be destroyed). From looking at
> BatteryStatusService::Shutdown(), I don't see any problem with this move. Tim,
> do you? 

hmm I am somewhat concerned that this would happen before ipc shutdown which
would be different from the current logic. Are you sure we won't hit 
DCHECK(!is_shutdown_) in AddCallback if such a call comes in after
BatteryStatusService::Shutdown ?

[blundell, 2017-01-30 14:28:24]

On 2017/01/27 18:16:27, timvolodine wrote:
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> File content/browser/browser_main_loop.cc (left):
> 
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> content/browser/browser_main_loop.cc:1372:
> device::BatteryStatusService::GetInstance()->Shutdown();
> On 2017/01/24 11:35:43, blundell wrote:
> > As I wrote in the CL description, the move of this call to the Device
Service
> > destructor has the effect of having this code execute earlier in this
method,
> at
> > line 1259 (ServiceManagerContext's destructor calls
> > ServiceManagerConnection::DestroyForProcess(), which destroys the
> > ServiceManagerConnectionImpl that the Device Service was added to, which
> causes
> > the Device Service to be destroyed). From looking at
> > BatteryStatusService::Shutdown(), I don't see any problem with this move.
Tim,
> > do you? 
> 
> hmm I am somewhat concerned that this would happen before ipc shutdown which
> would be different from the current logic. Are you sure we won't hit 
> DCHECK(!is_shutdown_) in AddCallback if such a call comes in after
> BatteryStatusService::Shutdown ?

AddCallback() comes in from BatteryMonitorImpl in response to a request to the
Device Service to connect to the Battery Monitor interface. The shutdown process
should (and I assume does) guarantee that the Device Service does not receive
any such requests after it's been destroyed; if the shutdown process violated
this guarantee, that would be a larger bug in the shutdown process that's
orthogonal to this code change.

[timvolodine, 2017-01-31 07:21:48]

On 2017/01/30 14:28:24, blundell wrote:
> On 2017/01/27 18:16:27, timvolodine wrote:
> >
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> > File content/browser/browser_main_loop.cc (left):
> > 
> >
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> > content/browser/browser_main_loop.cc:1372:
> > device::BatteryStatusService::GetInstance()->Shutdown();
> > On 2017/01/24 11:35:43, blundell wrote:
> > > As I wrote in the CL description, the move of this call to the Device
> Service
> > > destructor has the effect of having this code execute earlier in this
> method,
> > at
> > > line 1259 (ServiceManagerContext's destructor calls
> > > ServiceManagerConnection::DestroyForProcess(), which destroys the
> > > ServiceManagerConnectionImpl that the Device Service was added to, which
> > causes
> > > the Device Service to be destroyed). From looking at
> > > BatteryStatusService::Shutdown(), I don't see any problem with this move.
> Tim,
> > > do you? 
> > 
> > hmm I am somewhat concerned that this would happen before ipc shutdown which
> > would be different from the current logic. Are you sure we won't hit 
> > DCHECK(!is_shutdown_) in AddCallback if such a call comes in after
> > BatteryStatusService::Shutdown ?
> 
> AddCallback() comes in from BatteryMonitorImpl in response to a request to the
> Device Service to connect to the Battery Monitor interface. The shutdown
process
> should (and I assume does) guarantee that the Device Service does not receive
> any such requests after it's been destroyed; if the shutdown process violated
> this guarantee, that would be a larger bug in the shutdown process that's
> orthogonal to this code change.

if there is uncertainty maybe an option would be to leave the call to battery
shutdown where it is now?

[blundell, 2017-01-31 08:32:17]

On 2017/01/31 07:21:48, timvolodine wrote:
> On 2017/01/30 14:28:24, blundell wrote:
> > On 2017/01/27 18:16:27, timvolodine wrote:
> > >
> >
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> > > File content/browser/browser_main_loop.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> > > content/browser/browser_main_loop.cc:1372:
> > > device::BatteryStatusService::GetInstance()->Shutdown();
> > > On 2017/01/24 11:35:43, blundell wrote:
> > > > As I wrote in the CL description, the move of this call to the Device
> > Service
> > > > destructor has the effect of having this code execute earlier in this
> > method,
> > > at
> > > > line 1259 (ServiceManagerContext's destructor calls
> > > > ServiceManagerConnection::DestroyForProcess(), which destroys the
> > > > ServiceManagerConnectionImpl that the Device Service was added to, which
> > > causes
> > > > the Device Service to be destroyed). From looking at
> > > > BatteryStatusService::Shutdown(), I don't see any problem with this
move.
> > Tim,
> > > > do you? 
> > > 
> > > hmm I am somewhat concerned that this would happen before ipc shutdown
which
> > > would be different from the current logic. Are you sure we won't hit 
> > > DCHECK(!is_shutdown_) in AddCallback if such a call comes in after
> > > BatteryStatusService::Shutdown ?
> > 
> > AddCallback() comes in from BatteryMonitorImpl in response to a request to
the
> > Device Service to connect to the Battery Monitor interface. The shutdown
> process
> > should (and I assume does) guarantee that the Device Service does not
receive
> > any such requests after it's been destroyed; if the shutdown process
violated
> > this guarantee, that would be a larger bug in the shutdown process that's
> > orthogonal to this code change.
> 
> if there is uncertainty maybe an option would be to leave the call to battery
> shutdown where it is now?

The goal is to make //device/battery part of the internal implementation of the
Device Service, so direct code dependencies from //content need to be
eliminated; if we don't eliminate this dependency now, we'll just need to do it
later. What I was trying to express above is that it is conceptually disallowed
for the Device Service to receive new requests to connect to the battery monitor
(or any requests at all) after it has been destroyed, and if we saw that that in
fact happened, it would be a bug in the service manager infrastructure that we
would fix in short order.

[timvolodine, 2017-02-02 02:33:51]

On 2017/01/31 08:32:17, blundell wrote:
> On 2017/01/31 07:21:48, timvolodine wrote:
> > On 2017/01/30 14:28:24, blundell wrote:
> > > On 2017/01/27 18:16:27, timvolodine wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> > > > File content/browser/browser_main_loop.cc (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2643713002/diff/120001/content/browser/browse...
> > > > content/browser/browser_main_loop.cc:1372:
> > > > device::BatteryStatusService::GetInstance()->Shutdown();
> > > > On 2017/01/24 11:35:43, blundell wrote:
> > > > > As I wrote in the CL description, the move of this call to the Device
> > > Service
> > > > > destructor has the effect of having this code execute earlier in this
> > > method,
> > > > at
> > > > > line 1259 (ServiceManagerContext's destructor calls
> > > > > ServiceManagerConnection::DestroyForProcess(), which destroys the
> > > > > ServiceManagerConnectionImpl that the Device Service was added to,
which
> > > > causes
> > > > > the Device Service to be destroyed). From looking at
> > > > > BatteryStatusService::Shutdown(), I don't see any problem with this
> move.
> > > Tim,
> > > > > do you? 
> > > > 
> > > > hmm I am somewhat concerned that this would happen before ipc shutdown
> which
> > > > would be different from the current logic. Are you sure we won't hit 
> > > > DCHECK(!is_shutdown_) in AddCallback if such a call comes in after
> > > > BatteryStatusService::Shutdown ?
> > > 
> > > AddCallback() comes in from BatteryMonitorImpl in response to a request to
> the
> > > Device Service to connect to the Battery Monitor interface. The shutdown
> > process
> > > should (and I assume does) guarantee that the Device Service does not
> receive
> > > any such requests after it's been destroyed; if the shutdown process
> violated
> > > this guarantee, that would be a larger bug in the shutdown process that's
> > > orthogonal to this code change.
> > 
> > if there is uncertainty maybe an option would be to leave the call to
battery
> > shutdown where it is now?
> 
> The goal is to make //device/battery part of the internal implementation of
the
> Device Service, so direct code dependencies from //content need to be
> eliminated; if we don't eliminate this dependency now, we'll just need to do
it
> later. What I was trying to express above is that it is conceptually
disallowed
> for the Device Service to receive new requests to connect to the battery
monitor
> (or any requests at all) after it has been destroyed, and if we saw that that
in
> fact happened, it would be a bug in the service manager infrastructure that we
> would fix in short order.

OK, looks like this should be fine then. Thanks for the explanation.

//content/browser/battery_status -- lgtm

[blundell, 2017-03-22 17:23:01]

blundell@chromium.org changed reviewers:
+ jam@chromium.org, tsepez@chromium.org

[blundell, 2017-03-22 17:23:02]

jam@: Can you review //content?
tsepez@: Can you review the changes to manifest files?

Thanks!

[Tom Sepez, 2017-03-22 19:48:38]

lgtm

[jam, 2017-03-23 00:28:47]

lgtm

[blundell, 2017-03-23 10:45:37]

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

[commit-bot: I haz the power, 2017-03-23 10:45:51]

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, 2017-03-23 10:55:57]

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

[commit-bot: I haz the power, 2017-03-23 10:55:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[blundell, 2017-03-23 11:55:13]

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

[commit-bot: I haz the power, 2017-03-23 11:55:31]

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, 2017-03-23 14:00:36]

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

[commit-bot: I haz the power, 2017-03-23 14:00:38]

Dry run: This issue passed the CQ dry run.

[blundell, 2017-03-23 15:14:51]

The CQ bit was checked by blundell@chromium.org

[blundell, 2017-03-23 15:14:51]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
tsepez@chromium.org, jam@chromium.org, timvolodine@chromium.org,
sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/2643713002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-23 15:15:17]

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, 2017-03-23 15:21:46]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490282091097550,
"parent_rev": "b1087165c06ad1d860f4cab75f575a967814b2a5", "commit_rev":
"663fd6d75da6a49f89485181bc7ce91463b39f91"}

[commit-bot: I haz the power, 2017-03-23 15:23:15]

Description was changed from

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920, 680936
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.
==========

to

==========
Port BatteryMonitor into Device Service

This CL ports the BatteryMonitor Mojo interface from being hosted in
//content/browser to being hosted in the Device Service.

Of particular interest:

- Adds the ability for the Device Service to register interfaces in
  Java, taking the lead of //content/browser but being more simple as
  //content/browser is designed to allow an embedder to register
  such interfaces as well.
- Moves BatteryStatusService shutdown from BrowserMainLoop to the
  Device Service destructor. This has the concrete impact of moving
  this code to being called on Mojo shutdown, which is slightly earlier
  in BrowserMainLoop::ShutdownThreadsAndCleanUp() than it was
  previously being called.
- Eliminates the BatteryMonitorIntegration browsertests. These tests
  inject a dummy BatteryMonitor implementation and test that this
  dummy implementation is correctly exposed to the renderer. Injecting
  a dummy implementation into the Device Service from //content/browser
  is non-trivial, and the functionality being tested is already covered
  by the BatteryMonitorImpl browsertests, which remain.

BUG=678920, 680936
TEST=Visit http://pazguille.github.io/demo-battery-api/ and check that
     the information being displayed is correct.

Review-Url: https://codereview.chromium.org/2643713002
Cr-Commit-Position: refs/heads/master@{#459089}
Committed:
https://chromium.googlesource.com/chromium/src/+/663fd6d75da6a49f89485181bc7c...
==========

[commit-bot: I haz the power, 2017-03-23 15:23:17]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/663fd6d75da6a49f89485181bc7c...