[Offline pages] Adding GetRequestsTask to RequestQueue

[Offline pages] Adding GetRequestsTask to RequestQueue

This patch wraps the process of getting requests in a simple task
in order to ensure that getting requests can be serialized with
other tasks to prevent race conditions.

BUG=651238
Committed: https://crrev.com/298ed466951c8537b56a4a4a064ba47c59bc5993
Cr-Commit-Position: refs/heads/master@{#434760}

[fgorski, 2016-11-24 00:30:35]

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

[fgorski, 2016-11-24 00:30:36]

PTAL

[fgorski, 2016-11-28 17:03:05]

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

[fgorski, 2016-11-28 17:03:05]

Adding Doug.

[dougarnett, 2016-11-28 18:48:41]

lgtm

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

https://codereview.chromium.org/2517383008/diff/1/components/offline_pages/ba...
components/offline_pages/background/get_requests_task.h:27: // Step 1: Read the
reqests fromt he store.
from

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

https://codereview.chromium.org/2517383008/diff/1/components/offline_pages/ba...
components/offline_pages/background/get_requests_task_unittest.cc:119:
EXPECT_EQ(kRequestId1, last_requests().at(0)->request_id());
Is this a more strict check than necessary? That is, the store does not define
an ordering but these checks depend on the implemenation's ordering.

[fgorski, 2016-11-28 19:13:06]

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

[commit-bot: I haz the power, 2016-11-28 19:13:41]

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

[fgorski, 2016-11-28 19:14:57]

Addressed.

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

https://codereview.chromium.org/2517383008/diff/1/components/offline_pages/ba...
components/offline_pages/background/get_requests_task.h:27: // Step 1: Read the
reqests fromt he store.
On 2016/11/28 18:48:41, dougarnett wrote:
> from

Done.

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

https://codereview.chromium.org/2517383008/diff/1/components/offline_pages/ba...
components/offline_pages/background/get_requests_task_unittest.cc:119:
EXPECT_EQ(kRequestId1, last_requests().at(0)->request_id());
On 2016/11/28 18:48:41, dougarnett wrote:
> Is this a more strict check than necessary? That is, the store does not define
> an ordering but these checks depend on the implemenation's ordering.

Done. And yes, I was piggybacking on implementation details of in-memory store.

[commit-bot: I haz the power, 2016-11-28 20:15:22]

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

[commit-bot: I haz the power, 2016-11-28 20:15:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Pete Williamson, 2016-11-28 21:13:52]

lgtm

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

https://codereview.chromium.org/2517383008/diff/1/components/offline_pages/ba...
components/offline_pages/background/get_requests_task.h:28: void ReadRequest();
Good change, I like the idea of Step 1 having its own function instead of just
being "Run".  I think we should do this for the other tasks (but not in this
changelist).

[fgorski, 2016-11-28 21:38:38]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-11-28 21:38:38]

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/2517383008/#ps20001
(title: "Addressing CR feedback")

[commit-bot: I haz the power, 2016-11-28 21:40:07]

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

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480369118406340,
"parent_rev": "6d5fd258013f7693aad541cf2658b65ceab58a46", "commit_rev":
"07803b3f578498588bdfe83aae18425170ecd7be"}

[commit-bot: I haz the power, 2016-11-28 22:58:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-28 23:00:54]

Description was changed from

==========
[Offline pages] Adding GetRequestsTask to RequestQueue

This patch wraps the process of getting requests in a simple task
in order to ensure that getting requests can be serialized with
other tasks to prevent race conditions.

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

to

==========
[Offline pages] Adding GetRequestsTask to RequestQueue

This patch wraps the process of getting requests in a simple task
in order to ensure that getting requests can be serialized with
other tasks to prevent race conditions.

BUG=651238

Committed: https://crrev.com/298ed466951c8537b56a4a4a064ba47c59bc5993
Cr-Commit-Position: refs/heads/master@{#434760}
==========

[commit-bot: I haz the power, 2016-11-28 23:00:55]

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