Reconcile the request queue on startup.

Reconcile the request queue on startup.

If Chromium dies while an offline request is active, when we start back
up, the request will be in the "OFFLINING" state in the request queue.
However, unless it is in the AVAILABLE state, it will not be a candidate
for either cleanup or re-starting.  So, this change adds a fix when
the request coordinator first starts for every run changes requests
left in the OFFLINING state to the AVAILABLE state.

BUG=670343
Committed: https://crrev.com/18352e92d55f65f7bc986a535f797f5e5e52b69f
Cr-Commit-Position: refs/heads/master@{#439190}

[Pete Williamson, 2016-12-10 01:21:41]

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

[Pete Williamson, 2016-12-10 01:21:41]

LMK if you think I should also add a request coordinator level unit test.

[dougarnett, 2016-12-12 17:16:14]

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task.cc (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:28: // Get all the
requests from the queue, we will classify them in the callback.
classify => reconcile

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:43: // back to
available.
AVAILABLE

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:47:
request->set_request_state(SavePageRequest::RequestState::AVAILABLE);
Maybe TODO to add UMA since this might be pretty interesting to track frequency
of getting killed or crashing while active. Eg, might be nice to have this for
Finch experiment of concurrent loading on svelte.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:65: // Send a
notification to the UI that these items have updated
nit - period at end of sentence

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:70: }  // namespace
offlnie_pages
offline_pages

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task.h (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:18: class
ReconcileTask : public Task {
class comment please to state what it does

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:27: private:
nit - period at end of sentence here and Step 3 comment

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:31: // Step 2. Flip
OFFLINING requests to available and put back in queue.
available => AVAILABLE to match style with OFFLINING

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task_unittest.cc
(right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task_unittest.cc:114: void
ReconcileTaskTest::ReconcileCallback(
Should be able to test the updated requests pass in here

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_queue.h (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_queue.h:100: // Reconcile any
requests that were active when chrome exited.
... previously exited.  ?

[fgorski, 2016-12-12 17:35:18]

mostly looks good.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task.cc (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:38: TaskComplete();
what about calling the callback_?

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:43: // back to
available.
nit: is the line break intended?

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:54: TaskComplete();
ditto: callback_

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task.h (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:18: class
ReconcileTask : public Task {
On 2016/12/12 17:16:13, dougarnett wrote:
> class comment please to state what it does

+1

Include information about when you expect the task to run, please.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:24: // TaskQueue::Task
implementation, starts the async chain
nit: add colon after implementation.
Move the "Starts ..." part to a new line as a sentence or remove altogether.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:36: void
UpdateCompleted(std::unique_ptr<UpdateRequestsResult> update_result);
nit: empty line after this.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task_unittest.cc
(right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task_unittest.cc:33: const
SavePageRequest kEmptyRequest(0UL,
nit: wouldn't git cl format put that on a single line below?

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task_unittest.cc:144:
TEST_F(ReconcileTaskTest, Reconcile) {
What about testing for case, where no need to update was found?

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:187: // If we
exited with any items left in the OFFLINING state, move them
I think order between reconcile and clean up makes sense, but please document it
in a comment as well.

Did you consider the opposite order? Why did you choose that one?

[Pete Williamson, 2016-12-15 18:37:43]

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task.cc (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:28: // Get all the
requests from the queue, we will classify them in the callback.
On 2016/12/12 17:16:13, dougarnett wrote:
> classify => reconcile

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:38: TaskComplete();
On 2016/12/12 17:35:18, fgorski wrote:
> what about calling the callback_?

The callback is only to change state of notifications that are shown by android.
 If there are no notifications, we don't need to do the callback.  Updated
comments to reflect this.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:43: // back to
available.
On 2016/12/12 17:16:13, dougarnett wrote:
> AVAILABLE

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:43: // back to
available.
On 2016/12/12 17:35:18, fgorski wrote:
> nit: is the line break intended?

Fixed the line break, it broke too early.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:47:
request->set_request_state(SavePageRequest::RequestState::AVAILABLE);
On 2016/12/12 17:16:13, dougarnett wrote:
> Maybe TODO to add UMA since this might be pretty interesting to track
frequency
> of getting killed or crashing while active. Eg, might be nice to have this for
> Finch experiment of concurrent loading on svelte.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:54: TaskComplete();
On 2016/12/12 17:35:18, fgorski wrote:
> ditto: callback_

Comment updated here too.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:65: // Send a
notification to the UI that these items have updated
On 2016/12/12 17:16:13, dougarnett wrote:
> nit - period at end of sentence

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.cc:70: }  // namespace
offlnie_pages
On 2016/12/12 17:16:13, dougarnett wrote:
> offline_pages

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task.h (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:18: class
ReconcileTask : public Task {
On 2016/12/12 17:16:13, dougarnett wrote:
> class comment please to state what it does

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:18: class
ReconcileTask : public Task {
On 2016/12/12 17:35:18, fgorski wrote:
> On 2016/12/12 17:16:13, dougarnett wrote:
> > class comment please to state what it does
> 
> +1
> 
> Include information about when you expect the task to run, please.

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:24: // TaskQueue::Task
implementation, starts the async chain
On 2016/12/12 17:35:18, fgorski wrote:
> nit: add colon after implementation.
> Move the "Starts ..." part to a new line as a sentence or remove altogether.

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:27: private:
On 2016/12/12 17:16:13, dougarnett wrote:
> nit - period at end of sentence here and Step 3 comment

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:31: // Step 2. Flip
OFFLINING requests to available and put back in queue.
On 2016/12/12 17:16:13, dougarnett wrote:
> available => AVAILABLE to match style with OFFLINING

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task.h:36: void
UpdateCompleted(std::unique_ptr<UpdateRequestsResult> update_result);
On 2016/12/12 17:35:18, fgorski wrote:
> nit: empty line after this.

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/reconcile_task_unittest.cc
(right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task_unittest.cc:33: const
SavePageRequest kEmptyRequest(0UL,
On 2016/12/12 17:35:18, fgorski wrote:
> nit: wouldn't git cl format put that on a single line below?

Git cl format seems to prefer lining these up.  I did use git cl format here.  I
don't yet quite understand how it decides to order function parameters,
sometimes it prefers lining these up under the first parameter just after the
'(', sometimes it seems to prefer starting a fresh line.  Maybe it goes for this
style if it can, and then uses the other style only if a line overflows with
this style?

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task_unittest.cc:114: void
ReconcileTaskTest::ReconcileCallback(
On 2016/12/12 17:16:13, dougarnett wrote:
> Should be able to test the updated requests pass in here

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/reconcile_task_unittest.cc:144:
TEST_F(ReconcileTaskTest, Reconcile) {
On 2016/12/12 17:35:18, fgorski wrote:
> What about testing for case, where no need to update was found?

Done.

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_coordinator.cc (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_coordinator.cc:187: // If we
exited with any items left in the OFFLINING state, move them
On 2016/12/12 17:35:18, fgorski wrote:
> I think order between reconcile and clean up makes sense, but please document
it
> in a comment as well.
> 
> Did you consider the opposite order? Why did you choose that one?

Both orders should work fine.  I picked this one because cleanup will ignore
OFFLINING requests, and in the case the request expired or exceeded a retry
count while offlining, I thought it best to clean it up sooner instead of later.
(comment added)

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
File components/offline_pages/core/background/request_queue.h (right):

https://codereview.chromium.org/2568613002/diff/1/components/offline_pages/co...
components/offline_pages/core/background/request_queue.h:100: // Reconcile any
requests that were active when chrome exited.
On 2016/12/12 17:16:14, dougarnett wrote:
> ... previously exited.  ?

Done.

[dougarnett, 2016-12-15 20:30:09]

lgtm

https://codereview.chromium.org/2568613002/diff/20001/components/offline_page...
File components/offline_pages/core/background/reconcile_task.h (right):

https://codereview.chromium.org/2568613002/diff/20001/components/offline_page...
components/offline_pages/core/background/reconcile_task.h:19: // tasks that were
prerendering when chrome died, and change the state back to
prerendering => offlining

[Pete Williamson, 2016-12-15 20:50:24]

https://codereview.chromium.org/2568613002/diff/20001/components/offline_page...
File components/offline_pages/core/background/reconcile_task.h (right):

https://codereview.chromium.org/2568613002/diff/20001/components/offline_page...
components/offline_pages/core/background/reconcile_task.h:19: // tasks that were
prerendering when chrome died, and change the state back to
On 2016/12/15 20:30:09, dougarnett wrote:
> prerendering => offlining

Done.

[fgorski, 2016-12-15 21:50:00]

lgtm

[Pete Williamson, 2016-12-15 21:58:17]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-12-15 21:58:17]

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/2568613002/#ps40001
(title: "Comment change")

[commit-bot: I haz the power, 2016-12-15 21:58:59]

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-15 22:22:42]

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

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

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Pete Williamson, 2016-12-16 00:45:34]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-12-16 00:45:34]

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/2568613002/#ps60001
(title: "compile fix")

[commit-bot: I haz the power, 2016-12-16 00:46:45]

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-16 06:03:56]

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

[commit-bot: I haz the power, 2016-12-16 06:03:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Pete Williamson, 2016-12-16 18:34:32]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-12-16 18:35:00]

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-16 21:22:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481913272144210,
"parent_rev": "dff9d5018e088594a3ac12193cfe04643a524ebf", "commit_rev":
"3fb1fde910d6702e61ce990fce3a0e65cb26245b"}

[commit-bot: I haz the power, 2016-12-16 21:23:00]

Description was changed from

==========
Reconcile the request queue on startup.

If Chromium dies while an offline request is active, when we start back
up, the request will be in the "OFFLINING" state in the request queue.
However, unless it is in the AVAILABLE state, it will not be a candidate
for either cleanup or re-starting.  So, this change adds a fix when
the request coordinator first starts for every run changes requests
left in the OFFLINING state to the AVAILABLE state.

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

to

==========
Reconcile the request queue on startup.

If Chromium dies while an offline request is active, when we start back
up, the request will be in the "OFFLINING" state in the request queue.
However, unless it is in the AVAILABLE state, it will not be a candidate
for either cleanup or re-starting.  So, this change adds a fix when
the request coordinator first starts for every run changes requests
left in the OFFLINING state to the AVAILABLE state.

BUG=670343

Review-Url: https://codereview.chromium.org/2568613002
==========

[commit-bot: I haz the power, 2016-12-16 21:23:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-16 21:25:00]

Description was changed from

==========
Reconcile the request queue on startup.

If Chromium dies while an offline request is active, when we start back
up, the request will be in the "OFFLINING" state in the request queue.
However, unless it is in the AVAILABLE state, it will not be a candidate
for either cleanup or re-starting.  So, this change adds a fix when
the request coordinator first starts for every run changes requests
left in the OFFLINING state to the AVAILABLE state.

BUG=670343

Review-Url: https://codereview.chromium.org/2568613002
==========

to

==========
Reconcile the request queue on startup.

If Chromium dies while an offline request is active, when we start back
up, the request will be in the "OFFLINING" state in the request queue.
However, unless it is in the AVAILABLE state, it will not be a candidate
for either cleanup or re-starting.  So, this change adds a fix when
the request coordinator first starts for every run changes requests
left in the OFFLINING state to the AVAILABLE state.

BUG=670343

Committed: https://crrev.com/18352e92d55f65f7bc986a535f797f5e5e52b69f
Cr-Commit-Position: refs/heads/master@{#439190}
==========

[commit-bot: I haz the power, 2016-12-16 21:25:01]

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