DownloadFile no longer keeps track of whether or not the final rename has occurred.

DownloadFile no longer keeps track of whether or not the final rename has occurred.

Instead, DownloadFileManager puts the DownloadFile in a new map when it has the final name.

This is a step towards moving the 'final rename' determination happen in the UI thread.

BUG=None
TEST=None

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

[Randy Smith (Not in Mondays), 2011-02-15 22:18:10]

Background (sorta 50/50 for each of us): This CL keeps the state of
"is_final_rename", it just transfers the place where that state is kept to the
DownloadFileManager.  That state is used by the download file manager to
automatically decide if it can crop the download file on the floor
OnResponseCompleted() or on cancel.  

Long-term, I think we don't want the DownloadFileManager to make that decision;
I think we want it to come explicitly from the UI thread & the
DownloadManager/DownloadItem.  But I think you're right that moving it into the
DownloadFileManager makes it a bit easier to do that later, trickier refactor. 
So this approach looks good to me, with the caveat that you're putting in a fair
amount of code that I eventually want completely out of the pool.

So I think this approach is good.

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/base_fi...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/base_fi...
chrome/browser/download/base_file.h:58: virtual bool is_download_complete()
const { return download_complete_; }
This isn't used; why is it here?  If it's going to be used in a future CL, it
should probably be put in in that CL.

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_file_manager.cc:323: // We don't call this
function before the download is complete.
I believe we do call the function before the download is complete in the
dangerous name case.

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_file_manager.cc:359:
downloads_with_final_name_.erase(renamed_it);
This is a little confusing in that reading this code you'd think "but we just
put it into the downloads_with_final_name_ container up above; why are we
testing?"  I think it's still correct in that if the rename failed, we wouldn't
have put it in the container.  But it's tricky pulling out the error logic and
the non-error logic.  Are you willing to do a bit more refactoring and have the
error logic all in one place ended by a "return" so that the analysis is easier?
 (I'm thinking doing an "if (!download->Rename(full_path)) {" above, with the
error logic and a return in the test, and back out to the outer level for the
"good path".)

[ahendrickson, 2011-02-17 14:52:46]

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/base_fi...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/base_fi...
chrome/browser/download/base_file.h:58: virtual bool is_download_complete()
const { return download_complete_; }
On 2011/02/15 22:18:10, rdsmith wrote:
> This isn't used; why is it here?  If it's going to be used in a future CL, it
> should probably be put in in that CL.

Done.

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/downloa...
File chrome/browser/download/download_file_manager.cc (right):

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_file_manager.cc:323: // We don't call this
function before the download is complete.
On 2011/02/15 22:18:10, rdsmith wrote:
> I believe we do call the function before the download is complete in the
> dangerous name case.  

Changed the comment.

http://codereview.chromium.org/6480079/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_file_manager.cc:359:
downloads_with_final_name_.erase(renamed_it);
On 2011/02/15 22:18:10, rdsmith wrote:
> This is a little confusing in that reading this code you'd think "but we just
> put it into the downloads_with_final_name_ container up above; why are we
> testing?"  I think it's still correct in that if the rename failed, we
wouldn't
> have put it in the container.  But it's tricky pulling out the error logic and
> the non-error logic.  Are you willing to do a bit more refactoring and have
the
> error logic all in one place ended by a "return" so that the analysis is
easier?
>  (I'm thinking doing an "if (!download->Rename(full_path)) {" above, with the
> error logic and a return in the test, and back out to the outer level for the
> "good path".)

Refactored it a bit to have all the erase logic in one function.

[Randy Smith (Not in Mondays), 2011-02-17 15:26:26]

I'd also enhance the issue description to indicate that this change is in prep
for moving the "final rename" state off the file thread and over to the UI
thread/DownloadItem.

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/base...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/base...
chrome/browser/download/base_file.h:76: 
I think you removed the accessors for this, so it should go to?

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

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:271:
downloads_with_final_name_.clear();
Remember there are multiple DownloadManagers per DownloadFileManager; you need
to only remove the entries in downlaods_with_fnial_name_ that correspond to the
DownloadManager that's shutting down.

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

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:135: typedef base::hash_map<int,
DownloadFile*> DownloadFileMap;
For both this typedef and the EraseDownload() function, you should specify what
the id is; just "int" doesn't give a lot of information to potential consumers.

[ahendrickson, 2011-02-17 18:51:34]

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/base...
File chrome/browser/download/base_file.h (right):

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/base...
chrome/browser/download/base_file.h:76: 
On 2011/02/17 15:26:26, rdsmith wrote:
> I think you removed the accessors for this, so it should go to?

Done.

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

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:271:
downloads_with_final_name_.clear();
On 2011/02/17 15:26:26, rdsmith wrote:
> Remember there are multiple DownloadManagers per DownloadFileManager; you need
> to only remove the entries in downlaods_with_fnial_name_ that correspond to
the
> DownloadManager that's shutting down.

Fixed.

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

http://codereview.chromium.org/6480079/diff/5001/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:135: typedef base::hash_map<int,
DownloadFile*> DownloadFileMap;
On 2011/02/17 15:26:26, rdsmith wrote:
> For both this typedef and the EraseDownload() function, you should specify
what
> the id is; just "int" doesn't give a lot of information to potential
consumers.

Done.

[ahendrickson, 2011-02-17 22:50:43]

Sigh.  Fixed dumb mistake.

[Paweł Hajdan Jr., 2011-02-18 11:59:02]

LGTM with minor comments.

It's rather hard for me to determine whether this works, but it may help
simplify some things.

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

http://codereview.chromium.org/6480079/diff/7005/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:374: if
(ContainsKey(downloads_, id)) {
nit: Return early if !ContainsKey.

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

http://codereview.chromium.org/6480079/diff/7005/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:138: // A map of all in progress
downloads.
Add a comment that it owns the contents, and also contains downloads that are in
the |downloads_with_final_name_| map.

http://codereview.chromium.org/6480079/diff/7005/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:141: // A map of all in progress
downloads that have their final name.
Add a comment that it doesn't own the contents.

[ahendrickson, 2011-02-18 16:06:47]

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

http://codereview.chromium.org/6480079/diff/7005/chrome/browser/download/down...
chrome/browser/download/download_file_manager.cc:374: if
(ContainsKey(downloads_, id)) {
On 2011/02/18 11:59:02, Paweł Hajdan Jr. wrote:
> nit: Return early if !ContainsKey.

Done.

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

http://codereview.chromium.org/6480079/diff/7005/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:138: // A map of all in progress
downloads.
On 2011/02/18 11:59:02, Paweł Hajdan Jr. wrote:
> Add a comment that it owns the contents, and also contains downloads that are
in
> the |downloads_with_final_name_| map.

Done.

http://codereview.chromium.org/6480079/diff/7005/chrome/browser/download/down...
chrome/browser/download/download_file_manager.h:141: // A map of all in progress
downloads that have their final name.
On 2011/02/18 11:59:02, Paweł Hajdan Jr. wrote:
> Add a comment that it doesn't own the contents.

Done.