Record UMA for battery level when precache starts and ends

Record UMA for battery level when precache starts and ends

BUG=646184
Committed: https://crrev.com/868a1534ff4f840f367ba7f976176a147c385b7b
Cr-Commit-Position: refs/heads/master@{#419337}

[Raj, 2016-09-12 23:29:52]

rajendrant@chromium.org changed reviewers:
+ sclittle@chromium.org

[sclittle, 2016-09-14 16:44:44]

https://codereview.chromium.org/2333063002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2333063002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:67:
static final int WAIT_UNTIL_NEXT_PRECACHE_SECONDS = 2 * 60; // 6 hours.
You changed this to 2 minutes, was this intentional?

https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:44785: +<histogram
name="Precache.BatteryPercentageDiff.End" units="%">
I thought precaching only happens when the device is charging?

If it's being changed to happen when the device isn't charging, then should we
record something special here for when power is connected vs. when it isn't?

E.g., if you're experimenting with doing precaching even when the device isn't
charging, then you'll probably want to demonstrate that when the device isn't
charging, precaching doesn't use up too much power. Wouldn't you need to have a
histogram for measuring the battery percentage difference when the device isn't
charging, instead of having one that combines measurements for when it's
charging and not charging?

It's probably good enough to check if the device is charging when precaching
starts (or when precaching ends, if that's easier). There's probably no need to
do something more complicated to handle the case where the device is
connected/disconnected from power while it's precaching.

Or are you planning to do something special when analyzing this histogram to
answer this question without making this distinction?

[Raj, 2016-09-14 16:50:57]

https://codereview.chromium.org/2333063002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2333063002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:67:
static final int WAIT_UNTIL_NEXT_PRECACHE_SECONDS = 2 * 60; // 6 hours.
On 2016/09/14 16:44:44, sclittle wrote:
> You changed this to 2 minutes, was this intentional?

Done.

https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:44785: +<histogram
name="Precache.BatteryPercentageDiff.End" units="%">
On 2016/09/14 16:44:44, sclittle wrote:
> I thought precaching only happens when the device is charging?
> 
> If it's being changed to happen when the device isn't charging, then should we
> record something special here for when power is connected vs. when it isn't?
> 
> E.g., if you're experimenting with doing precaching even when the device isn't
> charging, then you'll probably want to demonstrate that when the device isn't
> charging, precaching doesn't use up too much power. Wouldn't you need to have
a
> histogram for measuring the battery percentage difference when the device
isn't
> charging, instead of having one that combines measurements for when it's
> charging and not charging?
> 
> It's probably good enough to check if the device is charging when precaching
> starts (or when precaching ends, if that's easier). There's probably no need
to
> do something more complicated to handle the case where the device is
> connected/disconnected from power while it's precaching.
> 
> Or are you planning to do something special when analyzing this histogram to
> answer this question without making this distinction?

Yes. Currently it happens only when device is charging.
The plan is to disable power requirement only in DEV and find the power usage.
I will disable the power checks in a separate CL.
After analyzing the power usage, we could further disable power checks in
STABLE.

[Raj, 2016-09-14 16:57:15]

On 2016/09/14 16:50:57, Raj wrote:
>
https://codereview.chromium.org/2333063002/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
> (right):
> 
>
https://codereview.chromium.org/2333063002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:67:
> static final int WAIT_UNTIL_NEXT_PRECACHE_SECONDS = 2 * 60; // 6 hours.
> On 2016/09/14 16:44:44, sclittle wrote:
> > You changed this to 2 minutes, was this intentional?
> 
> Done.
> 
>
https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:44785: +<histogram
> name="Precache.BatteryPercentageDiff.End" units="%">
> On 2016/09/14 16:44:44, sclittle wrote:
> > I thought precaching only happens when the device is charging?
> > 
> > If it's being changed to happen when the device isn't charging, then should
we
> > record something special here for when power is connected vs. when it isn't?
> > 
> > E.g., if you're experimenting with doing precaching even when the device
isn't
> > charging, then you'll probably want to demonstrate that when the device
isn't
> > charging, precaching doesn't use up too much power. Wouldn't you need to
have
> a
> > histogram for measuring the battery percentage difference when the device
> isn't
> > charging, instead of having one that combines measurements for when it's
> > charging and not charging?
> > 
> > It's probably good enough to check if the device is charging when precaching
> > starts (or when precaching ends, if that's easier). There's probably no need
> to
> > do something more complicated to handle the case where the device is
> > connected/disconnected from power while it's precaching.
> > 
> > Or are you planning to do something special when analyzing this histogram to
> > answer this question without making this distinction?
> 
> Yes. Currently it happens only when device is charging.
> The plan is to disable power requirement only in DEV and find the power usage.
> I will disable the power checks in a separate CL.
> After analyzing the power usage, we could further disable power checks in
> STABLE.

I do not intend to check if power is connected or disconnected at the start of
precache or during the precache.
The intention is to find the power usage of precache.
I simply do not log the BatteryPercentageDiff UMA if the battery level has
increased.
Only when battery usage is >=0 BatteryPercentageDiff is logged.
If device is charged anytime during the precache cycle, battery level may
increase, and it will not get logged.
WDYT.

[sclittle, 2016-09-14 17:17:13]

LGTM % nits

https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2333063002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:44785: +<histogram
name="Precache.BatteryPercentageDiff.End" units="%">
On 2016/09/14 16:50:57, Raj wrote:
> On 2016/09/14 16:44:44, sclittle wrote:
> > I thought precaching only happens when the device is charging?
> > 
> > If it's being changed to happen when the device isn't charging, then should
we
> > record something special here for when power is connected vs. when it isn't?
> > 
> > E.g., if you're experimenting with doing precaching even when the device
isn't
> > charging, then you'll probably want to demonstrate that when the device
isn't
> > charging, precaching doesn't use up too much power. Wouldn't you need to
have
> a
> > histogram for measuring the battery percentage difference when the device
> isn't
> > charging, instead of having one that combines measurements for when it's
> > charging and not charging?
> > 
> > It's probably good enough to check if the device is charging when precaching
> > starts (or when precaching ends, if that's easier). There's probably no need
> to
> > do something more complicated to handle the case where the device is
> > connected/disconnected from power while it's precaching.
> > 
> > Or are you planning to do something special when analyzing this histogram to
> > answer this question without making this distinction?
> 
> Yes. Currently it happens only when device is charging.
> The plan is to disable power requirement only in DEV and find the power usage.
> I will disable the power checks in a separate CL.
> After analyzing the power usage, we could further disable power checks in
> STABLE.

Acknowledged.

https://codereview.chromium.org/2333063002/diff/10001/components/precache/and...
File
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java
(right):

https://codereview.chromium.org/2333063002/diff/10001/components/precache/and...
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java:87:
int level = batteryStatus.getIntExtra(BatteryManager.EXTRA_LEVEL, -1);
Should there be a check here for if this returns -1? Same below.

https://codereview.chromium.org/2333063002/diff/10001/components/precache/and...
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java:91:
return Math.round(100 * level / (float) scale);
Do you need to use floating point arithmetic here? Or is it just good enough to
do (100 * level) / scale?

[Raj, 2016-09-14 18:56:31]

https://codereview.chromium.org/2333063002/diff/10001/components/precache/and...
File
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java
(right):

https://codereview.chromium.org/2333063002/diff/10001/components/precache/and...
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java:87:
int level = batteryStatus.getIntExtra(BatteryManager.EXTRA_LEVEL, -1);
On 2016/09/14 17:17:12, sclittle wrote:
> Should there be a check here for if this returns -1? Same below.

Done.

https://codereview.chromium.org/2333063002/diff/10001/components/precache/and...
components/precache/android/java/src/org/chromium/components/precache/DeviceState.java:91:
return Math.round(100 * level / (float) scale);
On 2016/09/14 17:17:11, sclittle wrote:
> Do you need to use floating point arithmetic here? Or is it just good enough
to
> do (100 * level) / scale?

Its not clear what will be the value of scale for different devices.
If its too low, it would affect the calculation.

[Raj, 2016-09-14 18:56:35]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-09-14 18:56:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/2333063002/#ps30001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-09-14 18:57:28]

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-09-14 19:09:30]

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

[commit-bot: I haz the power, 2016-09-14 19:09:32]

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...)

[Raj, 2016-09-14 20:17:21]

rajendrant@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Raj, 2016-09-14 20:17:22]

rkaplow: ptal histograms.xml

[rkaplow, 2016-09-14 20:25:38]

Description was changed from

==========
Record UMA for battery level when precache starts and ends

BUG=646184
==========

to

==========
Record UMA for battery level when precache starts and ends

BUG=646184
==========

[rkaplow, 2016-09-14 20:25:38]

rkaplow@chromium.org changed reviewers:
+ gayane@chromium.org
- rkaplow@chromium.org

[gayane -on leave until 09-2017, 2016-09-16 21:22:24]

gayane@chromium.org changed reviewers:
+ rkaplow@chromium.org

[gayane -on leave until 09-2017, 2016-09-16 21:22:25]

LGTM 

+rkaplow for approval

[rkaplow, 2016-09-16 21:24:47]

lgtm

[Raj, 2016-09-16 22:17:50]

The CQ bit was checked by rajendrant@chromium.org

[commit-bot: I haz the power, 2016-09-16 22:18:21]

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-09-17 00:12:53]

Description was changed from

==========
Record UMA for battery level when precache starts and ends

BUG=646184
==========

to

==========
Record UMA for battery level when precache starts and ends

BUG=646184
==========

[commit-bot: I haz the power, 2016-09-17 00:12:55]

Committed patchset #3 (id:30001)

[commit-bot: I haz the power, 2016-09-17 00:15:34]

Description was changed from

==========
Record UMA for battery level when precache starts and ends

BUG=646184
==========

to

==========
Record UMA for battery level when precache starts and ends

BUG=646184

Committed: https://crrev.com/868a1534ff4f840f367ba7f976176a147c385b7b
Cr-Commit-Position: refs/heads/master@{#419337}
==========

[commit-bot: I haz the power, 2016-09-17 00:15:36]

Patchset 3 (id:??) landed as
https://crrev.com/868a1534ff4f840f367ba7f976176a147c385b7b
Cr-Commit-Position: refs/heads/master@{#419337}