Replace the DownloadFileManager with direct ownership

Replace the DownloadFileManager with direct ownership 
of DownloadFileImpl by DownloadItemImpl.

BUG=123998
R=benjhayden@chromium.org


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

[Randy Smith (Not in Mondays), 2012-07-24 19:59:40]

PTAL:
   * Ben: Primary reviewer, look at everything.
   * John: content/* except for content/browser/downloads and the test
directories.
   * Scott: content/test, content/public/test.

Asanka is cc'd for his information, specifically because I'm playing a bit with
the downloads initiation flow (and will play with it more after his CL);
comments are welcome but not required.

(Primarily for Ben) A specific issue that I'm not certain if I took the right
approach with is where I'm creating the download file.  I chose to create it in
the DownloadManagerImpl and pass it into the DownloadItemImpl; this is parallel
to the DownloadRequestHandler, which the DII is also handed (in the form of the
request handle), and given that that those two things pretty much have the same
lifetime which doesn't match the lifetime of the DownloadItem, this seems
reasonable.  And it means that providing a DownloadFileFactory (to allow changes
to the files for testing) is relatively easy; I can just have a setter on the
DMI (the alternatives I explored for this involved a lot of plumbing and
delegation through fairly wide interfaces; the interface to
DownloadFileFactory::Create() was being duplicated all over the place).  OTOH,
it means that a reference to the DII as a DownloadDestinationController needs to
be public (i.e. we need the DestinationControllerAsWeakPtr() method) and it
means that we can't just fill in some of the DownloadFileImpl construction args
from the DownloadItemImpl.  

Cleaning up the DownloadFileImpl constructor (and things like DownloadSaveInfo)
would be nice, but I don't want to take any more hair off the yak in this CL.

[Randy Smith (Not in Mondays), 2012-07-24 20:13:22]

On 2012/07/24 19:59:40, rdsmith wrote:
> PTAL:
>    * Ben: Primary reviewer, look at everything.
>    * John: content/* except for content/browser/downloads and the test
> directories.

One extra note on this: download_destination_controller.h isn't needed in
content/public for this CL--it's only used by classes in
content/browser/download.  But the next refactor I'm planning will export a
DownloadDestination class to chrome (so that things like GData can specify a
different DownloadDestination) and for that, it'll need to be visible to the
embedder.  And a recent discussion on chromium-dev got me worried about doing
renames of small files through git--I don't have good SVN foo.  So I figured I'd
just put it where it was eventually going to go.  

>    * Scott: content/test, content/public/test.
> 
> Asanka is cc'd for his information, specifically because I'm playing a bit
with
> the downloads initiation flow (and will play with it more after his CL);
> comments are welcome but not required.
> 
> (Primarily for Ben) A specific issue that I'm not certain if I took the right
> approach with is where I'm creating the download file.  I chose to create it
in
> the DownloadManagerImpl and pass it into the DownloadItemImpl; this is
parallel
> to the DownloadRequestHandler, which the DII is also handed (in the form of
the
> request handle), and given that that those two things pretty much have the
same
> lifetime which doesn't match the lifetime of the DownloadItem, this seems
> reasonable.  And it means that providing a DownloadFileFactory (to allow
changes
> to the files for testing) is relatively easy; I can just have a setter on the
> DMI (the alternatives I explored for this involved a lot of plumbing and
> delegation through fairly wide interfaces; the interface to
> DownloadFileFactory::Create() was being duplicated all over the place).  OTOH,
> it means that a reference to the DII as a DownloadDestinationController needs
to
> be public (i.e. we need the DestinationControllerAsWeakPtr() method) and it
> means that we can't just fill in some of the DownloadFileImpl construction
args
> from the DownloadItemImpl.  
> 
> Cleaning up the DownloadFileImpl constructor (and things like
DownloadSaveInfo)
> would be nice, but I don't want to take any more hair off the yak in this CL.

[jam, 2012-07-24 20:47:02]

nice, just a nit about name

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
File content/public/browser/download_destination_controller.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
content/public/browser/download_destination_controller.h:14: // Class that
receives asynchronous events from a DownloadDestination
nit: here and below, make the comment take up to 80 chars

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
content/public/browser/download_destination_controller.h:23: class
DownloadDestinationController {
nit: this is "Controller" but it just observes things and doesn't control
anything. can it be called Observer to be more like other content api classes?

[Randy Smith (Not in Mondays), 2012-07-24 21:08:54]

On 2012/07/24 20:47:02, John Abd-El-Malek wrote:
http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
> content/public/browser/download_destination_controller.h:23: class
> DownloadDestinationController {
> nit: this is "Controller" but it just observes things and doesn't control
> anything. can it be called Observer to be more like other content api classes?

Will do.  I didn't do this originally just because it's used cross-thread, via
weak pointer, and the DownloadFile takes it in its constructor (so there's only
one of it and it always has that one), and all of that struck me as different
enough from the standard Chromium observer pattern that I was risking reader
confusion by naming it an observer.  But I agree that it is an observer not a
controller, so unless something I've said in this paragraph affects your
thinking, I'll make the change.  

(In the actual implementation the thing at the other end of the
DownloadDestinationController is the DownloadItem, which really does control the
DownloadFile.  But that's implementation, not interface.)

[sky, 2012-07-24 22:11:52]

John is a reviewer for all of content, so no need to have both him and myself
review this. Since John already started I'm removing myself.

[Randy Smith (Not in Mondays), 2012-07-24 22:14:39]

On 2012/07/24 22:11:52, sky wrote:
> John is a reviewer for all of content, so no need to have both him and myself
> review this. Since John already started I'm removing myself.

Ok.  John: Just confirming that that means you'll review the test/* directories
as well as the one's you've already reviewed (?).

[benjhayden, 2012-07-25 15:19:15]

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:27: // be
DOWNLOAD_INTERRUPT_REASON_NONE; on a failed rename, it will be
rename? I thought this was talking about Initialize?

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:79: static int number_active_objects_;
Could this be done more cleanly using another interface?

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:80: };
DISALLOW_COPY_AND_ASSIGN while you're here?

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_impl.cc:71: BrowserThread::UI, FROM_HERE,
base::Bind(
Document somewhere the fact that callback is called on the ui thread?

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:58: // Specific to this
class.
Specific to MockDDC or c::DDC? As opposed to generic to which other class?

And what is the significance of its being specific to which class?

Only three questions, you'll have to try harder to be more cryptic next time.
Greek is too easy, maybe use Egyptian hieroglyphs, I only know a couple of
those.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:119: {
brace on previous line, please.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:138:
content::DownloadSaveInfo(), GURL(), GURL(), 0, calculate_hash,
Can you please name all these fiddly little things somehow?
Us in a few weeks/months: "What is that '0' again? Is referrer the first or
second GURL?"

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_item_impl.h:221: // For weak pointer
downlcasting.
I have the same problem! You start typing some word that happens to start with
"down", and before you know it you've already finished typing "load".

We need foot pedals that type "Download" and "download" so that we never have to
type them and our brains can start to heal some of the damage.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_item_impl.h:431: // pointer may only be used
or destroyed on the FILE thread.
Then maybe it shouldn't be scoped_ptr? scoped_ptr suggests that this member will
be deleted when the owner is deleted.
Is there a generalization of scoped_ptr that affords deletion-on-another-thread?

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
File content/public/browser/download_destination_controller.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
content/public/browser/download_destination_controller.h:21: // Note that this
interface does not deal with cross-thread lifetime
Would it make sense to make this class SupportsWeakPtr so that DownloadItemImpl
doesn't need to gain a method?
Or make DII derive SWP directly?

http://codereview.chromium.org/10799005/diff/1038/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10799005/diff/1038/content/browser/download/do...
content/browser/download/download_item_impl.cc:327: return
weak_ptr_factory_.GetWeakPtr();
Why don't you want to use SupportsWeakPtr::AsWeakPtr()?

http://codereview.chromium.org/10799005/diff/1038/content/browser/download/do...
content/browser/download/download_item_impl.cc:486: // Safe because we control
download file lifetime.
Couldn't you use weak pointers to bounce a callback off DII?
Everytime I see base::Unretained, I have a heart attack. Reassuring comments are
like flattery from a stranger who wants your money. Maybe they're true, maybe
they used to be true, either way I have a hard time believing them.

http://codereview.chromium.org/10799005/diff/1038/content/public/browser/down...
File content/public/browser/download_destination_controller.h (right):

http://codereview.chromium.org/10799005/diff/1038/content/public/browser/down...
content/public/browser/download_destination_controller.h:17: // bytes_written,
and |reason| is errors occuring on writing data).
Then perhaps rename |bytes_so_far| to |bytes_written|?

[Randy Smith (Not in Mondays), 2012-07-30 01:07:23]

PTAL.  John, if you don't have any comments on the */test directories, could you
ack Scott's handoff just so I don't need to be paranoid that those directories
aren't being reviewed.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:27: // be
DOWNLOAD_INTERRUPT_REASON_NONE; on a failed rename, it will be
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> rename? I thought this was talking about Initialize?

