Adjust scheduling for non-user requested items

Adjust scheduling for non-user requested items

BUG=610521
Committed: https://crrev.com/da631f82220aa205ef6b6a7ef571351a077e965b
Cr-Commit-Position: refs/heads/master@{#416105}

[fgorski, 2016-08-24 16:07:03]

fgorski@chromium.org changed reviewers:
+ fgorski@chromium.org

[fgorski, 2016-08-24 16:07:04]

hey, overall approach looks good.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:279: if
(non_user_requested_tasks_remaining) {
nit: drop {}

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:413: bool power =
policy_->PowerRequiredForUserRequestedPage();
Here is an idea: how about policy_->PowerRequired(user_requested)?

This applies to all 6 calls.
WDYT?

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:423:
Scheduler::TriggerConditions trigger_conditions(power, battery, unmetered);
Can you return from here?

return Scheduler::TriggerConditions(power, ...);

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:178: 
nit: empty line

[dougarnett, 2016-08-24 17:32:00]

dougarnett@chromium.org changed reviewers:
+ dougarnett@chromium.org

[dougarnett, 2016-08-24 17:32:01]

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:274: void
RequestCoordinator::RequestQueueEmpty(
RequestNotPicked?

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:280:
scheduler_->Schedule(GetTriggerConditions(kNonUserRequest));
nit - harder to read than !kUserRequested

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_picker.cc (right):

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_picker.cc:114: if
(!current_conditions_->IsPowerConnected())
Think we should be using policy here instead of hardcoding. [Or else we may end
up considering AGSA as user-requested.]

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_picker.cc:118:
net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI) {
also wonder about considering policy unmetered connection setting here vs.
hardcode

[Pete Williamson, 2016-08-25 00:00:58]

All comments are addressed, and unit tests are added, but we will not commit
this for M54 since we do not need it for the existing non-user-requested
scenario.  We may or may not use this for M55.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:274: void
RequestCoordinator::RequestQueueEmpty(
On 2016/08/24 17:32:01, dougarnett wrote:
> RequestNotPicked?

Done.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:279: if
(non_user_requested_tasks_remaining) {
On 2016/08/24 16:07:04, fgorski wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:280:
scheduler_->Schedule(GetTriggerConditions(kNonUserRequest));
On 2016/08/24 17:32:01, dougarnett wrote:
> nit - harder to read than !kUserRequested

Done.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:413: bool power =
policy_->PowerRequiredForUserRequestedPage();
On 2016/08/24 16:07:04, fgorski wrote:
> Here is an idea: how about policy_->PowerRequired(user_requested)?
> 
> This applies to all 6 calls.
> WDYT?

Done.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:423:
Scheduler::TriggerConditions trigger_conditions(power, battery, unmetered);
On 2016/08/24 16:07:04, fgorski wrote:
> Can you return from here?
> 
> return Scheduler::TriggerConditions(power, ...);

Done.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:178: 
On 2016/08/24 16:07:04, fgorski wrote:
> nit: empty line

Done.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_picker.cc (right):

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_picker.cc:114: if
(!current_conditions_->IsPowerConnected())
On 2016/08/24 17:32:01, dougarnett wrote:
> Think we should be using policy here instead of hardcoding. [Or else we may
end
> up considering AGSA as user-requested.]

Done.

https://codereview.chromium.org/2269173003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_picker.cc:118:
net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI) {
On 2016/08/24 17:32:01, dougarnett wrote:
> also wonder about considering policy unmetered connection setting here vs.
> hardcode

Done.

[dougarnett, 2016-08-29 15:47:23]

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/offliner_policy.h (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/offliner_policy.h:88: int
BatteryPercentageRequiredForNonUserRequestedPage() const { return 25; }
delete this one too?

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:51: // Callback to
report when the processing of a triggered task is complete.
comment doesn't seem to match

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:199: void
RequestNotPicked(bool non_user_requested_tasks_remaining);
I wonder if this should be something like least_restrictive_task or
next_trigger_conditions to allow for other concerns (like activation time or
namespace) to be communicated as well.

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:579:
EXPECT_TRUE(scheduler_stub->schedule_called());
Seems like we also should check that Unschedule() was called

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_picker_unittest.cc (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_picker_unittest.cc:146: void
RequestPickerTest::RequestQueueEmpty(
rename here too

[Pete Williamson, 2016-09-01 00:10:31]

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/offliner_policy.h (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/offliner_policy.h:88: int
BatteryPercentageRequiredForNonUserRequestedPage() const { return 25; }
On 2016/08/29 15:47:22, dougarnett wrote:
> delete this one too?

Done.

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:51: // Callback to
report when the processing of a triggered task is complete.
On 2016/08/29 15:47:22, dougarnett wrote:
> comment doesn't seem to match

Done.

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:199: void
RequestNotPicked(bool non_user_requested_tasks_remaining);
On 2016/08/29 15:47:22, dougarnett wrote:
> I wonder if this should be something like least_restrictive_task or
> next_trigger_conditions to allow for other concerns (like activation time or
> namespace) to be communicated as well.

While it would be useful to have something like TriggerConditions to be passed
here, I'd prefer to have only request coordinator need to know about device
conditions, so I prefer passing back a boolean for request type instead of
device conditions.  While we could pass back a request, and may someday, which
one?  If there are multiple requests, one might not speak for all, so if we
expand the number of types, I'd prefer to make this boolean into an enum, and
pass back a type here.

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator_unittest.cc:579:
EXPECT_TRUE(scheduler_stub->schedule_called());
On 2016/08/29 15:47:22, dougarnett wrote:
> Seems like we also should check that Unschedule() was called

Done.

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_picker_unittest.cc (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_picker_unittest.cc:146: void
RequestPickerTest::RequestQueueEmpty(
On 2016/08/29 15:47:22, dougarnett wrote:
> rename here too

Done.

[dougarnett, 2016-09-01 18:24:13]

lgtm with comments

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:199: void
RequestNotPicked(bool non_user_requested_tasks_remaining);
On 2016/09/01 00:10:31, Pete Williamson wrote:
> On 2016/08/29 15:47:22, dougarnett wrote:
> > I wonder if this should be something like least_restrictive_task or
> > next_trigger_conditions to allow for other concerns (like activation time or
> > namespace) to be communicated as well.
> 
> While it would be useful to have something like TriggerConditions to be passed
> here, I'd prefer to have only request coordinator need to know about device
> conditions, so I prefer passing back a boolean for request type instead of
> device conditions.  While we could pass back a request, and may someday, which
> one?  If there are multiple requests, one might not speak for all, so if we
> expand the number of types, I'd prefer to make this boolean into an enum, and
> pass back a type here.

Ok, please comment the point of the arg so a bit easier to understand purpose.
Perhaps something along the lines of: 

// The parameter is a signal for what, if any, conditions to schedule future
processing for.

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.h:56: // Callback to
report when no request was available.
Could copy this comment to above RequestNotPickedCallback and update this one to
maybe "Callback with picked request"

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
File components/offline_pages/background/request_picker.h (right):

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
components/offline_pages/background/request_picker.h:35:
RequestCoordinator::RequestNotPickedCallback empty_callback,
not_picked_callback ?

[Pete Williamson, 2016-09-01 21:56:08]

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:199: void
RequestNotPicked(bool non_user_requested_tasks_remaining);
On 2016/09/01 18:24:13, dougarnett wrote:
> On 2016/09/01 00:10:31, Pete Williamson wrote:
> > On 2016/08/29 15:47:22, dougarnett wrote:
> > > I wonder if this should be something like least_restrictive_task or
> > > next_trigger_conditions to allow for other concerns (like activation time
or
> > > namespace) to be communicated as well.
> > 
> > While it would be useful to have something like TriggerConditions to be
passed
> > here, I'd prefer to have only request coordinator need to know about device
> > conditions, so I prefer passing back a boolean for request type instead of
> > device conditions.  While we could pass back a request, and may someday,
which
> > one?  If there are multiple requests, one might not speak for all, so if we
> > expand the number of types, I'd prefer to make this boolean into an enum,
and
> > pass back a type here.
> 
> Ok, please comment the point of the arg so a bit easier to understand purpose.
> Perhaps something along the lines of: 
> 
> // The parameter is a signal for what, if any, conditions to schedule future
> processing for.
> 

Done.

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.h:56: // Callback to
report when no request was available.
On 2016/09/01 18:24:13, dougarnett wrote:
> Could copy this comment to above RequestNotPickedCallback and update this one
to
> maybe "Callback with picked request"

Done.

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
File components/offline_pages/background/request_picker.h (right):

https://codereview.chromium.org/2269173003/diff/60001/components/offline_page...
components/offline_pages/background/request_picker.h:35:
RequestCoordinator::RequestNotPickedCallback empty_callback,
On 2016/09/01 18:24:13, dougarnett wrote:
> not_picked_callback ?

Done.

[Pete Williamson, 2016-09-01 21:56:15]

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

[commit-bot: I haz the power, 2016-09-01 21:57:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougarnett, 2016-09-01 22:09:23]

lgtm

[Pete Williamson, 2016-09-01 22:14:36]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-09-01 22:14:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org
Link to the patchset: https://codereview.chromium.org/2269173003/#ps100001
(title: "FGorski CR feedback")

[commit-bot: I haz the power, 2016-09-01 22:15:12]

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-01 23:01:01]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-01 23:03:40]

Description was changed from

==========
Adjust scheduling for non-user requested items

BUG=610521
==========

to

==========
Adjust scheduling for non-user requested items

BUG=610521

Committed: https://crrev.com/da631f82220aa205ef6b6a7ef571351a077e965b
Cr-Commit-Position: refs/heads/master@{#416105}
==========

[commit-bot: I haz the power, 2016-09-01 23:03:41]

Patchset 6 (id:??) landed as
https://crrev.com/da631f82220aa205ef6b6a7ef571351a077e965b
Cr-Commit-Position: refs/heads/master@{#416105}