Provide API in RequestCoordinator to remove results by client ID.

Provide API in RequestCoordinator to remove results by client ID.

Since the RequestQueue supports removing by request_id instead of
by client_id, I also added a new method to the RequestQueue to
remove by a list of client_ids.

BUG=610521
Committed: https://crrev.com/d01a2d6450812cc7f91a867621cc22e1b1f82727
Cr-Commit-Position: refs/heads/master@{#409617}

[Pete Williamson, 2016-07-29 17:49:00]

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

[Pete Williamson, 2016-07-29 17:49:01]

(DewittJ is optional)

[dougarnett, 2016-07-29 19:01:24]

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

https://codereview.chromium.org/2197573003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:54: // request queue,
but does not cancel an in-progress pre-render.
interesting question about how to deal with a pending request. Canceling would
be clear but if we don't, what happens when request completes and does not find
its original request?

[I think I favor this just being a queue operation and graceful handling of
missing request]

https://codereview.chromium.org/2197573003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:55: void
CancelRequests(const std::vector<ClientId>& client_ids);
RemoveRequests more accurate since removed requests could already be complete?

[Pete Williamson, 2016-07-29 22:48:52]

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

https://codereview.chromium.org/2197573003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:54: // request queue,
but does not cancel an in-progress pre-render.
On 2016/07/29 19:01:23, dougarnett wrote:
> interesting question about how to deal with a pending request. Canceling would
> be clear but if we don't, what happens when request completes and does not
find
> its original request?
> 
> [I think I favor this just being a queue operation and graceful handling of
> missing request]

Per offline discussion, let's leave this as is, and later add code to cancel the
pending request.  TODO added.

https://codereview.chromium.org/2197573003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:55: void
CancelRequests(const std::vector<ClientId>& client_ids);
On 2016/07/29 19:01:24, dougarnett wrote:
> RemoveRequests more accurate since removed requests could already be complete?
 

Done.

[dougarnett, 2016-08-01 18:28:35]

just nits

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:73: // If the client
does not need the pages after all, then cancel them.
drop comment here - just keep .h comment up-to-date?

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:53: // Cancel a list
of requests by |client_id|.  This removes requests from the
Remove ...

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.cc:138: // Removes the request
matching each id in |client_ids|. Result is returned
drop method comment from .cc?

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:270: if
(DeleteRequestsByClientIds(db, client_ids, &count))
Some naming inconsistencies here with Remove/Delete and ByClientId/ByClientIds.
I wonder if at least try to make the ByClientId[s] part consistent.

[chili, 2016-08-01 19:34:40]

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:29: void
RemoveRequests(const std::vector<int64_t>& request_ids,
just out of curiosity - are there plans to expose 'remove requests by request
id' as well?

[Dmitry Titov, 2016-08-01 19:37:27]

dimich@chromium.org changed reviewers:
+ dimich@chromium.org

[Dmitry Titov, 2016-08-01 19:37:32]

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:56: void
RemoveRequests(const std::vector<ClientId>& client_ids);
Can we add a callback? It's an async operation, and UI bridge has to provide an
observer call to UI when the item is actually removed for consistent update.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:83: const
UpdateRequestCallback& callback);
formatting - whitespace.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:60: // In
case the |client_ids| is empty, the result will be true, but the count
If this comment is moved to .h file, it'll be easier to find. It is 'interface'
kind of comment, so probably belongs to .h

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store.h:58: // Result of
remove should be false, when one of the provided items couldn't
This is better reconciled with the comment from .cc file that says if the ids
are empty, it returns 'true' (since this is ambiguous w/o explicit mention).

[Pete Williamson, 2016-08-01 23:23:08]

CR feedback for DougArnett, Chili, and Dimich

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:73: // If the client
does not need the pages after all, then cancel them.
On 2016/08/01 18:28:35, dougarnett wrote:
> drop comment here - just keep .h comment up-to-date?

Done.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:53: // Cancel a list
of requests by |client_id|.  This removes requests from the
On 2016/08/01 18:28:35, dougarnett wrote:
> Remove ...

Done.

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:56: void
RemoveRequests(const std::vector<ClientId>& client_ids);
On 2016/08/01 19:37:29, Dmitry Titov wrote:
> Can we add a callback? It's an async operation, and UI bridge has to provide
an
> observer call to UI when the item is actually removed for consistent update.

As we discussed offline, we are providing an observer we can use instead.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.cc:138: // Removes the request
matching each id in |client_ids|. Result is returned
On 2016/08/01 18:28:35, dougarnett wrote:
> drop method comment from .cc?

Done.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue.h:83: const
UpdateRequestCallback& callback);
On 2016/08/01 19:37:30, Dmitry Titov wrote:
> formatting - whitespace.

