Added DownloadProcessHandle class.

Added DownloadProcessHandle class.


BUG=None
TEST=Download tests still pass.


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

[Paweł Hajdan Jr., 2011-05-05 20:13:51]

Thank you for patience with my reviews, this is much better now.

Could you comment more on the CancelDownload method? I just don't think it's the
best place for it, and would prefer a method in RenderDispatcherHost or
something in download_util.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:50: #include
"chrome/browser/download/download_file.h"
Why does this #include download_file? Also, if you convert to const reference
below, you can forward declare the process handle instead of #including it
below.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:143: DownloadProcessHandle
process,
nit: Now why not const DownloadProcessHandle& process?

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:149: void
ResumeDownloadRequest(DownloadProcessHandle process);
nit: Now why not const DownloadProcessHandle& process?

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_history.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: Could you just remove this file from the CL? Updating only the copyright
year is slightly weird.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_history.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: Could you just remove this file from the CL? Updating only the copyright
year is slightly weird.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_item.h:264: int render_process_id() const {
return process_handle_.child_id(); }
Just wondering, can we remove those accessors completely in favor of
process_handle? Rather up to you, but I prefer when there is just one way to do
a thing.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_item.h:346: DownloadProcessHandle
process_handle_;
nit: There should be an empty line above an a comment. Also, it's probably
better to add it in place of removed members.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:983:
info->process_handle.child_id(),
nit: Please convert DownloadCancelledInternal to take const
DownloadProcessHandle& process_handle.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_manager.h:201: void
ShowDownloadInBrowser(DownloadProcessHandle process_handle,
nit: Why not const DownloadProcessHandle& process_handle, and forward-declare
instead of #include?

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.cc (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:31: void
DownloadProcessHandle::CancelDownload(ResourceDispatcherHost* rdh) {
I'm still not convinced about having this method here. How about adding it as a
method to ResourceDispatcherHost instead? Or maybe modify
download_util::CancelDownloadRequest. It seems we always call it from non-IO
threads, which results in need for this PostTask boilerplate.

Whatever happens to CancelDownload, let's sort out this CancelDownloadRequest
mess too. It should be quite simple.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:40: TabContents*
DownloadProcessHandle::GetTabContents() {
Please add a comment to the declaration that those methods must be called on the
UI thread.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:16: // This class needs to be
copyable, but has non-trivial methods.
nit: Just remove the part about non-trivial methods, no need to "justify" such
things in comments.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:32: int child_id_;
Could you make all member variables const?

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/history/downl...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/history/downl...
chrome/browser/history/download_database.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: Let's just remove this file from CL.

[Randy Smith (Not in Mondays), 2011-05-05 20:25:34]

Thank you for doing this.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_file.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_file.h:44: // IDs for looking up the tab we are
associated with.
Modify comment to fit new regime.  Maybe "Handle for tab lookup and informing
the ResourceDispatcherHost of a cancel."?

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:148: void
ResumeDownloadRequest(DownloadProcessHandle process);
I'd suggest folding this routine into DownloadProcessHandle.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_history.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
Any reason for including this file and the next one just to update the
copyright?

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_item.h:265: int request_id() const { return
process_handle_.request_id(); }
Are these two still needed, or can we get rid of them?

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_item.h:346: DownloadProcessHandle
process_handle_;
This looks strange without having a comment describing its purpose.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:711: void
DownloadManager::DownloadCancelledInternal(int download_id,
The places from which this is called both have a DownloadProcessHandle.  Pass
that in instead of the render_process_id and request_id--it'll be cleaner.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.cc (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:20: : child_id_(-1),
render_view_id_(-1), request_id_(-1) {
I believe it's invalid to call any of the routines in this with a default
constructed DownloadProcessHandle.  Can you put in DCHECKs for that case?  Under
what circumstances can we end up in the download system with an invalid
DownloadProcessHandle, and what happens if we need to do one of the below
operations on it in that case?

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:16: // This class needs to be
copyable, but has non-trivial methods.
A word about why it needs to be copiable (easy enough to copy to not make worth
tracking lifetime issues, being passed across threads) would be worthwhile here.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:26: int child_id() const {
return child_id_; }
I'd like to look closely at all places where these accessors are used.  I
suspect it would require very little extra work to make them superfluous and
remove them, which really improves the encapsulation of this class.

[Randy Smith (Not in Mondays), 2011-05-05 20:45:50]

On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> Could you comment more on the CancelDownload method? I just don't think it's
the
> best place for it, and would prefer a method in RenderDispatcherHost or
> something in download_util.

Pawel: I think (though I may be wrong; Andy can correct me if he's got his own
passion in this area) that most of the impetus for that method and ones similar
to it is coming from me, so I'll give you my perspective here.  

The primary reason why I'd like to see CancelDownload, PauseDownload, and
ResumeDownload (though I might prefer them to be named *Request, now that I
think about it) is that I'm looking for "What are we always doing with these
member variables, and what functions would we need to have to make them
private?".  But I can also make a conceptual argument for it as well.  The
DownloadProcessHandle is our handle for those objects outside of the download
(primarily, maybe only, the URLRequest; maybe we should call it
DownloadRequestHandle?) which are connected to the download.  It makes sense to
me that we be able to affect those entities through our handle.  And given that
I don't like either duplicating code all over the place or exposing member
variables if we don't have to, it just makes sense to me to put those methods in
it.  Does that make any sense?

[ahendrickson, 2011-05-06 16:16:50]

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:50: #include
"chrome/browser/download/download_file.h"
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> Why does this #include download_file? Also, if you convert to const reference
> below, you can forward declare the process handle instead of #including it
> below.

Removed the include.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:143: DownloadProcessHandle
process,
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Now why not const DownloadProcessHandle& process?

Because I need to use non-const member functions.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:149: void
ResumeDownloadRequest(DownloadProcessHandle process);
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Now why not const DownloadProcessHandle& process?

Same as above.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_history.cc (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_history.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Could you just remove this file from the CL? Updating only the copyright
> year is slightly weird.

Done.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_history.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Could you just remove this file from the CL? Updating only the copyright
> year is slightly weird.

Done.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_item.h:264: int render_process_id() const {
return process_handle_.child_id(); }
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> Just wondering, can we remove those accessors completely in favor of
> process_handle? Rather up to you, but I prefer when there is just one way to
do
> a thing.

Done.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_item.h:346: DownloadProcessHandle
process_handle_;
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: There should be an empty line above an a comment. Also, it's probably
> better to add it in place of removed members.

Done.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:983:
info->process_handle.child_id(),
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Please convert DownloadCancelledInternal to take const
> DownloadProcessHandle& process_handle.

Converted to use DownloadProcessHandle process_handle.

Can't use const version due to call of non-const member function.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_manager.h:201: void
ShowDownloadInBrowser(DownloadProcessHandle process_handle,
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Why not const DownloadProcessHandle& process_handle, and forward-declare
> instead of #include?

Because I need to use a non-const member function.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.cc (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:31: void
DownloadProcessHandle::CancelDownload(ResourceDispatcherHost* rdh) {
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> I'm still not convinced about having this method here. How about adding it as
a
> method to ResourceDispatcherHost instead? Or maybe modify
> download_util::CancelDownloadRequest. It seems we always call it from non-IO
> threads, which results in need for this PostTask boilerplate.
> 
> Whatever happens to CancelDownload, let's sort out this CancelDownloadRequest
> mess too. It should be quite simple.

Done.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:40: TabContents*
DownloadProcessHandle::GetTabContents() {
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> Please add a comment to the declaration that those methods must be called on
the
> UI thread.

OK, but I assumed the DCHECK was comment enough.

Done.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:16: // This class needs to be
copyable, but has non-trivial methods.
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Just remove the part about non-trivial methods, no need to "justify" such
> things in comments.

Done.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:32: int child_id_;
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> Could you make all member variables const?

That would prevent the class from being copyable.

Having all the members private, and not having mutators (other than the
assignment operator) achieves what we want.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/history/downl...
File chrome/browser/history/download_database.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/history/downl...
chrome/browser/history/download_database.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> nit: Let's just remove this file from CL.

Done.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_file.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_file.h:44: // IDs for looking up the tab we are
associated with.
On 2011/05/05 20:25:34, rdsmith wrote:
> Modify comment to fit new regime.  Maybe "Handle for tab lookup and informing
> the ResourceDispatcherHost of a cancel."?

Updated.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_history.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_history.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/05/05 20:25:34, rdsmith wrote:
> Any reason for including this file and the next one just to update the
> copyright?

Removed.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_item.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_item.h:265: int request_id() const { return
process_handle_.request_id(); }
On 2011/05/05 20:25:34, rdsmith wrote:
> Are these two still needed, or can we get rid of them?  

At the moment they are, but I can change DownloadManger::PauseDownloadRequest()
to take a DownloadProcessHandle.

Removed.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_item.h:346: DownloadProcessHandle
process_handle_;
On 2011/05/05 20:25:34, rdsmith wrote:
> This looks strange without having a comment describing its purpose.

Done.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:711: void
DownloadManager::DownloadCancelledInternal(int download_id,
On 2011/05/05 20:25:34, rdsmith wrote:
> The places from which this is called both have a DownloadProcessHandle.  Pass
> that in instead of the render_process_id and request_id--it'll be cleaner.

Done.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.cc (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:20: : child_id_(-1),
render_view_id_(-1), request_id_(-1) {
On 2011/05/05 20:25:34, rdsmith wrote:
> I believe it's invalid to call any of the routines in this with a default
> constructed DownloadProcessHandle.  Can you put in DCHECKs for that case? 
Under
> what circumstances can we end up in the download system with an invalid
> DownloadProcessHandle, and what happens if we need to do one of the below
> operations on it in that case?

In unit tests we call them with -1's; this just makes the GetTabContents
function return NULL.

I believe you have an invalid DFH when a DownloadItem is loaded from the history
database.  Not sure about Drag & Drop or Save Page As.

Waiting to get resolution on whether or not to have Cancel/Pause/Resume Download
in this class.

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.h (right):

http://codereview.chromium.org/6932046/diff/2019/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:16: // This class needs to be
copyable, but has non-trivial methods.
On 2011/05/05 20:25:34, rdsmith wrote:
> A word about why it needs to be copiable (easy enough to copy to not make
worth
> tracking lifetime issues, being passed across threads) would be worthwhile
here.

Done.

[Paweł Hajdan Jr., 2011-05-06 18:04:41]

Almost there, mostly nits.

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.h (right):

http://codereview.chromium.org/6932046/diff/2018/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:32: int child_id_;
On 2011/05/06 16:16:50, ahendrickson wrote:
> On 2011/05/05 20:13:51, Paweł Hajdan Jr. wrote:
> > Could you make all member variables const?
> 
> That would prevent the class from being copyable.

To be precise, I think it would prevent it from being _assignable_. Okay, it's
fine as-is.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file.cc:34: void
DownloadFile::CancelDownloadRequest(ResourceDispatcherHost* rdh) {
nit: Please restore the BrowserThread DCHECK.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:69: return;
This used to delete |info|, now that code is gone. Was that intentional? If so,
doesn't that introduce a leak?

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:160: info,
info->process_handle,
If we pass info, why do we also need info->process_handle?

Also, just wondering - if we can extract a DownloadManager from the process
handle, do we need to pass |manager| as a parameter here?

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:142: DownloadProcessHandle
process,
Okay, if we can't have a const reference here could you use a pointer? It still
seems better than copying the class, even if it's cheap.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:7: #include <string>
nit: Why?

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:9: #include
"base/file_util.h"
nit: What's going on with all those #includes? We don't use file_util,
StringPrintf here, or download_util... :-/

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.h (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:9: #include <string>
nit: Why?

[Randy Smith (Not in Mondays), 2011-05-09 00:57:45]

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file.cc:35:
download_util::CancelDownloadRequest(rdh, process_handle_);
I don't want to put more stuff into download_util.  If Pawel's really not
willing to put the methods into DownloadProcessHandle, could you expand them
back out inline?

Pawel, did you have any response to my note to you early on this CL about
encapsulation on DownloadProcessHandle?

[ahendrickson, 2011-05-09 15:57:31]

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file.cc:34: void
DownloadFile::CancelDownloadRequest(ResourceDispatcherHost* rdh) {
On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> nit: Please restore the BrowserThread DCHECK.

It doesn't matter what thread this is called from, as it just posts to the IO
thread.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:69: return;
On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> This used to delete |info|, now that code is gone. Was that intentional? If
so,
> doesn't that introduce a leak?

Ah, you're right.  This is left over from splitting the CL, where |info| was
deleted automatically in this routine.

Fixed.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:160: info,
info->process_handle,
On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> If we pass info, why do we also need info->process_handle?
> 
> Also, just wondering - if we can extract a DownloadManager from the process
> handle, do we need to pass |manager| as a parameter here?

We don't need to pass in process_handle.  Removed the parameter.

I believe that the intent was to make sure that download_manager stayed valid
until after |CreateDownloadFile| was called.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:142: DownloadProcessHandle
process,
On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> Okay, if we can't have a const reference here could you use a pointer? It
still
> seems better than copying the class, even if it's cheap.

The parameter has been removed.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:7: #include <string>
On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> nit: Why?

Removed.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:9: #include
"base/file_util.h"
On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> nit: What's going on with all those #includes? We don't use file_util,
> StringPrintf here, or download_util... :-/

Sorry about that.  I had split this file off from download_file.h, and forgot to
clean up the includes.

The download_util.h was included when we had a CancelDownload() function.

Fixed.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_process_handle.h (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:9: #include <string>
On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> nit: Why?

Removed.

[Paweł Hajdan Jr., 2011-05-09 19:47:30]

LGTM with comments (assuming all of them get applied).

I'd prefer not passing DownloadProcessHandle by value, but rather by pointer, if
possible (mostly to be able to forward-declare it).

Also, there is Randy's concern about download_util and CancelDownload method. I
won't be able to chat today, but feel free to schedule a chat on gtalk on irc
maybe tomorrow if there are still concerns/doubts about this part of the code.
Maybe I just don't get some rationale behind Randy's points.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file.cc:34: void
DownloadFile::CancelDownloadRequest(ResourceDispatcherHost* rdh) {
On 2011/05/09 15:57:31, ahendrickson wrote:
> On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> > nit: Please restore the BrowserThread DCHECK.
> 
> It doesn't matter what thread this is called from, as it just posts to the IO
> thread.

Agreed, but it's very important to ensure that DownloadFile is only ever used on
the IO thread. It simplifies reasoning about the code a lot. Please restore the
DCHECK.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file.cc:35:
download_util::CancelDownloadRequest(rdh, process_handle_);
On 2011/05/09 00:57:45, rdsmith wrote:
> I don't want to put more stuff into download_util.  If Pawel's really not
> willing to put the methods into DownloadProcessHandle, could you expand them
> back out inline?

Wait, what's wrong with this change? We're not adding anything to download_util,
it's just changing the implementation and adding an internal function. All of
that seems simple, clean, and good to me. Maybe I just don't get the idea behind
not touching download_util.

> Pawel, did you have any response to my note to you early on this CL about
> encapsulation on DownloadProcessHandle?

Yes, I did, sorry if I didn't respond. I think that the CL in current state is
the best solution. I see no need for a CancelDownload method in
DownloadProcessHandle, and fixing download_util like here seems more correct.
I'm not a fan of the DownloadProcessHandle::CancelDownload method because it was
mostly looking like "OK, I give you this RDH, and you call some method on it".
It seems backwards. Of course the current code is not ideal either, but it's not
a regression. I'd like all new code/classes to be extremely clean.

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.h:147: void
ResumeDownloadRequest(DownloadProcessHandle process);
nit: Why not a pointer?

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:201: void
ShowDownloadInBrowser(DownloadProcessHandle process_handle,
nit: Why not a pointer?

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:293: DownloadProcessHandle
process_handle);
nit: Why not a pointer?

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:309: DownloadProcessHandle
process_handle,
nit: Why not a pointer?

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_util.h:247: DownloadProcessHandle
process_handle);
nit: Why not a pointer?

[Randy Smith (Not in Mondays), 2011-05-09 21:42:20]

LGTM.

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file.cc:35:
download_util::CancelDownloadRequest(rdh, process_handle_);
On 2011/05/09 19:47:30, Paweł Hajdan Jr. wrote:
> On 2011/05/09 00:57:45, rdsmith wrote:
> > I don't want to put more stuff into download_util.  If Pawel's really not
> > willing to put the methods into DownloadProcessHandle, could you expand them
> > back out inline?
> 
> Wait, what's wrong with this change? We're not adding anything to
download_util,
> it's just changing the implementation and adding an internal function. All of
> that seems simple, clean, and good to me. Maybe I just don't get the idea
behind
> not touching download_util.

Ah, whoops, I see.  Quite right.  I rescind my objection.

> > Pawel, did you have any response to my note to you early on this CL about
> > encapsulation on DownloadProcessHandle?
> 
> Yes, I did, sorry if I didn't respond. I think that the CL in current state is
> the best solution. I see no need for a CancelDownload method in
> DownloadProcessHandle, and fixing download_util like here seems more correct.
> I'm not a fan of the DownloadProcessHandle::CancelDownload method because it
was
> mostly looking like "OK, I give you this RDH, and you call some method on it".
> It seems backwards. Of course the current code is not ideal either, but it's
not
> a regression. I'd like all new code/classes to be extremely clean.

I'll follow up on this in email--I'm ok with the CL as it is.

[ahendrickson, 2011-05-10 18:04:43]

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/6932046/diff/4026/chrome/browser/download/down...
chrome/browser/download/download_file.cc:34: void
DownloadFile::CancelDownloadRequest(ResourceDispatcherHost* rdh) {
On 2011/05/09 19:47:30, Paweł Hajdan Jr. wrote:
> On 2011/05/09 15:57:31, ahendrickson wrote:
> > On 2011/05/06 18:04:42, Paweł Hajdan Jr. wrote:
> > > nit: Please restore the BrowserThread DCHECK.
> > 
> > It doesn't matter what thread this is called from, as it just posts to the
IO
> > thread.
> 
> Agreed, but it's very important to ensure that DownloadFile is only ever used
on
> the IO thread. It simplifies reasoning about the code a lot. Please restore
the
> DCHECK.

Done.

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.h:147: void
ResumeDownloadRequest(DownloadProcessHandle process);
On 2011/05/09 19:47:31, Paweł Hajdan Jr. wrote:
> nit: Why not a pointer?

Because it's called from other threads, and this way we don't have to worry
about object lifetimes.

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:201: void
ShowDownloadInBrowser(DownloadProcessHandle process_handle,
On 2011/05/09 19:47:31, Paweł Hajdan Jr. wrote:
> nit: Why not a pointer?

Done.

This function is only called from within one thread, so it's safe to pass in a
pointer.

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:293: DownloadProcessHandle
process_handle);
On 2011/05/09 19:47:31, Paweł Hajdan Jr. wrote:
> nit: Why not a pointer?

Because it is ultimately passed to another thread, and this way we don't have to
worry about object lifetimes.

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_manager.h:309: DownloadProcessHandle
process_handle,
On 2011/05/09 19:47:31, Paweł Hajdan Jr. wrote:
> nit: Why not a pointer?

Because it's called from other threads, and this way we don't have to worry
about object lifetimes.

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
File chrome/browser/download/download_util.h (right):

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_util.h:247: DownloadProcessHandle
process_handle);
On 2011/05/09 19:47:31, Paweł Hajdan Jr. wrote:
> nit: Why not a pointer?

Because it is ultimately passed to another thread, and this way we don't have to
worry about object lifetimes.

[Paweł Hajdan Jr., 2011-05-10 18:58:04]

LGTM

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
File chrome/browser/download/download_file_manager.h (right):

http://codereview.chromium.org/6932046/diff/12001/chrome/browser/download/dow...
chrome/browser/download/download_file_manager.h:147: void
ResumeDownloadRequest(DownloadProcessHandle process);
On 2011/05/10 18:04:43, ahendrickson wrote:
> On 2011/05/09 19:47:31, Paweł Hajdan Jr. wrote:
> > nit: Why not a pointer?
> 
> Because it's called from other threads, and this way we don't have to worry
> about object lifetimes.

I see, makes sense. Could you add a short comment in each place which does the
copy?

[commit-bot: I haz the power, 2011-05-10 21:29:41]

Try job failure for 6932046-26001 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-10 22:47:47]

Try job failure for 6932046-26001 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-11 05:51:53]

Try job failure for 6932046-26001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2011-05-11 12:14:15]

Change committed as 84967