[Offline Pages] Converts MarkAttemptCompleted to use TaskQueue

[Offline Pages] Converts MarkAttemptCompleted to a Task

BUG=651238
Committed: https://crrev.com/594de46700a111e7f2f08594eb601854ece1790e
Cr-Commit-Position: refs/heads/master@{#429021}

[dougarnett, 2016-10-31 20:15:23]

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

[commit-bot: I haz the power, 2016-10-31 20:16:06]

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-31 20:16:13]

Description was changed from

==========
[Offline Pages] Converts MarkAttemptCompleted to a Task

BUG=651238
==========

to

==========
[Offline Pages] Converts MarkAttemptCompleted to a Task

BUG=651238
==========

[dougarnett, 2016-10-31 20:23:35]

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

[commit-bot: I haz the power, 2016-10-31 20:24:22]

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-31 20:25:54]

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

[fgorski, 2016-10-31 20:33:50]

mostly looks great. Some suggestions to consider.

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
File components/offline_pages/background/mark_attempt_completed_task_unittest.cc
(left):

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task_unittest.cc:118:
TEST_F(MarkAttemptStartedTaskTest, MarkAttemptStartedWhenItemMissing) {
It's worth adding a test when the item is missing to set the expectation for
what is a proper result.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:373: if
(result->store_state != StoreState::LOADED ||
see if in line 301 please and consider if you could extract some code.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:83: void UpdateRequest(const
SavePageRequest& request,
Please check if this could be removed with the change you are making, or is
there anything blocking that. I would love to see this one go.

[Pete Williamson, 2016-10-31 21:38:27]

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task.cc:7: #include
<utility>
I wonder if <utility> should be added to mark_attempt_started_task.h too.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task_unittest.cc:113:

Should we have a test where there are multiple items in the queue, and only one
gets completed?

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:379:
result->store_state != StoreState::LOADED
I think having the : ? inside the parameter list makes the code less readable, I
think the code would be easier to read if the check is put outside the function
call, and just use the result of the check here.  Yes, that is an extra local
variable, but the compiler optimizes it out, and I think the extra variable
pulls its weight in making the code more readable.

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:383:
NotifyChanged(result->updated_items.at(0));
The old code notified changed even if the database update failed, perhaps we
should still do that.

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:719:
base::Bind(&RequestCoordinator::MarkAttemptCompletedDoneCallback,
Hmm, if this is really the only place left we used UpdateRequestCallback, it
should be OK to rename.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:246: void
MarkAttemptCompletedDoneCallback(
I thought this callback was used for lots of things, this name change might not
be a good idea.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_unittest.cc:485:
TEST_F(RequestQueueTest, MarkAttemptAbortedRequestNotPresent) {
Does it also make sense to test the "not present" case for mark completed? 
Perhaps there was a race condition, and it got removed before the callback
arrived to cause it to be marked completed?

[commit-bot: I haz the power, 2016-10-31 21:40:35]

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

[commit-bot: I haz the power, 2016-10-31 21:40:36]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-10-31 22:05:52]

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
File components/offline_pages/background/mark_attempt_completed_task_unittest.cc
(left):

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task_unittest.cc:118:
TEST_F(MarkAttemptStartedTaskTest, MarkAttemptStartedWhenItemMissing) {
On 2016/10/31 20:33:49, fgorski wrote:
> It's worth adding a test when the item is missing to set the expectation for
> what is a proper result.

There already is a test for item not being found above - *WhenStoreEmpty.
There is no additional coverage nor user expectation setting in this test so
rather not bloat the test set unproductively. I went with the first
one because it was easier to read but this one is slightly more strict so
will switch to this one with some comments to try to make easier to read.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:373: if
(result->store_state != StoreState::LOADED ||
On 2016/10/31 20:33:50, fgorski wrote:
> see if in line 301 please and consider if you could extract some code.

Done.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:83: void UpdateRequest(const
SavePageRequest& request,
On 2016/10/31 20:33:50, fgorski wrote:
> Please check if this could be removed with the change you are making, or is
> there anything blocking that. I would love to see this one go.

Done.

[dougarnett, 2016-10-31 22:49:46]

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

[commit-bot: I haz the power, 2016-10-31 22:50: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-31 22:51:18]

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task.cc:7: #include
<utility>
On 2016/10/31 21:38:26, Pete Williamson wrote:
> I wonder if <utility> should be added to mark_attempt_started_task.h too.

Done.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task_unittest.cc:113:

On 2016/10/31 21:38:26, Pete Williamson wrote:
> Should we have a test where there are multiple items in the queue, and only
one
> gets completed?

Done.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:379:
result->store_state != StoreState::LOADED
On 2016/10/31 21:38:26, Pete Williamson wrote:
> I think having the : ? inside the parameter list makes the code less readable,
I
> think the code would be easier to read if the check is put outside the
function
> call, and just use the result of the check here.  Yes, that is an extra local
> variable, but the compiler optimizes it out, and I think the extra variable
> pulls its weight in making the code more readable.

Actually mirroring pattern already in this file, not introducing new pattern.

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:383:
NotifyChanged(result->updated_items.at(0));
On 2016/10/31 21:38:26, Pete Williamson wrote:
> The old code notified changed even if the database update failed, perhaps we
> should still do that.

Great question. I'm not sure. Would we want to NotifyCompleted() instead with
some type of failed status? Also, we may not have the request to call
NotifyChange
with (or NotifyCompleted) - especially if we stop caching the full active
request.

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:719:
base::Bind(&RequestCoordinator::MarkAttemptCompletedDoneCallback,
On 2016/10/31 21:38:26, Pete Williamson wrote:
> Hmm, if this is really the only place left we used UpdateRequestCallback, it
> should be OK to rename.

Yes. Also have now deleted RequestQueue::UpdateRequest() as well

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:246: void
MarkAttemptCompletedDoneCallback(
On 2016/10/31 21:38:27, Pete Williamson wrote:
> I thought this callback was used for lots of things, this name change might
not
> be a good idea.

No, this was last usage.

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_unittest.cc:485:
TEST_F(RequestQueueTest, MarkAttemptAbortedRequestNotPresent) {
On 2016/10/31 21:38:27, Pete Williamson wrote:
> Does it also make sense to test the "not present" case for mark completed? 
> Perhaps there was a race condition, and it got removed before the callback
> arrived to cause it to be marked completed?

That logic resides in a base class now and so this Aborted test
covers testing that base class behavior. Adding comparable Completed
test would not actually add any test coverage.

[Pete Williamson, 2016-10-31 23:01:27]

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task_unittest.cc:113:

On 2016/10/31 22:51:17, dougarnett wrote:
> On 2016/10/31 21:38:26, Pete Williamson wrote:
> > Should we have a test where there are multiple items in the queue, and only
> one
> > gets completed?
> 
> Done.

I don't see the test, did your patch get uploaded?

[Pete Williamson, 2016-10-31 23:02:05]

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/mark_attempt_completed_task_unittest.cc:113:

On 2016/10/31 23:01:27, Pete Williamson wrote:
> On 2016/10/31 22:51:17, dougarnett wrote:
> > On 2016/10/31 21:38:26, Pete Williamson wrote:
> > > Should we have a test where there are multiple items in the queue, and
only
> > one
> > > gets completed?
> > 
> > Done.
> 
> I don't see the test, did your patch get uploaded?

Nevermind, I hit refresh, and then I saw the patch.

[fgorski, 2016-10-31 23:05:28]

lgtm

[Pete Williamson, 2016-10-31 23:10:08]

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:379:
result->store_state != StoreState::LOADED
On 2016/10/31 22:51:18, dougarnett wrote:
> On 2016/10/31 21:38:26, Pete Williamson wrote:
> > I think having the : ? inside the parameter list makes the code less
readable,
> I
> > think the code would be easier to read if the check is put outside the
> function
> > call, and just use the result of the check here.  Yes, that is an extra
local
> > variable, but the compiler optimizes it out, and I think the extra variable
> > pulls its weight in making the code more readable.
> 
> Actually mirroring pattern already in this file, not introducing new pattern.
> 

Let's still fix it, the pattern is not a great one.  It looks like the other use
case got abstracted out into a function, perhaps we can do that here too.

[commit-bot: I haz the power, 2016-10-31 23:59:27]

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

[commit-bot: I haz the power, 2016-10-31 23:59:27]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-11-01 00:16:37]

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

https://codereview.chromium.org/2463713003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:379:
result->store_state != StoreState::LOADED
On 2016/10/31 23:10:08, Pete Williamson wrote:
> On 2016/10/31 22:51:18, dougarnett wrote:
> > On 2016/10/31 21:38:26, Pete Williamson wrote:
> > > I think having the : ? inside the parameter list makes the code less
> readable,
> > I
> > > think the code would be easier to read if the check is put outside the
> > function
> > > call, and just use the result of the check here.  Yes, that is an extra
> local
> > > variable, but the compiler optimizes it out, and I think the extra
variable
> > > pulls its weight in making the code more readable.
> > 
> > Actually mirroring pattern already in this file, not introducing new
pattern.
> > 
> 
> Let's still fix it, the pattern is not a great one.  It looks like the other
use
> case got abstracted out into a function, perhaps we can do that here too.

ok, done

[Pete Williamson, 2016-11-01 00:32:33]

lgtm

[dougarnett, 2016-11-01 15:42:17]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-11-01 15:42:18]

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/2463713003/#ps80001
(title: "Ternary nit and format fix")

[commit-bot: I haz the power, 2016-11-01 15:42:32]

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-11-01 16:39:57]

Description was changed from

==========
[Offline Pages] Converts MarkAttemptCompleted to a Task

BUG=651238
==========

to

==========
[Offline Pages] Converts MarkAttemptCompleted to a Task

BUG=651238
==========

[commit-bot: I haz the power, 2016-11-01 16:39:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-01 17:22:27]

Description was changed from

==========
[Offline Pages] Converts MarkAttemptCompleted to a Task

BUG=651238
==========

to

==========
[Offline Pages] Converts MarkAttemptCompleted to a Task

BUG=651238

Committed: https://crrev.com/594de46700a111e7f2f08594eb601854ece1790e
Cr-Commit-Position: refs/heads/master@{#429021}
==========

[commit-bot: I haz the power, 2016-11-01 17:22:29]

Patchset 5 (id:??) landed as
https://crrev.com/594de46700a111e7f2f08594eb601854ece1790e
Cr-Commit-Position: refs/heads/master@{#429021}