Use a DownloadRequestHandle pointer in construction to allow mocking for tests.

Use a DownloadRequestHandle pointer in construction to allow mocking for tests.

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

[Randy Smith (Not in Mondays), 2011-10-27 02:25:46]

Chris, Andy: Could you take a look at this?  Chris for architectural crunchy
goodness (or not :-}), Andy for downloads flow.

I considered a couple of alternatives:
* Always pass around DownloadRequestHandles by pointer.  Would have worked,
would have involved doing lifetime tracking on a pretty simple class.
* Expand DownloadRequestHandle to support what I'd need for testing.  I couldn't
come up with a clean way to do this that didn't involve a pointer inside to the
class that did the "real" work.  
* What I did here--pass around by value, but explicitly construct a pointer
version that the base classes (DownloadFile & DownloadItem) take ownership of. 
That seemed the best compromise.

[ahendrickson, 2011-10-27 15:00:04]

LGTM.

This doesn't actually create a mock class but only allows one to be created
later, right?

Andy

[Randy Smith (Not in Mondays), 2011-10-27 15:28:44]

On 2011/10/27 15:00:04, ahendrickson wrote:
> LGTM.
> 
> This doesn't actually create a mock class but only allows one to be created
> later, right?

Yep.  Actually creating the class (probably from Gmock) is part of writing the
unit tests; these CLs are setting up so that I *can* write the unit tests.

[cbentzel, 2011-10-27 17:20:39]

