[Offline pages] Implementation of RQStore.UpdateRequests with StoreUpdateResult

[Offline pages] Implementation of RQStore.UpdateRequests with StoreUpdateResult

* Creates RequestQueue store specific instantiation of StoreUdpateResult
  template
* Adds UpdateRequests to RequestQueueStore
* Remove AddOrUpdateRequest from RequestQueueStore
* Updates tests.

BUG=645522
R=petewil@chromium.org

Committed: https://crrev.com/c42be44ac50c01d3ff2e472426a57186625a4dc0
Cr-Commit-Position: refs/heads/master@{#420377}

[fgorski, 2016-09-21 22:30:17]

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

[commit-bot: I haz the power, 2016-09-21 22:30:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2016-09-21 22:49:06]

PTAL

[commit-bot: I haz the power, 2016-09-21 23:33:49]

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

[commit-bot: I haz the power, 2016-09-21 23:33:50]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-09-22 00:12:50]

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:76: void
UpdateMultipleRequestsDone(
We should remove all references to UpdateMultipleRequestsXxx.

The original idea was that UpdateRequestDone was used for a single request, and
UpdateMultipleRequests was used for many.  Since we now always do many, we don't
need both flavors of the function.

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:39:
UpdateMultipleRequestsCallback;
Let's remove all references to UpdateMultipleRequestXxx.

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_sql.cc:254: " SET
creation_time = ?, activation_time = ?, last_attempt_time = ?,"
Why are these in a different order than before? (I'm presuming it is because
once you use the parameterized form that order no longer has to match column
order, but I wanted to double check.)

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_sql.h:46: const
UpdateMultipleRequestsCallback& callback) override;
This should be updated to use UpdateCallback instead of
UpdateMultipleRequestCallback

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_unittest.cc:156:
last_requests_ = std::move(requests);
Let's remove UpdateMultipleRequestsDone - The idea was to have one function for
a single request, and one for multiple, and UpdateRequestDone now handles
multiples.

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_unittest.cc:274:
TYPED_TEST(RequestQueueStoreTest, UpdateRequest) {
This test only tests a single request at a time, we should test updating more
than one at a time to make sure we fully exercise the code under test.

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_unittest.cc:318:
this->ClearResults();
This section is a nice addition to the test, thanks!

[fgorski, 2016-09-22 06:10:54]

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

[commit-bot: I haz the power, 2016-09-22 06:11:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fgorski, 2016-09-22 06:11:43]

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

[commit-bot: I haz the power, 2016-09-22 06:11:55]

Dry run: 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-09-22 06:55:31]

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

[commit-bot: I haz the power, 2016-09-22 06:55:33]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-09-22 15:47:52]

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

[fgorski, 2016-09-22 15:47:54]

Addressable comments applied.
I'll remove method changing status separately.

Dmitry, please take a look. I think Pete mention something being out until
Tuesday.

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:76: void
UpdateMultipleRequestsDone(
On 2016/09/22 00:12:50, Pete Williamson wrote:
> We should remove all references to UpdateMultipleRequestsXxx.
> 
> The original idea was that UpdateRequestDone was used for a single request,
and
> UpdateMultipleRequests was used for many.  Since we now always do many, we
don't
> need both flavors of the function.

I agree with you, but let's go step by step. This is still used right now and
I'd like to remove ChangeRequestsState in a separate patch. Hope that is ok.

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:39:
UpdateMultipleRequestsCallback;
On 2016/09/22 00:12:50, Pete Williamson wrote:
> Let's remove all references to UpdateMultipleRequestXxx.

Still used by ChangeRequestState. Separate patch.

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_sql.cc:254: " SET
creation_time = ?, activation_time = ?, last_attempt_time = ?,"
On 2016/09/22 00:12:50, Pete Williamson wrote:
> Why are these in a different order than before? (I'm presuming it is because
> once you use the parameterized form that order no longer has to match column
> order, but I wanted to double check.)

You are correct that it does not matter.
If you look below however you will notice that except for request_id, which was
moved to the WHERE clause at the end, they are in exactly the same order.

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_sql.h:46: const
UpdateMultipleRequestsCallback& callback) override;
On 2016/09/22 00:12:50, Pete Williamson wrote:
> This should be updated to use UpdateCallback instead of
> UpdateMultipleRequestCallback

Acknowledged.

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

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_unittest.cc:156:
last_requests_ = std::move(requests);
On 2016/09/22 00:12:50, Pete Williamson wrote:
> Let's remove UpdateMultipleRequestsDone - The idea was to have one function
for
> a single request, and one for multiple, and UpdateRequestDone now handles
> multiples.

Acknowledged. Separate patch.

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_unittest.cc:274:
TYPED_TEST(RequestQueueStoreTest, UpdateRequest) {
On 2016/09/22 00:12:50, Pete Williamson wrote:
> This test only tests a single request at a time, we should test updating more
> than one at a time to make sure we fully exercise the code under test.

Done. Updated to run both tests in one call to Update.

https://codereview.chromium.org/2363563002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store_unittest.cc:318:
this->ClearResults();
On 2016/09/22 00:12:50, Pete Williamson wrote:
> This section is a nice addition to the test, thanks!

Acknowledged.

[Pete Williamson, 2016-09-22 16:38:18]

lgtm

[fgorski, 2016-09-22 17:05:48]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-09-22 17:06:10]

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-09-22 17:11:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-22 17:15:09]

Description was changed from

==========
[Offline pages] Implementation of RQStore.UpdateRequests with StoreUpdateResult

* Creates RequestQueue store specific instantiation of StoreUdpateResult
  template
* Adds UpdateRequests to RequestQueueStore
* Remove AddOrUpdateRequest from RequestQueueStore
* Updates tests.

BUG=645522
R=petewil@chromium.org
==========

to

==========
[Offline pages] Implementation of RQStore.UpdateRequests with StoreUpdateResult

* Creates RequestQueue store specific instantiation of StoreUdpateResult
  template
* Adds UpdateRequests to RequestQueueStore
* Remove AddOrUpdateRequest from RequestQueueStore
* Updates tests.

BUG=645522
R=petewil@chromium.org

Committed: https://crrev.com/c42be44ac50c01d3ff2e472426a57186625a4dc0
Cr-Commit-Position: refs/heads/master@{#420377}
==========

[commit-bot: I haz the power, 2016-09-22 17:15:11]

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