[Offline pages] Adding in memory request queue store with tests

[Offline pages] Adding in memory request queue store with tests

* Creates interface for RequestQueueStore
* Creates a basic in memory implementation of the store
* Creates tests for implementation
* Adds a copy constructor to SavePageRequest (implicitly tested
  in request queue store implementation).

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org

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

[fgorski, 2016-05-03 18:43:27]

PTAL

[Pete Williamson, 2016-05-03 20:38:53]

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:18: void
RequestQueueInMemoryStore::GetRequests(
In the future, it might be nice to be able to search requests by some condition
(newest first, oldest first, least retried first).  No action needed now.

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:24:
FROM_HERE, base::Bind(callback, true, result));
Would it make sense to batch these?  This seems like a lot of async calls, it
might be inefficient.

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:42: //
least one request.
So how will the caller know when the list was empty and when at least one got
deleted?  It might be better to return true only when we delete something.

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.h:27: void
AddOrUpdateRequest(const SavePageRequest& request,
Would it make sense for this to return a bool indicating if it was updating? 
Having said that, I can't immediately think of any use cases, but I have seen
code in the past where this info was important.

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:28: virtual void
GetRequests(const GetRequestsCallback& callback) = 0;
Why make this async again?  (Sorry if I already asked that and forgot the
answer.)  It seems that having this async will make the scheduling much harder
and less efficient.

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:36: // Result of the
update is passed in the callback.
Let's be a bit more specific - returns true if anything gets deleted.

[fgorski, 2016-05-03 21:50:21]

Addressed and also fixed build dependency.

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:18: void
RequestQueueInMemoryStore::GetRequests(
On 2016/05/03 20:38:52, Pete Williamson wrote:
> In the future, it might be nice to be able to search requests by some
condition
> (newest first, oldest first, least retried first).  No action needed now.

Acknowledged.

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:24:
FROM_HERE, base::Bind(callback, true, result));
On 2016/05/03 20:38:52, Pete Williamson wrote:
> Would it make sense to batch these?  This seems like a lot of async calls, it
> might be inefficient.

What is there to batch here exactly?
This method makes a single call with a vector containing results.

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:42: //
least one request.
On 2016/05/03 20:38:52, Pete Williamson wrote:
> So how will the caller know when the list was empty and when at least one got
> deleted?  It might be better to return true only when we delete something.

Done.

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.h:27: void
AddOrUpdateRequest(const SavePageRequest& request,
On 2016/05/03 20:38:52, Pete Williamson wrote:
> Would it make sense for this to return a bool indicating if it was updating? 
> Having said that, I can't immediately think of any use cases, but I have seen
> code in the past where this info was important.

Done.

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:28: virtual void
GetRequests(const GetRequestsCallback& callback) = 0;
On 2016/05/03 20:38:52, Pete Williamson wrote:
> Why make this async again?  (Sorry if I already asked that and forgot the
> answer.)  It seems that having this async will make the scheduling much harder
> and less efficient.

You know we need to read these from the drive either way, right?

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:36: // Result of the
update is passed in the callback.
On 2016/05/03 20:38:52, Pete Williamson wrote:
> Let's be a bit more specific - returns true if anything gets deleted.

Done.

[Pete Williamson, 2016-05-03 22:00:25]

lgtm

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:24:
FROM_HERE, base::Bind(callback, true, result));
On 2016/05/03 21:50:21, fgorski wrote:
> On 2016/05/03 20:38:52, Pete Williamson wrote:
> > Would it make sense to batch these?  This seems like a lot of async calls,
it
> > might be inefficient.
> 
> What is there to batch here exactly?
> This method makes a single call with a vector containing results.

Ah, never mind, I missed that the result was a vector.  Objections withdrawn.

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:28: virtual void
GetRequests(const GetRequestsCallback& callback) = 0;
On 2016/05/03 21:50:21, fgorski wrote:
> On 2016/05/03 20:38:52, Pete Williamson wrote:
> > Why make this async again?  (Sorry if I already asked that and forgot the
> > answer.)  It seems that having this async will make the scheduling much
harder
> > and less efficient.
> 
> You know we need to read these from the drive either way, right?

Ah yes, that was why.  This is OK as is.

[dougarnett, 2016-05-03 22:11:54]

lgtm

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:24:
FROM_HERE, base::Bind(callback, true, result));
On 2016/05/03 21:50:21, fgorski wrote:
> On 2016/05/03 20:38:52, Pete Williamson wrote:
> > Would it make sense to batch these?  This seems like a lot of async calls,
it
> > might be inefficient.
> 
> What is there to batch here exactly?
> This method makes a single call with a vector containing results.

perhaps if "result" was named "results", it would have been easier to read?

[fgorski, 2016-05-03 23:01:42]

Addressed. I updated the code per suggestion for readability. This is a good
catch and I should have done it earlier.

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

https://codereview.chromium.org/1951483002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:24:
FROM_HERE, base::Bind(callback, true, result));
On 2016/05/03 22:11:54, dougarnett wrote:
> On 2016/05/03 21:50:21, fgorski wrote:
> > On 2016/05/03 20:38:52, Pete Williamson wrote:
> > > Would it make sense to batch these?  This seems like a lot of async calls,
> it
> > > might be inefficient.
> > 
> > What is there to batch here exactly?
> > This method makes a single call with a vector containing results.
> 
> perhaps if "result" was named "results", it would have been easier to read?

I see. I updated this to result_requests. I hope it will be more readable now.

[fgorski, 2016-05-03 23:01:52]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-05-03 23:01:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/1951483002/#ps40001
(title: "Udpating name of variable to make code more readable")

[commit-bot: I haz the power, 2016-05-03 23:02:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951483002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1951483002/40001

[commit-bot: I haz the power, 2016-05-04 00:06:41]

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

[commit-bot: I haz the power, 2016-05-04 00:06:43]

Failed to apply the patch.

[fgorski, 2016-05-04 03:52:44]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-05-04 03:52:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951483002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1951483002/40001

[commit-bot: I haz the power, 2016-05-04 03:56:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-04 03:57:51]

Description was changed from

==========
[Offline pages] Adding in memory request queue store with tests

* Creates interface for RequestQueueStore
* Creates a basic in memory implementation of the store
* Creates tests for implementation
* Adds a copy constructor to SavePageRequest (implicitly tested
  in request queue store implementation).

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org
==========

to

==========
[Offline pages] Adding in memory request queue store with tests

* Creates interface for RequestQueueStore
* Creates a basic in memory implementation of the store
* Creates tests for implementation
* Adds a copy constructor to SavePageRequest (implicitly tested
  in request queue store implementation).

BUG=607221
R=dougarnett@chromium.org,petewil@chromium.org

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

[commit-bot: I haz the power, 2016-05-04 03:57:52]

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