Done.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:79: static int number_active_objects_;
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Could this be done more cleanly using another interface?

I'm not quite sure what you have in mind, but I'm happy to consider cleaner ways
of accomplishing this :-J.  Say a bit more?

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:80: };
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> DISALLOW_COPY_AND_ASSIGN while you're here?

My inclination is to avoid DISALLOW_COPY_AND_ASSIGN for abstract base classes. 
They can't be copied anyway (because they can't exist, and the copy operation
involves constructing an object of the class), and specifying a copy constructor
(as DCAA does) means that there is no default constructor for the class, which
means you have to write one, which for a class with no data members and no
defined methods seems extreme.  For instance, DownloadItem doesn't have DCAA,
but DownloadItemImpl does.  DownloadFileImpl also has it.  How badly do you want
it here?

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_impl.cc:71: BrowserThread::UI, FROM_HERE,
base::Bind(
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Document somewhere the fact that callback is called on the ui thread?

Done in the comment before the DownloadFile::Initialize routine.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:58: // Specific to this
class.
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Specific to MockDDC or c::DDC? As opposed to generic to which other class?
> 
> And what is the significance of its being specific to which class?
> 
> Only three questions, you'll have to try harder to be more cryptic next time.
> Greek is too easy, maybe use Egyptian hieroglyphs, I only know a couple of
> those.

Comment rewritten in Egyptian; let me know if it's properly confusing now :-}.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:119: {
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> brace on previous line, please.

Whoops; done

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file_unittest.cc:138:
content::DownloadSaveInfo(), GURL(), GURL(), 0, calculate_hash,
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Can you please name all these fiddly little things somehow?
> Us in a few weeks/months: "What is that '0' again? Is referrer the first or
> second GURL?"

I've commented those arguments that don't seem obvious to me by type; let me
know what you think.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_item_impl.h:221: // For weak pointer
downlcasting.
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> I have the same problem! You start typing some word that happens to start with
> "down", and before you know it you've already finished typing "load".
> 
> We need foot pedals that type "Download" and "download" so that we never have
to
> type them and our brains can start to heal some of the damage.

I couldn't figure out what you were talking about for a moment (I mean, I know
exactly the experience you're talking about, but couldn't figure out how it
related to this line), but you got me LOLing when I did.  Fixed.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_item_impl.h:431: // pointer may only be used
or destroyed on the FILE thread.
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Then maybe it shouldn't be scoped_ptr? scoped_ptr suggests that this member
will
> be deleted when the owner is deleted.
> Is there a generalization of scoped_ptr that affords
deletion-on-another-thread?

