[Offline pages] Adding capability to query a subset of requests based on IDs

[Offline pages] Adding capability to query a subset of requests based on IDs

* Adding GetRequestsByIds to RequestQueueStore and its implementations
* Adding tests for GetRequestsByIds.

BUG=630657
Committed: https://crrev.com/906dee0ebd4beaa745d1ac1b425d2d11cf41c2d4
Cr-Commit-Position: refs/heads/master@{#425071}

[fgorski, 2016-10-11 21:11:40]

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

[fgorski, 2016-10-11 21:11:40]

Good news: you might like this patch ;)

[fgorski, 2016-10-11 21:11:50]

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

[commit-bot: I haz the power, 2016-10-11 21:12:05]

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-11 22:15:59]

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

[commit-bot: I haz the power, 2016-10-11 22:16:00]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-10-11 22:16:22]

And I do like it!

It might be even cooler if the patch also changed the request coordinator to use
the new method when updating a single item (which was, as I recall, the
motivating use case).  However, if that is coming with the
MarkRequestStarted()... family of changes, I'm happy to wait.

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:35: const
UpdateCallback& callback) {
I think the name of the callback should have Get in it instead of Update. 
Alternatively, UpdateCallback could be renamed to something more generic.

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:42: if
(!processed_ids.insert(request_id).second)
Why do we keep a list of processed ids?  Maybe add the same comment you had in
the SQL file.

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:43: virtual void
GetRequestsByIds(const std::vector<int64_t>& request_ids,
I'm not sure it makes sense to have something getting more than one request.  If
the user needs more than one, they will likely do filtering anyway, and it would
be more efficient to have a single SQL query than many SQL queries, one for each
item.

[fgorski, 2016-10-12 20:51:29]

Responded to comments.

I'll handle renaming the UpdateCallback separately (there is more thinking that
needs to happen there.

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:35: const
UpdateCallback& callback) {
On 2016/10/11 22:16:21, Pete Williamson wrote:
> I think the name of the callback should have Get in it instead of Update. 
> Alternatively, UpdateCallback could be renamed to something more generic.

Yes, but I'd prefer to do that separately.

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:42: if
(!processed_ids.insert(request_id).second)
On 2016/10/11 22:16:21, Pete Williamson wrote:
> Why do we keep a list of processed ids?  Maybe add the same comment you had in
> the SQL file.

Defensive programming for cases where someone specified the same ID multiple
times. This code prevents that.

This is explicitly commented on SQL side:
// Make sure not to include the same request multiple times, preserving the
// order of non-duplicated IDs in the result.

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:43: virtual void
GetRequestsByIds(const std::vector<int64_t>& request_ids,
On 2016/10/11 22:16:21, Pete Williamson wrote:
> I'm not sure it makes sense to have something getting more than one request. 
If
> the user needs more than one, they will likely do filtering anyway, and it
would
> be more efficient to have a single SQL query than many SQL queries, one for
each
> item.

This is actually what we already need:
https://cs.chromium.org/chromium/src/components/offline_pages/background/chan...

[Pete Williamson, 2016-10-12 22:23:36]

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:42: if
(!processed_ids.insert(request_id).second)
On 2016/10/12 20:51:29, fgorski wrote:
> On 2016/10/11 22:16:21, Pete Williamson wrote:
> > Why do we keep a list of processed ids?  Maybe add the same comment you had
in
> > the SQL file.
> 
> Defensive programming for cases where someone specified the same ID multiple
> times. This code prevents that.
> 
> This is explicitly commented on SQL side:
> // Make sure not to include the same request multiple times, preserving the
> // order of non-duplicated IDs in the result.

Right, I understand why, I just think we should add the comment here too.

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:43: virtual void
GetRequestsByIds(const std::vector<int64_t>& request_ids,
On 2016/10/12 20:51:29, fgorski wrote:
> On 2016/10/11 22:16:21, Pete Williamson wrote:
> > I'm not sure it makes sense to have something getting more than one request.

> If
> > the user needs more than one, they will likely do filtering anyway, and it
> would
> > be more efficient to have a single SQL query than many SQL queries, one for
> each
> > item.
> 
> This is actually what we already need:
>
https://cs.chromium.org/chromium/src/components/offline_pages/background/chan...
> 

In that case, should the changerequeststate task be calling the regular get
instead if there are many entries so we only do one SQL query?  Or should we do
a single SQL query for an any item with an id in a set?  It seems non-performant
to do many SQL queries where one would suffice.

[fgorski, 2016-10-12 23:03:52]

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

[commit-bot: I haz the power, 2016-10-12 23:04:25]

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-12 23:05:30]

Addressed.

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_in_memory_store.cc:42: if
(!processed_ids.insert(request_id).second)
On 2016/10/12 22:23:36, Pete Williamson wrote:
> On 2016/10/12 20:51:29, fgorski wrote:
> > On 2016/10/11 22:16:21, Pete Williamson wrote:
> > > Why do we keep a list of processed ids?  Maybe add the same comment you
had
> in
> > > the SQL file.
> > 
> > Defensive programming for cases where someone specified the same ID multiple
> > times. This code prevents that.
> > 
> > This is explicitly commented on SQL side:
> > // Make sure not to include the same request multiple times, preserving the
> > // order of non-duplicated IDs in the result.
> 
> Right, I understand why, I just think we should add the comment here too.

Done. Sorry, I misread your comment.

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

https://codereview.chromium.org/2410903003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue_store.h:43: virtual void
GetRequestsByIds(const std::vector<int64_t>& request_ids,
On 2016/10/12 22:23:36, Pete Williamson wrote:
> On 2016/10/12 20:51:29, fgorski wrote:
> > On 2016/10/11 22:16:21, Pete Williamson wrote:
> > > I'm not sure it makes sense to have something getting more than one
request.
> 
> > If
> > > the user needs more than one, they will likely do filtering anyway, and it
> > would
> > > be more efficient to have a single SQL query than many SQL queries, one
for
> > each
> > > item.
> > 
> > This is actually what we already need:
> >
>
https://cs.chromium.org/chromium/src/components/offline_pages/background/chan...
> > 
> 
> In that case, should the changerequeststate task be calling the regular get
> instead if there are many entries so we only do one SQL query?  Or should we
do
> a single SQL query for an any item with an id in a set?  It seems
non-performant
> to do many SQL queries where one would suffice.

Done. added in _sql.h, as that is where we are pothered by this

[Pete Williamson, 2016-10-12 23:13:01]

lgtm

[commit-bot: I haz the power, 2016-10-12 23:51:00]

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

[commit-bot: I haz the power, 2016-10-12 23:51:01]

Dry run: This issue passed the CQ dry run.

[fgorski, 2016-10-13 17:03:59]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-10-13 17:04:19]

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-13 17:11:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-13 17:14:40]

Description was changed from

==========
[Offline pages] Adding capability to query a subset of requests based on IDs

* Adding GetRequestsByIds to RequestQueueStore and its implementations
* Adding tests for GetRequestsByIds.

BUG=630657
==========

to

==========
[Offline pages] Adding capability to query a subset of requests based on IDs

* Adding GetRequestsByIds to RequestQueueStore and its implementations
* Adding tests for GetRequestsByIds.

BUG=630657

Committed: https://crrev.com/906dee0ebd4beaa745d1ac1b425d2d11cf41c2d4
Cr-Commit-Position: refs/heads/master@{#425071}
==========

[commit-bot: I haz the power, 2016-10-13 17:14:42]

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