[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

* Switching the interface of RequestQueue/RequestStore::RemoveRequests
* Converting implementation to Task based: RemoveRequestsTask with tests.
* Removing most of old callbacks and typedefs

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

[fgorski, 2016-09-28 22:45:21]

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:45:34]

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:46:13]

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

[fgorski, 2016-09-28 22:46:14]

PTAL

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

Description was changed from

==========
[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

* Switching the interface of RequestQueue/RequestStore::RemoveRequests
* Converting implementation to Task based: RemoveRequestsTask with tests.
* Removing most of old callbacks and typedefs

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

to

==========
[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

* Switching the interface of RequestQueue/RequestStore::RemoveRequests
* Converting implementation to Task based: RemoveRequestsTask with tests.
* Removing most of old callbacks and typedefs

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

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

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Pete Williamson, 2016-09-30 18:03:54]

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/remove_requests_task.cc:14: const
RequestQueueStore::UpdateCallback& callback)
Let's apply the renames from the other (TaskQueue and Task) change to this
changelist too.

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/remove_requests_task_unittest.cc:54: void
RemoveRequestsTaskTest::SetUpStore(RequestQueueStore* store) {
A better name might be "AddARequestToStore".  This method doesn't really set up
the store, the constructor did that.  This just fills the store with the first
request.

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/remove_requests_task_unittest.cc:90:
TEST_F(RemoveRequestsTaskTest, RemoveSingleItem) {
Maybe add a test for removing multiple items.

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:98: void
RemoveRequests(const std::vector<int64_t>& request_ids,
Why do we need two functions?

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:98: void
RemoveRequests(const std::vector<int64_t>& request_ids,
Why do we need two function declarations for RemoveRequests?  Can we get rid of
the other one?

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:330: 
It might be good to call this->ClearResults() before the following "get"
operation.

[fgorski, 2016-10-01 17:37:21]

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

[commit-bot: I haz the power, 2016-10-01 17:37:35]

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

[fgorski, 2016-10-01 18:02:23]

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

[commit-bot: I haz the power, 2016-10-01 18:02:28]

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-10-01 20:45:40]

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

[commit-bot: I haz the power, 2016-10-01 20:45:41]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-10-04 17:18:24]

PTAL

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/remove_requests_task.cc:14: const
RequestQueueStore::UpdateCallback& callback)
On 2016/09/30 18:03:53, Pete Williamson wrote:
> Let's apply the renames from the other (TaskQueue and Task) change to this
> changelist too.

Acknowledged.

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/remove_requests_task_unittest.cc:54: void
RemoveRequestsTaskTest::SetUpStore(RequestQueueStore* store) {
On 2016/09/30 18:03:53, Pete Williamson wrote:
> A better name might be "AddARequestToStore".  This method doesn't really set
up
> the store, the constructor did that.  This just fills the store with the first
> request.

Done.

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/remove_requests_task_unittest.cc:90:
TEST_F(RemoveRequestsTaskTest, RemoveSingleItem) {
On 2016/09/30 18:03:53, Pete Williamson wrote:
> Maybe add a test for removing multiple items.

Done.

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:98: void
RemoveRequests(const std::vector<int64_t>& request_ids,
On 2016/09/30 18:03:53, Pete Williamson wrote:
> Why do we need two functions?

Done.

That was the plan. As you probably guessed, during development I had 2 functions
side by side to gently switch from one to the other.

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

https://codereview.chromium.org/2373933003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:330: 
On 2016/09/30 18:03:53, Pete Williamson wrote:
> It might be good to call this->ClearResults() before the following "get"
> operation.

Done.

[Pete Williamson, 2016-10-05 20:17:26]

mostly looks good, one remaining issue...

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

https://codereview.chromium.org/2373933003/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:471:
queue_->UpdateRequest(
Why two calls to UpdateRequest - merge problem?

[Pete Williamson, 2016-10-05 20:39:20]

lgtm after we talked in person about the duplicate call to update the request
state.

[fgorski, 2016-10-05 20:43:43]

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

[fgorski, 2016-10-05 20:43:44]

Michael, PTAL at the web ui stuff. Thanks

[fgorski, 2016-10-05 20:43:51]

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

[commit-bot: I haz the power, 2016-10-05 20:44:25]

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-10-05 21:49:39]

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

[commit-bot: I haz the power, 2016-10-05 21:49:40]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2016-10-06 01:57:35]

webui stuff lgtm pending comment spelling and IWYU suggestion

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h
(right):

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:75: const
offline_pages::MultipleItemStatuses& results);
#include (or fwd declare, include in the .cc) what you use?

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:81: //
Summarizes the MultpleItemStatuses vector with a string.
Multiple

[fgorski, 2016-10-06 04:35:28]

Comments addressed. Thanks for review.

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h
(right):

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:75: const
offline_pages::MultipleItemStatuses& results);
On 2016/10/06 01:57:35, michaelpg wrote:
> #include (or fwd declare, include in the .cc) what you use?

Done.

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:81: //
Summarizes the MultpleItemStatuses vector with a string.
On 2016/10/06 01:57:35, michaelpg wrote:
> Multiple

Done.

[fgorski, 2016-10-06 04:35:28]

Comments addressed. Thanks for review.

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h
(right):

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:75: const
offline_pages::MultipleItemStatuses& results);
On 2016/10/06 01:57:35, michaelpg wrote:
> #include (or fwd declare, include in the .cc) what you use?

Done.

https://codereview.chromium.org/2373933003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:81: //
Summarizes the MultpleItemStatuses vector with a string.
On 2016/10/06 01:57:35, michaelpg wrote:
> Multiple

Done.

[fgorski, 2016-10-06 04:35:35]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-10-06 04:35:35]

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

[commit-bot: I haz the power, 2016-10-06 04:36: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-10-06 06:21:34]

Description was changed from

==========
[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

* Switching the interface of RequestQueue/RequestStore::RemoveRequests
* Converting implementation to Task based: RemoveRequestsTask with tests.
* Removing most of old callbacks and typedefs

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

to

==========
[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

* Switching the interface of RequestQueue/RequestStore::RemoveRequests
* Converting implementation to Task based: RemoveRequestsTask with tests.
* Removing most of old callbacks and typedefs

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

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

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-06 06:23:49]

Description was changed from

==========
[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

* Switching the interface of RequestQueue/RequestStore::RemoveRequests
* Converting implementation to Task based: RemoveRequestsTask with tests.
* Removing most of old callbacks and typedefs

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

to

==========
[Offline pages] Updating RequestQueue::RemoveRequests to use a TaskQueue

* Switching the interface of RequestQueue/RequestStore::RemoveRequests
* Converting implementation to Task based: RemoveRequestsTask with tests.
* Removing most of old callbacks and typedefs

BUG=645522,651238

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

[commit-bot: I haz the power, 2016-10-06 06:23:50]

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