DownloadUrlParameters

Refactor DownloadManager::DownloadUrl() to allow it to take many parameters.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135403

[benjhayden, 2012-04-26 20:25:01]

PTAL

[jam, 2012-04-26 20:55:16]

http://codereview.chromium.org/10232010/diff/6/content/public/browser/downloa...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10232010/diff/6/content/public/browser/downloa...
content/public/browser/download_manager.h:76: class CONTENT_EXPORT
DownloadUrlParameters {
please put this in a separate file. in general for the content api we put every
struct/enum in a separate file (to match the WebKit API).

btw i'm curious why this is a class instead of struct?

[benjhayden, 2012-04-27 14:17:08]

http://codereview.chromium.org/10232010/diff/6/content/public/browser/downloa...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10232010/diff/6/content/public/browser/downloa...
content/public/browser/download_manager.h:76: class CONTENT_EXPORT
DownloadUrlParameters {
On 2012/04/26 20:55:16, John Abd-El-Malek wrote:
> please put this in a separate file. in general for the content api we put
every
> struct/enum in a separate file (to match the WebKit API).
> 
> btw i'm curious why this is a class instead of struct?

Some of the members are non-POD, and one (extra_headers) cannot be copied, so
DISALLOW_COPY_AND_ASSIGN seems like a good idea.

I'd like to only provide setters for optional parameters, and force users to
specify all required parameters in a ctor.

These small bits of smarts seem to push it just over the edge from struct to
class. Somehow struct didn't feel right. I'm open to switching it back to struct
if you really want.

[Randy Smith (Not in Mondays), 2012-04-27 18:40:41]

Very basic question, for both John and Ben: How would you folks feel about
shrinking the structure down to only those things having to do with the
URLRequest/network side of the download, and passing other info separately in
parameters to the DownloadUrl() function?  That would lead to a class with, to
me, more conceptual unity, and I think would still keep the number of parameters
to DownloadUrl() sane.  

Also, Ben: Did you skim the code to see if any other creators of URLRequest had
run into this problem and how they solved it?  My guess is that we're unique
(well, *more* unique :-}) but I still think it'd be worth a little bit of time
(15 minutes?) scanning the code to see if there's any inspiration or copyable
patterns elsewhere.

[benjhayden, 2012-04-27 20:11:41]

On 2012/04/27 18:40:41, rdsmith wrote:
> Very basic question, for both John and Ben: How would you folks feel about
> shrinking the structure down to only those things having to do with the
> URLRequest/network side of the download, and passing other info separately in
> parameters to the DownloadUrl() function?  That would lead to a class with, to
> me, more conceptual unity, and I think would still keep the number of
parameters
> to DownloadUrl() sane.  

There's something special about the number one: after zero, it's the number that
is least likely to grow to an insane number.

Also, DownloadUrlParameters supports all valid permutations of parameters. If
you move the WebContents-derived triplet outside DownloadUrlParameters, then you
suddenly have two different forms of DownloadUrl() methods. If you move the
callback out of DUP, then either all callers must specify either a null callback
or a non-null callback, or you have four forms.

Conceptual unity is a good thing, but perhaps we can define that concept broadly
enough that DUP unifies it cleanly  as it is. That might sound like a cop out,
but bear with me.
DUP encapsulates your choices for how you can download a URL. Not all of the
parameters are orthogonal, and DUP encapsulates the interplay between them in
its interface. Under the covers, some of the parameters are stuffed in a
URLRequest and some aren't, but that's an implementation detail.

> 
> Also, Ben: Did you skim the code to see if any other creators of URLRequest
had
> run into this problem and how they solved it?  My guess is that we're unique
> (well, *more* unique :-}) but I still think it'd be worth a little bit of time
> (15 minutes?) scanning the code to see if there's any inspiration or copyable
> patterns elsewhere.

http://code.google.com/p/chromium/source/search?q=new%5C+net%3A%3AURLRequest%...

Most creators of URLRequest either call its Start() method themselves or pass it
to RDH.

Why does DM need to go through RDH?

[jam, 2012-04-27 21:05:56]

