Replace the DownloadFileManager with direct ownership of DownloadFileImpl

Replace the DownloadFileManager with direct ownership of DownloadFileImpl
by DownloadItemImpl. 

Relanding of http://codereview.chromium.org/10799005.

BUG=123998
R=benjhayden@chromium.org
R=jam@chromium.org


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

[Randy Smith (Not in Mondays), 2012-09-12 17:56:59]

Ok, this is the latest attempt to land the DownloadFileManager removal.  

Ben: Overall review.
John: content/browser/!download, content/public, content/shell.  I don't think
there's anything materially different here from what you reviewed originally,
but I've done enough transformations that I'd like you to at least scan through
it.  Sorry :-{.
Asanka: There's a conceptual issue I want you to review; I'm changing the timing
of the transition to the COMPLETE state, and I'd like you to evaluate what if
any UI changes are needed.  You may not need to read the CL for this :-}.

Details:

After a whole lot of lands and reverts, I decided to peel off CLs that are
somewhat orthogonal to this and land them first.  As a result, this CL is
dependent on:

* http://codereview.chromium.org/10916201: Making the SavePackage interaction
with its fake download item cleaner, and cleaning up related tests.

* http://codereview.chromium.org/10918136: Making the web store installation be
insensitive to whether or not the completion transition happens before the open
is complete.

Folded into this CL is one big conceptual change from the original one, which is
that the transition to COMPLETE now occurs when we fire off the file detach from
the UI thread.  This has always been our conceptual "commit point"; before this,
downloads could be cancelled or interrupted, after it, we're committed to
completing them (I can talk more about this if needed).  But the code got fuzzy
during some of the refactoring I was doing, and when I looked at making it
unfuzzy, the simplest way was to transition to COMPLETE at the commit point. 
This means that auto-opening (including chrome-internal auto-opening; see web
store installation, above) will occur after the download completes.  This
necessitates creating waiters for the auto-open state transition.

I could have split that change out as well (and arguably, should have) but the
code for that change looks fairly different pre- and post- this CL, so I gave
into a bit of laziness and folded it in.  That decision may need to be
revisited. 

Asanka: One thing I didn't think about was how this affects the UI.  I'm not
sure it makes sense to make a distinction in the UI between chrome-internal
opening and chrome-external opening, so maybe we want to have an "opening" state
that the UI properly expresses?  Let me know what you think I should do, and
I'll implement it.  Thanks ....