Done.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:60: // In
case the |client_ids| is empty, the result will be true, but the count
On 2016/08/01 19:37:31, Dmitry Titov wrote:
> If this comment is moved to .h file, it'll be easier to find. It is
'interface'
> kind of comment, so probably belongs to .h

Done.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:29: void
RemoveRequests(const std::vector<int64_t>& request_ids,
On 2016/08/01 19:34:40, chili wrote:
> just out of curiosity - are there plans to expose 'remove requests by request
> id' as well?

It is already exposed, RemoveReqeusts removes them by request id.
However, I am considering removing it in a future change, since we don't
actually use it.  It turns out that its utility is limited by the fact that the
request coordinator doesn't keep the request ids around, and its clients don't
use them either.

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store.h:58: // Result of
remove should be false, when one of the provided items couldn't
On 2016/08/01 19:37:31, Dmitry Titov wrote:
> This is better reconciled with the comment from .cc file that says if the ids
> are empty, it returns 'true' (since this is ambiguous w/o explicit mention).

Done.

[Pete Williamson, 2016-08-01 23:23:38]

Oops, this time with the changed code.

[Dmitry Titov, 2016-08-01 23:38:19]

lgtm

[dougarnett, 2016-08-02 00:12:39]

lgtm with one remaining nit to consider

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:270: if
(DeleteRequestsByClientIds(db, client_ids, &count))
On 2016/08/01 18:28:35, dougarnett wrote:
> Some naming inconsistencies here with Remove/Delete and
ByClientId/ByClientIds.
> I wonder if at least try to make the ByClientId[s] part consistent.

That is, either never *"ByClientIds" or else make RequestQueue
RemoveRequestsByClientId() => RemoveRequestsByClientIds() to match style here

[Pete Williamson, 2016-08-02 00:35:08]

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

https://codereview.chromium.org/2197573003/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:270: if
(DeleteRequestsByClientIds(db, client_ids, &count))
On 2016/08/02 00:12:39, dougarnett wrote:
> On 2016/08/01 18:28:35, dougarnett wrote:
> > Some naming inconsistencies here with Remove/Delete and
> ByClientId/ByClientIds.
> > I wonder if at least try to make the ByClientId[s] part consistent.
> 
> That is, either never *"ByClientIds" or else make RequestQueue
> RemoveRequestsByClientId() => RemoveRequestsByClientIds() to match style here

My plan here is to just remove the "RemoveRequests" in a future checkin.  TODO
added (but I didn't rename it, since it will be going away).

[fgorski, 2016-08-02 05:01:23]

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:75: 
nit: remove empty line.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:61: for
(auto client_id : client_ids) {
const auto&

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:65: ++iter)
{
(I am aware this is only used for testing, but consider the following)

There are at least 2 problems with this code:
* It does not work for non-unique ClientID
* It is O(m*n)

How about:
1. putting client IDs into a <set>
2. iterate over requests_ with a proper pre- and post- increment?

for (auto iter = requests_.begin(); iter != requests_.end(); ) {
  if (client_id_set.find(iter->second.client_id()) != client_id_set.end()) {
    requests_.erase(iter++);
    ++count;
  } else {
    ++iter;
  }
}

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store.h:52: virtual void
RemoveRequests(const std::vector<int64_t>& request_ids,
thought: should we rename this to  RemoveRequestByRequestId?
WDYT?

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:59: bool
DeleteRequestByClientId(sql::Connection* db, const ClientId client_id) {
const& client_id

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:95: const
std::vector<ClientId>& client_ids,
nit: alignment issues. Please run git cl format.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:105: for (auto
client_id : client_ids) {
const auto&

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:261: 
nit: remove empty line.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:325: //
RemoveRequestsByRequestId to be more parallell with RemoveRequestsByClientId.
cool. Only saw it now.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:295: 
nit: remove empty line.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:296:
TYPED_TEST(RequestQueueStoreTest, RemoveRequestByClientId) {
I recommend testing with more than a single item, and as per earlier comments,
there are 2 variants of that:
* 2 different client IDs
* 2 requests with the same client ID.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:307: // For
debugging only, remove next code paragraph before checkin.
hmmm.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:332: //
Removing a request that is missing fails.
wow. Your copy-paste-fu is no good :)
Please fix the comment here and above. It is not  a failure, simply nothing to
delete.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:343: 
nit: empty line.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_unittest.cc:214: 
nit: empty line is not needed.

[Pete Williamson, 2016-08-02 23:27:17]

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:75: 
On 2016/08/02 05:01:22, fgorski wrote:
> nit: remove empty line.

Done.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:61: for
(auto client_id : client_ids) {
On 2016/08/02 05:01:22, fgorski wrote:
> const auto&

Done.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:65: ++iter)
{
On 2016/08/02 05:01:22, fgorski wrote:
> (I am aware this is only used for testing, but consider the following)
> 
> There are at least 2 problems with this code:
> * It does not work for non-unique ClientID
> * It is O(m*n)
> 
> How about:
> 1. putting client IDs into a <set>
> 2. iterate over requests_ with a proper pre- and post- increment?
> 
> for (auto iter = requests_.begin(); iter != requests_.end(); ) {
>   if (client_id_set.find(iter->second.client_id()) != client_id_set.end()) {
>     requests_.erase(iter++);
>     ++count;
>   } else {
>     ++iter;
>   }
> }

OK, but I had to add operator< to ClientID to make it work in a set.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store.h:52: virtual void
RemoveRequests(const std::vector<int64_t>& request_ids,
On 2016/08/02 05:01:22, fgorski wrote:
> thought: should we rename this to  RemoveRequestByRequestId?
> WDYT?

I think it is not being used, and I should just remove it in a follow on checkin
(there is already a TODO to do this.).  Since I'm removing it, it doesn't seem
worthwhile to rename it.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:59: bool
DeleteRequestByClientId(sql::Connection* db, const ClientId client_id) {
On 2016/08/02 05:01:23, fgorski wrote:
> const& client_id

Done.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:95: const
std::vector<ClientId>& client_ids,
On 2016/08/02 05:01:23, fgorski wrote:
> nit: alignment issues. Please run git cl format.

Done.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:105: for (auto
client_id : client_ids) {
On 2016/08/02 05:01:23, fgorski wrote:
> const auto&

Done.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:261: 
On 2016/08/02 05:01:23, fgorski wrote:
> nit: remove empty line.

Done.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:325: //
RemoveRequestsByRequestId to be more parallell with RemoveRequestsByClientId.
On 2016/08/02 05:01:22, fgorski wrote:
> cool. Only saw it now.

Acknowledged.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:295: 
On 2016/08/02 05:01:23, fgorski wrote:
> nit: remove empty line.

Done.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:296:
TYPED_TEST(RequestQueueStoreTest, RemoveRequestByClientId) {
On 2016/08/02 05:01:23, fgorski wrote:
> I recommend testing with more than a single item, and as per earlier comments,
> there are 2 variants of that:
> * 2 different client IDs
> * 2 requests with the same client ID.

Added new test, made this one take two different ids.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:307: // For
debugging only, remove next code paragraph before checkin.
On 2016/08/02 05:01:23, fgorski wrote:
> hmmm.

Removed (normally I use a TODO so I can spot these in the self-review process,
but I missed it this time.)  Thanks for the catch!

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:332: //
Removing a request that is missing fails.
On 2016/08/02 05:01:23, fgorski wrote:
> wow. Your copy-paste-fu is no good :)
> Please fix the comment here and above. It is not  a failure, simply nothing to
> delete.

Done.

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store_unittest.cc:343: 
On 2016/08/02 05:01:23, fgorski wrote:
> nit: empty line.

Done.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_unittest.cc:214: 
On 2016/08/02 05:01:23, fgorski wrote:
> nit: empty line is not needed.

Done.

[fgorski, 2016-08-03 03:29:41]

lgtm with nits.

Alternative to set is unordered_set.

I wonder if we should add a test to the clientID.
I don't see an error in your code, but I tested it on paper only.

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:65: ++iter)
{
On 2016/08/02 23:27:17, Pete Williamson wrote:
> On 2016/08/02 05:01:22, fgorski wrote:
> > (I am aware this is only used for testing, but consider the following)
> > 
> > There are at least 2 problems with this code:
> > * It does not work for non-unique ClientID
> > * It is O(m*n)
> > 
> > How about:
> > 1. putting client IDs into a <set>
> > 2. iterate over requests_ with a proper pre- and post- increment?
> > 
> > for (auto iter = requests_.begin(); iter != requests_.end(); ) {
> >   if (client_id_set.find(iter->second.client_id()) != client_id_set.end()) {
> >     requests_.erase(iter++);
> >     ++count;
> >   } else {
> >     ++iter;
> >   }
> > }
> 
> OK, but I had to add operator< to ClientID to make it work in a set.

That is because it is backed by BST. Have you tried unordered_set?

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_store.h:52: virtual void
RemoveRequests(const std::vector<int64_t>& request_ids,
On 2016/08/02 23:27:17, Pete Williamson wrote:
> On 2016/08/02 05:01:22, fgorski wrote:
> > thought: should we rename this to  RemoveRequestByRequestId?
> > WDYT?
> 
> I think it is not being used, and I should just remove it in a follow on
checkin
> (there is already a TODO to do this.).  Since I'm removing it, it doesn't seem
> worthwhile to rename it.

Acknowledged.

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.cc
(right):

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:71: 
nit: remove 2 emtpy lines.

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:135: <<
kUserRequested;
nit: do you intentionally print out a const here?

[Pete Williamson, 2016-08-03 19:51:09]

Post L G T M with nits replies

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

https://codereview.chromium.org/2197573003/diff/60001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:65: ++iter)
{
On 2016/08/03 03:29:41, fgorski wrote:
> On 2016/08/02 23:27:17, Pete Williamson wrote:
> > On 2016/08/02 05:01:22, fgorski wrote:
> > > (I am aware this is only used for testing, but consider the following)
> > > 
> > > There are at least 2 problems with this code:
> > > * It does not work for non-unique ClientID
> > > * It is O(m*n)
> > > 
> > > How about:
> > > 1. putting client IDs into a <set>
> > > 2. iterate over requests_ with a proper pre- and post- increment?
> > > 
> > > for (auto iter = requests_.begin(); iter != requests_.end(); ) {
> > >   if (client_id_set.find(iter->second.client_id()) != client_id_set.end())
{
> > >     requests_.erase(iter++);
> > >     ++count;
> > >   } else {
> > >     ++iter;
> > >   }
> > > }
> > 
> > OK, but I had to add operator< to ClientID to make it work in a set.
> 
> That is because it is backed by BST. Have you tried unordered_set?

I think it might be faster as an ordered set - using the ordering, it might be
able to search faster, we could (in theory, depending on implementation) end up
as O(M*log(N)) instead of O(M*N)

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.cc
(right):

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.cc:71: 
On 2016/08/03 03:29:41, fgorski wrote:
> nit: remove 2 emtpy lines.

Done.

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2197573003/diff/80001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:135: <<
kUserRequested;
On 2016/08/03 03:29:41, fgorski wrote:
> nit: do you intentionally print out a const here?

Not sure what you mean.  I am intentionally printing out a request that we are
adding to the queue if debug logging level 2 is turned on, so we know what is
going into the queue.

[Pete Williamson, 2016-08-03 19:51:17]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-08-03 19:51:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org, dimich@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2197573003/#ps100001
(title: "Comment changes per feedback")

[commit-bot: I haz the power, 2016-08-03 19:52:16]

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-08-03 20:53:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-03 20:55:45]

Description was changed from

==========
Provide API in RequestCoordinator to remove results by client ID.

Since the RequestQueue supports removing by request_id instead of
by client_id, I also added a new method to the RequestQueue to
remove by a list of client_ids.

BUG=610521
==========

to

==========
Provide API in RequestCoordinator to remove results by client ID.

Since the RequestQueue supports removing by request_id instead of
by client_id, I also added a new method to the RequestQueue to
remove by a list of client_ids.

BUG=610521

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

[commit-bot: I haz the power, 2016-08-03 20:55:46]

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