Split the RequestPicker task into two separate tasks.

Split the RequestPicker task into two separate tasks.

We still have a picker task, but now we also have a cleanup task.  The
cleanup task will remove expired entries, and entries that exceed their
start or completion limit.  It does not yet flip offlining tasks to
available on startup, I plan that for a separate changelist, and likely
a new task.

BUG=670343
Committed: https://crrev.com/55fc16c382cd559fca177abd0ae291213bd5893b
Cr-Commit-Position: refs/heads/master@{#436671}

[Pete Williamson, 2016-12-02 01:21:12]

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

[dougarnett, 2016-12-02 17:20:27]

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:29: void
CleanupTask::GetExpiredRequestIds(
should this be positioned later in file per ordering in .h file?

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:77: // TODO: Add UMA when we
remove them, saying why
TODO(petewil):

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:86:
RequestNotifier::BackgroundSavePageResult::EXPIRED);
I think we want to figure out which of the 3 BackgroundSavePageResult codes to
use here - to pass to RecordDroppedSavePageRequest and NotifyCompleted

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.h:21: // TODO: Should I
rename this since it now has two uses?
Think just one at the moment

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.h:30:
std::unique_ptr<CleanupTask> CreatePickerTask(RequestQueueStore* store);
Remove this declaration, right?

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:63:
not_picked_callback_.Run(false, false);
...(false /* non user requests */, false /* cleanup needed*/);   ?

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.h:34: typedef
base::Callback<void(bool, bool)> RequestNotPickedCallback;
(bool non_user_requests, bool cleanup_needed)

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.h:57: void Choose(bool
success,
what does success refer to? Maybe get_successful instead?

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task_unittest.cc:109: void
MakeTask();
MakePickerTask?

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:646: //
TODO(petewil): What if the request we chose expires before calling cleanup?
Might consider adding some delta over expiration time to cleanup task check (eg,
policy immediate request timeout value) whereas picker task won't pick over the
expiration time.

[fgorski, 2016-12-02 21:44:05]

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:29: void
CleanupTask::GetExpiredRequestIds(
On 2016/12/02 17:20:26, dougarnett wrote:
> should this be positioned later in file per ordering in .h file?

Actually this can be defined as a function in anonymous namespace if you pass in
policy.

This is implementation detail that does not need to be part of the class.

Alternatively I can think of a set of utils that can be tested independently.
(e.g. Policy utils/helper for this particular function). We can test it
separately from this class then.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:54: void CleanupTask::Prune(
what is the meaning of prune?
Why does it deserve a separate step from RemoveStaleRequests?

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.cc:20: :
policy_(policy), notifier_(notifier), event_logger_(event_logger) {}
would it make sense to DCHECK these inputs?

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.h:21: // TODO: Should I
rename this since it now has two uses?
What do you mean by 2 uses?

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.h:30:
std::unique_ptr<CleanupTask> CreatePickerTask(RequestQueueStore* store);
On 2016/12/02 17:20:26, dougarnett wrote:
> Remove this declaration, right? 

+1

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_unittest.cc:148: void
CleanupTaskTest::AddRequestDone(ItemActionStatus status) {}
how about ASSERT/EXPECT that this was successful?

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:88: if
(base::Time::Now() - requests[i]->creation_time() >=
One more reason to extract a method as suggested earlier and at this point I
don't see it as an optional thing. WDYT?

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:126: if (picked_request
!= nullptr)
add {} to both clauses, because your else spans multiple lines now.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.h:34: typedef
base::Callback<void(bool, bool)> RequestNotPickedCallback;
On 2016/12/02 17:20:27, dougarnett wrote:
> (bool non_user_requests, bool cleanup_needed)

(bool /* non_user_requests */, bool /* cleanup_needed */)
that should do it as a good alternative.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task_unittest.cc:401:
EXPECT_EQ((size_t)2, total_request_count_);
wouldn't 2ul simply work?

applies to every size_t cast in this file.

Otherwise, please fix the spacing. (Also multiple lines)

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:635: const bool
cleanup_needed) {
const bool -> bool here as well.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:653: const bool
cleanup_needed) {
const bool -> bool

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:268: void
RequestNotPicked(const bool non_user_requested_tasks_remaining,
I don't think you need to define "const bool" here, just "bool". (primitive
type)

Applies above and below.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:101: void
CleanupRequestQueue();
add documentation please.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:104: // request queue to not
need a PickerFactory to create it, since we have lots
fix

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:106: // factory before
calling PickNextRequest.
fix

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_unittest.cc:539: // is why
this test calls PickNextRequest().
fix

[dougarnett, 2016-12-02 23:00:11]

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:120: // the count of
valid entries, but now we report all entries. Reviewers, is
Looks fine. Before valid_requests.size() was the total count considering that
the expired ones were going to be deleted. We capture UMA for the difference of
these two counts - so it can now include expired requests that haven't been
deleted yet which seems fine (since they are cleaned up at start up and once
detected).

[Pete Williamson, 2016-12-05 20:39:46]

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:29: void
CleanupTask::GetExpiredRequestIds(
On 2016/12/02 17:20:26, dougarnett wrote:
> should this be positioned later in file per ordering in .h file?

Moved.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:29: void
CleanupTask::GetExpiredRequestIds(
On 2016/12/02 21:44:04, fgorski wrote:
> On 2016/12/02 17:20:26, dougarnett wrote:
> > should this be positioned later in file per ordering in .h file?
> 
> Actually this can be defined as a function in anonymous namespace if you pass
in
> policy.
> 
> This is implementation detail that does not need to be part of the class.
> 
> Alternatively I can think of a set of utils that can be tested independently.
> (e.g. Policy utils/helper for this particular function). We can test it
> separately from this class then.

Created a new class, OfflinerPolicyUtils for the boolean operation.  It actually
got a bit more complex because we need to track why it is being deleted for
metrics further down the async call chain.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:54: void CleanupTask::Prune(
On 2016/12/02 21:44:04, fgorski wrote:
> what is the meaning of prune?
> Why does it deserve a separate step from RemoveStaleRequests?

No reason to keep RemoveStaleRequests, so I got rid of the second function.  I
think this might be a leftover vestige of the structure we used to have when we
had a combined pick and prune task.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:77: // TODO: Add UMA when we
remove them, saying why
On 2016/12/02 17:20:26, dougarnett wrote:
> TODO(petewil): 

Done.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:86:
RequestNotifier::BackgroundSavePageResult::EXPIRED);
On 2016/12/02 17:20:26, dougarnett wrote:
> I think we want to figure out which of the 3 BackgroundSavePageResult codes to
> use here - to pass to RecordDroppedSavePageRequest and NotifyCompleted

Can we do that in a later changelist?  It involves a lot more engineering to
pair the reasons with the list of request IDs, and round trip them through the
request queue.  It was also that way before this change. TODO Added.  I also
captured the exact reason for expiration when we build our list, so we just need
to get that reason from there to here.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.cc:20: :
policy_(policy), notifier_(notifier), event_logger_(event_logger) {}
On 2016/12/02 21:44:04, fgorski wrote:
> would it make sense to DCHECK these inputs?

Done.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.h:21: // TODO: Should I
rename this since it now has two uses?
On 2016/12/02 21:44:04, fgorski wrote:
> What do you mean by 2 uses?
Oops, this is an old comment, I refactored so that there is only one factory
now, CleanupTask has one, PickerTask does not.  Comment removed.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_factory.h:30:
std::unique_ptr<CleanupTask> CreatePickerTask(RequestQueueStore* store);
On 2016/12/02 21:44:04, fgorski wrote:
> On 2016/12/02 17:20:26, dougarnett wrote:
> > Remove this declaration, right? 
> 
> +1

Done.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task_unittest.cc:148: void
CleanupTaskTest::AddRequestDone(ItemActionStatus status) {}
On 2016/12/02 21:44:04, fgorski wrote:
> how about ASSERT/EXPECT that this was successful?

Done.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:63:
not_picked_callback_.Run(false, false);
On 2016/12/02 17:20:27, dougarnett wrote:
> ...(false /* non user requests */, false /* cleanup needed*/);   ?

I did this, but with enums, not comments, WDYT?

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:88: if
(base::Time::Now() - requests[i]->creation_time() >=
On 2016/12/02 21:44:04, fgorski wrote:
> One more reason to extract a method as suggested earlier and at this point I
> don't see it as an optional thing. WDYT?

Done, new class is OfflinerPolicyUtils.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:120: // the count of
valid entries, but now we report all entries. Reviewers, is
On 2016/12/02 23:00:11, dougarnett wrote:
> Looks fine. Before valid_requests.size() was the total count considering that
> the expired ones were going to be deleted. We capture UMA for the difference
of
> these two counts - so it can now include expired requests that haven't been
> deleted yet which seems fine (since they are cleaned up at start up and once
> detected).

Acknowledged.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:126: if (picked_request
!= nullptr)
On 2016/12/02 21:44:04, fgorski wrote:
> add {} to both clauses, because your else spans multiple lines now.

Ah, I must have missed seeing it wrap.  Fixed.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.h:34: typedef
base::Callback<void(bool, bool)> RequestNotPickedCallback;
On 2016/12/02 17:20:27, dougarnett wrote:
> (bool non_user_requests, bool cleanup_needed)

Done.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.h:57: void Choose(bool
success,
On 2016/12/02 17:20:27, dougarnett wrote:
> what does success refer to? Maybe get_successful instead?

went with get_succeeded (get_successful sounds like an accessor to me)

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task_unittest.cc:109: void
MakeTask();
On 2016/12/02 17:20:27, dougarnett wrote:
> MakePickerTask?

Changed to MakePickRequestTask ('Picker' isn't actually in the class name).

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task_unittest.cc:401:
EXPECT_EQ((size_t)2, total_request_count_);
On 2016/12/02 21:44:04, fgorski wrote:
> wouldn't 2ul simply work?
> 
> applies to every size_t cast in this file.
> 
> Otherwise, please fix the spacing. (Also multiple lines)

Fixed the cast (that was supposed to get fixed before I first checked in the
file, I must have missed it).  

Not sure what you mean by fixing the spacing.  If you mean being inconsistent
about whether there is a space after the cast, that should be fixed by virtue of
removing the casts.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:635: const bool
cleanup_needed) {
On 2016/12/02 21:44:04, fgorski wrote:
> const bool -> bool here as well.

Done.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:646: //
TODO(petewil): What if the request we chose expires before calling cleanup?
On 2016/12/02 17:20:27, dougarnett wrote:
> Might consider adding some delta over expiration time to cleanup task check
(eg,
> policy immediate request timeout value) whereas picker task won't pick over
the
> expiration time.

Instead, to be sure, I pass in the request_id we sent to the offliner, so that
we won't clean it up while offlining.  Timeout values are always fragile.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:653: const bool
cleanup_needed) {
On 2016/12/02 21:44:04, fgorski wrote:
> const bool -> bool 

Done.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:268: void
RequestNotPicked(const bool non_user_requested_tasks_remaining,
On 2016/12/02 21:44:04, fgorski wrote:
> I don't think you need to define "const bool" here, just "bool". (primitive
> type)
> 
> Applies above and below.

Done.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:101: void
CleanupRequestQueue();
On 2016/12/02 21:44:04, fgorski wrote:
> add documentation please.

Done.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:104: // request queue to not
need a PickerFactory to create it, since we have lots
On 2016/12/02 21:44:04, fgorski wrote:
> fix

Done.

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:106: // factory before
calling PickNextRequest.
On 2016/12/02 21:44:05, fgorski wrote:
> fix

Done.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_unittest.cc:539: // is why
this test calls PickNextRequest().
On 2016/12/02 21:44:05, fgorski wrote:
> fix

Done.

[fgorski, 2016-12-05 21:05:37]

almost there. I don't think we should be relying on current_request_id for clean
up task.

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

https://codereview.chromium.org/2543093002/diff/20001/components/offline_page...
components/offline_pages/background/cleanup_task.cc:97: request->request_id() !=
current_request_id_) {
This does not scale to multiple offliners.
can you instead check that request is avialable/paused as opposed to being
actively worked on?

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

https://codereview.chromium.org/2543093002/diff/20001/components/offline_page...
components/offline_pages/background/offliner_policy_utils.cc:15: OfflinerPolicy*
policy) {
Could you dcheck parameters here?

Also, is there a way to use const qualifiers with the parameters? This function
is not expected to change anything about the inputs after all.

[dougarnett, 2016-12-05 21:27:45]

looking good - couple comments

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:86:
RequestNotifier::BackgroundSavePageResult::EXPIRED);
On 2016/12/05 20:39:45, Pete Williamson wrote:
> On 2016/12/02 17:20:26, dougarnett wrote:
> > I think we want to figure out which of the 3 BackgroundSavePageResult codes
to
> > use here - to pass to RecordDroppedSavePageRequest and NotifyCompleted
> 
> Can we do that in a later changelist?  It involves a lot more engineering to
> pair the reasons with the list of request IDs, and round trip them through the
> request queue.  It was also that way before this change. TODO Added.  I also
> captured the exact reason for expiration when we build our list, so we just
need
> to get that reason from there to here.

Sure, we can TODO and follow-up. This is new behavior though in that now
COUNT_EXCEEDED cases get deleted and newly get event logging and notification
updates albeit as EXPIRED. I don't think we have any UMA affected by this though
so seems benign for event log to be a bit wrong in the interim. Or one might
argue that "expired" is a more general classification than expired by time.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:63:
not_picked_callback_.Run(false, false);
On 2016/12/05 20:39:45, Pete Williamson wrote:
> On 2016/12/02 17:20:27, dougarnett wrote:
> > ...(false /* non user requests */, false /* cleanup needed*/);   ?
> 
> I did this, but with enums, not comments, WDYT?

The constants are fine. I have noticed the parm comment style in some other team
member CLs so it might be good to discuss that style more broadly at some point.
I came to prefer that comment style but I think it takes some getting used to.

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

https://codereview.chromium.org/2543093002/diff/20001/components/offline_page...
components/offline_pages/background/cleanup_task.h:29: int64_t
current_request_id);
Maybe active_request_id or excluded_request_id as "current" may not clear in
this context as being current in an offliner.

Or perhaps be good to comment what this parameter means.

Also, seems like you ought to mention invalid id value.

[Pete Williamson, 2016-12-06 00:25:38]

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/cleanup_task.cc:86:
RequestNotifier::BackgroundSavePageResult::EXPIRED);
On 2016/12/05 21:27:45, dougarnett wrote:
> On 2016/12/05 20:39:45, Pete Williamson wrote:
> > On 2016/12/02 17:20:26, dougarnett wrote:
> > > I think we want to figure out which of the 3 BackgroundSavePageResult
codes
> to
> > > use here - to pass to RecordDroppedSavePageRequest and NotifyCompleted
> > 
> > Can we do that in a later changelist?  It involves a lot more engineering to
> > pair the reasons with the list of request IDs, and round trip them through
the
> > request queue.  It was also that way before this change. TODO Added.  I also
> > captured the exact reason for expiration when we build our list, so we just
> need
> > to get that reason from there to here.
> 
> Sure, we can TODO and follow-up. This is new behavior though in that now
> COUNT_EXCEEDED cases get deleted and newly get event logging and notification
> updates albeit as EXPIRED. I don't think we have any UMA affected by this
though
> so seems benign for event log to be a bit wrong in the interim. Or one might
> argue that "expired" is a more general classification than expired by time.

Acknowledged.

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

https://codereview.chromium.org/2543093002/diff/1/components/offline_pages/ba...
components/offline_pages/background/pick_request_task.cc:63:
not_picked_callback_.Run(false, false);
On 2016/12/05 21:27:45, dougarnett wrote:
> On 2016/12/05 20:39:45, Pete Williamson wrote:
> > On 2016/12/02 17:20:27, dougarnett wrote:
> > > ...(false /* non user requests */, false /* cleanup needed*/);   ?
> > 
> > I did this, but with enums, not comments, WDYT?
> 
> The constants are fine. I have noticed the parm comment style in some other
team
> member CLs so it might be good to discuss that style more broadly at some
point.
> I came to prefer that comment style but I think it takes some getting used to.

I've always preferred named constants to comments.  If nothing else, it is nice
to avoid using the open/close comment operators so that you can use them for
commenting out large swaths of code when doing binary search for problems by
removing code.  However, I don't feel strongly enough to force my opinions on
others, or that we all need to be consistent on this.  I *do* feel strongly that
we should never call with just plain true or false.

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

https://codereview.chromium.org/2543093002/diff/20001/components/offline_page...
components/offline_pages/background/cleanup_task.cc:97: request->request_id() !=
current_request_id_) {
On 2016/12/05 21:05:37, fgorski wrote:
> This does not scale to multiple offliners.
> can you instead check that request is avialable/paused as opposed to being
> actively worked on?

Done.

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

https://codereview.chromium.org/2543093002/diff/20001/components/offline_page...
components/offline_pages/background/cleanup_task.h:29: int64_t
current_request_id);
On 2016/12/05 21:27:45, dougarnett wrote:
> Maybe active_request_id or excluded_request_id as "current" may not clear in
> this context as being current in an offliner.
> 
> Or perhaps be good to comment what this parameter means.
> 
> Also, seems like you ought to mention invalid id value.

Removed the parameter, since I took FGorski's suggestion to check the state
instead as being more scalable.  That meant I also removed the use of the
invalid ID value elsewhere.

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

https://codereview.chromium.org/2543093002/diff/20001/components/offline_page...
components/offline_pages/background/offliner_policy_utils.cc:15: OfflinerPolicy*
policy) {
On 2016/12/05 21:05:37, fgorski wrote:
> Could you dcheck parameters here?
> 
> Also, is there a way to use const qualifiers with the parameters? This
function
> is not expected to change anything about the inputs after all.

Done.

[dougarnett, 2016-12-06 00:59:41]

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

https://codereview.chromium.org/2543093002/diff/40001/components/offline_page...
components/offline_pages/background/cleanup_task.cc:95: request->request_state()
!= SavePageRequest::RequestState::OFFLINING) {
This check relies on the reconciliation task to happen.

[Pete Williamson, 2016-12-06 02:06:28]

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

https://codereview.chromium.org/2543093002/diff/40001/components/offline_page...
components/offline_pages/background/cleanup_task.cc:95: request->request_state()
!= SavePageRequest::RequestState::OFFLINING) {
On 2016/12/06 00:59:41, dougarnett wrote:
> This check relies on the reconciliation task to happen.

Yes, but we are planning on making a reconciliation task.  Do you have some
other solution that you would prefer?  Maybe we could pass in a list of ids that
are in progress instead of a single one.  Maybe we could add a TODO here to make
sure we don't forget to start the reconciliation task (although it is next on my
plate).

[dougarnett, 2016-12-06 02:11:13]

lgtm with comment

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

https://codereview.chromium.org/2543093002/diff/40001/components/offline_page...
components/offline_pages/background/cleanup_task.cc:95: request->request_state()
!= SavePageRequest::RequestState::OFFLINING) {
On 2016/12/06 02:06:28, Pete Williamson wrote:
> On 2016/12/06 00:59:41, dougarnett wrote:
> > This check relies on the reconciliation task to happen.
> 
> Yes, but we are planning on making a reconciliation task.  Do you have some
> other solution that you would prefer?  Maybe we could pass in a list of ids
that
> are in progress instead of a single one.  Maybe we could add a TODO here to
make
> sure we don't forget to start the reconciliation task (although it is next on
my
> plate).

Please document the dependency

[fgorski, 2016-12-06 17:05:35]

lgtm.

This is great work. Thanks for addressing all my comments!

Filip

[Pete Williamson, 2016-12-06 17:59:40]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-12-06 17:59:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2543093002/#ps60001
(title: "ADD TODO")

[commit-bot: I haz the power, 2016-12-06 18:00:22]

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-12-06 19:03:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481047180424570,
"parent_rev": "8263019dcbde24c7c0c3f853cb6c0921b331fb01", "commit_rev":
"ce05270699a9b693a7067481f93cb5841b232b09"}

[commit-bot: I haz the power, 2016-12-06 19:04:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-06 19:07:12]

Description was changed from

==========
Split the RequestPicker task into two separate tasks.

We still have a picker task, but now we also have a cleanup task.  The
cleanup task will remove expired entries, and entries that exceed their
start or completion limit.  It does not yet flip offlining tasks to
available on startup, I plan that for a separate changelist, and likely
a new task.

BUG=670343
==========

to

==========
Split the RequestPicker task into two separate tasks.

We still have a picker task, but now we also have a cleanup task.  The
cleanup task will remove expired entries, and entries that exceed their
start or completion limit.  It does not yet flip offlining tasks to
available on startup, I plan that for a separate changelist, and likely
a new task.

BUG=670343

Committed: https://crrev.com/55fc16c382cd559fca177abd0ae291213bd5893b
Cr-Commit-Position: refs/heads/master@{#436671}
==========

[commit-bot: I haz the power, 2016-12-06 19:07:13]

Patchset 4 (id:??) landed as
https://crrev.com/55fc16c382cd559fca177abd0ae291213bd5893b
Cr-Commit-Position: refs/heads/master@{#436671}