[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

* Creates a task to change requests state with tests
* Updates how RequestQueue performs ChangeRequestsState
* Removes ChangeRequestsState from the store

BUG=645522, 651238
Committed: https://crrev.com/1a98a5ef6b896c298d56f941108ce8df56ef0fde
Cr-Commit-Position: refs/heads/master@{#422316}

[fgorski, 2016-09-26 23:46:47]

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

[fgorski, 2016-09-26 23:46:47]

Patch 2/2. To discuss tomorrow. PTAL

[Pete Williamson, 2016-09-27 19:34:05]

On 2016/09/26 23:46:47, fgorski wrote:
> Patch 2/2. To discuss tomorrow. PTAL

While I like the idea and approach in general, ChangeRequestState may not be the
best place to apply it since my upcoming patch will be removing
ChangeRequestState.  I'd be more interested in seeing a change for Update().

[Pete Williamson, 2016-09-27 23:49:56]

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

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task.h:21: class
ChangeRequestsStateTask : public TaskQueue::Task {
Does chrome already have a way to do this  It seems so useful, that maybe
somebody already wrote one.

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task.h:49: //
TODO(fgorski): perhaps convert to unique_ptr to a vector.
comment out of date?

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

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task_unittest.cc:18:
// const int64_t kRequestId2 = 43;
Let's remove or comment in (and use) the commented out code here.

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task_unittest.cc:94:
TEST_F(ChangeRequestsStateTaskTest, UpdateSingleItem) {
Maybe add a test to update multiple items?

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

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:76: // Handles updating
multiple requests at the same time.
Comment can go too if you re removing the method.

[fgorski, 2016-09-28 22:37:56]

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

[commit-bot: I haz the power, 2016-09-28 22:38:27]

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-28 22:39:07]

Addressed, rebased, PTAL

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

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task.h:21: class
ChangeRequestsStateTask : public TaskQueue::Task {
On 2016/09/27 23:49:56, Pete Williamson wrote:
> Does chrome already have a way to do this  It seems so useful, that maybe
> somebody already wrote one.

I wasn't able to find anything like that. I think common pattern in Chrome is to
load lots of stuff in memory (e.g. bookmarks) therefor not having to put extra
read before write.

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task.h:49: //
TODO(fgorski): perhaps convert to unique_ptr to a vector.
On 2016/09/27 23:49:56, Pete Williamson wrote:
> comment out of date?

Done

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

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task_unittest.cc:18:
// const int64_t kRequestId2 = 43;
On 2016/09/27 23:49:56, Pete Williamson wrote:
> Let's remove or comment in (and use) the commented out code here.

Done.

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/change_requests_state_task_unittest.cc:94:
TEST_F(ChangeRequestsStateTaskTest, UpdateSingleItem) {
On 2016/09/27 23:49:56, Pete Williamson wrote:
> Maybe add a test to update multiple items?

Done.

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

https://codereview.chromium.org/2372043002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.cc:76: // Handles updating
multiple requests at the same time.
On 2016/09/27 23:49:56, Pete Williamson wrote:
> Comment can go too if you re removing the method.

Done.

[fgorski, 2016-09-28 23:00:57]

Description was changed from

==========
[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

* Creates a task to change requests state with tests
* Updates how RequestQueue performs ChangeRequestsState
* Removes ChangeRequestsState from the store

BUG=645522
==========

to

==========
[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

* Creates a task to change requests state with tests
* Updates how RequestQueue performs ChangeRequestsState
* Removes ChangeRequestsState from the store

BUG=645522, 651238
==========

[commit-bot: I haz the power, 2016-09-28 23:34:22]

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

[commit-bot: I haz the power, 2016-09-28 23:34:23]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-09-29 21:02:50]

lgtm with few comments

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

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task.h:30: // Step 1.
Reading the requests.
I wonder about a pattern of separate Step 1 method ReadRequests() and then Run()
just calls Step 1 method. Just a thought.

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task.h:47:
RequestQueueStore* store_;
this is kind of interesting - not having the TaskQueue bound to a store. I
hadn't pictured that way initially but seems fine.

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

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:303: for (const auto&
request : result->updated_items) {
Could have just one iteration through updated_items

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

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store.h:52: const
RequestQueue::UpdateCallback& callback) = 0;
Is this redundant with the "using" you added on line 27?

[Pete Williamson, 2016-09-29 22:44:24]

lgtm

[fgorski, 2016-09-30 17:53:29]

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

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

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-30 18:51:38]

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

[commit-bot: I haz the power, 2016-09-30 18:51:39]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-09-30 23:14:35]

All addressed.

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

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task.h:30: // Step 1.
Reading the requests.
On 2016/09/29 21:02:49, dougarnett wrote:
> I wonder about a pattern of separate Step 1 method ReadRequests() and then
Run()
> just calls Step 1 method. Just a thought.

Done. Actually a very good idea, I also noticed that the individual steps can be
private. Moved all three below private:

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/change_requests_state_task.h:47:
RequestQueueStore* store_;
On 2016/09/29 21:02:49, dougarnett wrote:
> this is kind of interesting - not having the TaskQueue bound to a store. I
> hadn't pictured that way initially but seems fine.

Acknowledged. That is actually intended. TaskQueue does not care what it guards,
it just does it.

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

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:303: for (const auto&
request : result->updated_items) {
On 2016/09/29 21:02:49, dougarnett wrote:
> Could have just one iteration through updated_items

I know, but I prefer this, because it is the same complexity, but clearer
separation (notify changed can be extracted from all methods and augmented by
event logger) and readability that way.

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

https://codereview.chromium.org/2372043002/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store.h:52: const
RequestQueue::UpdateCallback& callback) = 0;
On 2016/09/29 21:02:49, dougarnett wrote:
> Is this redundant with the "using" you added on line 27?

Done. Good catch. Thanks.

[fgorski, 2016-10-01 14:40:10]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-10-01 14:40:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, dougarnett@chromium.org
Link to the patchset: https://codereview.chromium.org/2372043002/#ps40001
(title: "Addressing feedback from dougarnett")

[commit-bot: I haz the power, 2016-10-01 14:40:20]

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-10-01 14:45:02]

Description was changed from

==========
[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

* Creates a task to change requests state with tests
* Updates how RequestQueue performs ChangeRequestsState
* Removes ChangeRequestsState from the store

BUG=645522, 651238
==========

to

==========
[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

* Creates a task to change requests state with tests
* Updates how RequestQueue performs ChangeRequestsState
* Removes ChangeRequestsState from the store

BUG=645522, 651238
==========

[commit-bot: I haz the power, 2016-10-01 14:45:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-01 14:47:34]

Description was changed from

==========
[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

* Creates a task to change requests state with tests
* Updates how RequestQueue performs ChangeRequestsState
* Removes ChangeRequestsState from the store

BUG=645522, 651238
==========

to

==========
[Offline pages] Applying TaskQueue to RequestQueue::ChangeRequestsState

* Creates a task to change requests state with tests
* Updates how RequestQueue performs ChangeRequestsState
* Removes ChangeRequestsState from the store

BUG=645522, 651238

Committed: https://crrev.com/1a98a5ef6b896c298d56f941108ce8df56ef0fde
Cr-Commit-Position: refs/heads/master@{#422316}
==========

[commit-bot: I haz the power, 2016-10-01 14:47:35]

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