Changes to fit better with the needs of the download manager.

Changes to fit better with the needs of the download manager.

I thought that the client would only have clientIDs available, so I
created a new set of RemoveRequestByClientId functions.  However, it
turns out that the client won't be persisting these either, so I am
now removing them

They were added in https://codereview.chromium.org/2197573003/

Also, the "Get" functions now return full SavePageRequests instead
of only clientIds.

The intended usage pattern is for a client to make a request then
forget about it.  When the client wants to do something with a
request, it will first need to make a query using the
GetQueuedRequest API, then it will refer to specific requests by
request_id.

I also made RequestQueue::RemoveRequest into RemoveRequests, since
both the higher and lower level functions take a vector of request ids,
to better match the calling code.

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

[Pete Williamson, 2016-08-09 18:15:46]

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

[commit-bot: I haz the power, 2016-08-09 18:16:23]

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-08-09 19:03:19]

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

[commit-bot: I haz the power, 2016-08-09 19:03:20]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2016-08-10 02:52:13]

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

[Dmitry Titov, 2016-08-10 02:52:14]

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

https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:61: typedef
base::Callback<void(const std::vector<SavePageRequest>&)>
this is a strange type. A reference to a vector of actual instances. I think it
will take a copy on each SavePageRequest to construct this, and then the
receiver has to copy the whole thing again.

does std::move() work on SavePageRequests? In other words, can receiver do this:
void myCallback(const std::vector<SavePageRequest>& requests) {
  std::vector<SavePageRequest> mystorage;
  mystorage.push_back(std::move(requests[0]));
  ...
so that there is no additional copying?

https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:65: void
GetQueuedRequests(const QueuedRequestCallback& callback);
Can we rename it to GetAllRequests? It does return all, and there are no other
requests other than queued...

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

https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:77: // Removes the requests
matching the |request_ids|. Result is returned through
This returns result from UpdateRequestResult. That enum assumes one request. Now
that there are many, perhaps enum should be changed and comment updated for
partial success/failure, as in: *some* ids did not map to existing requests.
Also, in that case we should wither delete all that do map, or don't delete
enything.

[Pete Williamson, 2016-08-10 21:15:25]

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

https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:61: typedef
base::Callback<void(const std::vector<SavePageRequest>&)>
On 2016/08/10 02:52:14, Dmitry Titov wrote:
> this is a strange type. A reference to a vector of actual instances. I think
it
> will take a copy on each SavePageRequest to construct this, and then the
> receiver has to copy the whole thing again.
> 
> does std::move() work on SavePageRequests? In other words, can receiver do
this:
> void myCallback(const std::vector<SavePageRequest>& requests) {
>   std::vector<SavePageRequest> mystorage;
>   mystorage.push_back(std::move(requests[0]));
>   ...
> so that there is no additional copying?

We copy the SavePageRequest items into the vector, then pass a reference to that
up through several levels of callbacks, so typically the reference is getting
copied, not the vector.  My plan was for the consumer at the top level to then
make another copy.  I don't think that std::move is safe here because there
isn't a smart pointer underlying it, the vector is just on the stack.

If you like, I could change this to return a smart pointer to a const vector of
SavePageResults, that would allow a caller to use std::move

https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:65: void
GetQueuedRequests(const QueuedRequestCallback& callback);
On 2016/08/10 02:52:14, Dmitry Titov wrote:
> Can we rename it to GetAllRequests? It does return all, and there are no other
> requests other than queued...

Done.

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

https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_queue.h:77: // Removes the requests
matching the |request_ids|. Result is returned through
On 2016/08/10 02:52:14, Dmitry Titov wrote:
> This returns result from UpdateRequestResult. That enum assumes one request.
Now
> that there are many, perhaps enum should be changed and comment updated for
> partial success/failure, as in: *some* ids did not map to existing requests.
> Also, in that case we should wither delete all that do map, or don't delete
> enything. 

OK, changed the callback to return an array of statuses (along with a matching
array of request ids) so the client can tell what didn't get removed, if they
care.  The client will also be getting notifications eventually for items that
do get removed.

[Dmitry Titov, 2016-08-11 04:17:03]

Almost there!

On 2016/08/10 21:15:25, Pete Williamson wrote:
>
https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
> File components/offline_pages/background/request_coordinator.h (right):
> 
>
https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
> components/offline_pages/background/request_coordinator.h:61: typedef
> base::Callback<void(const std::vector<SavePageRequest>&)>
> On 2016/08/10 02:52:14, Dmitry Titov wrote:
> > this is a strange type. A reference to a vector of actual instances. I think
> it
> > will take a copy on each SavePageRequest to construct this, and then the
> > receiver has to copy the whole thing again.
> > 
> > does std::move() work on SavePageRequests? In other words, can receiver do
> this:
> > void myCallback(const std::vector<SavePageRequest>& requests) {
> >   std::vector<SavePageRequest> mystorage;
> >   mystorage.push_back(std::move(requests[0]));
> >   ...
> > so that there is no additional copying?
> 
> We copy the SavePageRequest items into the vector, then pass a reference to
that
> up through several levels of callbacks, so typically the reference is getting
> copied, not the vector.  My plan was for the consumer at the top level to then
> make another copy.  I don't think that std::move is safe here because there
> isn't a smart pointer underlying it, the vector is just on the stack.
> 
> If you like, I could change this to return a smart pointer to a const vector
of
> SavePageResults, that would allow a caller to use std::move

Since GetAllRequests creates a list of SavePageRequest objects (it doesn't have
in-memory list to just return) and passes that list back to caller for the
caller to either assume the ownership of SavePageRequests or discard them, the
best pattern I know is to pass

const std::vector<std::unique_ptr<SavePageRequest>>& 

This way, it is very possible to avoid multiple copies and it is safe since each
instance of SavePageRequest is owned by std::unique_ptr. 
std::move can be then used, per-request, to assume the ownership w/o copying.

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

https://codereview.chromium.org/2228813003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:130: const
std::vector<RequestQueue::UpdateRequestResult>& results);
It's better to return a single vector of pairs.

[Pete Williamson, 2016-08-12 00:00:40]

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

https://codereview.chromium.org/2228813003/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:61: typedef
base::Callback<void(const std::vector<SavePageRequest>&)>
On 2016/08/10 21:15:25, Pete Williamson wrote:
> On 2016/08/10 02:52:14, Dmitry Titov wrote:
> > this is a strange type. A reference to a vector of actual instances. I think
> it
> > will take a copy on each SavePageRequest to construct this, and then the
> > receiver has to copy the whole thing again.
> > 
> > does std::move() work on SavePageRequests? In other words, can receiver do
> this:
> > void myCallback(const std::vector<SavePageRequest>& requests) {
> >   std::vector<SavePageRequest> mystorage;
> >   mystorage.push_back(std::move(requests[0]));
> >   ...
> > so that there is no additional copying?
> 
> We copy the SavePageRequest items into the vector, then pass a reference to
that
> up through several levels of callbacks, so typically the reference is getting
> copied, not the vector.  My plan was for the consumer at the top level to then
> make another copy.  I don't think that std::move is safe here because there
> isn't a smart pointer underlying it, the vector is just on the stack.
> 
> If you like, I could change this to return a smart pointer to a const vector
of
> SavePageResults, that would allow a caller to use std::move

As we dicussed, we'll leave this behavior in for now.  crbug.com/637077 tracks
fixing the copying for both this and OfflinePageItem.

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

https://codereview.chromium.org/2228813003/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:130: const
std::vector<RequestQueue::UpdateRequestResult>& results);
On 2016/08/11 04:17:03, Dmitry Titov wrote:
> It's better to return a single vector of pairs.

Done.

[Dmitry Titov, 2016-08-12 00:18:17]

lgtm with a nit to fix before landing:

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

https://codereview.chromium.org/2228813003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:129: const
std::vector<std::pair<int64_t, RequestQueue::UpdateRequestResult>>&
Lets have a typedef for 
std::vector<std::pair<int64_t, RequestQueue::UpdateRequestResult>>
It will be much more readable with that.
UpdateRequestResultMultiple? Declared in RequestQueue.h

[Pete Williamson, 2016-08-12 00:53:43]

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

https://codereview.chromium.org/2228813003/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.h:129: const
std::vector<std::pair<int64_t, RequestQueue::UpdateRequestResult>>&
On 2016/08/12 00:18:17, Dmitry Titov wrote:
> Lets have a typedef for 
> std::vector<std::pair<int64_t, RequestQueue::UpdateRequestResult>>
> It will be much more readable with that.
> UpdateRequestResultMultiple? Declared in RequestQueue.h

Done.

[Pete Williamson, 2016-08-12 00:53:54]

The CQ bit was checked by petewil@chromium.org

[Pete Williamson, 2016-08-12 00:53:55]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2228813003/#ps60001
(title: "Use typedef to hide an ugly type.")

[commit-bot: I haz the power, 2016-08-12 00:54:13]

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-12 02:03:00]

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

[commit-bot: I haz the power, 2016-08-12 02:03:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Pete Williamson, 2016-08-12 02:06:37]

The CQ bit was checked by petewil@chromium.org

[commit-bot: I haz the power, 2016-08-12 02:07:18]

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-12 03:05:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-12 03:06:31]