Patch set details which may help reviewers:
* PS1 == original (first) CL (http://codereview.chromium.org/10799005).
* PS2 == original CL sync'd to r155400 (past DownloadItem reordering change).
* PS3 == Merge of above two dependent CLs (possibly should have skipped this)
* PS4 == First upload of actual work to review.  Diffing this against PS2 should
show you the new stuff that I've done.
* PS5 == Another sync to LKGR.

[benjhayden, 2012-09-12 21:01:01]

http://codereview.chromium.org/10912173/diff/12001/chrome/browser/download/do...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10912173/diff/12001/chrome/browser/download/do...
chrome/browser/download/download_browsertest.cc:1608: downloads[0],
base::Bind(WasAutoOpened)).WaitForEvent();
Why don't you need/want the address-of '&' operator before the function name?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
File content/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:55: 
Don't you need a virtual destructor?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:64: // storing it in  the
factory that produced this object for later
double space

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:76: // Must be called on the UI
thread.
Instead of these comments, can you just have the DCHECK(IsOnThread...) pattern?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:81: // Must only be accessed on
the FILE thread; must be used only on the
What's the difference between access and use?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:104: void
PostRenameCallback(base::Closure callback);
s/Post/Add/g ?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:106: void
GetRenameCallbacks(std::vector<base::Closure>* results);
s/Get/GetAll/g ?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:114:
base::WeakPtrFactory<DownloadFileWithDelayFactory> factory_;
weak_ptr_factor_?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:118: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:120:
DownloadFileWithDelay::DownloadFileWithDelay(
Since we're in a test, would you mind defining these methods inline with the
class definition?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:225: while
(rename_callbacks_.empty() && detach_callbacks_.empty()) {
'while' or 'if'?

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_file.h:77: // For testing.  Must be called on
FILE thread.
Do you still need this? If so, can you put a TODO here outlining how we might
kill this?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_file_impl.cc:151: // Other platforms don't
currently do source annotation.
Linux does now:
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/download/b...

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.cc:931: // Return does private
downcast.
Actually, this is up-casting.
And also obvious.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1111: download_file_ =
download_file.Pass();
DCHECK(download_file_.get());?

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.h:221: // For weak pointer
downcasting.
Can you explain this a bit more? Who downcasts what when where and why?

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:349:
CreateDownloadItem(info.get(), bound_net_log);
If this is the only caller of CreateDownloadItem, then why not merge it into
this method so we don't need to bounce up and down the file?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:359: // to download and all
the usually setters (e.g. Cancel) work.
usual*

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/s...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/s...
content/browser/download/save_package.cc:761: FinalizeDownloadEntry();
FDE DCHECKs(download_). Can you put this in a new if (download_) block, or else
make FDE() return early if !download_?

http://codereview.chromium.org/10912173/diff/12001/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/public/test/downlo...
content/public/test/download_test_observer.cc:66: if (!item_)
What happened to event_seen_?

http://codereview.chromium.org/10912173/diff/12001/content/shell/shell_downlo...
File content/shell/shell_download_manager_delegate.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/shell/shell_downlo...
content/shell/shell_download_manager_delegate.cc:115:
suggested_path.AddExtension(FILE_PATH_LITERAL(".crdownload")));
Why do this in this CL?

[asanka, 2012-09-13 15:00:21]

On 2012/09/12 17:56:59, rdsmith wrote:
> Ok, this is the latest attempt to land the DownloadFileManager removal.  
> 
> Ben: Overall review.
> John: content/browser/!download, content/public, content/shell.  I don't think
> there's anything materially different here from what you reviewed originally,
> but I've done enough transformations that I'd like you to at least scan
through
> it.  Sorry :-{.
> Asanka: There's a conceptual issue I want you to review; I'm changing the
timing
> of the transition to the COMPLETE state, and I'd like you to evaluate what if
> any UI changes are needed.  You may not need to read the CL for this :-}.
> 
> Details:
> 
> After a whole lot of lands and reverts, I decided to peel off CLs that are
> somewhat orthogonal to this and land them first.  As a result, this CL is
> dependent on:
> 
> * http://codereview.chromium.org/10916201: Making the SavePackage interaction
> with its fake download item cleaner, and cleaning up related tests.
> 
> * http://codereview.chromium.org/10918136: Making the web store installation
be
> insensitive to whether or not the completion transition happens before the
open
> is complete.
> 
> Folded into this CL is one big conceptual change from the original one, which
is
> that the transition to COMPLETE now occurs when we fire off the file detach
from
> the UI thread.  This has always been our conceptual "commit point"; before
this,
> downloads could be cancelled or interrupted, after it, we're committed to
> completing them (I can talk more about this if needed).  But the code got
fuzzy
> during some of the refactoring I was doing, and when I looked at making it
> unfuzzy, the simplest way was to transition to COMPLETE at the commit point. 
> This means that auto-opening (including chrome-internal auto-opening; see web
> store installation, above) will occur after the download completes.  This
> necessitates creating waiters for the auto-open state transition.
> 
> I could have split that change out as well (and arguably, should have) but the
> code for that change looks fairly different pre- and post- this CL, so I gave
> into a bit of laziness and folded it in.  That decision may need to be
> revisited. 
> 
> Asanka: One thing I didn't think about was how this affects the UI.  I'm not
> sure it makes sense to make a distinction in the UI between chrome-internal
> opening and chrome-external opening, so maybe we want to have an "opening"
state
> that the UI properly expresses?  Let me know what you think I should do, and
> I'll implement it.  Thanks ....

I think this affects extension installation. Prior to this patch, once all the
data had been saved and the download had been renamed to the final name, we kept
the download in IN_PROGRESS state and initiated installation of the extension.
Once the installation completed, we transitioned the download item to COMPLETE.

On the shelf, the download item status read 'Adding to Chrome...' while the
installation progresses (download_item.GetState() == IN_PROGRESS &&
download_item.AllDataSaved() && IsExtensionDownload()), and the download item is
removed automatically when installation is done. This was done, as I recall,
because the process of extraction and installation of an extension can take a
while.

With this patch, the download item on the shelf will display the completion
animation and will remain in an interactive state with no indication of the
on-going installation.

> Patch set details which may help reviewers:
> * PS1 == original (first) CL (http://codereview.chromium.org/10799005).
> * PS2 == original CL sync'd to r155400 (past DownloadItem reordering change).
> * PS3 == Merge of above two dependent CLs (possibly should have skipped this)
> * PS4 == First upload of actual work to review.  Diffing this against PS2
should
> show you the new stuff that I've done.
> * PS5 == Another sync to LKGR.

[Randy Smith (Not in Mondays), 2012-09-13 20:15:12]

Responses to comments here, but you may want to hold off on re-review, as I have
to come up with some way to deal with the UI implications of the state
transition change, and that may be a moderately large change.  Stay tuned.

http://codereview.chromium.org/10912173/diff/12001/chrome/browser/download/do...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10912173/diff/12001/chrome/browser/download/do...
chrome/browser/download/download_browsertest.cc:1608: downloads[0],
base::Bind(WasAutoOpened)).WaitForEvent();
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Why don't you need/want the address-of '&' operator before the function name?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
File content/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:55: 
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Don't you need a virtual destructor?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:64: // storing it in  the
factory that produced this object for later
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> double space

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:76: // Must be called on the UI
thread.
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Instead of these comments, can you just have the DCHECK(IsOnThread...)
pattern?

I don't consider the two equivalent; one's interface, and one's implementation. 
However, I noted that that's about as weak an argument as I've personally ever
heard, because a) these are private methods, b) everything is in the same file,
and c) this is test code.  So done.  But it's still not equivalent :-}.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:81: // Must only be accessed on
the FILE thread; must be used only on the
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> What's the difference between access and use?

