[Offline Pages] Add evaluation test support in RequestCoordinator.

[Offline Pages] Add evaluation test support in RequestCoodinator.

Added the function to add customized callback which would be called
after an user request is completed. Also fixed the issue where the
scheduler callback would not be invoked if there's a failure during the
offlining, also made it have the same behavior as described in comments.
Changed some tests regarding to the changes above.

BUG=653708, 646947
Committed: https://crrev.com/1c1f5c1b9927576a524b1fe3476035f4d1128e79
Cr-Commit-Position: refs/heads/master@{#426115}

[romax, 2016-10-18 01:09:39]

Description was changed from

==========
[Offline Pages] Add evaluation test support in RequestCoodinator.

Added the function to add customized callback which would be called
after an user request is completed. Also fixed the issue where the
scheduler callback would not be invoked if there's a failure during the
offlining, also made it have the same behavior as described in comments.
Changed some tests regarding to the changes above.

BUG=653708,646947
==========

to

==========
[Offline Pages] Add evaluation test support in RequestCoodinator.

Added the function to add customized callback which would be called
after an user request is completed. Also fixed the issue where the
scheduler callback would not be invoked if there's a failure during the
offlining, also made it have the same behavior as described in comments.
Changed some tests regarding to the changes above.

BUG=653708,646947
==========

[romax, 2016-10-18 01:09:39]

romax@chromium.org changed reviewers:
+ dougarnett@chromium.org, petewil@chromium.org

[romax, 2016-10-18 01:10:05]

Sorry for the late patch. PTAL!

Thanks!

[dougarnett, 2016-10-18 17:26:32]

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

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/offliner_policy.h:25: true,
Hard to read what these bools apply to. I would typically add comments on them
but Pete may suggest another way to document.

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/offliner_policy.h:29:
kBackgroundProcessingTimeBudgetSeconds) {}
We might consider renaming to identify this constant now as the default

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

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:471:
device_conditions, user_request_callback_))
immediate_schedule_callback ?

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:649: // No further
processing in this service window.
Nice, maybe also add same comment as other call sites too:

// Let the scheduler know we are done processing.

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

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:329: bool
last_empty_callback_called() const {
don't like this naming - maybe immediate_schedule_callback_called() ???

[Pete Williamson, 2016-10-18 19:24:32]

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

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/offliner_policy.h:25: true,
On 2016/10/18 17:26:32, dougarnett wrote:
> Hard to read what these bools apply to. I would typically add comments on them
> but Pete may suggest another way to document.

boolean arguments are always hard to understand at the point of call, like here.
 Normally I suggest using a const kPreferUntried = true;
and at the point of call, setting either kPreferUntried or !kPreferUntried as
the argument supplied.
Another solution that works is to use an enum with values such as
PREFER_UNTRIED_REQUESTS and PREFER_TRIED_REQUESTS, but that change will touch a
lot more places than here.

Why not just add background_processing_time_budget to the existing second
constructor?  It is already for tests, and your code uses the test hooks.

Alternatively, you could add a method to set the background processing time
budget.

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

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:471:
device_conditions, user_request_callback_))
On 2016/10/18 17:26:32, dougarnett wrote:
> immediate_schedule_callback ?

+1 to dougarnett's suggestion.

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

https://codereview.chromium.org/2425873003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:241: void
EmptyCallbackFunction(bool result) {
This should be renamed consistent with whatever name you end up with for the
user request callback.

[romax, 2016-10-18 20:31:51]

Thanks for the comments. PTAL

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
File components/offline_pages/background/offliner_policy.h (right):

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
components/offline_pages/background/offliner_policy.h:25: true,
On 2016/10/18 19:24:32, Pete Williamson wrote:
> On 2016/10/18 17:26:32, dougarnett wrote:
> > Hard to read what these bools apply to. I would typically add comments on
them
> > but Pete may suggest another way to document.
> 
> boolean arguments are always hard to understand at the point of call, like
here.
>  Normally I suggest using a const kPreferUntried = true;
> and at the point of call, setting either kPreferUntried or !kPreferUntried as
> the argument supplied.
> Another solution that works is to use an enum with values such as
> PREFER_UNTRIED_REQUESTS and PREFER_TRIED_REQUESTS, but that change will touch
a
> lot more places than here.
> 
> Why not just add background_processing_time_budget to the existing second
> constructor?  It is already for tests, and your code uses the test hooks.
> 
> Alternatively, you could add a method to set the background processing time
> budget.

I forgot why I used this but seems like simply adding the time_budget to the
second c-tor is better. I'll do this.

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
components/offline_pages/background/offliner_policy.h:29:
kBackgroundProcessingTimeBudgetSeconds) {}
On 2016/10/18 17:26:32, dougarnett wrote:
> We might consider renaming to identify this constant now as the default

Done.

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
File components/offline_pages/background/request_coordinator.cc (right):

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
components/offline_pages/background/request_coordinator.cc:471:
device_conditions, user_request_callback_))
On 2016/10/18 19:24:32, Pete Williamson wrote:
> On 2016/10/18 17:26:32, dougarnett wrote:
> > immediate_schedule_callback ?
> 
> +1 to dougarnett's suggestion.

