Call scheduler when we get a save page later request.

Call scheduler when we get a save page later request.

This hooks up the request coordinator to the scheduler, and calls the scheduler to schedule a later callback once the
network is ready.

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

[Pete Williamson, 2016-05-12 00:52:56]

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

[Pete Williamson, 2016-05-12 00:53:43]

Description was changed from

==========
Call scheduler when we get a save page later request.

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

to

==========
Call scheduler when we get a save page later request.

This hooks up the request coordinator to the scheduler, and calls the scheduler
to schedule a later callback once the 
network is ready.

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

[fgorski, 2016-05-12 14:32:34]

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

[fgorski, 2016-05-12 14:32:36]

Hey, this is exactly the right size of a patch :)

Only nits and usage of C++ constructors to be fixed.

lgtm otherwise.

https://codereview.chromium.org/1971033002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/1971033002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:55:
std::move(prerendererOffliner),
nit: fix to a proper C++ name

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:24: // TODO(petewil):
Assert policy not null.
can you already do this?

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:28: scheduler_ =
std::move(scheduler);
C++: move all assignments to the initializer list.

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

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:71: // Scheduler is
owned by the RequestCoordiator.
nit: document as above.

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:32:
schedule_called_ = false;
C++ again, use initializer list, please.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:45: bool
WasScheduleCalled() { return schedule_called_; }
can this be a const method?
Did you consider simple schedule_called()?

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:47: bool
WasUnscheduleCalled() { return unschedule_called_; }
ditto.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:64:
RequestCoordinator* getCoordinator() {
nit: can you rename it to coordinator() or GetCoordinator()?

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:105:
std::unique_ptr<Scheduler> schedulerStub(new SchedulerStub());
nit: alignment

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:151: 
nit: empty lines.

[dougarnett, 2016-05-12 15:33:33]

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:62: // Inform the
scheduler that we have an outstanding task.
Here is where it looked like you were intending to call scheduler?

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:105:
std::unique_ptr<Scheduler> schedulerStub(new SchedulerStub());
scheduler_stub I suppose

[Pete Williamson, 2016-05-12 17:21:19]

Replies and fixed for FGorski and DougArnett

https://codereview.chromium.org/1971033002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/1971033002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:55:
std::move(prerendererOffliner),
On 2016/05/12 14:32:35, fgorski wrote:
> nit: fix to a proper C++ name

Done.

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:24: // TODO(petewil):
Assert policy not null.
On 2016/05/12 14:32:35, fgorski wrote:
> can you already do this?

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:28: scheduler_ =
std::move(scheduler);
On 2016/05/12 14:32:35, fgorski wrote:
> C++: move all assignments to the initializer list.

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:62: // Inform the
scheduler that we have an outstanding task.
On 2016/05/12 15:33:33, dougarnett wrote:
> Here is where it looked like you were intending to call scheduler?

Ah, right, that code merged in from another changelist.  Moved the schedule call
to here as intended.

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:60: 
On 2016/05/12 14:32:35, fgorski wrote:
> nit: empty line

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:71: // Scheduler is
owned by the RequestCoordiator.
On 2016/05/12 14:32:35, fgorski wrote:
> nit: document as above.

Done.

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

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:32:
schedule_called_ = false;
On 2016/05/12 14:32:35, fgorski wrote:
> C++ again, use initializer list, please.

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:45: bool
WasScheduleCalled() { return schedule_called_; }
On 2016/05/12 14:32:35, fgorski wrote:
> can this be a const method?
> Did you consider simple schedule_called()?

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:47: bool
WasUnscheduleCalled() { return unschedule_called_; }
On 2016/05/12 14:32:35, fgorski wrote:
> ditto.

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:64:
RequestCoordinator* getCoordinator() {
On 2016/05/12 14:32:35, fgorski wrote:
> nit: can you rename it to coordinator() or GetCoordinator()?

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:105:
std::unique_ptr<Scheduler> schedulerStub(new SchedulerStub());
On 2016/05/12 14:32:35, fgorski wrote:
> nit: alignment

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:105:
std::unique_ptr<Scheduler> schedulerStub(new SchedulerStub());
On 2016/05/12 15:33:33, dougarnett wrote:
> scheduler_stub I suppose

Done.

https://codereview.chromium.org/1971033002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_unittest.cc:151: 
On 2016/05/12 14:32:35, fgorski wrote:
> nit: empty lines.

Done.

[Pete Williamson, 2016-05-12 17:24:12]

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

[commit-bot: I haz the power, 2016-05-12 17:24:27]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971033002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971033002/20001

[dougarnett, 2016-05-12 17:50:11]

lgtm

[commit-bot: I haz the power, 2016-05-12 18:03:51]

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

[commit-bot: I haz the power, 2016-05-12 18:03:52]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-05-12 18:04:45]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-05-12 18:04:45]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1971033002/#ps20001
(title: "CR feedback per FGorski and DougArnett")

[commit-bot: I haz the power, 2016-05-12 18:05:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971033002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971033002/20001

[commit-bot: I haz the power, 2016-05-12 18:08:45]

Description was changed from

==========
Call scheduler when we get a save page later request.

This hooks up the request coordinator to the scheduler, and calls the scheduler
to schedule a later callback once the 
network is ready.

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

to

==========
Call scheduler when we get a save page later request.

This hooks up the request coordinator to the scheduler, and calls the scheduler
to schedule a later callback once the 
network is ready.

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

[commit-bot: I haz the power, 2016-05-12 18:08:46]

Committed patchset #2 (id:20001)

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

Description was changed from

==========
Call scheduler when we get a save page later request.

This hooks up the request coordinator to the scheduler, and calls the scheduler
to schedule a later callback once the 
network is ready.

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

to

==========
Call scheduler when we get a save page later request.

This hooks up the request coordinator to the scheduler, and calls the scheduler
to schedule a later callback once the
network is ready.

BUG=610521

Committed: https://crrev.com/551f6cab0bf3f6200041101a4b75aad64efcd968
Cr-Commit-Position: refs/heads/master@{#393302}
==========

[commit-bot: I haz the power, 2016-05-12 18:09:54]

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