The variable must be read on the FILE thread, and the value used (indirected
through to get the underlying object) on the UI thread.  Is there a way I could
make this more clear?

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:104: void
PostRenameCallback(base::Closure callback);
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> s/Post/Add/g ?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:106: void
GetRenameCallbacks(std::vector<base::Closure>* results);
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> s/Get/GetAll/g ?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:114:
base::WeakPtrFactory<DownloadFileWithDelayFactory> factory_;
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> weak_ptr_factor_?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:118: };
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:120:
DownloadFileWithDelay::DownloadFileWithDelay(
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Since we're in a test, would you mind defining these methods inline with the
> class definition?

I started out doing that.  The problem is that the two classes refer to each
other, so each's definition section needs the other's declaration section before
it.  Result->Split.  But if there's some way to do this in C++ inline, I agree
with you--it would be nicer.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_browsertest.cc:225: while
(rename_callbacks_.empty() && detach_callbacks_.empty()) {
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> 'while' or 'if'?

Presuming comment means you prefer 'if'; done.

(The argument for 'while' is that it guarantees the state for the caller of
WaitForSomeCallback, i.e. no other message loop tasks stole the callbacks.  But
if someone's stealing the callbacks that code's probably pretty messed up, so
I'm good either way.)

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_file.h:77: // For testing.  Must be called on
FILE thread.
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Do you still need this? If so, can you put a TODO here outlining how we might
> kill this?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
File content/browser/download/download_file_impl.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_file_impl.cc:151: // Other platforms don't
currently do source annotation.
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Linux does now:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/download/b...

Fair enough, but whoever put that in should have updated the comment :-} (it
used to be in download_file_manager.cc).  I've consed up something halfway
plausible.

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.cc:931: // Return does private
downcast.
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Actually, this is up-casting.
> And also obvious.

The fact that it's private wasn't completely obvious (at least, it wasn't too
me).  But the comment's gone now.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1111: download_file_ =
download_file.Pass();
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> DCHECK(download_file_.get());?

Equivalent done.

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_item_impl.h:221: // For weak pointer
downcasting.
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Can you explain this a bit more? Who downcasts what when where and why?

Whoops, no longer needed; removed.

It was originally needed when DownloadItemImpl inherited privately from
DownloadDestinationObserver; at that point the template class had to be a friend
so that it could do the casting to DDO* from DII* implied in the weak pointer
cast.

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

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:349:
CreateDownloadItem(info.get(), bound_net_log);
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> If this is the only caller of CreateDownloadItem, then why not merge it into
> this method so we don't need to bounce up and down the file?

It's currently used in the unit tests for creating regular downloads on the
manager.  In an ideal world the routine you're putting together for the history
observer change would allow creating arbitrary (not just history) downloads, and
we could pull this out (or call that).  But for now, I'd like to keep it.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:359: // to download and all
the usually setters (e.g. Cancel) work.
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> usual*

Done.

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/s...
File content/browser/download/save_package.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/browser/download/s...
content/browser/download/save_package.cc:761: FinalizeDownloadEntry();
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> FDE DCHECKs(download_). Can you put this in a new if (download_) block, or
else
> make FDE() return early if !download_?

Done.

http://codereview.chromium.org/10912173/diff/12001/content/public/test/downlo...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/public/test/downlo...
content/public/test/download_test_observer.cc:66: if (!item_)
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> What happened to event_seen_?

I think that's a merge failure/I haven't merged in that CL yet.  I'll check
after the next sync and make sure it's there.

http://codereview.chromium.org/10912173/diff/12001/content/shell/shell_downlo...
File content/shell/shell_download_manager_delegate.cc (right):

http://codereview.chromium.org/10912173/diff/12001/content/shell/shell_downlo...
content/shell/shell_download_manager_delegate.cc:115:
suggested_path.AddExtension(FILE_PATH_LITERAL(".crdownload")));
On 2012/09/12 21:01:01, benjhayden_chromium wrote:
> Why do this in this CL?

As it turns out, without this change, we don't *have* an intermediate name in
the content_browsertests, and we take the !NeedsRename() path.  This means we
can't test the commit point logic (cancel succeeds before, fails after) the same
way.  And the "real code" (chrome) always has an intermediate name, so making
the shell match that made sense to me.

[Randy Smith (Not in Mondays), 2012-09-23 23:46:06]

John, Ben, Asanka: PTAL?  

The major change here is that I've peeled off
http://codereview.chromium.org/10950015/ to be the change in "commit point", and
implemented that somewhat differently than I did in earlier versions of this CL
(separate internal and external state for the DI, so that we can separate out
the state transitions from the interface we show the world).  This CL depends on
that one (is a diff against that one), and should contain only (sic?) the
changes needed for removing the DownloadFileManager.

[benjhayden, 2012-09-24 17:43:27]

http://codereview.chromium.org/10912173/diff/20001/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10912173/diff/20001/content/browser/browser_co...
content/browser/browser_context.cc:93: scoped_ptr<DownloadItemFactory>(),
Would it make more sense to not pass the factories into the ctor, and instead
provide setters for the factories?
If the factories are only overloaded in tests, you could name the setters
*ForTest().

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_file.h:79: // on the DownloadManager is
replaced by a test-specific DownloadFileFactory
Grammar check

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1055:
DCHECK(download_file.get());
It took me a full 10 seconds to notice the underscore at the end of
download_file_ versus download_file in this line. Could you make the parameter
name a little more different? 'new_file', 'the_file', or just 'file'?

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1061: // Safe because we control
download file lifetime.
Just to check: even if an observer Cancels in OnDownloadCreated?
Unretained is still a little unnerving. A shim class could use weak pointers to
itself.

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1124:
DCHECK(!is_save_package_download_);
Why shouldn't SavePackage use OnDownloadTargetDetermined to set the target path?

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.h:473: // pointer may only be used
or destroyed on the FILE thread.
So, the scoped_ptr will never delete the DownloadFile when the DownloadItemImpl
goes out of scope.
Should this really be a scoped_ptr?
Would a slightly different kind of magic pointer or a raw pointer be more
appropriate? It looks like you've already built a custom deletion management
mechanism.
If it can't be de-ref'd on the UI thread at all, then perhaps a shim would be
more appropriate?

[jam, 2012-09-24 19:13:34]

lgtm

[asanka, 2012-09-24 20:43:53]

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
File content/browser/download/download_item_impl_delegate.cc (right):

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl_delegate.cc:30: DownloadItemImpl*
download_item) {}
I know this is perfectly legitimate C++, but why provide a body to a pure
virtual method? Is the subclass supposed to call this?

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:216: void
DownloadManagerImpl::DelegateStart(DownloadItemImpl* item) {
Instead of DelegateStart(), would it make sense to move this logic to
DownloadItemImpl and have it ask the delegate to determine the download target?

[Randy Smith (Not in Mondays), 2012-09-26 21:01:05]

Asanka, Ben: PTAL?  (My apologies about the combination of the sync and the
comment incorporation; if it causes any major problems in review let me know and
I'll back out this PS and create two separate ones).

http://codereview.chromium.org/10912173/diff/20001/content/browser/browser_co...
File content/browser/browser_context.cc (right):

http://codereview.chromium.org/10912173/diff/20001/content/browser/browser_co...
content/browser/browser_context.cc:93: scoped_ptr<DownloadItemFactory>(),
On 2012/09/24 17:43:27, benjhayden_chromium wrote:
> Would it make more sense to not pass the factories into the ctor, and instead
> provide setters for the factories?
> If the factories are only overloaded in tests, you could name the setters
> *ForTest().

Yeah, I think this makes sense.  Done.

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_file.h:79: // on the DownloadManager is
replaced by a test-specific DownloadFileFactory
On 2012/09/24 17:43:27, benjhayden_chromium wrote:
> Grammar check

Done.

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1055:
DCHECK(download_file.get());
On 2012/09/24 17:43:27, benjhayden_chromium wrote:
> It took me a full 10 seconds to notice the underscore at the end of
> download_file_ versus download_file in this line. Could you make the parameter
> name a little more different? 'new_file', 'the_file', or just 'file'?

Done.

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1061: // Safe because we control
download file lifetime.
On 2012/09/24 17:43:27, benjhayden_chromium wrote:
> Just to check: even if an observer Cancels in OnDownloadCreated?

Yep.  Our safety is guaranteed by two things:
* No-one besides the DownloadItem will ever initiate destruction of the
DownloadFile.
* If the DownloadItem initiates it, it'll be by a post to the file thread.  So
all previous file thread methods will have completed before that destruction
occurs. 

> Unretained is still a little unnerving. A shim class could use weak pointers
to
> itself.

So I'm reluctant to put in boilerplate to obscure the central fact here: We own
the download file, and are responsible for tracking its lifetime and making sure
that it is used properly and only while it is alive.  No-one else.  Just us. 
Another way to put it is that no matter what boilerplate I put in, I think I'd
feel uncomfortable with the idea that anyone who didn't understand the lifetime
relationships and guarantees between DI and DF was modifying the DI->DF
communication (DF->DI is much more of a "fire-and-forget; if it's not there I
don't care", which is well summarized by the weak pointer semantics we have in
place).  

This may mean that I haven't explained or commented that lifetime relationship
sufficiently.  If you feel that way, please let me know that and let's work on
finding a better way to communicate it.  But I'm nervous about any coding here
that is designed to let the future maintainer get away with not understanding
the lifetime relationships in their gut--I think it'd fail.  Better to leave the
power tool accessible, and hopefully make people nervous when they pick it up
:-}.

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1124:
DCHECK(!is_save_package_download_);
On 2012/09/24 17:43:27, benjhayden_chromium wrote:
> Why shouldn't SavePackage use OnDownloadTargetDetermined to set the target
path?

I feel like I answered this question in commentary on your CL--if you don't
agree, could you re-raise it?

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl.h:473: // pointer may only be used
or destroyed on the FILE thread.
On 2012/09/24 17:43:27, benjhayden_chromium wrote:
> So, the scoped_ptr will never delete the DownloadFile when the
DownloadItemImpl
> goes out of scope.
> Should this really be a scoped_ptr?
> Would a slightly different kind of magic pointer or a raw pointer be more
> appropriate? It looks like you've already built a custom deletion management
> mechanism.
> If it can't be de-ref'd on the UI thread at all, then perhaps a shim would be
> more appropriate?

Hmmm.  I'm not certain what the right thing to do is here.  What I was looking
for was a way to say "THIS OBJECT IS OWNED BY THE DOWNLOAD ITEM", and
scoped_ptr<> is a generic way to do that.  And yes, the basic "will delete
object when pointer is destroyed" logic isn't very useful here.  

Having said that, I think I'd like to keep it, though I'm open to you pushing
back.  This requires users of the pointer to keep track of ownership; they have
to do a Pass(), or a release(), and you can glance at the code and see the
passing of ownership (to the file thread for deletion in this case).  I think
that has maintainability value even though we're not actually using the
destruction semantics.  WDYT?

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
File content/browser/download/download_item_impl_delegate.cc (right):

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_item_impl_delegate.cc:30: DownloadItemImpl*
download_item) {}
On 2012/09/24 20:43:53, asanka wrote:
> I know this is perfectly legitimate C++, but why provide a body to a pure
> virtual method? Is the subclass supposed to call this?