I was surprised that there weren't mock versions being used in the tests - is
that in the next CL?

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_item.cc:706: if (request_handle_.get())
Why is this needed? DownloadItem::MatchesQuery doesn't use it, for example, and
it looks like other places don't check for NULL either.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
File content/browser/download/download_request_handle.h (right):

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:23: class
DownloadRequestHandleInterface {
I haven't seen the "Interface" suffix used too heavily in the chromium code
base, but it is not disallowed.

Usually I've seen the abstract base class called DownloadRequestHandle, followed
by DownloadRequestHandleImpl for the concrete implementation.

IMO this is the way to go - you want to encourage people to use the interfaces
as much as possible, and having a shorter name is just one way to do it.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Interf...

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:31: // Pause or resume the
matching URL request.
Do these also need to be called on the UI thread?

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:43: class
DownloadRequestHandle : public DownloadRequestHandleInterface {
Do you want to move this into a separate header? Probably not a big deal since
this forward declares RDH, but it does have some details not needed by consumers
of the interface.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:61: virtual TabContents*
GetTabContents() const OVERRIDE;
The OVERRIDE isn't completely needed here because of the =0's in the base class,
but are probably good practice to keep in.

[Randy Smith (Not in Mondays), 2011-10-27 18:07:59]

On 2011/10/27 17:20:39, cbentzel wrote:
> I was surprised that there weren't mock versions being used in the tests - is
> that in the next CL?

Yup.  I take it you think I'm going too far in the direction of Salami CLs?

[cbentzel, 2011-10-27 18:10:10]

On Thu, Oct 27, 2011 at 2:08 PM, <rdsmith@chromium.org> wrote:

> On 2011/10/27 17:20:39, cbentzel wrote:
>
>> I was surprised that there weren't mock versions being used in the tests -
>> is
>> that in the next CL?
>>
>
> Yup.  I take it you think I'm going too far in the direction of Salami CLs?
>

Yeah - I think the changes are pretty small and localized. Not opposed to
this, however.

>
>
>
>
http://codereview.chromium.**org/8399001/<http://codereview.chromium.org/8399...
>

[Randy Smith (Not in Mondays), 2011-10-27 18:21:21]

Responses to Chris' comments; will wait on his responses for any code changes.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_item.cc:706: if (request_handle_.get())
On 2011/10/27 17:20:39, cbentzel wrote:
> Why is this needed? DownloadItem::MatchesQuery doesn't use it, for example,
and
> it looks like other places don't check for NULL either.

See DownloadManager::RestartDownload.  I ran into this because in some tests, a
null DownloadRequestHandle is now represented by a null pointer rather than a
default constructed download handle.  But for cleanliness of mocking, I'd rather
avoid cases where something external to a class gets a reference to an object
contained within the class, so I prefer the wrapping pattern anyway.

If you'd like me to merge in the switch over to mocks for DownloadRequestHandle
in this CL, I will, but I'd like to keep this wrapper.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
File content/browser/download/download_request_handle.h (right):

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:23: class
DownloadRequestHandleInterface {
On 2011/10/27 17:20:39, cbentzel wrote:
> I haven't seen the "Interface" suffix used too heavily in the chromium code
> base, but it is not disallowed.
> 
> Usually I've seen the abstract base class called DownloadRequestHandle,
followed
> by DownloadRequestHandleImpl for the concrete implementation.
> 
> IMO this is the way to go - you want to encourage people to use the interfaces
> as much as possible, and having a shorter name is just one way to do it.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Interf...

Well, that's actually precisely why I *didn't* go with "" vs. "Impl" (I did
consider it).  Everywhere except for inside of DownloadFile and DownloadItem,
DownloadRequestHandle is the primary object, and it's passed around by value. 
So I think this is tied to the basic design choice for this CL (pass around by
value except for construction of DownloadItem/DownloadFile), and I'd rather not
change the naming unless we change that choice.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:31: // Pause or resume the
matching URL request.
On 2011/10/27 17:20:39, cbentzel wrote:
> Do these also need to be called on the UI thread?

Nope; they're ok to call anywhere.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:43: class
DownloadRequestHandle : public DownloadRequestHandleInterface {
On 2011/10/27 17:20:39, cbentzel wrote:
> Do you want to move this into a separate header? Probably not a big deal since
> this forward declares RDH, but it does have some details not needed by
consumers
> of the interface.

I'm neutral on it--happy to move it if you want, but didn't think it was worth
it originally.  Let me know.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:61: virtual TabContents*
GetTabContents() const OVERRIDE;
On 2011/10/27 17:20:39, cbentzel wrote:
> The OVERRIDE isn't completely needed here because of the =0's in the base
class,
> but are probably good practice to keep in.

Done.

[Randy Smith (Not in Mondays), 2011-10-27 18:23:41]

On 2011/10/27 18:10:10, cbentzel wrote:
> On Thu, Oct 27, 2011 at 2:08 PM, <mailto:rdsmith@chromium.org> wrote:
> 
> > On 2011/10/27 17:20:39, cbentzel wrote:
> >
> >> I was surprised that there weren't mock versions being used in the tests -
> >> is
> >> that in the next CL?
> >>
> >
> > Yup.  I take it you think I'm going too far in the direction of Salami CLs?
> >
> 
> Yeah - I think the changes are pretty small and localized. Not opposed to
> this, however.

Ok.  I'll calibrate a little coarser for the future, but not change this CL
(unless you specifically request that in response to my comments).
> 
> >
> >
> >
> >
>
http://codereview.chromium.**org/8399001/%3Chttp://codereview.chromium.org/83...>
> >

[cbentzel, 2011-10-27 18:45:34]

LGTM

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
File content/browser/download/download_item.cc (right):

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_item.cc:706: if (request_handle_.get())
On 2011/10/27 18:21:22, rdsmith wrote:
> On 2011/10/27 17:20:39, cbentzel wrote:
> > Why is this needed? DownloadItem::MatchesQuery doesn't use it, for example,
> and
> > it looks like other places don't check for NULL either.
> 
> See DownloadManager::RestartDownload.  I ran into this because in some tests,
a
> null DownloadRequestHandle is now represented by a null pointer rather than a
> default constructed download handle.  But for cleanliness of mocking, I'd
rather
> avoid cases where something external to a class gets a reference to an object
> contained within the class, so I prefer the wrapping pattern anyway.
> 
> If you'd like me to merge in the switch over to mocks for
DownloadRequestHandle
> in this CL, I will, but I'd like to keep this wrapper.

I have no problem with the wrapper - just was more confused about why we were
checking for NULL here but not in other cases.

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
File content/browser/download/download_request_handle.h (right):

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:23: class
DownloadRequestHandleInterface {
On 2011/10/27 18:21:22, rdsmith wrote:
> On 2011/10/27 17:20:39, cbentzel wrote:
> > I haven't seen the "Interface" suffix used too heavily in the chromium code
> > base, but it is not disallowed.
> > 
> > Usually I've seen the abstract base class called DownloadRequestHandle,
> followed
> > by DownloadRequestHandleImpl for the concrete implementation.
> > 
> > IMO this is the way to go - you want to encourage people to use the
interfaces
> > as much as possible, and having a shorter name is just one way to do it.
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Interf...
> 
> Well, that's actually precisely why I *didn't* go with "" vs. "Impl" (I did
> consider it).  Everywhere except for inside of DownloadFile and DownloadItem,
> DownloadRequestHandle is the primary object, and it's passed around by value. 
> So I think this is tied to the basic design choice for this CL (pass around by
> value except for construction of DownloadItem/DownloadFile), and I'd rather
not
> change the naming unless we change that choice.

Maybe it's me, but it doesn't look like the current DownloadRequestHandle is
used in too many places.

http://codesearch.google.com/codesearch#search&q=DownloadRequestHandle&exact_...

Perhaps this is because codesearch hasn't caught up to the DownloadCreateInfo
split. 

Why wouldn't you want to use the interface, except in DownloadResourceHandle
where it's created? Lifetime issues?

http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
content/browser/download/download_request_handle.h:43: class
DownloadRequestHandle : public DownloadRequestHandleInterface {
On 2011/10/27 18:21:22, rdsmith wrote:
> On 2011/10/27 17:20:39, cbentzel wrote:
> > Do you want to move this into a separate header? Probably not a big deal
since
> > this forward declares RDH, but it does have some details not needed by
> consumers
> > of the interface.
> 
> I'm neutral on it--happy to move it if you want, but didn't think it was worth
> it originally.  Let me know.

Not too concerned. I'll revisit after you add the mocks.

[Randy Smith (Not in Mondays), 2011-10-27 18:54:12]

On 2011/10/27 18:45:34, cbentzel wrote:
> LGTM
> 
>
http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
> File content/browser/download/download_item.cc (right):
> 
>
http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
> content/browser/download/download_item.cc:706: if (request_handle_.get())
> On 2011/10/27 18:21:22, rdsmith wrote:
> > On 2011/10/27 17:20:39, cbentzel wrote:
> > > Why is this needed? DownloadItem::MatchesQuery doesn't use it, for
example,
> > and
> > > it looks like other places don't check for NULL either.
> > 
> > See DownloadManager::RestartDownload.  I ran into this because in some
tests,
> a
> > null DownloadRequestHandle is now represented by a null pointer rather than
a
> > default constructed download handle.  But for cleanliness of mocking, I'd
> rather
> > avoid cases where something external to a class gets a reference to an
object
> > contained within the class, so I prefer the wrapping pattern anyway.
> > 
> > If you'd like me to merge in the switch over to mocks for
> DownloadRequestHandle
> > in this CL, I will, but I'd like to keep this wrapper.
> 
> I have no problem with the wrapper - just was more confused about why we were
> checking for NULL here but not in other cases.
> 
>
http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
> File content/browser/download/download_request_handle.h (right):
> 
>
http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
> content/browser/download/download_request_handle.h:23: class
> DownloadRequestHandleInterface {
> On 2011/10/27 18:21:22, rdsmith wrote:
> > On 2011/10/27 17:20:39, cbentzel wrote:
> > > I haven't seen the "Interface" suffix used too heavily in the chromium
code
> > > base, but it is not disallowed.
> > > 
> > > Usually I've seen the abstract base class called DownloadRequestHandle,
> > followed
> > > by DownloadRequestHandleImpl for the concrete implementation.
> > > 
> > > IMO this is the way to go - you want to encourage people to use the
> interfaces
> > > as much as possible, and having a shorter name is just one way to do it.
> > > 
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Interf...
> > 
> > Well, that's actually precisely why I *didn't* go with "" vs. "Impl" (I did
> > consider it).  Everywhere except for inside of DownloadFile and
DownloadItem,
> > DownloadRequestHandle is the primary object, and it's passed around by
value. 
> > So I think this is tied to the basic design choice for this CL (pass around
by
> > value except for construction of DownloadItem/DownloadFile), and I'd rather
> not
> > change the naming unless we change that choice.
> 
> Maybe it's me, but it doesn't look like the current DownloadRequestHandle is
> used in too many places.
> 
>
http://codesearch.google.com/codesearch#search&q=DownloadRequestHandle&exact_...
> 
> Perhaps this is because codesearch hasn't caught up to the DownloadCreateInfo
> split. 
> 
> Why wouldn't you want to use the interface, except in DownloadResourceHandle
> where it's created? Lifetime issues?

Precisely.  It's passed cross-thread, and I have to explicitly manage new s,
deletes, and copies if I pass a pointer around.  Probably I'd need to create a
Copy method on the interface, which just feels like a lot of machinery for a
pretty simple class.

> 
>
http://codereview.chromium.org/8399001/diff/2021/content/browser/download/dow...
> content/browser/download/download_request_handle.h:43: class
> DownloadRequestHandle : public DownloadRequestHandleInterface {
> On 2011/10/27 18:21:22, rdsmith wrote:
> > On 2011/10/27 17:20:39, cbentzel wrote:
> > > Do you want to move this into a separate header? Probably not a big deal
> since
> > > this forward declares RDH, but it does have some details not needed by
> > consumers
> > > of the interface.
> > 
> > I'm neutral on it--happy to move it if you want, but didn't think it was
worth
> > it originally.  Let me know.
> 
> Not too concerned. I'll revisit after you add the mocks.

[commit-bot: I haz the power, 2011-10-31 16:01:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/8399001/2021

[commit-bot: I haz the power, 2011-10-31 16:01:22]

Can't apply patch for file content/browser/download/download_file.cc.
While running patch -p1 --forward --force;
patching file content/browser/download/download_file.cc
Hunk #3 FAILED at 58.
1 out of 3 hunks FAILED -- saving rejects to file
content/browser/download/download_file.cc.rej

[commit-bot: I haz the power, 2011-11-01 18:01:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/8399001/14001

[commit-bot: I haz the power, 2011-11-01 19:17:31]

Change committed as 108149