Well, I want to strongly indicate the ownership relationship.  I don't think
there's a generalization of scoped_ptr<>, but I do have a DCHECK() in the
destructor of DownloadItem that the scoped_ptr is null at that point.  That good
enough for expressing ownership but warding off wrong-thread destruction?

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
File content/public/browser/download_destination_controller.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
content/public/browser/download_destination_controller.h:14: // Class that
receives asynchronous events from a DownloadDestination
On 2012/07/24 20:47:02, John Abd-El-Malek wrote:
> nit: here and below, make the comment take up to 80 chars

I've reformatted to come closer to the 80 character limit, which is what I think
you wanted; let me know if I missed the point.

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
content/public/browser/download_destination_controller.h:21: // Note that this
interface does not deal with cross-thread lifetime
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Would it make sense to make this class SupportsWeakPtr so that
DownloadItemImpl
> doesn't need to gain a method?

That's not the reason for the method on DownloadItemImpl; that method is there
so that I can make DII inherit privately from this class and only export the
pointer where I choose.  Of course, the method is public, so that's a bit
disingenuous :-}.  Also ...

> Or make DII derive SWP directly?

... there's a thread on chromium-dev about the sublties of SWP and why you
should prefer weak pointer factories.  I think it's possible to use SWP
correctly, but that it's easier to use factories correctly, so I went in this
direction.  But I don't have strong objections to SWP for DII (not DI :-}) if I
can convince myself that it doesn't fall into the traps discussed on
chromium-dev.  How much would you prefer DII inheriting from SWP?  (I *think*
the dangers are less if we make the derived class inherit from SWP rather than
the base class.)

http://codereview.chromium.org/10799005/diff/7001/content/public/browser/down...
content/public/browser/download_destination_controller.h:23: class
DownloadDestinationController {
On 2012/07/24 20:47:02, John Abd-El-Malek wrote:
> nit: this is "Controller" but it just observes things and doesn't control
> anything. can it be called Observer to be more like other content api classes?


Done.

http://codereview.chromium.org/10799005/diff/1038/content/browser/download/do...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10799005/diff/1038/content/browser/download/do...
content/browser/download/download_item_impl.cc:327: return
weak_ptr_factory_.GetWeakPtr();
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Why don't you want to use SupportsWeakPtr::AsWeakPtr()?

A couple of reasons.  My usual reason (I don't want to export it as part of the
public interface) doesn't apply since this method is part of the public
interface.  But there was a recent thread on chromium-dev about the relationship
between weak pointer lifetimes and the lifetimes of the objects they point at,
and using a factory allows you to guarantee that the weak pointer is invalidated
before the member variable that you can get at through it are destructed.  Also,
to use SupportsWeakPtr I'd have to make the base class inherit (which is even
worse for the lifetime issue) or make the derived class inherit and downcast the
weak pointer (which I think I could do, but if I do it externally I'd have to do
the friend declaration for whatever method I actually did the downcasting in).

The above is a bit of a handwave, in that I'm not sure that I couldn't use
SupportsWeakPtr cleanly without lifetime concerns.  But I am (pretty :-}) sure I
can use the factory that way, so I figured I'd go with it.

http://codereview.chromium.org/10799005/diff/1038/content/browser/download/do...
content/browser/download/download_item_impl.cc:486: // Safe because we control
download file lifetime.
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Couldn't you use weak pointers to bounce a callback off DII?

I assume you mean off of the DownloadFile; I am using weak pointers for the
DownloadItemImpl.

> Everytime I see base::Unretained, I have a heart attack. Reassuring comments
are
> like flattery from a stranger who wants your money. Maybe they're true, maybe
> they used to be true, either way I have a hard time believing them.

So trust but verify :-}.  I think it's a pretty important part of the design of
this CL that DownloadItemImpl owns the DownloadFile and controls its lifetime. 
For reassurance around this PostTask, you simply need to believe that nothing
besides the DII can destroy the DF, and that the DII doesn't hold on to a
pointer to the DF once it's destroyed it.  I think those two assumptions are
core enough to this CL that they can be relied on :-}.

I don't want to use a WeakPtr because then I need to expose the DownloadFile
supporting weak pointers as part of the DownloadFile interface, which
complicates the interface.

http://codereview.chromium.org/10799005/diff/1038/content/public/browser/down...
File content/public/browser/download_destination_controller.h (right):

http://codereview.chromium.org/10799005/diff/1038/content/public/browser/down...
content/public/browser/download_destination_controller.h:17: // bytes_written,
and |reason| is errors occuring on writing data).
On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> Then perhaps rename |bytes_so_far| to |bytes_written|?

Hmmm.  What I was trying to express with this comment was the point at which the
byte flow was being measured/should be reported from was when the bytes had
landed in their final location, not when they had been consumed from the
incoming stream.  I'm not sure "bytes_written" does that any better than
"bytes_so_far" as the (abstract) download destination may not write the bytes,
it may display them, or make origami with them, or something else.  

I'll try to rewrite this comment to be clearer about what I'm trying to get at. 
If you find "bytes_so_far" confusing in itself, let me know.

[jam, 2012-07-31 22:52:03]

(ugh, I swear I typed my reply but I must have not sent it)

On Sun, Jul 29, 2012 at 6:07 PM, <rdsmith@chromium.org> wrote:

> PTAL.  John, if you don't have any comments on the */test directories,
> could you
> ack Scott's handoff just so I don't need to be paranoid that those
> directories
> aren't being reviewed.


sure. i was just waiting for the other reviewer. also, the new file you're
adding in contnet/public isn't there.