Fair point, but mooted by your other comment about changing the flow.

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

http://codereview.chromium.org/10912173/diff/20001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:216: void
DownloadManagerImpl::DelegateStart(DownloadItemImpl* item) {
On 2012/09/24 20:43:53, asanka wrote:
> Instead of DelegateStart(), would it make sense to move this logic to
> DownloadItemImpl and have it ask the delegate to determine the download
target?

Given that I'm adding DelegateStart(), I agree with you that doing it right from
the beginning is the right thing.  Dunno why I didn't do it that way originally.
 Done.

[asanka, 2012-09-28 20:22:42]

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

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_file_factory.h:33: const
content::DownloadSaveInfo& save_info,
Here's that extra content:: namespace again :)

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

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl.cc:413:
base::Bind(&DeleteDownloadedFile, current_path_));
Since we can now readily tell whether the DownloadFile is around or not, we can
avoid deleting the file from under it. Correct?

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1253: UpdateObservers();
Why is this call necessary?

http://codereview.chromium.org/10912173/diff/32003/content/browser/download/d...
File content/browser/download/download_item_impl_delegate.cc (right):

http://codereview.chromium.org/10912173/diff/32003/content/browser/download/d...
content/browser/download/download_item_impl_delegate.cc:29: DownloadItemImpl*
download, DownloadTargetCallback callback) {
Why is this a concrete class?

[benjhayden, 2012-09-28 20:49:41]

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

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl.cc:858: if
(!is_save_package_download_) {
Factor this out into a method and share with Cancel()?

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
File content/browser/download/download_item_impl_unittest.cc (right):

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl_unittest.cc:333:
RunAllPendingInMessageLoops();
Why are all these RunAllPending calls necessary?
I've started to think of these as flags that the test code might be flaky.

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

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:595:
DownloadManagerImpl::GetDownloadFileFactoryForTesting() {
Why do we need a getter for the FileFactory but not the ItemFactory?

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

PTAL?

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

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_file_factory.h:33: const
content::DownloadSaveInfo& save_info,
On 2012/09/28 20:22:42, asanka wrote:
> Here's that extra content:: namespace again :)

Indeed.  Just when you think you've gotten rid of it, it sneaks back in :-}. 
Done.

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

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl.cc:413:
base::Bind(&DeleteDownloadedFile, current_path_));
On 2012/09/28 20:22:42, asanka wrote:
> Since we can now readily tell whether the DownloadFile is around or not, we
can
> avoid deleting the file from under it. Correct?

Done.

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl.cc:858: if
(!is_save_package_download_) {
On 2012/09/28 20:49:42, benjhayden_chromium wrote:
> Factor this out into a method and share with Cancel()?

Done.

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl.cc:1253: UpdateObservers();
On 2012/09/28 20:22:42, asanka wrote:
> Why is this call necessary?

Hmmm.  I think it's left-over from before the internal state change, when we
weren't actually doing a state change with corresponding UpdateObservers() in
Completed.  I'll delete it and see if that causes any problems.

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
File content/browser/download/download_item_impl_unittest.cc (right):

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_item_impl_unittest.cc:333:
RunAllPendingInMessageLoops();
On 2012/09/28 20:49:42, benjhayden_chromium wrote:
> Why are all these RunAllPending calls necessary?
> I've started to think of these as flags that the test code might be flaky.

I don't have the faintest idea; the tests run fine without them. I'm going to
blame it on historical reasons (this CL has been around long enough to deserve a
history text) and just remove them.  Done.

(I basically agree with your flaky instinct, by the way.  Best case, it's ugly
white box testing (I know that I'll need to bounce off this thread, this thread,
and this thread, so I'll put in three message loop polls ...) and worse case
it's shotgun "debugging".  Which doesn't mean I may not do it in the
future--I'll just make sure to have my justification ready :-}).

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

http://codereview.chromium.org/10912173/diff/30001/content/browser/download/d...
content/browser/download/download_manager_impl.cc:595:
DownloadManagerImpl::GetDownloadFileFactoryForTesting() {
On 2012/09/28 20:49:42, benjhayden_chromium wrote:
> Why do we need a getter for the FileFactory but not the ItemFactory?

Caveat: I'm not claiming this is a good reason, it's simply the answer to your
question.

In test_file_error_injector.cc we set the file factory, and then somewhat later
we inject errors into the file factory.  The file factory is owned by the
DownloadFileManager, and might have been replaced on us by "something else".  So
I considered it a reasonably high priority to confirm that that hasn't happened
at the injection point, so we don't do a use after free without a pretty clear
assertion.  There's no similar problem for the download item factory (it's only
used in unit tests, which have tighter control--TFEI is used in browser_tests,
where there's less control), and no need for a getter ==> didn't put one in.

Having written this out, I'm thinking that a weak pointer based implementation
would probably be a better plan, but Andy & I weren't as familiar with weak
pointers when TFEI was implemented.  I'll put that on my list :-}.

http://codereview.chromium.org/10912173/diff/32003/content/browser/download/d...
File content/browser/download/download_item_impl_delegate.cc (right):

http://codereview.chromium.org/10912173/diff/32003/content/browser/download/d...
content/browser/download/download_item_impl_delegate.cc:29: DownloadItemImpl*
download, DownloadTargetCallback callback) {
On 2012/09/28 20:22:42, asanka wrote:
> Why is this a concrete class?

As per offline conversation: The chrome/content delegate pattern is now to have
default implementations to make creating an embedder as easy as possible.  I'm
following that pattern here even thought this isn't a chrome/content class.

Having said that, I'm not sure this is the right thing to do--there's a
noticeable win in making these kind of classes ABCs in that you find out PDQ
(:-}) when you mock them if you've overridden all the methods or not.  I'm not
going to make the change in this CL, but I'll think about doing it in a separate
CL.

[benjhayden, 2012-10-09 20:36:39]

lgtm

[asanka, 2012-10-10 16:04:40]

LGTM

http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
content/browser/download/base_file.cc:212: const linked_ptr<net::FileStream>&
file_stream,
linked_ptr<> isn't thread safe. Can a note be added about why the copy (while
initializing file_stream_) is safe?

http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
content/browser/download/base_file.cc:224: if (file_stream_.get()) {
Suggestion: If the constructor can be called from any thread, shall we move this
use of file_stream_ to Initialize() or Open()? Open() already calls
SetBoundNetLogSource().

[Randy Smith (Not in Mondays), 2012-10-10 21:29:22]

On 2012/10/10 16:04:40, asanka wrote:
> LGTM
> 
>
http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
> File content/browser/download/base_file.cc (right):
> 
>
http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
> content/browser/download/base_file.cc:212: const linked_ptr<net::FileStream>&
> file_stream,
> linked_ptr<> isn't thread safe. Can a note be added about why the copy (while
> initializing file_stream_) is safe?

Nope, 'cause I can't convince myself that it is.  Nice catch.  Holding this CL
while I chase after all the worms that just escaped the can ....

> 
>
http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
> content/browser/download/base_file.cc:224: if (file_stream_.get()) {
> Suggestion: If the constructor can be called from any thread, shall we move
this
> use of file_stream_ to Initialize() or Open()? Open() already calls
> SetBoundNetLogSource().

[Randy Smith (Not in Mondays), 2012-10-16 18:30:28]

Incorporated Asanka's comments, have all needed LGTMs, will throw at CQ when the
file stream scoped_ptr change clears.  Note that as per PS18's title, this CL is
now rebased on top of that one @ http://codereview.chromium.org/11028131/ .

http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
File content/browser/download/base_file.cc (right):

http://codereview.chromium.org/10912173/diff/43001/content/browser/download/b...
content/browser/download/base_file.cc:224: if (file_stream_.get()) {
On 2012/10/10 16:04:40, asanka wrote:
> Suggestion: If the constructor can be called from any thread, shall we move
this
> use of file_stream_ to Initialize() or Open()? Open() already calls
> SetBoundNetLogSource().

Done.

[Randy Smith (Not in Mondays), 2012-10-18 00:03:14]

Ben: Could you review the changes since PS22?  They're *almost* trivial, but not
quite enough for me to feel comfortable committing without a re-review.

[benjhayden, 2012-10-18 13:01:48]

lgtm

[commit-bot: I haz the power, 2012-10-18 17:04:24]

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

[commit-bot: I haz the power, 2012-10-18 17:04:48]

Failed to apply patch for content/browser/download/download_item_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/download/download_item_impl.cc
  Hunk #1 FAILED at 2.
  Hunk #19 succeeded at 1197 (offset 36 lines).
  Hunk #20 succeeded at 1248 (offset 36 lines).
  Hunk #21 succeeded at 1334 with fuzz 2 (offset 61 lines).
  1 out of 21 hunks FAILED -- saving rejects to file
content/browser/download/download_item_impl.cc.rej

Patch:       content/browser/download/download_item_impl.cc
Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc
b/content/browser/download/download_item_impl.cc
index
8ab5872cd5ee8a3e3345a72d136fcd4490d790a0..7b62d8698cb49c8c05dc7af58fc9d5e41a863574
100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -2,12 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// File method ordering: Methods in this file are in the same order
-// as in download_item_impl.h, with the following exception: The public
-// interfaces DelayedDownloadOpened, OnDownloadTargetDetermined, and
-// OnDownloadCompleting are placed in chronological order with the other
-// (private) routines that together define a DownloadItem's state transitions
-// as the download progresses.  See "Download progression cascade" later in
+// File method ordering: Methods in this file are in the same order as
+// in download_item_impl.h, with the following exception: The public
+// interfaces Start, DelayedDownloadOpened, and OnDownloadCompleting
+// are placed in chronological order with the other (private) routines
+// that together define a DownloadItem's state transitions as the
+// download progresses.  See "Download progression cascade" later in
 // this file.
 
 // A regular DownloadItem (created for a download in this session of the
@@ -37,7 +37,6 @@
 #include "base/utf_string_conversions.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file.h"
-#include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/download_request_handle.h"
@@ -104,6 +103,20 @@ class NullDownloadRequestHandle : public
DownloadRequestHandleInterface {
   }
 };
 
+// Wrapper around DownloadFile::Detach and DownloadFile::Cancel that
+// takes ownership of the DownloadFile and hence implicitly destroys it
+// at the end of the function.
+static void DownloadFileDetach(
+    scoped_ptr<DownloadFile> download_file, base::Closure callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Detach(callback);
+}
+
+static void DownloadFileCancel(scoped_ptr<DownloadFile> download_file) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  download_file->Cancel();
+}
+
 }  // namespace
 
 namespace content {
@@ -129,7 +142,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate*
delegate,
                                    DownloadId download_id,
                                    const DownloadPersistentStoreInfo& info,
                                    const net::BoundNetLog& bound_net_log)
-    : download_id_(download_id),
+    : is_save_package_download_(false),
+      download_id_(download_id),
       current_path_(info.path),
       target_path_(info.path),
       target_disposition_(TARGET_DISPOSITION_OVERWRITE),
@@ -176,7 +190,8 @@ DownloadItemImpl::DownloadItemImpl(
     const DownloadCreateInfo& info,
     scoped_ptr<DownloadRequestHandleInterface> request_handle,
     const net::BoundNetLog& bound_net_log)
-    : request_handle_(request_handle.Pass()),
+    : is_save_package_download_(false),
+      request_handle_(request_handle.Pass()),
       download_id_(info.download_id),
       target_disposition_(
           (info.prompt_user_for_save_location) ?
@@ -236,7 +251,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate*
delegate,
                                    DownloadId download_id,
                                    const std::string& mime_type,
                                    const net::BoundNetLog& bound_net_log)
-    : request_handle_(new NullDownloadRequestHandle()),
+    : is_save_package_download_(true),
+      request_handle_(new NullDownloadRequestHandle()),
       download_id_(download_id),
       current_path_(path),
       target_path_(path),
@@ -277,6 +293,11 @@
DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
 
 DownloadItemImpl::~DownloadItemImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // Should always have been nuked before now, at worst in
+  // DownloadManager shutdown.
+  DCHECK(!download_file_.get());
+
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
   delegate_->AssertStateConsistent(this);
   delegate_->Detach();
@@ -328,6 +349,11 @@ void DownloadItemImpl::TogglePause() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(state_ == IN_PROGRESS_INTERNAL || state_ == COMPLETING_INTERNAL);
+
+  // Ignore pauses when we've passed the commit point.
+  if (state_ == COMPLETING_INTERNAL)
+    return;
+
   if (is_paused_)
     request_handle_->ResumeRequest();
   else
@@ -353,6 +379,12 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
   TransitionTo(CANCELLED_INTERNAL);
+
+  CancelDownloadFile();
+
+  // Cancel the originating URL request.
+  request_handle_->CancelRequest();
+
   delegate_->DownloadStopped(this);
 }
 
@@ -374,10 +406,12 @@ void DownloadItemImpl::Delete(DeleteReason reason) {
       NOTREACHED();
   }
 
-  // TODO(asanka): Avoid deleting a file that is still owned by DownloadFile.
-  if (!current_path_.empty())
+  // Delete the file if it exists and is not owned by a DownloadFile object.
+  // (In the latter case the DownloadFile object will delete it on cancel.)
+  if (!current_path_.empty() && download_file_.get() == NULL) {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                             base::Bind(&DeleteDownloadedFile, current_path_));
+  }
   Remove();
   // We have now been deleted.
 }
