Add the request picker and a unit test

Add the request picker and a unit test

The RequestPicker is responsible for choosing which request from the
request queue to process next.  It will apply policies from the policy
class to the data returned by the request queue, pick a likely
candidate, and call back to the request coordinator so it can start
offlining the candidate.  If there is no candidate, it will call back
to the request coordinator to let it know processing is done for this
cycle.

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

[Pete Williamson, 2016-05-27 23:40:02]

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

[Pete Williamson, 2016-05-27 23:40:02]

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

[chili, 2016-05-27 23:54:13]

A few comments - more questions than anything
Looking good!

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

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:81: // Send the
request on to the offliner.
I don't quite understand this comment.  Which request would it be sending to the
offliner?

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:87: const
ProcessingDoneCallback& callback) {
nit: does this fit on previous line?

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

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:35: typedef
base::Callback<void()> RequestQueueEmptyCallback;
nit: -2 indent?

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

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_picker.cc:40: const SavePageRequest&
picked_request = requests[0];
Should we have a TODO here to actually check all the conditions and 'manually'
filter? or are we going to be guaranteed in later RequestQueue cycles to add
filtering mechanism natively?

[Pete Williamson, 2016-05-28 00:08:52]

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

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:81: // Send the
request on to the offliner.
On 2016/05/27 23:54:13, chili wrote:
> I don't quite understand this comment.  Which request would it be sending to
the
> offliner?

Good catch, cut and paste bug, comment meant for another function.  Removed now.

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:87: const
ProcessingDoneCallback& callback) {
On 2016/05/27 23:54:13, chili wrote:
> nit: does this fit on previous line?

Nope, just a bit over 80.

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

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:35: typedef
base::Callback<void()> RequestQueueEmptyCallback;
On 2016/05/27 23:54:13, chili wrote:
> nit: -2 indent?

Done.

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

https://codereview.chromium.org/2020833002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_picker.cc:40: const SavePageRequest&
picked_request = requests[0];
On 2016/05/27 23:54:13, chili wrote:
> Should we have a TODO here to actually check all the conditions and 'manually'
> filter? or are we going to be guaranteed in later RequestQueue cycles to add
> filtering mechanism natively?

The whole purpose of this class is to make a more complicated decision, so there
is not much danger of forgetting to come back and fix it.  However, LMK if you
like, and I can add a TODO.

[Dmitry Titov, 2016-05-28 00:36:56]

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

[Dmitry Titov, 2016-05-28 00:36:57]

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

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker.h:14: class RequestPicker {
So the pattern of usage is:

requestPicker->ChooseNextRequest();
... this does async lookup into request queue
... then calls a callback.

Typically, this is just:
requestPicker->ChooseNextRequest(callback)
at which point why not

requestQueue->FetchNextRequest(callback)?

[Pete Williamson, 2016-05-28 00:53:47]

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

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker.h:14: class RequestPicker {
On 2016/05/28 00:36:57, Dmitry Titov wrote:
> So the pattern of usage is:
> 
> requestPicker->ChooseNextRequest();
> ... this does async lookup into request queue
> ... then calls a callback.
> 
> Typically, this is just:
> requestPicker->ChooseNextRequest(callback)
> at which point why not
> 
> requestQueue->FetchNextRequest(callback)?
> 

The RequestPicker is just a skeleton so far.  Eventually it will house the logic
for picking which request from the request queue to do next.

You do make a good point that I should pass the callbacks as parameters to the
method being called, (ChooseNextRequest) and not in the constructor, and I will
do that in my next revision to this changelist.

[dougarnett, 2016-05-31 17:45:37]

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:31: picker_.reset(new
RequestPicker(
Doesn't look like we are accessing as KeyedService after all

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

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:29: class
RequestCoordinator : public KeyedService {
I don't have any sense of what is meaty enough to merit being a KeyedService so
I wonder if this is meaty enough (vs. binding more directly to the
RequestCoordinator).

[dougarnett, 2016-05-31 17:47:26]

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

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:29: class
RequestCoordinator : public KeyedService {
On 2016/05/31 17:45:37, dougarnett wrote:
> I don't have any sense of what is meaty enough to merit being a KeyedService
so
> I wonder if this is meaty enough (vs. binding more directly to the
> RequestCoordinator).

Never mind ;-)

[dougarnett, 2016-05-31 18:03:04]

lgtm

I would like to see some protection on StartProcessing being called multiple
times when chosen next request is processing. Next iteration if not this one.

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:31: picker_.reset(new
RequestPicker(
On 2016/05/31 17:45:37, dougarnett wrote:
> Doesn't look like we are accessing as KeyedService after all

never mind this too

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:92:
picker_->ChooseNextRequest();
We might want to keep track that we have a request pending so that we don't
choose again and try to start offlining it again concurrently. Can be TODO for
the wire frame though.

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

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker.h:33: // Callback for when we
are done picking a request to do next
nit - period at end

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

https://codereview.chromium.org/2020833002/diff/20001/components/offline_page...
components/offline_pages/background/request_picker_unittest.cc:48: // The
request queue is simple enough we will use a real queue, not a stub.
... use a real queue with memory store.    ?

[chili, 2016-05-31 18:06:39]

lgtm

[Pete Williamson, 2016-05-31 19:29:36]

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

[commit-bot: I haz the power, 2016-05-31 19:30:10]

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

[commit-bot: I haz the power, 2016-05-31 22:58:43]

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

[commit-bot: I haz the power, 2016-05-31 22:58:44]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-05-31 23:06:34]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-05-31 23:06:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, chili@chromium.org
Link to the patchset: https://codereview.chromium.org/2020833002/#ps40001
(title: "CR feedback per Dimich and DougArnett")

[commit-bot: I haz the power, 2016-05-31 23:07:20]

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

[commit-bot: I haz the power, 2016-05-31 23:13:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-31 23:16:20]

Description was changed from

==========
Add the request picker and a unit test

The RequestPicker is responsible for choosing which request from the
request queue to process next.  It will apply policies from the policy
class to the data returned by the request queue, pick a likely
candidate, and call back to the request coordinator so it can start
offlining the candidate.  If there is no candidate, it will call back
to the request coordinator to let it know processing is done for this
cycle.

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

to

==========
Add the request picker and a unit test

The RequestPicker is responsible for choosing which request from the
request queue to process next.  It will apply policies from the policy
class to the data returned by the request queue, pick a likely
candidate, and call back to the request coordinator so it can start
offlining the candidate.  If there is no candidate, it will call back
to the request coordinator to let it know processing is done for this
cycle.

BUG=610521

Committed: https://crrev.com/8ba587222e4138e227f9770df9d8237576342b3a
Cr-Commit-Position: refs/heads/master@{#396967}
==========

[commit-bot: I haz the power, 2016-05-31 23:16:21]

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