>
>
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file.h<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file.h>
> File content/browser/download/**download_file.h (right):
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file.h#newcode27<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file.h#newcode27>
> content/browser/download/**download_file.h:27: // be
> DOWNLOAD_INTERRUPT_REASON_**NONE; on a failed rename, it will be
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> rename? I thought this was talking about Initialize?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file.h#newcode79<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file.h#newcode79>
> content/browser/download/**download_file.h:79: static int
> number_active_objects_;
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Could this be done more cleanly using another interface?
>>
>
> I'm not quite sure what you have in mind, but I'm happy to consider
> cleaner ways of accomplishing this :-J.  Say a bit more?
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file.h#newcode80<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file.h#newcode80>
> content/browser/download/**download_file.h:80: };
>
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> DISALLOW_COPY_AND_ASSIGN while you're here?
>>
>
> My inclination is to avoid DISALLOW_COPY_AND_ASSIGN for abstract base
> classes.  They can't be copied anyway (because they can't exist, and the
> copy operation involves constructing an object of the class), and
> specifying a copy constructor (as DCAA does) means that there is no
> default constructor for the class, which means you have to write one,
> which for a class with no data members and no defined methods seems
> extreme.  For instance, DownloadItem doesn't have DCAA, but
> DownloadItemImpl does.  DownloadFileImpl also has it.  How badly do you
> want it here?
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file_impl.cc<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file_impl.cc>
> File content/browser/download/**download_file_impl.cc (right):
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file_impl.cc#**newcode71<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file_impl.cc#newcode71>
> content/browser/download/**download_file_impl.cc:71: BrowserThread::UI,
> FROM_HERE, base::Bind(
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Document somewhere the fact that callback is called on the ui thread?
>>
>
> Done in the comment before the DownloadFile::Initialize routine.
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file_unittest.cc<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file_unittest.cc>
> File content/browser/download/**download_file_unittest.cc (right):
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file_unittest.cc#**newcode58<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file_unittest.cc#newcode58>
> content/browser/download/**download_file_unittest.cc:58: // Specific to
> this class.
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Specific to MockDDC or c::DDC? As opposed to generic to which other
>>
> class?
>
>  And what is the significance of its being specific to which class?
>>
>
>  Only three questions, you'll have to try harder to be more cryptic
>>
> next time.
>
>> Greek is too easy, maybe use Egyptian hieroglyphs, I only know a
>>
> couple of
>
>> those.
>>
>
> Comment rewritten in Egyptian; let me know if it's properly confusing
> now :-}.
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file_unittest.cc#**newcode119<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file_unittest.cc#newcode119>
> content/browser/download/**download_file_unittest.cc:119: {
>
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> brace on previous line, please.
>>
>
> Whoops; done
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_file_unittest.cc#**newcode138<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_file_unittest.cc#newcode138>
> content/browser/download/**download_file_unittest.cc:138:
> content::DownloadSaveInfo(), GURL(), GURL(), 0, calculate_hash,
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Can you please name all these fiddly little things somehow?
>> Us in a few weeks/months: "What is that '0' again? Is referrer the
>>
> first or
>
>> second GURL?"
>>
>
> I've commented those arguments that don't seem obvious to me by type;
> let me know what you think.
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_item_impl.h<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_item_impl.h>
> File content/browser/download/**download_item_impl.h (right):
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_item_impl.h#**newcode221<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_item_impl.h#newcode221>
> content/browser/download/**download_item_impl.h:221: // For weak pointer
> downlcasting.
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> I have the same problem! You start typing some word that happens to
>>
> start with
>
>> "down", and before you know it you've already finished typing "load".
>>
>
>  We need foot pedals that type "Download" and "download" so that we
>>
> never have to
>
>> type them and our brains can start to heal some of the damage.
>>
>
> I couldn't figure out what you were talking about for a moment (I mean,
> I know exactly the experience you're talking about, but couldn't figure
> out how it related to this line), but you got me LOLing when I did.
> Fixed.
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/browser/download/**download_item_impl.h#**newcode431<http://codereview.chromium.org/10799005/diff/7001/content/browser/download/download_item_impl.h#newcode431>
> content/browser/download/**download_item_impl.h:431: // pointer may only
> be used or destroyed on the FILE thread.
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Then maybe it shouldn't be scoped_ptr? scoped_ptr suggests that this
>>
> member will
>
>> be deleted when the owner is deleted.
>> Is there a generalization of scoped_ptr that affords
>>
> deletion-on-another-thread?
>
> Well, I want to strongly indicate the ownership relationship.  I don't
> think there's a generalization of scoped_ptr<>, but I do have a DCHECK()
> in the destructor of DownloadItem that the scoped_ptr is null at that
> point.  That good enough for expressing ownership but warding off
> wrong-thread destruction?
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/public/browser/**download_destination_**controller.h<http://codereview.chromium.org/10799005/diff/7001/content/public/browser/download_destination_controller.h>
> File content/public/browser/**download_destination_**controller.h (right):
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/public/browser/**download_destination_**controller.h#newcode14<http://codereview.chromium.org/10799005/diff/7001/content/public/browser/download_destination_controller.h#newcode14>
> content/public/browser/**download_destination_**controller.h:14: // Class
> that receives asynchronous events from a DownloadDestination
> On 2012/07/24 20:47:02, John Abd-El-Malek wrote:
>
>> nit: here and below, make the comment take up to 80 chars
>>
>
> I've reformatted to come closer to the 80 character limit, which is what
> I think you wanted; let me know if I missed the point.
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/public/browser/**download_destination_**controller.h#newcode21<http://codereview.chromium.org/10799005/diff/7001/content/public/browser/download_destination_controller.h#newcode21>
> content/public/browser/**download_destination_**controller.h:21: // Note
> that this interface does not deal with cross-thread lifetime
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Would it make sense to make this class SupportsWeakPtr so that
>>
> DownloadItemImpl
>
>> doesn't need to gain a method?
>>
>
> That's not the reason for the method on DownloadItemImpl; that method is
> there so that I can make DII inherit privately from this class and only
> export the pointer where I choose.  Of course, the method is public, so
> that's a bit disingenuous :-}.  Also ...
>
>
>  Or make DII derive SWP directly?
>>
>
> ... there's a thread on chromium-dev about the sublties of SWP and why
> you should prefer weak pointer factories.  I think it's possible to use
> SWP correctly, but that it's easier to use factories correctly, so I
> went in this direction.  But I don't have strong objections to SWP for
> DII (not DI :-}) if I can convince myself that it doesn't fall into the
> traps discussed on chromium-dev.  How much would you prefer DII
> inheriting from SWP?  (I *think* the dangers are less if we make the
> derived class inherit from SWP rather than the base class.)
>
>
> http://codereview.chromium.**org/10799005/diff/7001/**
>
content/public/browser/**download_destination_**controller.h#newcode23<http://codereview.chromium.org/10799005/diff/7001/content/public/browser/download_destination_controller.h#newcode23>
> content/public/browser/**download_destination_**controller.h:23: class
> DownloadDestinationController {
> On 2012/07/24 20:47:02, John Abd-El-Malek wrote:
>
>> nit: this is "Controller" but it just observes things and doesn't
>>
> control
>
>> anything. can it be called Observer to be more like other content api
>>
> classes?
>
> Done.
>
>
> http://codereview.chromium.**org/10799005/diff/1038/**
>
content/browser/download/**download_item_impl.cc<http://codereview.chromium.org/10799005/diff/1038/content/browser/download/download_item_impl.cc>
> File content/browser/download/**download_item_impl.cc (right):
>
> http://codereview.chromium.**org/10799005/diff/1038/**
>
content/browser/download/**download_item_impl.cc#**newcode327<http://codereview.chromium.org/10799005/diff/1038/content/browser/download/download_item_impl.cc#newcode327>
> content/browser/download/**download_item_impl.cc:327: return
> weak_ptr_factory_.GetWeakPtr()**;
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Why don't you want to use SupportsWeakPtr::AsWeakPtr()?
>>
>
> A couple of reasons.  My usual reason (I don't want to export it as part
> of the public interface) doesn't apply since this method is part of the
> public interface.  But there was a recent thread on chromium-dev about
> the relationship between weak pointer lifetimes and the lifetimes of the
> objects they point at, and using a factory allows you to guarantee that
> the weak pointer is invalidated before the member variable that you can
> get at through it are destructed.  Also, to use SupportsWeakPtr I'd have
> to make the base class inherit (which is even worse for the lifetime
> issue) or make the derived class inherit and downcast the weak pointer
> (which I think I could do, but if I do it externally I'd have to do the
> friend declaration for whatever method I actually did the downcasting
> in).
>
> The above is a bit of a handwave, in that I'm not sure that I couldn't
> use SupportsWeakPtr cleanly without lifetime concerns.  But I am (pretty
> :-}) sure I can use the factory that way, so I figured I'd go with it.
>
>
> http://codereview.chromium.**org/10799005/diff/1038/**
>
content/browser/download/**download_item_impl.cc#**newcode486<http://codereview.chromium.org/10799005/diff/1038/content/browser/download/download_item_impl.cc#newcode486>
> content/browser/download/**download_item_impl.cc:486: // Safe because we
> control download file lifetime.
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Couldn't you use weak pointers to bounce a callback off DII?
>>
>
> I assume you mean off of the DownloadFile; I am using weak pointers for
> the DownloadItemImpl.
>
>
>  Everytime I see base::Unretained, I have a heart attack. Reassuring
>>
> comments are
>
>> like flattery from a stranger who wants your money. Maybe they're
>>
> true, maybe
>
>> they used to be true, either way I have a hard time believing them.
>>
>
> So trust but verify :-}.  I think it's a pretty important part of the
> design of this CL that DownloadItemImpl owns the DownloadFile and
> controls its lifetime.  For reassurance around this PostTask, you simply
> need to believe that nothing besides the DII can destroy the DF, and
> that the DII doesn't hold on to a pointer to the DF once it's destroyed
> it.  I think those two assumptions are core enough to this CL that they
> can be relied on :-}.
>
> I don't want to use a WeakPtr because then I need to expose the
> DownloadFile supporting weak pointers as part of the DownloadFile
> interface, which complicates the interface.
>
>
> http://codereview.chromium.**org/10799005/diff/1038/**
>
content/public/browser/**download_destination_**controller.h<http://codereview.chromium.org/10799005/diff/1038/content/public/browser/download_destination_controller.h>
> File content/public/browser/**download_destination_**controller.h (right):
>
> http://codereview.chromium.**org/10799005/diff/1038/**
>
content/public/browser/**download_destination_**controller.h#newcode17<http://codereview.chromium.org/10799005/diff/1038/content/public/browser/download_destination_controller.h#newcode17>
> content/public/browser/**download_destination_**controller.h:17: //
> bytes_written, and |reason| is errors occuring on writing data).
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
>
>> Then perhaps rename |bytes_so_far| to |bytes_written|?
>>
>
> Hmmm.  What I was trying to express with this comment was the point at
> which the byte flow was being measured/should be reported from was when
> the bytes had landed in their final location, not when they had been
> consumed from the incoming stream.  I'm not sure "bytes_written" does
> that any better than "bytes_so_far" as the (abstract) download
> destination may not write the bytes, it may display them, or make
> origami with them, or something else.
>
> I'll try to rewrite this comment to be clearer about what I'm trying to
> get at.  If you find "bytes_so_far" confusing in itself, let me know.
>
>
http://codereview.chromium.**org/10799005/<http://codereview.chromium.org/107...
>

[benjhayden, 2012-08-01 18:00:18]

Can you update the pathways graphs and link to them? I'm having trouble keeping
track of how everything fits together in this brave new world. It might be
faster to talk about it in person, especially since I'm about to mail off
DownloadHistory Observer, which, as Jobs would say, changes everything. Again.

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:79: static int number_active_objects_;
On 2012/07/30 01:07:23, rdsmith wrote:
> On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> > Could this be done more cleanly using another interface?
> 
> I'm not quite sure what you have in mind, but I'm happy to consider cleaner
ways
> of accomplishing this :-J.  Say a bit more?
> 

Methods on DownloadDestinationObserver like OnInitialized()/OnDestroyed(), or
use a MockDownloadFileFactory to monitor DownloadFile creation events?
If you don't like either of those, let's look at what you need this field for. I
presume you want to test that the right number of DownloadFiles were
created/destroyed, but that seems like an unusual assertion to want to make in
the first place. So what are you really trying to test? Maybe there's a more
direct way to test it using mocks.

I traced this to EnsureNoPendingDownloads in download_browsertest.cc, which I
see predates this CL. Is this probing for in-progress downloads that haven't
been persisted yet? Could you use OnDownloadCreated() for that instead? Perhaps
a job for another CL, perhaps not; I'd like to see where you plan on taking this
mechanism.

I only challenge this field because it seems like something you would challenge
if I wrote it, although perhaps not. It *looks* like a hack to me, which, of
course, says nothing about whether or not it actually is a hack, which is why I
ask what you're really trying to do. Does it look like a hack to you? If not,
why not?

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_file_factory.h (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_file_factory.h:42: };  // namespace content
No semicolon after namespace close brace.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_file_unittest.cc:58: // Doesn't override any
methods in the base class.  Used to make sure
Much better!
Could you expose bytes_ from the fixture to the tests instead?
EXPECT_EQ(0, bytes());
EXPECT_EQ(strlen(...), bytes());

[benjhayden, 2012-08-01 18:06:01]

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:84:
scoped_ptr<content::DownloadFile> download_file) {
OVERRIDE?

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:209: // Save to null
this out even if it doesn't do anything because none
Safe*?

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:213: virtual void
DelegateStart(DownloadItemImpl* download) {
OVERRIDE?

[Randy Smith (Not in Mondays), 2012-08-02 20:41:23]

Results of in-person discussion between Ben & I.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:125: // as the function accepts
ownership and does not transfer it on.
Describe this as "PostTaskAndReply" with target object deletion on target thread
rather than original thread.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:531: DCHECK(!all_data_saved_);
DCHECK is in progress.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:729: // Lost Cancel race.
Nuke comment.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:763: // Lost Cancel race.
Nuke comment.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:785:
DCHECK(download_file_.get());
Stick in TODO about unification with above and dependencies on removing last
couple of lines of code.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:958:
DCHECK(download_file_.get());
Regularize CL so I only have existence DCHECKs when I would crash on a different
thread.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:986:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Pull up into callers in this routine--ordering change looks ok.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:1033: Interrupt(reason);
Put a comment in about not wanting to merge these two routines because we want
to merge interrupt with cancel.

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

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:400:
CreateDownloadItem(info.get(), bound_net_log);
Add comment about why create download item first (want complexity of two states
(have/not-have DF) in DI since DI is higher level and already needs those two
states).

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl.h:103: virtual
content::DownloadFileFactory* GetDownloadFileFactory();
Add "ForTesting" suffix and change use inside DMI to use member variable.

[Randy Smith (Not in Mondays), 2012-08-02 23:07:54]

On 2012/07/31 22:52:03, John Abd-El-Malek wrote:
> (ugh, I swear I typed my reply but I must have not sent it)
> 
> On Sun, Jul 29, 2012 at 6:07 PM, <mailto:rdsmith@chromium.org> wrote:
> 
> > PTAL.  John, if you don't have any comments on the */test directories,
> > could you
> > ack Scott's handoff just so I don't need to be paranoid that those
> > directories
> > aren't being reviewed.
> 
> 
> sure. i was just waiting for the other reviewer. also, the new file you're
> adding in contnet/public isn't there.