On 2012/04/27 14:17:08, benjhayden_chromium wrote:
>
http://codereview.chromium.org/10232010/diff/6/content/public/browser/downloa...
> File content/public/browser/download_manager.h (right):
> 
>
http://codereview.chromium.org/10232010/diff/6/content/public/browser/downloa...
> content/public/browser/download_manager.h:76: class CONTENT_EXPORT
> DownloadUrlParameters {
> On 2012/04/26 20:55:16, John Abd-El-Malek wrote:
> > please put this in a separate file. in general for the content api we put
> every
> > struct/enum in a separate file (to match the WebKit API).
> > 
> > btw i'm curious why this is a class instead of struct?
> 
> Some of the members are non-POD, and one (extra_headers) cannot be copied, so
> DISALLOW_COPY_AND_ASSIGN seems like a good idea.
> 
> I'd like to only provide setters for optional parameters, and force users to
> specify all required parameters in a ctor.
> 
> These small bits of smarts seem to push it just over the edge from struct to
> class. Somehow struct didn't feel right. I'm open to switching it back to
struct
> if you really want.

sgtm, fine as class

[jam, 2012-04-27 21:06:21]

On 2012/04/27 18:40:41, rdsmith wrote:
> Very basic question, for both John and Ben: How would you folks feel about
> shrinking the structure down to only those things having to do with the
> URLRequest/network side of the download, and passing other info separately in
> parameters to the DownloadUrl() function?  That would lead to a class with, to
> me, more conceptual unity, and I think would still keep the number of
parameters
> to DownloadUrl() sane.  

I don't particularly feel strongly about this, so up to you guys
> 
> Also, Ben: Did you skim the code to see if any other creators of URLRequest
had
> run into this problem and how they solved it?  My guess is that we're unique
> (well, *more* unique :-}) but I still think it'd be worth a little bit of time
> (15 minutes?) scanning the code to see if there's any inspiration or copyable
> patterns elsewhere.

[Randy Smith (Not in Mondays), 2012-04-30 19:44:47]

A more detailed review--sorry for the delay.

http://codereview.chromium.org/10232010/diff/17002/chrome/browser/tab_content...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/10232010/diff/17002/chrome/browser/tab_content...
chrome/browser/tab_contents/render_view_context_menu.cc:93: using
WebKit::WebURL;
nit: Why does WebKit go first?  It's not alphabetical?

http://codereview.chromium.org/10232010/diff/17002/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10232010/diff/17002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:109: }
I'm not sure I completely follow this format, but it looks complex for this
level fo the system--it looks more "extension-y".  Is there a reason not to have
a list or vector of pairs of std::string s, representing a list of header/value
pairs?

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
File content/public/browser/download_manager.h (left):

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_manager.h:64: typedef
base::Callback<void(DownloadId, net::Error)> OnStartedCallback;
Suggestion: Several of the files in this CL have only the change of getting
OnStartedCallback from DownloadUrlParameters instead of DownloadManager.  That's
suggestive that it's at least worth asking the question whether we should leave
the callback defined on DownloadManager (even if used in DownloadUrlParamters);
if there are several locations that only need to know about the callback, should
we be making them include the whole include file?