Done.

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
components/offline_pages/background/request_coordinator.cc:649: // No further
processing in this service window.
On 2016/10/18 17:26:32, dougarnett wrote:
> Nice, maybe also add same comment as other call sites too:
> 
> // Let the scheduler know we are done processing.

Done.

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
File components/offline_pages/background/request_coordinator_unittest.cc
(right):

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
components/offline_pages/background/request_coordinator_unittest.cc:241: void
EmptyCallbackFunction(bool result) {
On 2016/10/18 19:24:32, Pete Williamson wrote:
> This should be renamed consistent with whatever name you end up with for the
> user request callback.

Done.

https://chromiumcodereview.appspot.com/2425873003/diff/1/components/offline_p...
components/offline_pages/background/request_coordinator_unittest.cc:329: bool
last_empty_callback_called() const {
On 2016/10/18 17:26:32, dougarnett wrote:
> don't like this naming - maybe immediate_schedule_callback_called() ???

Done.

[Pete Williamson, 2016-10-18 22:39:16]

lgtm

[romax, 2016-10-18 23:47:15]

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

[commit-bot: I haz the power, 2016-10-18 23:48:05]

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

[dougarnett, 2016-10-19 00:58:58]

https://chromiumcodereview.appspot.com/2425873003/diff/20001/components/offli...
File components/offline_pages/background/request_picker_unittest.cc (right):

https://chromiumcodereview.appspot.com/2425873003/diff/20001/components/offli...
components/offline_pages/background/request_picker_unittest.cc:41: const int
kBackgroundProcessingTimeBudgetSeconds = 170;
hmm, I wonder about creating instance of OfflinerPolicy and then getting value
from it instead of needing to try to match value here with it.

[romax, 2016-10-19 01:03:21]

https://chromiumcodereview.appspot.com/2425873003/diff/20001/components/offli...
File components/offline_pages/background/request_picker_unittest.cc (right):

https://chromiumcodereview.appspot.com/2425873003/diff/20001/components/offli...
components/offline_pages/background/request_picker_unittest.cc:41: const int
kBackgroundProcessingTimeBudgetSeconds = 170;
On 2016/10/19 00:58:58, dougarnett wrote:
> hmm, I wonder about creating instance of OfflinerPolicy and then getting value
> from it instead of needing to try to match value here with it.

Then I guess it's better to go with the idea which would add setter/getter on
OfflinerPolicy because the changes below would utilize those better. WDYT? I can
make another patch on it.

[commit-bot: I haz the power, 2016-10-19 01:35:12]

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

[commit-bot: I haz the power, 2016-10-19 01:35:13]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-10-19 01:36:39]

lgtm

https://chromiumcodereview.appspot.com/2425873003/diff/20001/components/offli...
File components/offline_pages/background/request_picker_unittest.cc (right):

https://chromiumcodereview.appspot.com/2425873003/diff/20001/components/offli...
components/offline_pages/background/request_picker_unittest.cc:41: const int
kBackgroundProcessingTimeBudgetSeconds = 170;
On 2016/10/19 01:03:21, romax wrote:
> On 2016/10/19 00:58:58, dougarnett wrote:
> > hmm, I wonder about creating instance of OfflinerPolicy and then getting
value
> > from it instead of needing to try to match value here with it.
> 
> Then I guess it's better to go with the idea which would add setter/getter on
> OfflinerPolicy because the changes below would utilize those better. WDYT? I
can
> make another patch on it.

Sorry, never mind

[romax, 2016-10-19 01:40:05]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2016-10-19 01:40:39]

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-10-19 01:50:12]

Description was changed from

==========
[Offline Pages] Add evaluation test support in RequestCoodinator.

Added the function to add customized callback which would be called
after an user request is completed. Also fixed the issue where the
scheduler callback would not be invoked if there's a failure during the
offlining, also made it have the same behavior as described in comments.
Changed some tests regarding to the changes above.

BUG=653708,646947
==========

to

==========
[Offline Pages] Add evaluation test support in RequestCoodinator.

Added the function to add customized callback which would be called
after an user request is completed. Also fixed the issue where the
scheduler callback would not be invoked if there's a failure during the
offlining, also made it have the same behavior as described in comments.
Changed some tests regarding to the changes above.

BUG=653708,646947
==========

[commit-bot: I haz the power, 2016-10-19 01:50:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:05:44]

Description was changed from

==========
[Offline Pages] Add evaluation test support in RequestCoodinator.

Added the function to add customized callback which would be called
after an user request is completed. Also fixed the issue where the
scheduler callback would not be invoked if there's a failure during the
offlining, also made it have the same behavior as described in comments.
Changed some tests regarding to the changes above.

BUG=653708,646947
==========

to

==========
[Offline Pages] Add evaluation test support in RequestCoodinator.

Added the function to add customized callback which would be called
after an user request is completed. Also fixed the issue where the
scheduler callback would not be invoked if there's a failure during the
offlining, also made it have the same behavior as described in comments.
Changed some tests regarding to the changes above.

BUG=653708,646947

Committed: https://crrev.com/1c1f5c1b9927576a524b1fe3476035f4d1128e79
Cr-Commit-Position: refs/heads/master@{#426115}
==========

[commit-bot: I haz the power, 2016-10-21 13:05:45]

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