Re-enable DownloadsApiTest.Downloads

Re-enable DownloadsApiTest.Downloads

This had three issues which are addressed in this CL:
  - The in progress downloads need to be canceled at test shutdown to prevent the browser from bringing up the "Downloads are in progress" prompt at attempted shutdown. This caused the test to periodically timeout.

  - The filename validation logic triggered an out-of-bounds check when the filenames were less than 2 characters. The validation was also done even if no filename was specified in the dictionary.

  - The download api configuration no longer had HttpHeaders after this was split to multiple files. I added it here.

BUG=109436
TEST=browser_tests --gtest_filter=DownloadsApiTest.Downloads


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

[cbentzel, 2012-01-06 21:41:11]

I need to regenerate the extension docs due to the change in the .json file -
and I'm also interested in whether the type can go in an include-like file so
it's not duplicated across webRequest.json

[benjhayden, 2012-01-09 15:22:49]

I seem to recall trying this back in November or December, but I can't remember
exactly where the race was that I couldn't fix. Did you run it 100 times on all
of your boxes?

Also, Asanka implemented cancel(), so the extension test should be able to use
that for each of the downloads that it initiates.

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

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_api.cc:117: bool
ValidateFilename(const string16& filename) {
It strikes me that a more robust, perhaps equally simple method would be to
concat the DownloadsFolder pref with this filename, evaluate '..'s but not
symlinks, and check to see that the resulting filename begins with
DownloadsFolder. You can either try implementing that here, or add it to my
todo, or don't worry about it.

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_api.cc:210: if
(options->HasKey(kSaveAsKey)) {
I thought that the style guide forbade braces around single-statement blocks.

[asanka, 2012-01-09 17:22:16]

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

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_apitest.cc:45:
download_manager->GetAllDownloads(FilePath(), &all_downloads);
There's a race here where the download item might not be present in
history_downloads_ but is in_progress_.

Extension tests run into this because the downloads.download callback is issued
when we receive a response from the server for a newly initiated download, but
the download isn't added to history_downloads_ until name determination is
complete. Therefore some tests can complete before the download it creates is
visible to GetAllDownloads().

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

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

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_apitest.cc:45:
download_manager->GetAllDownloads(FilePath(), &all_downloads);
On 2012/01/09 17:22:17, asanka wrote:
> There's a race here where the download item might not be present in
> history_downloads_ but is in_progress_.
> 
> Extension tests run into this because the downloads.download callback is
issued
> when we receive a response from the server for a newly initiated download, but
> the download isn't added to history_downloads_ until name determination is
> complete. Therefore some tests can complete before the download it creates is
> visible to GetAllDownloads().

Chris: Can you get ahold of the list of downloads in progress and wait until
they show up as visible before canceling them?

[cbentzel, 2012-01-09 19:16:41]

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

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_api.cc:210: if
(options->HasKey(kSaveAsKey)) {
On 2012/01/09 15:22:49, b s h wrote:
> I thought that the style guide forbade braces around single-statement blocks.

Not if the body spans multiple lines.

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

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_apitest.cc:45:
download_manager->GetAllDownloads(FilePath(), &all_downloads);
On 2012/01/09 17:22:17, asanka wrote:
> There's a race here where the download item might not be present in
> history_downloads_ but is in_progress_.

Thanks for the explanation of when this could happen.

It looks like I need to add another method to DownloadManager to correctly clear
all of these, unfortunately.

Now that "cancel" support is in, though, I may be able to do this in the
extension as Ben suggested. 

 

> 
> Extension tests run into this because the downloads.download callback is
issued
> when we receive a response from the server for a newly initiated download, but
> the download isn't added to history_downloads_ until name determination is
> complete. Therefore some tests can complete before the download it creates is
> visible to GetAllDownloads().

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_apitest.cc:45:
download_manager->GetAllDownloads(FilePath(), &all_downloads);
On 2012/01/09 19:05:45, rdsmith wrote:
> On 2012/01/09 17:22:17, asanka wrote:
> > There's a race here where the download item might not be present in
> > history_downloads_ but is in_progress_.
> > 
> > Extension tests run into this because the downloads.download callback is
> issued
> > when we receive a response from the server for a newly initiated download,
but
> > the download isn't added to history_downloads_ until name determination is
> > complete. Therefore some tests can complete before the download it creates
is
> > visible to GetAllDownloads().
> 
> Chris: Can you get ahold of the list of downloads in progress and wait until
> they show up as visible before canceling them?

It doesn't look there is a way to get to this from the DownloadManager
interface. I could potentially create a new DownloadManagerDelegate class which
passes most calls through to the original Delegate, and keeps track of the id's
passed into ShouldStartDownload.

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

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

http://codereview.chromium.org/9110042/diff/1/chrome/browser/download/downloa...
chrome/browser/download/download_extension_apitest.cc:45:
download_manager->GetAllDownloads(FilePath(), &all_downloads);
On 2012/01/09 19:16:41, cbentzel wrote:
> On 2012/01/09 19:05:45, rdsmith wrote:
> > On 2012/01/09 17:22:17, asanka wrote:
> > > There's a race here where the download item might not be present in
> > > history_downloads_ but is in_progress_.
> > > 
> > > Extension tests run into this because the downloads.download callback is
> > issued
> > > when we receive a response from the server for a newly initiated download,
> but
> > > the download isn't added to history_downloads_ until name determination is
> > > complete. Therefore some tests can complete before the download it creates
> is
> > > visible to GetAllDownloads().
> > 
> > Chris: Can you get ahold of the list of downloads in progress and wait until
> > they show up as visible before canceling them?
> 
> It doesn't look there is a way to get to this from the DownloadManager
> interface. I could potentially create a new DownloadManagerDelegate class
which
> passes most calls through to the original Delegate, and keeps track of the
id's
> passed into ShouldStartDownload.

Oh, goodness, please don't do that; an interface on DownloadManager is a much
cleaner solution.  (For context, this problem will go away when we do download
id part II, albeit with the creation of a couple other problems around making
sure that the download item presents a consistent state as it goes through the
file name determination pathway.)

What I meant was more "when the test creates a download, can you note the
download id associated with that download and then wait for all of those to
become visible and kill them?"  If the answer's no for some reason, another
interface on DownloadManager makes sense :-{.

[cbentzel, 2012-01-11 16:19:53]

I ended up moving the clean up logic into test.js, rather than the
DownloadsApiTest framework. I also added a test which triggers a download that
starts but never completes, to help validate that the cleanup logic works
correctly.

Also, formatted test.js to better conform to the Javascript style guide. If you
want me to move these changes to another CL I understand.

[benjhayden, 2012-01-11 17:39:44]

LGTM if this passes 100 out of 100 times on windows and one of mac/linux/cros.
Thank you!

http://codereview.chromium.org/9110042/diff/13001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/downloads/test.js (right):

http://codereview.chromium.org/9110042/diff/13001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:137: function
downloadNoComplete() {
Comment that this is to test cleanUp?

[asanka, 2012-01-11 18:22:59]

LGTM

http://codereview.chromium.org/9110042/diff/13001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.downloads.json (right):

http://codereview.chromium.org/9110042/diff/13001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:84: {
Nit: indentation.

http://codereview.chromium.org/9110042/diff/13001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/downloads/test.js (right):

http://codereview.chromium.org/9110042/diff/13001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:73:
chrome.test.callbackFail('I\'m afraid I can\'t do that.'));
Nit: ERROR_GENERIC

[cbentzel, 2012-01-11 18:43:20]

http://codereview.chromium.org/9110042/diff/13001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.downloads.json (right):

http://codereview.chromium.org/9110042/diff/13001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.downloads.json:84: {
On 2012/01/11 18:22:59, asanka wrote:
> Nit: indentation.

Done.

http://codereview.chromium.org/9110042/diff/13001/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/downloads/test.js (right):

http://codereview.chromium.org/9110042/diff/13001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:73:
chrome.test.callbackFail('I\'m afraid I can\'t do that.'));
On 2012/01/11 18:22:59, asanka wrote:
> Nit: ERROR_GENERIC

Done.

http://codereview.chromium.org/9110042/diff/13001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:137: function
downloadNoComplete() {
On 2012/01/11 17:39:44, b s h wrote:
> Comment that this is to test cleanUp?

Done.

[commit-bot: I haz the power, 2012-01-12 14:54:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/9110042/18006

[commit-bot: I haz the power, 2012-01-12 16:34:55]

Try job failure for 9110042-18006 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-01-12 16:58:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/9110042/18006

[commit-bot: I haz the power, 2012-01-12 19:00:09]

Change committed as 117463