Change DownloadProcessHandle to be more of an encapsulated class.

Change DownloadProcessHandle to be more of an encapsulated class.

BUG=None
TEST=None


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

[Randy Smith (Not in Mondays), 2011-06-03 18:23:39]

Pawel: We talked on Andy's CL about how much to encapsulate
DownloadProcessHandle.  I was going to come back to that, and decided I'd just
do an example CL instead.  Could you give me your high-level thoughts on this
CL?  

I know it's got details to fix before it goes in (specifically, I haven't even
run tests, and I want to change some names) but this presents what I think is a
nice, clean encapsulation of DownloadProcessHandle that removes a lot of
duplicate code over several parts of the download system and concentrates it in
one place in a conceptual unit.  So if you don't like it, I'd like to understand
why :-}.

Low priority--I've probably put more time than I should have into doing this
anyway.  Get to it whenever you have a chance.

[Paweł Hajdan Jr., 2011-06-04 08:58:51]

Nice change. Mostly minor comments.

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_file.cc:33: void
DownloadFile::CancelDownloadRequest() {
nit: Please add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)).

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:157: bool hash_needed =
resource_dispatcher_host_->safe_browsing_service()->
It seems that this is now the only place where resource_dispatcher_host_ is used
in DownloadFileManager. How about adding this accessor also to
DownloadProcessHandle, and removing resource_dispatcher_host_ from
DownloadFileManager?

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:752:
DownloadCancelledInternal(download_id, download->process_handle());
It seems this is the only place that calls DownloadCancelledInternal. Could you
remove the Internal method and just paste its contents (without the DCHECK)
here?

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:55: if (rdh_)
nit: Please use braces {} for multi-line "if" body.

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:63: void
DownloadProcessHandle::RDHPauseRequest(
Why are those methods declared as static members of DownloadProcessHandle and
not just function in the anonymous namespace in the .cc file?

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:33: // If the
DownloadProcessHandle was constructed with a null
I'm worried about this possibly silent no-op. Is |rdh| null sometimes? If
possible, we should require RDH to be non-null. Otherwise, make this return bool
to indicate success, and use WARN_UNUSED_RESULT from base/compiler_specific to
make sure people check it.

[hendrickson_a, 2011-06-04 16:01:45]

A couple of drive-by comments:

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_manager.cc (left):

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:758: // Cancel the network request. 
RDH is guaranteed to outlive the IO thread.
I'd like this comment to be preserved (in download_process_handle.cc?).

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:56: ResourceDispatcherHost*
rdh_;
Nit: It seems unintuitive from the name of the class that there is a pointer to
ResourceDispatcherHost as a member.

[Randy Smith (Not in Mondays), 2011-06-07 22:41:11]

Ok, there are a fair number of changes in this version (I did warn you in the
first publishing that I was looking for high level comments :-}).  Specifically:
* I've incorporated all comments as noted below.
* I've changed the name of the class to DownloadRequestHandle, which (as I've
commented) I think better names what it's actually doing.
* I've split PauseRequest(bool) into two functions, one for each direction.
* I've gotten rid of DownloadFileManager::ResumeDownloadRequest.
* (In response to comments, but it's a big change) I've created two special
types of DownloadRequestHandle.  One (the result of the default constructor) is
null and you can't do anything with it (the getters get you null, the actions
assert in debug build, no-op in release).  The other (the result of a special
static getter only available to tests) is a Nop; it doesn't assert, but it
doesn't do anything either.

Let me know what you think.

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_file.cc:33: void
DownloadFile::CancelDownloadRequest() {
On 2011/06/04 08:58:51, Paweł Hajdan Jr. wrote:
> nit: Please add DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)).

Done.

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:157: bool hash_needed =
resource_dispatcher_host_->safe_browsing_service()->
On 2011/06/04 08:58:51, Paweł Hajdan Jr. wrote:
> It seems that this is now the only place where resource_dispatcher_host_ is
used
> in DownloadFileManager. How about adding this accessor also to
> DownloadProcessHandle, and removing resource_dispatcher_host_ from
> DownloadFileManager?

I'm reluctant to do that, as it doesn't feel like it's conceptually unified with
the DownloadProcessHandle to me.  That class/objects of that class are about
having a handle to the URLRequest and doing things with it (and I'm going to
change the name to DownloadRequestHandle for this reason).  What we're doing
here is getting some global state that isn't per-request.  