(It's up to you, but for what it's worth when I ask the question I lean towards
leaving it on DownloadManager, but not strongly.  Looking over the locations
that need it without DownloadUrlParameters, there are two categories: Test files
(which don't count), and download system implementation (which sorta count and
sorta don't).  On the other hand, I have no problem imagining a wider range of
consumers needing the callback than need all the nitty-gritty of the
parameters.)

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_manager.h:48: #include "net/base/net_errors.h"
Why are there so many include files (and other stuff) being added to this file
for the removal of a lot of things and a parameter that can be forward declared?

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_manager.h:177: // Takes ownership of
|parameters|.
nit, suggestion: "See DownloadUrlParameters for details about controlling the
download."?

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
File content/public/browser/download_url_parameters.h (right):

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:34: // |callback| will be
called when the download starts, or if an error occurs
nit: "If not null".

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:37: // the object isn't made.
Could you specify the format of the extra headers?

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:77: set_prefer_cache(false);
I'm a bit concerned about this violating principle of least astonishment as
it'll produce different results if the consumer calls
set_prefer_cache/set_post_id in different orders.  Can you arrange for
set_prefer_cache() to override no matter what the order is?

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:120: DownloadSaveInfo
save_info_;
Given the creation of DownloadUrlParameters, I'd like to take a look at getting
rid of DownloadSaveInfo.  It looks to me like DownloadSaveInfo is used in three
classes of places:
* At call sites into DownloadManager::DownloadUrl.  Easy to fold into
DownloadUrlParameters.
* At calls sites into ResourceDispatcherHost::BeginDownload.  Harder, but I'd
like to look at getting rid of this/making it only called from the download
system anyway.  
* In the construction and member variables of DownloadResourceHandler.  

Any urge to look at whether this refactor/removal makes sense?  (If you don't
want to I'll put it on my queue.)  Fewer classes is better, and DownloadSaveInfo
isn't seeming very useful anymore.  I don't think it makes sense for this CL,
but this CL does spark the thought.

[benjhayden, 2012-05-02 15:11:54]

PTAL
I made gmock play nice with scoped_ptr using the trick mentioned in the recent
thread about it.

http://codereview.chromium.org/10232010/diff/17002/chrome/browser/tab_content...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/10232010/diff/17002/chrome/browser/tab_content...
chrome/browser/tab_contents/render_view_context_menu.cc:93: using
WebKit::WebURL;
On 2012/04/30 19:44:48, rdsmith wrote:
> nit: Why does WebKit go first?  It's not alphabetical?

'W'=87
'c'=99

http://codereview.chromium.org/10232010/diff/17002/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10232010/diff/17002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:109: }
On 2012/04/30 19:44:48, rdsmith wrote:
> I'm not sure I completely follow this format, but it looks complex for this
> level fo the system--it looks more "extension-y".  Is there a reason not to
have
> a list or vector of pairs of std::string s, representing a list of
header/value
> pairs?

Done.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
File content/public/browser/download_manager.h (left):

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_manager.h:64: typedef
base::Callback<void(DownloadId, net::Error)> OnStartedCallback;
On 2012/04/30 19:44:48, rdsmith wrote:
> Suggestion: Several of the files in this CL have only the change of getting
> OnStartedCallback from DownloadUrlParameters instead of DownloadManager. 
That's
> suggestive that it's at least worth asking the question whether we should
leave
> the callback defined on DownloadManager (even if used in
DownloadUrlParamters);
> if there are several locations that only need to know about the callback,
should
> we be making them include the whole include file?
> 
> (It's up to you, but for what it's worth when I ask the question I lean
towards
> leaving it on DownloadManager, but not strongly.  Looking over the locations
> that need it without DownloadUrlParameters, there are two categories: Test
files
> (which don't count), and download system implementation (which sorta count and
> sorta don't).  On the other hand, I have no problem imagining a wider range of
> consumers needing the callback than need all the nitty-gritty of the
> parameters.)

It seems strange to define a callback in an interface that doesn't use it. It
seems like that might encourage throwing other stuff that doesn't belong
anywhere in DownloadManager.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_manager.h:48: #include "net/base/net_errors.h"
On 2012/04/30 19:44:48, rdsmith wrote:
> Why are there so many include files (and other stuff) being added to this file
> for the removal of a lot of things and a parameter that can be forward
declared?

Done.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_manager.h:177: // Takes ownership of
|parameters|.
On 2012/04/30 19:44:48, rdsmith wrote:
> nit, suggestion: "See DownloadUrlParameters for details about controlling the
> download."?

Done.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
File content/public/browser/download_url_parameters.h (right):

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:34: // |callback| will be
called when the download starts, or if an error occurs
On 2012/04/30 19:44:48, rdsmith wrote:
> nit: "If not null".

Done.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:37: // the object isn't made.
On 2012/04/30 19:44:48, rdsmith wrote:
> Could you specify the format of the extra headers?

PTAL

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:77: set_prefer_cache(false);
On 2012/04/30 19:44:48, rdsmith wrote:
> I'm a bit concerned about this violating principle of least astonishment as
> it'll produce different results if the consumer calls
> set_prefer_cache/set_post_id in different orders.  Can you arrange for
> set_prefer_cache() to override no matter what the order is?

You're right, this behavior was surprising, and it didn't even guarantee what it
was supposed to guarantee.
I was trying to guarantee the DCHECKs in BeginDownload(). If post_id>=0, then
method needs to be POST and prefer_cache needs to be true, regardless of what
the caller tries to do.
I removed this logic from set_post_id() and made method() and prefer_cache()
return "POST" and true if post_id>=0, regardless of
set_method()/set_prefer_cache().
This way, callers don't need to think about the relationship between
post_id/method/prefer_cache; that relationship is encapsulated here in DUP.
PTAL.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:120: DownloadSaveInfo
save_info_;
On 2012/04/30 19:44:48, rdsmith wrote:
> Given the creation of DownloadUrlParameters, I'd like to take a look at
getting
> rid of DownloadSaveInfo.  It looks to me like DownloadSaveInfo is used in
three
> classes of places:
> * At call sites into DownloadManager::DownloadUrl.  Easy to fold into
> DownloadUrlParameters.
> * At calls sites into ResourceDispatcherHost::BeginDownload.  Harder, but I'd
> like to look at getting rid of this/making it only called from the download
> system anyway.  
> * In the construction and member variables of DownloadResourceHandler.  
> 
> Any urge to look at whether this refactor/removal makes sense?  (If you don't
> want to I'll put it on my queue.)  Fewer classes is better, and
DownloadSaveInfo
> isn't seeming very useful anymore.  I don't think it makes sense for this CL,
> but this CL does spark the thought.

Agreed, that's definitely worth trying, but not for this CL. I'm happy to take
it.

[Randy Smith (Not in Mondays), 2012-05-02 18:36:21]

http://codereview.chromium.org/10232010/diff/17002/chrome/browser/tab_content...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/10232010/diff/17002/chrome/browser/tab_content...
chrome/browser/tab_contents/render_view_context_menu.cc:93: using
WebKit::WebURL;
On 2012/05/02 15:11:54, benjhayden_chromium wrote:
> On 2012/04/30 19:44:48, rdsmith wrote:
> > nit: Why does WebKit go first?  It's not alphabetical?
> 
> 'W'=87
> 'c'=99

Very true :-}.  Ok.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
File content/public/browser/download_url_parameters.h (right):

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:37: // the object isn't made.
On 2012/05/02 15:11:54, benjhayden_chromium wrote:
> On 2012/04/30 19:44:48, rdsmith wrote:
> > Could you specify the format of the extra headers?
> 
> PTAL

Looks good.

http://codereview.chromium.org/10232010/diff/17002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:120: DownloadSaveInfo
save_info_;
On 2012/05/02 15:11:54, benjhayden_chromium wrote:
> On 2012/04/30 19:44:48, rdsmith wrote:
> > Given the creation of DownloadUrlParameters, I'd like to take a look at
> getting
> > rid of DownloadSaveInfo.  It looks to me like DownloadSaveInfo is used in
> three
> > classes of places:
> > * At call sites into DownloadManager::DownloadUrl.  Easy to fold into
> > DownloadUrlParameters.
> > * At calls sites into ResourceDispatcherHost::BeginDownload.  Harder, but
I'd
> > like to look at getting rid of this/making it only called from the download
> > system anyway.  
> > * In the construction and member variables of DownloadResourceHandler.  
> > 
> > Any urge to look at whether this refactor/removal makes sense?  (If you
don't
> > want to I'll put it on my queue.)  Fewer classes is better, and
> DownloadSaveInfo
> > isn't seeming very useful anymore.  I don't think it makes sense for this
CL,
> > but this CL does spark the thought.
> 
> Agreed, that's definitely worth trying, but not for this CL. I'm happy to take
> it.

Awesome; it's yours.

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
content/public/browser/download_manager.h:59: class ResourceDispatcherHostImpl;
Why the additional forward decls for Resource*?

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
File content/public/browser/download_url_parameters.h (right):

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:60: void
set_request_header(const std::string& name, const std::string& value) {
nit, suggestion: I think of "set" as implying "overwrite"; might
add_request_header be a better name?

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:96: return post_id_set() ||
prefer_cache_;
Actually, my apologies, but I'd rather not do it this way; this hard codes a
policy decision that we may want to change at some point.  Specifically, it's a
perfectly reasonable functionality (and one I was going to implement at one
point) to retrieve a post from the net if it's not from cache.  Good UE suggests
that we ask the user before we do it (because it can change state) but reposting
if something's not in cache is reasonable.  So I'd be inclined to say that at
this level we should allow it, and the blockage should be at the higher level
when we're closer to the user/policy.

http://codereview.chromium.org/10232010/diff/26002/content/test/mock_download...
File content/test/mock_download_manager.h (right):

http://codereview.chromium.org/10232010/diff/26002/content/test/mock_download...
content/test/mock_download_manager.h:58: }
Nice hack--thanks for making me aware of this option.

[benjhayden, 2012-05-02 20:01:26]

PTAL

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
File content/public/browser/download_manager.h (right):

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
content/public/browser/download_manager.h:59: class ResourceDispatcherHostImpl;
On 2012/05/02 18:36:22, rdsmith wrote:
> Why the additional forward decls for Resource*?

Done.

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
File content/public/browser/download_url_parameters.h (right):

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:60: void
set_request_header(const std::string& name, const std::string& value) {
On 2012/05/02 18:36:22, rdsmith wrote:
> nit, suggestion: I think of "set" as implying "overwrite"; might
> add_request_header be a better name?

Done.

http://codereview.chromium.org/10232010/diff/26002/content/public/browser/dow...
content/public/browser/download_url_parameters.h:96: return post_id_set() ||
prefer_cache_;
On 2012/05/02 18:36:22, rdsmith wrote:
> Actually, my apologies, but I'd rather not do it this way; this hard codes a
> policy decision that we may want to change at some point.  Specifically, it's
a
> perfectly reasonable functionality (and one I was going to implement at one
> point) to retrieve a post from the net if it's not from cache.  Good UE
suggests
> that we ask the user before we do it (because it can change state) but
reposting
> if something's not in cache is reasonable.  So I'd be inclined to say that at
> this level we should allow it, and the blockage should be at the higher level
> when we're closer to the user/policy.

Done.

http://codereview.chromium.org/10232010/diff/26002/content/test/mock_download...
File content/test/mock_download_manager.h (right):

http://codereview.chromium.org/10232010/diff/26002/content/test/mock_download...
content/test/mock_download_manager.h:58: }
On 2012/05/02 18:36:22, rdsmith wrote:
> Nice hack--thanks for making me aware of this option.

Done.

[jam, 2012-05-02 20:05:30]

http://codereview.chromium.org/10232010/diff/45002/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10232010/diff/45002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:143:
DownloadUrlParameters::DownloadUrlParameters(
the DownloadUrlParameters construtors belong in 
content/public/browser/download_url_parameters.cc

[benjhayden, 2012-05-02 20:56:46]

http://codereview.chromium.org/10232010/diff/45002/content/browser/download/d...
File content/browser/download/download_manager_impl.cc (right):

http://codereview.chromium.org/10232010/diff/45002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:143:
DownloadUrlParameters::DownloadUrlParameters(
On 2012/05/02 20:05:30, John Abd-El-Malek wrote:
> the DownloadUrlParameters construtors belong in 
> content/public/browser/download_url_parameters.cc

Done.

[jam, 2012-05-02 21:01:29]

content/public lgtm

[Randy Smith (Not in Mondays), 2012-05-03 18:34:51]

lgtm

[commit-bot: I haz the power, 2012-05-04 17:40:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10232010/64002

[commit-bot: I haz the power, 2012-05-04 18:00:52]

Try job failure for 10232010-64002 (retry) on win for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-04 18:36:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10232010/64002

[commit-bot: I haz the power, 2012-05-04 20:05:21]

Change committed as 135403