Ooops, quite right.  Should be fixed in PS7 (only change in that PS).

[Randy Smith (Not in Mondays), 2012-08-03 17:32:44]

I've incorporated all comments; PTAL.

Items still on my plate:

* Fix two try job problems (one in internet zone setting on windows, one a
use-after-free in the file errors test).

* Sync up to LKGR (specifically including merging with Asanka's File Name
Determination Refactor, which may produce some important changes in the
initiation flow).

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10799005/diff/7001/content/browser/download/do...
content/browser/download/download_file.h:79: static int number_active_objects_;
On 2012/08/01 18:00:18, benjhayden_chromium wrote:
> On 2012/07/30 01:07:23, rdsmith wrote:
> > On 2012/07/25 15:19:16, benjhayden_chromium wrote:
> > > Could this be done more cleanly using another interface?
> > 
> > I'm not quite sure what you have in mind, but I'm happy to consider cleaner
> ways
> > of accomplishing this :-J.  Say a bit more?
> > 
> 
> Methods on DownloadDestinationObserver like OnInitialized()/OnDestroyed(), or
> use a MockDownloadFileFactory to monitor DownloadFile creation events?
> If you don't like either of those, let's look at what you need this field for.
I
> presume you want to test that the right number of DownloadFiles were
> created/destroyed, but that seems like an unusual assertion to want to make in
> the first place. So what are you really trying to test? Maybe there's a more
> direct way to test it using mocks.
> 
> I traced this to EnsureNoPendingDownloads in download_browsertest.cc, which I
> see predates this CL. Is this probing for in-progress downloads that haven't
> been persisted yet? Could you use OnDownloadCreated() for that instead?
Perhaps
> a job for another CL, perhaps not; I'd like to see where you plan on taking
this
> mechanism.
> 
> I only challenge this field because it seems like something you would
challenge
> if I wrote it, although perhaps not. It *looks* like a hack to me, which, of
> course, says nothing about whether or not it actually is a hack, which is why
I
> ask what you're really trying to do. Does it look like a hack to you? If not,
> why not?