I thought about a couple of other alternatives:
* Switch this over to using the BrowserProcess global for
ResourceDispatcherHost; we're on the UI thread, so that's safe.  I'm a bit
reluctant, as it reduces possible mocking opportunities in the future, but we'd
probably have to do some careful thinking about interfaces to be able to mock
download_file_manager.h with other structures, and it's a pretty small bit of
state to want to mock, so I could be ok with this.
* Switching the constructor to DownloadFileManager to take a hash needed
boolean.  This means we can't change that value during a running session of the
browser.
* Take the safe browsing service in the constructor rather than the resource
dispatcher host.  This is a minor plus, but doesn't reduce the member
variables/dependencies in the same way.

WDYT?

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_manager.cc (left):

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:758: // Cancel the network request. 
RDH is guaranteed to outlive the IO thread.
On 2011/06/04 16:01:45, hendrickson_a wrote:
> I'd like this comment to be preserved (in download_process_handle.cc?).

Done.

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_manager.cc:752:
DownloadCancelledInternal(download_id, download->process_handle());
On 2011/06/04 08:58:51, Paweł Hajdan Jr. wrote:
> It seems this is the only place that calls DownloadCancelledInternal. Could
you
> remove the Internal method and just paste its contents (without the DCHECK)
> here?

It's also called from FileSelectionCancelled, at least in my sandbox.

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:55: if (rdh_)
On 2011/06/04 08:58:51, Paweł Hajdan Jr. wrote:
> nit: Please use braces {} for multi-line "if" body.