Description was changed from

==========
Changes to fit better with the needs of the download manager.

I thought that the client would only have clientIDs available, so I
created a new set of RemoveRequestByClientId functions.  However, it
turns out that the client won't be persisting these either, so I am
now removing them

They were added in https://codereview.chromium.org/2197573003/

Also, the "Get" functions now return full SavePageRequests instead
of only clientIds.

The intended usage pattern is for a client to make a request then
forget about it.  When the client wants to do something with a
request, it will first need to make a query using the
GetQueuedRequest API, then it will refer to specific requests by
request_id.

I also made RequestQueue::RemoveRequest into RemoveRequests, since
both the higher and lower level functions take a vector of request ids,
to better match the calling code.

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

to

==========
Changes to fit better with the needs of the download manager.

I thought that the client would only have clientIDs available, so I
created a new set of RemoveRequestByClientId functions.  However, it
turns out that the client won't be persisting these either, so I am
now removing them

They were added in https://codereview.chromium.org/2197573003/

Also, the "Get" functions now return full SavePageRequests instead
of only clientIds.

The intended usage pattern is for a client to make a request then
forget about it.  When the client wants to do something with a
request, it will first need to make a query using the
GetQueuedRequest API, then it will refer to specific requests by
request_id.

I also made RequestQueue::RemoveRequest into RemoveRequests, since
both the higher and lower level functions take a vector of request ids,
to better match the calling code.

BUG=610521

Committed: https://crrev.com/5f04432056986c94d2d5cf325a9addec96631547
Cr-Commit-Position: refs/heads/master@{#411533}
==========

[commit-bot: I haz the power, 2016-08-12 03:06:33]

Patchset 4 (id:??) landed as
https://crrev.com/5f04432056986c94d2d5cf325a9addec96631547
Cr-Commit-Position: refs/heads/master@{#411533}