We talked about this some in person, and I suspect you got from my tone of voice
that I do indeed consider this a hack :-}.  And you're quite right, if you had
something like this, I'd challenge it (though I hope I'd be a bit less inclined
to challenge it if you were cleaning it up than if you created it in the first
place :-}).

Having said all that.  Hmmm.  The question the test that uses this is trying to
answer is "Did I do something stupid, way down there at the bottom of the stack,
during the course of  doing Everything Involved In a Download?"  So I do think
that we need to have this accessible from a browser test.  Your idea of using a
DownloadFileFactory to wrap DownloadFiles and count it that way is an
interesting one, though, and one I think I like better than what we're currently
doing.  The code would be more complex overall, but all the complexity would be
in test code and we could simply yank the static field and the "Ensure" method
out of the chrome pool.  Which I like.  

Having said *that*: Doing that change isn't really in line with what this CL is
doing.  Willing to let me do it in a separate CL?

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_file_factory.h (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_file_factory.h:42: };  // namespace content
On 2012/08/01 18:00:18, benjhayden_chromium wrote:
> No semicolon after namespace close brace.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_file_unittest.cc (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_file_unittest.cc:58: // Doesn't override any
methods in the base class.  Used to make sure
On 2012/08/01 18:00:18, benjhayden_chromium wrote:
> Much better!
> Could you expose bytes_ from the fixture to the tests instead?
> EXPECT_EQ(0, bytes());
> EXPECT_EQ(strlen(...), bytes());

I want to make sure that the last progress update before the
DestinationCompleted() (not any either before or after that) has the correct
value.  So the DestinationCompleted itself has to do the check.  I'm not sure
how to pass the value it should check against through the GMock interface; I'm
currently using "InvokeWithoutArgs" (and with args is with the args to argument
of the EXPECT_CALL).  I suppose I could have a separate member variable in the
fixture "expected_bytes_", which I could set before calling FinishStream, and do
an EXPECT_EQ(expected_bytes_, bytes_) inside of ConfirmUpdateDownloadInfo; would
you consider that better or worse than what I currently have?  (I don't find
myself with an opinion--neither's great, and it's in test code.)

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_item_impl.cc (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:125: // as the function accepts
ownership and does not transfer it on.
On 2012/08/02 20:41:23, rdsmith wrote:
> Describe this as "PostTaskAndReply" with target object deletion on target
thread
> rather than original thread.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:531: DCHECK(!all_data_saved_);
On 2012/08/02 20:41:23, rdsmith wrote:
> DCHECK is in progress.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:729: // Lost Cancel race.
On 2012/08/02 20:41:23, rdsmith wrote:
> Nuke comment.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:763: // Lost Cancel race.
On 2012/08/02 20:41:23, rdsmith wrote:
> Nuke comment.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:785:
DCHECK(download_file_.get());
On 2012/08/02 20:41:23, rdsmith wrote:
> Stick in TODO about unification with above and dependencies on removing last
> couple of lines of code.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:958:
DCHECK(download_file_.get());
On 2012/08/02 20:41:23, rdsmith wrote:
> Regularize CL so I only have existence DCHECKs when I would crash on a
different
> thread.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:986:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/08/02 20:41:23, rdsmith wrote:
> Pull up into callers in this routine--ordering change looks ok.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_item_impl.cc:1033: Interrupt(reason);
On 2012/08/02 20:41:23, rdsmith wrote:
> Put a comment in about not wanting to merge these two routines because we want
> to merge interrupt with cancel.

Done.

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

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl.cc:400:
CreateDownloadItem(info.get(), bound_net_log);
On 2012/08/02 20:41:23, rdsmith wrote:
> Add comment about why create download item first (want complexity of two
states
> (have/not-have DF) in DI since DI is higher level and already needs those two
> states).

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_manager_impl.h (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl.h:103: virtual
content::DownloadFileFactory* GetDownloadFileFactory();
On 2012/08/02 20:41:23, rdsmith wrote:
> Add "ForTesting" suffix and change use inside DMI to use member variable.

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
File content/browser/download/download_manager_impl_unittest.cc (right):

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:84:
scoped_ptr<content::DownloadFile> download_file) {
On 2012/08/01 18:06:02, benjhayden_chromium wrote:
> OVERRIDE?

Ooops.  Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:209: // Save to null
this out even if it doesn't do anything because none
On 2012/08/01 18:06:02, benjhayden_chromium wrote:
> Safe*?

Done.

http://codereview.chromium.org/10799005/diff/21002/content/browser/download/d...
content/browser/download/download_manager_impl_unittest.cc:213: virtual void
DelegateStart(DownloadItemImpl* download) {
On 2012/08/01 18:06:02, benjhayden_chromium wrote:
> OVERRIDE?

Done.

[Randy Smith (Not in Mondays), 2012-08-05 05:35:09]

I've fixed the try job problems and merged to LKGR.  I think this is getting
close to ready to land.  WDYT? :-}

Asanka, if you could, I'd like you to review my merge with FNDR III.  That's
PS9->10 (though 10->11 had only one change, so you could just do "since PS 9")
download_manager_impl.cc, download_item_impl.cc.  Thanks!

[jam, 2012-08-06 18:16:37]

(I mostly looked at my files, but I scanned a couple of other headers and hence
the comments)

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_file.h:77: static int
GetNumberOfDownloadFiles();
nit: if this is just for testing, add a "ForTesting" suffix. that way presubmit
checks ensure that only test code uses this, and production code doesn't
inadvertently start using it.

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_item_impl.h:36: private
content::DownloadDestinationObserver {
nit: this is against the style guide:
"All inheritance should be public. If you want to do private inheritance, you
should be including an instance of the base class as a member instead."

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

in this case, the convention is to use public inheritance, but put the
overridden methods in private. it's possible to cast to the parent to get to
them, of course, but that's a strong indication that something wrong is being
done.

http://codereview.chromium.org/10799005/diff/33001/content/public/browser/dow...
File content/public/browser/download_destination_observer.h (right):

http://codereview.chromium.org/10799005/diff/33001/content/public/browser/dow...
content/public/browser/download_destination_observer.h:21: // All methods must
be invoked on the UI thread.
nit: since the content api's documentation is for the embedder, and they're
called back here (instead of calling methods), perhaps "All methods are invoked
on the UI thread."?

[benjhayden, 2012-08-10 15:56:41]

Please also address John's comments.

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_file.h:77: static int
GetNumberOfDownloadFiles();
On 2012/08/06 18:16:37, John Abd-El-Malek wrote:
> nit: if this is just for testing, add a "ForTesting" suffix. that way
presubmit
> checks ensure that only test code uses this, and production code doesn't
> inadvertently start using it.

Looks like this is only called in download_manager_impl.cc, but is indirectly
only called from download_browsertest.cc. How does presubmit feel about that?

Randy, I know we talked about how to get rid of this mechanism in another CL,
but I don't remember if we came up with a particular substitute. Do you
remember?

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

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:215: factory_.reset(new
DownloadItemFactoryImpl());
rename to item_factory_?

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:418: return
download->GetGlobalId();
could StartDownload() return the local id now that DRH is no longer talking to a
DFM?

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:679: void
DownloadManagerImpl::SetDownloadFileFactoryForTesting(
This should be up by GetDownloadFileFactoryForTesting.

[Randy Smith (Not in Mondays), 2012-08-10 20:09:00]

PTAL.  I think this is ready for landing.

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
File content/browser/download/download_file.h (right):

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_file.h:77: static int
GetNumberOfDownloadFiles();
On 2012/08/06 18:16:37, John Abd-El-Malek wrote:
> nit: if this is just for testing, add a "ForTesting" suffix. that way
presubmit
> checks ensure that only test code uses this, and production code doesn't
> inadvertently start using it.

See Ben's comment--this is called from download_manager_impl.cc, but only on the
behalf of download_browsertest.cc.  Leaving as is unless you can guide me as to
a better way to manage this.

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_file.h:77: static int
GetNumberOfDownloadFiles();
On 2012/08/10 15:56:41, benjhayden_chromium wrote:
> On 2012/08/06 18:16:37, John Abd-El-Malek wrote:
> > nit: if this is just for testing, add a "ForTesting" suffix. that way
> presubmit
> > checks ensure that only test code uses this, and production code doesn't
> > inadvertently start using it.
> 
> Looks like this is only called in download_manager_impl.cc, but is indirectly
> only called from download_browsertest.cc. How does presubmit feel about that?
> 
> Randy, I know we talked about how to get rid of this mechanism in another CL,
> but I don't remember if we came up with a particular substitute. Do you
> remember?

Yep.  I hope I wrote it down in my bucket of download cleanup CLs list; checking
.... yep.  The idea is that we use the DownloadFileFactory method on the
DownloadManagerImpl to put in a DownloadFileFactory that creates objects derived
from DownloadFiles that track their creation and destruction this way.  It's
more complex code, but the complexity will all be in the testing code, so it's
worth it.  I intend (:-J) to do that reasonably soon after I land this CL, as I
feel like I sorta promised it to you in another context.

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
File content/browser/download/download_item_impl.h (right):

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_item_impl.h:36: private
content::DownloadDestinationObserver {
On 2012/08/06 18:16:37, John Abd-El-Malek wrote:
> nit: this is against the style guide:
> "All inheritance should be public. If you want to do private inheritance, you
> should be including an instance of the base class as a member instead."
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
> 
> in this case, the convention is to use public inheritance, but put the
> overridden methods in private. it's possible to cast to the parent to get to
> them, of course, but that's a strong indication that something wrong is being
> done.

I had a moment of wanting to do exactly that, but I really need a weak pointer
to the observer, and I want to do that through a factory rather than
SupportsWeakPtr as per recent chromium-dev discussions, so ....  Done.

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

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:215: factory_.reset(new
DownloadItemFactoryImpl());
On 2012/08/10 15:56:41, benjhayden_chromium wrote:
> rename to item_factory_?

Done.

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:418: return
download->GetGlobalId();
On 2012/08/10 15:56:41, benjhayden_chromium wrote:
> could StartDownload() return the local id now that DRH is no longer talking to
a
> DFM?

As discussed offline, punting for another CL.  This could be done, but involves
changing all the callbacks from programmatic downloads, and creating the idea of
an invalid local id.  IMO It's worth doing, just not as extra yak shaving here

http://codereview.chromium.org/10799005/diff/33001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:679: void
DownloadManagerImpl::SetDownloadFileFactoryForTesting(
On 2012/08/10 15:56:41, benjhayden_chromium wrote:
> This should be up by GetDownloadFileFactoryForTesting.

Done.

http://codereview.chromium.org/10799005/diff/33001/content/public/browser/dow...
File content/public/browser/download_destination_observer.h (right):

http://codereview.chromium.org/10799005/diff/33001/content/public/browser/dow...
content/public/browser/download_destination_observer.h:21: // All methods must
be invoked on the UI thread.
On 2012/08/06 18:16:37, John Abd-El-Malek wrote:
> nit: since the content api's documentation is for the embedder, and they're
> called back here (instead of calling methods), perhaps "All methods are
invoked
> on the UI thread."?

Done.

[benjhayden, 2012-08-10 20:16:01]

LGTM Thanks!

[jam, 2012-08-10 23:46:05]

lgtm

http://codereview.chromium.org/10799005/diff/21009/content/public/browser/dow...
File content/public/browser/download_destination_observer.h (right):

http://codereview.chromium.org/10799005/diff/21009/content/public/browser/dow...
content/public/browser/download_destination_observer.h:5: #ifndef
CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_DESTINATION_OBSERVER_H_
nit: add PUBLIC_

[Randy Smith (Not in Mondays), 2012-08-13 18:07:40]

http://codereview.chromium.org/10799005/diff/21009/content/public/browser/dow...
File content/public/browser/download_destination_observer.h (right):

http://codereview.chromium.org/10799005/diff/21009/content/public/browser/dow...
content/public/browser/download_destination_observer.h:5: #ifndef
CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_DESTINATION_OBSERVER_H_
On 2012/08/10 23:46:05, John Abd-El-Malek wrote:
> nit: add PUBLIC_

Whoops.  Done.  Also removed a DOWNLOAD; this was leftover from where the file
used to be.

[commit-bot: I haz the power, 2012-08-13 18:59:51]

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

[commit-bot: I haz the power, 2012-08-13 21:21:29]

Change committed as 151351