Done.

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.cc:63: void
DownloadProcessHandle::RDHPauseRequest(
On 2011/06/04 08:58:51, Paweł Hajdan Jr. wrote:
> Why are those methods declared as static members of DownloadProcessHandle and
> not just function in the anonymous namespace in the .cc file?

D'oh!  Quite right, much cleaner.

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

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:33: // If the
DownloadProcessHandle was constructed with a null
On 2011/06/04 08:58:51, Paweł Hajdan Jr. wrote:
> I'm worried about this possibly silent no-op. Is |rdh| null sometimes? If
> possible, we should require RDH to be non-null. Otherwise, make this return
bool
> to indicate success, and use WARN_UNUSED_RESULT from base/compiler_specific to
> make sure people check it.

Hmmm.  The silent no-op is copied from the download_util functions I'm
eliminating.  It's there because some unit tests are testing a subset of the
system that doesn't include the RDH.

I'm going to "solve" this by creating two special DownloadPRocessHandles.  One
(default constructor) is a Null handle, and it's a DCHECK violation to call any
of the action routines on it.  The other (special private static member function
only available to tests) is a Nop handle, and it silently drops the actions. 
Let me know if this addresses your concerns.

http://codereview.chromium.org/7112011/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_process_handle.h:56: ResourceDispatcherHost*
rdh_;
On 2011/06/04 16:01:45, hendrickson_a wrote:
> Nit: It seems unintuitive from the name of the class that there is a pointer
to
> ResourceDispatcherHost as a member.

Yep.  I think of that as being part of the encpasulation; we need the
ResourceDispatcherHost to implement the functionality of the class, but ther'es
no need for it in the interface.  I couldn't offhand think of a way to get rid
of the pointer (the global pointer to RDH that I'm aware of is only available on
the UI thread) and I think this is much cleaner than passing it in all the time
(because however unintuitive it is to have it as part of the implementation,
it's more unintuitive to have it as part of the interface as well).  But if you
have some ideas for how to get rid of it, I'd love to hear them.

[Paweł Hajdan Jr., 2011-06-08 09:36:57]

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
File chrome/browser/download/download_request_handle.cc (right):

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:22: {
nit: Shouldn't that be on the previous line?

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:31: {
nit: Shouldn't that be on the previous line?

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:37: : rdh_(NULL),
child_id_(-1), render_view_id_(-1), request_id_(-1) {
nit: I think each initializer should be on its own line.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:124: bool
DownloadRequestHandle::IsNull() const {
Ouch, I don't like this distinction between null and nop. So we want nop in
tests, and where is null used? How hard would it be to not require nop for tests
(for example by converting them to browser tests)?

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
File chrome/browser/download/download_request_handle.h (right):

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.h:21: 
nit: Remove empty line.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.h:26: // Note that rdh is
required to be non-null.
nit: |rdh|

[Randy Smith (Not in Mondays), 2011-06-08 12:52:13]

On 2011/06/08 09:36:57, Paweł Hajdan Jr. wrote:
http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
> chrome/browser/download/download_request_handle.cc:124: bool
> DownloadRequestHandle::IsNull() const {
> Ouch, I don't like this distinction between null and nop. 

Fair enough.

> So we want nop in
> tests, and where is null used? 

Null is used in the default constructor (what else would you use? :-}).  I
looked at getting rid of the default constructor, but DownloadProcessHandle is
part of DownloadItem, which is sometimes constructed from the history in which
case there is no corresponding URLRequest, so the handle should be null.

> How hard would it be to not require nop for tests
> (for example by converting them to browser tests)?

I'm pretty solidly against making architectural choices in the download code
that reduces the scope for unit tests--I think we should be going in the
opposite direction.  Our current unit tests aren't anything to write home about,
but a solid, complete set of unit tests really helps in both debugging and
stability (they can probe things that higher level tests have a harder time
getting at).  

Having said that, a DownloadProcessHandle Nop is a hack; the right way to solve
this is by mocking resource dispatcher host for unit tests.  But that requires a
refactor of resource dispatcher host, and while I think that's worth doing, I
also think it's going to be a world of pain and I don't have the stomach for it
as part of this CL.  (I also feel like there's a rearchitecture of the download
system in here somewhere to allow more probing of single-thread functionality;
the reason we're in this position is that we want to probe individual classes,
but those classes are by nature cross-thread, and so we need to be mocking out
all sorts of threads and objects on other threads and ... you get the idea.)

The original code special cased rdh == null in the same way I started out as
doing it, it was just not done inside a class so it wasn't quite as obvious it
was a hack.  Would you be ok with my going back to that (which I think of as
moving code around) and putting a TODO in place with the above thoughts?



> 
>
http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
> File chrome/browser/download/download_request_handle.h (right):
> 
>
http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
> chrome/browser/download/download_request_handle.h:21: 
> nit: Remove empty line.
> 
>
http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
> chrome/browser/download/download_request_handle.h:26: // Note that rdh is
> required to be non-null.
> nit: |rdh|

[Paweł Hajdan Jr., 2011-06-09 08:35:20]

Okay, could you just go back to the code version that doesn't have those IsNull,
IsNop methods? I think it was slightly better. Please add a TODO(phajdan.jr) to
make it harder to misuse.

[Randy Smith (Not in Mondays), 2011-06-09 13:17:43]

Next revision; all comments incorporated, reverted to no-op if RDH null.

Andy, you've been quiet.  Remember that you're on the hook for an LGTM,
comments, or removing yourself from the reviewers list :-}.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
File chrome/browser/download/download_request_handle.cc (right):

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:22: {
On 2011/06/08 09:36:58, Paweł Hajdan Jr. wrote:
> nit: Shouldn't that be on the previous line?

Done.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:31: {
On 2011/06/08 09:36:58, Paweł Hajdan Jr. wrote:
> nit: Shouldn't that be on the previous line?

Done.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:37: : rdh_(NULL),
child_id_(-1), render_view_id_(-1), request_id_(-1) {
On 2011/06/08 09:36:58, Paweł Hajdan Jr. wrote:
> nit: I think each initializer should be on its own line.

Done.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.cc:124: bool
DownloadRequestHandle::IsNull() const {
On 2011/06/08 09:36:58, Paweł Hajdan Jr. wrote:
> Ouch, I don't like this distinction between null and nop. So we want nop in
> tests, and where is null used? How hard would it be to not require nop for
tests
> (for example by converting them to browser tests)?

Went back to just making no RDH be a no-op, as discussed separately.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
File chrome/browser/download/download_request_handle.h (right):

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.h:21: 
On 2011/06/08 09:36:58, Paweł Hajdan Jr. wrote:
> nit: Remove empty line.

Done.

http://codereview.chromium.org/7112011/diff/9001/chrome/browser/download/down...
chrome/browser/download/download_request_handle.h:26: // Note that rdh is
required to be non-null.
On 2011/06/08 09:36:58, Paweł Hajdan Jr. wrote:
> nit: |rdh|

Done.

[hendrickson_a, 2011-06-09 14:33:59]

LGTM.

I'd just been waiting for the dust to settle $-).

Andy

On 2011/06/09 13:17:43, rdsmith wrote:
> Next revision; all comments incorporated, reverted to no-op if RDH null.
> 
> Andy, you've been quiet.  Remember that you're on the hook for an LGTM,
> comments, or removing yourself from the reviewers list :-}.

[Paweł Hajdan Jr., 2011-06-09 19:16:51]

LGTM

[commit-bot: I haz the power, 2011-06-09 21:08:31]

Change committed as 88585