Basic tests for DownloadUrl/DownloadUrlToFile.

Basic tests for DownloadUrl/DownloadUrlToFile.


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

[Randy Smith (Not in Mondays), 2011-11-06 19:46:43]

Andy: Could you review this CL (all files)?

Jian, Achuith: I'm changing the behavior of GetTemporaryDownloads() in a way
that I believe is harmless, but I want to make sure you agree that it won't hurt
the imageburner or drag file subsystems.  The change is that a null directory
path will now act as a wildcard and get all temporary downloads.  If you're
comfortable that that change won't hurt your subsystem, I'm comfortable with
your LGTM without reading the code.

[achuithb, 2011-11-06 20:43:59]

LGTM

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

http://codereview.chromium.org/8487002/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:1576: url, GURL(""), "",
tab_contents);
nit: shouldn't GURL() suffice?

http://codereview.chromium.org/8487002/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_browsertest.cc:1603: url, GURL(""), "",
save_info, tab_contents);
nit: same

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

Removed a line to make the assertion of first setting in
ContinueDownloadWithPath true.  Andy: I did what I think is a thorough dataflow
analysis of this setting, and the only use before death (re-setting) was in the
DCHECK that I already added back in.  So I think we're safe in removing this,
though I'm looking forward to seeing the try job results. 

Jon: I've added you to the review list, as this is putting back in the assertion
you took out in your recent CL.  I don't remember if you had a test for that
case in your CL, so I wanted to give you a chance to take a look at this (or
give me a manual test case to confirm I'm not breaking your application).  This
CL also includes tests for the code path you're using, so from this point on
you'll probably be protected against random download system changes :-}.  

Achuith: Thanks, and I'll be incorporating your comments in a later PS; I just
wanted to get this out quickly so that future reviews saw it.  (I don't think
you need to review this change unless you want to.)

[jstritar, 2011-11-07 16:38:36]

On 2011/11/06 22:00:52, rdsmith wrote:
> Jon: I've added you to the review list, as this is putting back in the
assertion
> you took out in your recent CL.  I don't remember if you had a test for that
> case in your CL, so I wanted to give you a chance to take a look at this (or
> give me a manual test case to confirm I'm not breaking your application). 
This
> CL also includes tests for the code path you're using, so from this point on
> you'll probably be protected against random download system changes :-}.  

Most of the *WebstorePrivate* browser_tests trigger the code path we're using,
and those passed in your try jobs. Thanks!

[ahendrickson, 2011-11-08 06:19:57]

http://codereview.chromium.org/8487002/diff/4003/content/browser/download/dow...
File content/browser/download/download_file_manager.cc (left):

http://codereview.chromium.org/8487002/diff/4003/content/browser/download/dow...
content/browser/download/download_file_manager.cc:139: info->path =
info->save_info.file_path;
The only effect I can see of removing this line is that potentially these
functions:

GetTemporaryDownloads()
GetAllDownloads()

will return a different result if called before ContinueDownloadWithPath().

[Randy Smith (Not in Mondays), 2011-11-09 03:13:55]

On 2011/11/08 06:19:57, ahendrickson wrote:
>
http://codereview.chromium.org/8487002/diff/4003/content/browser/download/dow...
> File content/browser/download/download_file_manager.cc (left):
> 
>
http://codereview.chromium.org/8487002/diff/4003/content/browser/download/dow...
> content/browser/download/download_file_manager.cc:139: info->path =
> info->save_info.file_path;
> The only effect I can see of removing this line is that potentially these
> functions:
> 
> GetTemporaryDownloads()
> GetAllDownloads()
> 
> will return a different result if called before ContinueDownloadWithPath().

Hmmm.  They both search history_downloads_, and I don't think we put download
items onto history_downloads_ until after the call to ContinueDownloadWithPath. 
So I don't think that represents a change in behavior.  Please let me know if
you don't agree.

I don't yet have your LGTM?  (No large hurry--I've got a tricky chromeos failure
I'm having problems debugging.  Just wanted to confirm that that was
intentional.)

[Randy Smith (Not in Mondays), 2011-11-09 03:14:11]

Jian, are you going to have a chance to take a look?

[jianli, 2011-11-09 05:25:52]

LGTM.

[ahendrickson, 2011-11-09 16:22:40]

Ah, you are correct -- I missed the fact that they were only searching
history_downloads_.

LGTM.

On 2011/11/09 03:13:55, rdsmith wrote:
> On 2011/11/08 06:19:57, ahendrickson wrote:
> >
>
http://codereview.chromium.org/8487002/diff/4003/content/browser/download/dow...
> > File content/browser/download/download_file_manager.cc (left):
> > 
> >
>
http://codereview.chromium.org/8487002/diff/4003/content/browser/download/dow...
> > content/browser/download/download_file_manager.cc:139: info->path =
> > info->save_info.file_path;
> > The only effect I can see of removing this line is that potentially these
> > functions:
> > 
> > GetTemporaryDownloads()
> > GetAllDownloads()
> > 
> > will return a different result if called before ContinueDownloadWithPath().
> 
> Hmmm.  They both search history_downloads_, and I don't think we put download
> items onto history_downloads_ until after the call to
ContinueDownloadWithPath. 
> So I don't think that represents a change in behavior.  Please let me know if
> you don't agree.
> 
> I don't yet have your LGTM?  (No large hurry--I've got a tricky chromeos
failure
> I'm having problems debugging.  Just wanted to confirm that that was
> intentional.)

[Randy Smith (Not in Mondays), 2011-12-08 20:41:55]

FYI: Resurrecting this CL.  I've done minimal changes other than merging to
LKGR, so no need for a re-review except for Achuith.  Achuith: I've commented
out the ChromeOS UI test at the end of the DownloadUrlToFile; could you glance
at that?

Chris, cc'ing you as you may want to be aware of this CL for your work with
SaveAsPost.

[achuithb, 2011-12-08 20:52:10]

LGTM on browser_test change

[commit-bot: I haz the power, 2011-12-09 18:33:43]

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

[commit-bot: I haz the power, 2011-12-09 22:24:22]

Change committed as 113864