Rewrite the Android implementation of BatteryMonitor directly in Java.

Rewrite the Android implementation of BatteryMonitor directly in Java.

This patch introduces a standalone Android implementation of the BatteryMonitor
service. New implementation is written in Java without any JNI code, and is made
available in the render process host using the Java ServiceRegistry.

BUG=420623
Committed: https://crrev.com/1dfc74724f2acfefbb4d783c65511d6a3f74cd9f
Cr-Commit-Position: refs/heads/master@{#309029}

[ppi, 2014-10-08 14:53:37]

ppi@chromium.org changed reviewers:
+ qsr@chromium.org

[ppi, 2014-10-08 14:58:45]

Status as of patch set 1:

- it works :) (in the browser with http://jsbin.com/battery-status-test)

- we're currently creating a separate BatteryStatusManager for every service
instance - we might want to consider a singleton aggregating the consumers,
similarly to what BatteryStatusService is doing in native

- the browsertests still don't work - they're injecting a fake native
BatteryStatusManager in the BatteryStatusService, which obviously wouldn't
affect our Java service. I need to think about it some more.

Ptal.

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java
(right):

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:60:
// ?
Ben, what do we need to do here? Log the error message and .stop() ?

[qsr, 2014-10-08 16:01:29]

About the tests, you should register the specific implementation as a mojo
service, which will override the service both for android and the other
platforms.

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java
(right):

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:42:
mManager = new BatteryStatusManager(appContext, mCallback);
As you said in your comment, you do not want a manager per connection.

Take the manager as a parameter, and have the factory owns it, it can then pass
it.

Also you will need to register to the factory when starting and deregistring
when either closing or on connection errors.

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:60:
// ?
On 2014/10/08 14:58:45, ppi wrote:
> Ben, what do we need to do here? Log the error message and .stop() ?

 Just stop. Do not log, it will happen when the other side close the connection.

[ppi, 2014-12-12 14:55:29]

This should be ready for a second pass, ptal.

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java
(right):

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:42:
mManager = new BatteryStatusManager(appContext, mCallback);
On 2014/10/08 16:01:29, qsr wrote:
> As you said in your comment, you do not want a manager per connection.
> 
> Take the manager as a parameter, and have the factory owns it, it can then
pass
> it.
> 
> Also you will need to register to the factory when starting and deregistring
> when either closing or on connection errors.

Done.

https://codereview.chromium.org/641703002/diff/1/device/battery/android/java/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorImpl.java:60:
// ?
On 2014/10/08 16:01:29, qsr wrote:
> On 2014/10/08 14:58:45, ppi wrote:
> > Ben, what do we need to do here? Log the error message and .stop() ?
> 
>  Just stop. Do not log, it will happen when the other side close the
connection.

Done.

[qsr, 2014-12-12 15:31:57]

https://codereview.chromium.org/641703002/diff/60001/content/browser/mojo/moj...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/641703002/diff/60001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.h:48: #endif
Keep a space before private:

https://codereview.chromium.org/641703002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java
(right):

https://codereview.chromium.org/641703002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java:19:
private static class BatteryMonitorImplementationFactory extends
BatteryMonitorFactory
Hum... You need this class because this is in content and BatterMonitorFactory
doesn't depend on content?

Also, not sure you want to extends the class. I'll prefer we use composition
there.

https://codereview.chromium.org/641703002/diff/60001/device/battery/android/j...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java
(right):

https://codereview.chromium.org/641703002/diff/60001/device/battery/android/j...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java:22:
public class BatteryMonitorFactory {
Can this class be merged into BatteryStatusManager, or do you have a reason to
keep it separate.

[ppi, 2014-12-12 16:02:16]

Thanks, ptal.

https://codereview.chromium.org/641703002/diff/60001/content/browser/mojo/moj...
File content/browser/mojo/mojo_application_host.h (right):

https://codereview.chromium.org/641703002/diff/60001/content/browser/mojo/moj...
content/browser/mojo/mojo_application_host.h:48: #endif
On 2014/12/12 15:31:57, qsr wrote:
> Keep a space before private:

Done.

https://codereview.chromium.org/641703002/diff/60001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java
(right):

https://codereview.chromium.org/641703002/diff/60001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ServiceRegistrar.java:19:
private static class BatteryMonitorImplementationFactory extends
BatteryMonitorFactory
On 2014/12/12 15:31:57, qsr wrote:
> Hum... You need this class because this is in content and BatterMonitorFactory
> doesn't depend on content?
> 
> Also, not sure you want to extends the class. I'll prefer we use composition
> there.

Yes. Commented on that and switched to composition.

https://codereview.chromium.org/641703002/diff/60001/device/battery/android/j...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java
(right):

https://codereview.chromium.org/641703002/diff/60001/device/battery/android/j...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java:22:
public class BatteryMonitorFactory {
On 2014/12/12 15:31:57, qsr wrote:
> Can this class be merged into BatteryStatusManager, or do you have a reason to
> keep it separate.

I feel that a combined class would be harder to read - there's nice separation
of responsibilities now, BatteryStatusManager fetching the information, and
BatteryMonitorFactory keeping track of listeners.

Also, we have tests for BatteryStatusManager in it's current form and interface.

Wdyt?

[qsr, 2014-12-12 16:49:08]

LGTM with nits.

https://codereview.chromium.org/641703002/diff/60001/device/battery/android/j...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java
(right):

https://codereview.chromium.org/641703002/diff/60001/device/battery/android/j...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java:22:
public class BatteryMonitorFactory {
On 2014/12/12 16:02:16, ppi wrote:
> On 2014/12/12 15:31:57, qsr wrote:
> > Can this class be merged into BatteryStatusManager, or do you have a reason
to
> > keep it separate.
> 
> I feel that a combined class would be harder to read - there's nice separation
> of responsibilities now, BatteryStatusManager fetching the information, and
> BatteryMonitorFactory keeping track of listeners.
> 
> Also, we have tests for BatteryStatusManager in it's current form and
interface.
> 
> Wdyt?

Acknowledged.

https://codereview.chromium.org/641703002/diff/100001/device/battery/android/...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java
(right):

https://codereview.chromium.org/641703002/diff/100001/device/battery/android/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java:34:
boolean charging, double chargingTime, double dischargingTime, double level) {
As discussed, could you pass a BatteryStatus there so that updates to the mojom
will not change this.

[ppi, 2014-12-12 17:10:41]

Thanks!

https://codereview.chromium.org/641703002/diff/100001/device/battery/android/...
File
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java
(right):

https://codereview.chromium.org/641703002/diff/100001/device/battery/android/...
device/battery/android/java/src/org/chromium/device/battery/BatteryMonitorFactory.java:34:
boolean charging, double chargingTime, double dischargingTime, double level) {
On 2014/12/12 16:49:08, qsr wrote:
> As discussed, could you pass a BatteryStatus there so that updates to the
mojom
> will not change this.

Done.

[ppi, 2014-12-12 17:17:00]

ppi@chromium.org changed reviewers:
+ jam@chromium.org, timvolodine@chromium.org

[ppi, 2014-12-12 17:17:01]

+ Tim, Jam

Tim, could you review the device/battery part?
Jam, could you review the content/ part or stamp based on Benjamin's review?

[timvolodine, 2014-12-15 12:47:21]

this is great :). just one question below.

https://codereview.chromium.org/641703002/diff/160001/device/battery/battery_...
File device/battery/battery_status_manager_android.cc (left):

https://codereview.chromium.org/641703002/diff/160001/device/battery/battery_...
device/battery/battery_status_manager_android.cc:49:
UMA_HISTOGRAM_BOOLEAN("BatteryStatus.StartAndroid", result);
any chance we could keep UMA logging with the new approach? I looks like you are
dropping support for this in the patch.

[ppi, 2014-12-15 13:52:38]

https://codereview.chromium.org/641703002/diff/160001/device/battery/battery_...
File device/battery/battery_status_manager_android.cc (left):

https://codereview.chromium.org/641703002/diff/160001/device/battery/battery_...
device/battery/battery_status_manager_android.cc:49:
UMA_HISTOGRAM_BOOLEAN("BatteryStatus.StartAndroid", result);
On 2014/12/15 12:47:21, timvolodine wrote:
> any chance we could keep UMA logging with the new approach? I looks like you
are
> dropping support for this in the patch.

Good catch, thanks!

The way UMA works now (with no real Java support), adding the call to
UMA_HISTOGRAM_BOOLEAN would unfortunately undo much of the benefits of this
change - the implementation would not be pure Java and we would need to bring
the JNI back.

Given that this particular histogram seems to be recording 100.00% of successes
for the last one month, I wonder if we should replace it with an assert on the
Java side in the new implementation, as a provisional measure until we get
resolution on http://crbug.com/442300 .

Wdyt?

[timvolodine, 2014-12-15 18:04:36]

On 2014/12/15 13:52:38, ppi wrote:
>
https://codereview.chromium.org/641703002/diff/160001/device/battery/battery_...
> File device/battery/battery_status_manager_android.cc (left):
> 
>
https://codereview.chromium.org/641703002/diff/160001/device/battery/battery_...
> device/battery/battery_status_manager_android.cc:49:
> UMA_HISTOGRAM_BOOLEAN("BatteryStatus.StartAndroid", result);
> On 2014/12/15 12:47:21, timvolodine wrote:
> > any chance we could keep UMA logging with the new approach? I looks like you
> are
> > dropping support for this in the patch.
> 
> Good catch, thanks!
> 
> The way UMA works now (with no real Java support), adding the call to
> UMA_HISTOGRAM_BOOLEAN would unfortunately undo much of the benefits of this
> change - the implementation would not be pure Java and we would need to bring
> the JNI back.
> 
> Given that this particular histogram seems to be recording 100.00% of
successes
> for the last one month, I wonder if we should replace it with an assert on the
> Java side in the new implementation, as a provisional measure until we get
> resolution on http://crbug.com/442300 .
> 
> Wdyt?

Hi ppi@,

hmm, I don't feel too strongly about it, but the UMA logging is a useful thing
in general. Even if it succeeds all the time it gives an indication of the
usage..

What I am concerned about is that this gets pushed on the long cycle. It is not
very clear to me how difficult it is to add UMA support in Java, and when that
would happen. For the battery API this UMA logging is not critical as you
observed, but we have other APIs which do need UMA support. So I think before
going ahead there should be at least some kind of plan for this.

[jam, 2014-12-15 19:22:31]

lgtm

[ppi, 2014-12-16 15:37:01]

Patchset #15 (id:280001) has been deleted

[ppi, 2014-12-16 15:37:07]

Patchset #14 (id:260001) has been deleted

[ppi, 2014-12-16 15:37:14]

Patchset #13 (id:240001) has been deleted

[ppi, 2014-12-16 15:37:31]

Patchset #12 (id:220001) has been deleted

[ppi, 2014-12-16 15:37:43]

Patchset #11 (id:200001) has been deleted

[timvolodine, 2014-12-17 13:27:17]

On 2014/12/15 19:22:31, jam wrote:
> lgtm

lgtm device/battery, if you are landing this without UMA for now, could you pls
add a TODO with crbug reference, thanks!

[ppi, 2014-12-17 14:34:49]

Patchset #13 (id:340001) has been deleted

[ppi, 2014-12-17 14:34:57]

Patchset #12 (id:320001) has been deleted

[ppi, 2014-12-17 14:35:14]

Patchset #11 (id:300001) has been deleted

[ppi, 2014-12-17 14:44:27]

Patchset #10 (id:180001) has been deleted

[ppi, 2014-12-17 19:02:50]

Thanks, Tim!

I'm adding the Java API for UMA in https://codereview.chromium.org/794273004/ .
When it lands, I will restore the histogram. (might be after and might be before
landing this one, as we're blocked on the webview build issues)

Added the TODO as requested.

[ppi, 2014-12-18 10:34:43]

Patchset #14 (id:440001) has been deleted

[ppi, 2014-12-18 14:19:26]

Patchset #18 (id:530001) has been deleted

[ppi, 2014-12-18 14:19:39]

Patchset #15 (id:480001) has been deleted

[ppi, 2014-12-18 15:39:17]

Patchset #15 (id:500001) has been deleted

[ppi, 2014-12-18 15:39:39]

Patchset #19 (id:610001) has been deleted

[ppi, 2014-12-18 15:39:47]

Patchset #18 (id:590001) has been deleted

[ppi, 2014-12-18 15:39:54]

Patchset #17 (id:570001) has been deleted

[ppi, 2014-12-18 15:40:02]

Patchset #16 (id:550001) has been deleted

[ppi, 2014-12-18 15:47:42]

ppi@chromium.org changed reviewers:
+ mkosiba@chromium.org

[ppi, 2014-12-18 15:51:13]

+Marcin for the webview build changes

Marcin, could you review:

 - android_webview/java_library_common.mk
 - android_webview/libwebviewchromium.gypi
 - device/battery/battery.gyp

[mkosiba (inactive), 2014-12-18 16:12:08]

mkosiba@chromium.org changed reviewers:
+ torne@chromium.org

[mkosiba (inactive), 2014-12-18 16:12:09]

this looks good to me but deferring to torne@ for final judgement

[Torne, 2014-12-18 16:20:47]

One suggestion, but then LGTM

https://codereview.chromium.org/641703002/diff/630001/device/battery/battery.gyp
File device/battery/battery.gyp (right):

https://codereview.chromium.org/641703002/diff/630001/device/battery/battery....
device/battery/battery.gyp:46: 'action': ['python', '--version'],
Not sure if this will actually make a difference in practise, but I would
suggest making the action here ['touch', '<@(_outputs)'] so that it will
actually ensure that the output files are newer than the inputs. The android
tree build is using make and so doesn't ahve ninja's restat behaviour that makes
it "okay" to not actually update all your outputs.

It might not really matter but I don't think it hurts :)

[ppi, 2014-12-18 16:54:15]

Thanks, Marcin and Torne!

https://codereview.chromium.org/641703002/diff/630001/device/battery/battery.gyp
File device/battery/battery.gyp (right):

https://codereview.chromium.org/641703002/diff/630001/device/battery/battery....
device/battery/battery.gyp:46: 'action': ['python', '--version'],
On 2014/12/18 16:20:47, Torne wrote:
> Not sure if this will actually make a difference in practise, but I would
> suggest making the action here ['touch', '<@(_outputs)'] so that it will
> actually ensure that the output files are newer than the inputs. The android
> tree build is using make and so doesn't ahve ninja's restat behaviour that
makes
> it "okay" to not actually update all your outputs.
> 
> It might not really matter but I don't think it hurts :)

Neat:), done.

[ppi, 2014-12-18 16:54:24]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-12-18 16:55:28]

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

[commit-bot: I haz the power, 2014-12-18 17:43:22]

Committed patchset #17 (id:650001)

[commit-bot: I haz the power, 2014-12-18 18:05:53]

Patchset 17 (id:??) landed as
https://crrev.com/1dfc74724f2acfefbb4d783c65511d6a3f74cd9f
Cr-Commit-Position: refs/heads/master@{#309029}