Battery listener for download service in native code.

Battery listener for download service in native code.

We need to listen to battery change when Chrome is running.

This class decouples the battery listening details from download
service. Any underlying implementation change can be limited inside
this class.

In the future we may use device service to retrieve battery level,
right now it seems only content can embbed the device service.

BUG=717180
Review-Url: https://codereview.chromium.org/2867073004
Cr-Commit-Position: refs/heads/master@{#470676}
Committed: https://chromium.googlesource.com/chromium/src/+/6aa839b75ec10cf23591c1dfc2f9b4b910102a8c

[xingliu, 2017-05-09 03:01:12]

Description was changed from

==========
Battery listener for download service in native code.

We need to listen to battery change when Chrome is running.

In the future we may use device service to retrieve battery level,
right now it seems only content can embbed the device service.

BUG=717180
==========

to

==========
Battery listener for download service in native code.

We need to listen to battery change when Chrome is running.

This class decouple the battery listening details from download service.
Any underlying implementation change can be limited inside these two
files.

In the future we may use device service to retrieve battery level,
right now it seems only content can embbed the device service.

BUG=717180
==========

[xingliu, 2017-05-09 03:02:55]

Description was changed from

==========
Battery listener for download service in native code.

We need to listen to battery change when Chrome is running.

This class decouple the battery listening details from download service.
Any underlying implementation change can be limited inside these two
files.

In the future we may use device service to retrieve battery level,
right now it seems only content can embbed the device service.

BUG=717180
==========

to

==========
Battery listener for download service in native code.

We need to listen to battery change when Chrome is running.

This class decouples the battery listening details from download
service. Any underlying implementation change can be limited inside
this class.

In the future we may use device service to retrieve battery level,
right now it seems only content can embbed the device service.

BUG=717180
==========

[xingliu, 2017-05-09 03:05:36]

xingliu@chromium.org changed reviewers:
+ dtrainor@chromium.org, qinmin@chromium.org

[xingliu, 2017-05-09 03:05:37]

Hi, PTAL, thanks.

[David Trainor- moved to gerrit, 2017-05-09 06:03:31]

lgtm

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
File components/download/internal/scheduler/battery_listener.cc (right):

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.cc:52: for (auto it =
observers_.begin(); it != observers_.end(); ++it) {
Maybe:

for (auto& observer : observers)
  observer.OnBatteryChange(current_battery);

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
File components/download/internal/scheduler/battery_listener.h (right):

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.h:46: static
SchedulingParams::BatteryRequirements ToBatteryRequirement(
Do we need this in the .h file?  Maybe anonymous namespace method in the .cc?

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
File components/download/internal/scheduler/battery_listener_unittest.cc
(right):

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener_unittest.cc:56: // not
charing but higher than the threshold.
charing -> charging?

What does the threshold part mean?

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener_unittest.cc:67:
OnBatteryChange(SchedulingParams::BatteryRequirements::BATTERY_SENSITIVE))
That is one *long* enum.  :(

[qinmin, 2017-05-09 17:48:11]

lgtm % comments

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
File components/download/internal/scheduler/battery_listener.cc (right):

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.cc:21:
BatteryListener::~BatteryListener() = default;
should this call stop()?

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.cc:31:
monitor->AddObserver(this);
should we notify all observers if they are added before start()?

[xingliu, 2017-05-09 18:34:39]

Thanks for the feedback.

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
File components/download/internal/scheduler/battery_listener.cc (right):

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.cc:21:
BatteryListener::~BatteryListener() = default;
On 2017/05/09 17:48:10, qinmin wrote:
> should this call stop()?

Done. Good catch.

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.cc:31:
monitor->AddObserver(this);
On 2017/05/09 17:48:11, qinmin wrote:
> should we notify all observers if they are added before start()?

I'm thinking let the caller check the state from CurrentBatteryStatus() when
they add themselves to BatteryListener.
So the behavior of BatteryListener is similar to the underlying PowerMonitor,
and we don't need to cache extra states.

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.cc:52: for (auto it =
observers_.begin(); it != observers_.end(); ++it) {
On 2017/05/09 06:03:31, David Trainor-ping if over 24h wrote:
> Maybe:
> 
> for (auto& observer : observers)
>   observer.OnBatteryChange(current_battery);

Done.

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
File components/download/internal/scheduler/battery_listener.h (right):

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener.h:46: static
SchedulingParams::BatteryRequirements ToBatteryRequirement(
On 2017/05/09 06:03:31, David Trainor-ping if over 24h wrote:
> Do we need this in the .h file?  Maybe anonymous namespace method in the .cc?

Done, yeah, that's probably better.

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
File components/download/internal/scheduler/battery_listener_unittest.cc
(right):

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener_unittest.cc:56: // not
charing but higher than the threshold.
On 2017/05/09 06:03:31, David Trainor-ping if over 24h wrote:
> charing -> charging?
> 
> What does the threshold part mean?

Removed.

https://codereview.chromium.org/2867073004/diff/20001/components/download/int...
components/download/internal/scheduler/battery_listener_unittest.cc:67:
OnBatteryChange(SchedulingParams::BatteryRequirements::BATTERY_SENSITIVE))
On 2017/05/09 06:03:31, David Trainor-ping if over 24h wrote:
> That is one *long* enum.  :(

Done.

[xingliu, 2017-05-09 21:37:59]

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

[commit-bot: I haz the power, 2017-05-09 21:39:46]

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-05-09 22:49:27]

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

[commit-bot: I haz the power, 2017-05-09 22:49:30]

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

[xingliu, 2017-05-09 23:56:57]

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

[commit-bot: I haz the power, 2017-05-09 23:58:02]

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-05-10 02:00:40]

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

[commit-bot: I haz the power, 2017-05-10 02:00:41]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))

[xingliu, 2017-05-10 17:03:42]

The CQ bit was checked by xingliu@chromium.org

[xingliu, 2017-05-10 17:03:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/2867073004/#ps40001
(title: "Work on feedbacks.")

[commit-bot: I haz the power, 2017-05-10 17:05:03]

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-05-10 19:50:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494435822782910,
"parent_rev": "a42af916f8540c65e68d780e37bc07a8230eb5ba", "commit_rev":
"6aa839b75ec10cf23591c1dfc2f9b4b910102a8c"}

[commit-bot: I haz the power, 2017-05-10 19:50:21]

Description was changed from

==========
Battery listener for download service in native code.

We need to listen to battery change when Chrome is running.

This class decouples the battery listening details from download
service. Any underlying implementation change can be limited inside
this class.

In the future we may use device service to retrieve battery level,
right now it seems only content can embbed the device service.

BUG=717180
==========

to

==========
Battery listener for download service in native code.

We need to listen to battery change when Chrome is running.

This class decouples the battery listening details from download
service. Any underlying implementation change can be limited inside
this class.

In the future we may use device service to retrieve battery level,
right now it seems only content can embbed the device service.

BUG=717180

Review-Url: https://codereview.chromium.org/2867073004
Cr-Commit-Position: refs/heads/master@{#470676}
Committed:
https://chromium.googlesource.com/chromium/src/+/6aa839b75ec10cf23591c1dfc2f9...
==========

[commit-bot: I haz the power, 2017-05-10 19:50:28]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6aa839b75ec10cf23591c1dfc2f9...