@@ -765,7 +799,8 @@ std::string DownloadItemImpl::DebugString(bool verbose)
const {
         " etag = '%s'"
         " url_chain = \n\t\"%s\"\n\t"
         " full_path = \"%" PRFilePath "\""
-        " target_path = \"%" PRFilePath "\"",
+        " target_path = \"%" PRFilePath "\""
+        " has download file = %s",
         GetDbHandle(),
         GetTotalBytes(),
         GetReceivedBytes(),
@@ -776,7 +811,8 @@ std::string DownloadItemImpl::DebugString(bool verbose)
const {
         GetETag().c_str(),
         url_list.c_str(),
         GetFullPath().value().c_str(),
-        GetTargetFilePath().value().c_str());
+        GetTargetFilePath().value().c_str(),
+        download_file_.get() ? "true" : "false");
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
@@ -799,16 +835,6 @@ void DownloadItemImpl::OnDownloadedFileRemoved() {
   UpdateObservers();
 }
 
-void DownloadItemImpl::OffThreadCancel() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle_->CancelRequest();
-
-  BrowserThread::PostTask(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&DownloadFileManager::CancelDownload,
-                 delegate_->GetDownloadFileManager(), download_id_));
-}
-
 // An error occurred somewhere.
 void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
   // Somewhat counter-intuitively, it is possible for us to receive an
@@ -826,11 +852,26 @@ void
DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
 
   last_reason_ = reason;
   TransitionTo(INTERRUPTED_INTERNAL);
+
+  CancelDownloadFile();
+
+  // Cancel the originating URL request.
+  request_handle_->CancelRequest();
+
   download_stats::RecordDownloadInterrupted(
       reason, received_bytes_, total_bytes_);
   delegate_->DownloadStopped(this);
 }
 
+base::WeakPtr<content::DownloadDestinationObserver>
+DownloadItemImpl::DestinationObserverAsWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+bool DownloadItemImpl::IsSavePackageDownload() const {
+  return is_save_package_download_;
+}
+
 void DownloadItemImpl::SetTotalBytes(int64 total_bytes) {
   total_bytes_ = total_bytes;
 }
@@ -873,13 +914,17 @@ void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
   UpdateObservers();
 }
 
-void DownloadItemImpl::OnAllDataSaved(
-    int64 size, const std::string& final_hash) {
+void DownloadItemImpl::OnAllDataSaved(const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);
   DCHECK(!all_data_saved_);
   all_data_saved_ = true;
-  ProgressComplete(size, final_hash);
+
+  // Store final hash and null out intermediate serialized hash state.
+  hash_ = final_hash;
+  hash_state_ = "";
+
   UpdateObservers();
 }
 
@@ -904,6 +949,55 @@ void DownloadItemImpl::SetDbHandle(int64 handle) {
       net::NetLog::Int64Callback("db_handle", db_handle_));
 }
 
+void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
+                                         int64 bytes_per_sec,
+                                         const std::string& hash_state) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  if (!IsInProgress()) {
+    // Ignore if we're no longer in-progress.  This can happen if we race a
+    // Cancel on the UI thread with an update on the FILE thread.
+    //
+    // TODO(rdsmith): Arguably we should let this go through, as this means
+    // the download really did get further than we know before it was
+    // ca…
(message too large)

[commit-bot: I haz the power, 2012-10-18 17:59:43]

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

[commit-bot: I haz the power, 2012-10-18 19:44:54]

Retried try job too often for step(s) sync_integration_tests

[commit-bot: I haz the power, 2012-10-18 20:04:18]

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

[commit-bot: I haz the power, 2012-10-18 20:59:22]

Change committed as 162786