Add downloads.onCreated apitest and fix a few bugs

The apitest needs a handler on the testserver that does not return a dangerous file and finishes quickly. "/slow?0" appears to work.

DownloadRequestHandle::GetTabContents() returns NULL in the test framework. Make DRH::GetDownloadManager() use RVH->process().browser_context() directly instead.

Make MockDownloadManagerDelegate::AddItemToPersistentStore() call DM::OnItemAddedToPersistentStore() so that items are added to the history_downloads_ map even in the test framework.


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

[Randy Smith (Not in Mondays), 2011-10-17 17:39:55]

LGTM, but I'd recommend you also get a reviewer that's familiar with the syntax
and usual usage pattern for extensions tests.

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

http://codereview.chromium.org/8060042/diff/6001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/downloads/test.js:37: {"url":
getURL("slow?0"), "filename": "foo"},
What pathway does "slow?0" use?  Is that URLRequestSlowDownloadJob?

http://codereview.chromium.org/8060042/diff/6001/content/browser/download/dow...
File content/browser/download/download_request_handle.cc (right):

http://codereview.chromium.org/8060042/diff/6001/content/browser/download/dow...
content/browser/download/download_request_handle.cc:65:
->browser_context()->GetDownloadManager();
Random thought that you're welcome to bounce as not being appropriate for this
CL: Now that we have global ids that include the DownloadManager, could we nuke
this method on DownloadRequestHandle and just get the DM from there?  I *think*
we have access to a global_id everywhere we have a DownloadRequestHandle.

[benjhayden, 2011-10-17 17:54:29]

Mihai, would you mind taking a look at this small change to the downloads
extension api_test?

Randy has already LGd the changes in downloads/.

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

http://codereview.chromium.org/8060042/diff/6001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/downloads/test.js:37: {"url":
getURL("slow?0"), "filename": "foo"},
On 2011/10/17 17:39:56, rdsmith wrote:
> What pathway does "slow?0" use?  Is that URLRequestSlowDownloadJob?

It's just a handler in testserver.py that returns a safe file quickly.

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/net/tools/testserver/...

http://codereview.chromium.org/8060042/diff/6001/content/browser/download/dow...
File content/browser/download/download_request_handle.cc (right):

http://codereview.chromium.org/8060042/diff/6001/content/browser/download/dow...
content/browser/download/download_request_handle.cc:65:
->browser_context()->GetDownloadManager();
On 2011/10/17 17:39:56, rdsmith wrote:
> Random thought that you're welcome to bounce as not being appropriate for this
> CL: Now that we have global ids that include the DownloadManager, could we
nuke
> this method on DownloadRequestHandle and just get the DM from there?  I
*think*
> we have access to a global_id everywhere we have a DownloadRequestHandle.  

I thought that we decided that that would be breaking the DownloadId
abstraction? Meh, we can do it in DownloadId part 2 if that SGTY. I'd like to
try to keep this cl small.

[Mihai Parparita -not on Chrome, 2011-10-18 19:59:36]

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

http://codereview.chromium.org/8060042/diff/11002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:37: {"url":
getURL("slow?0"), "filename": "foo"},
Can you add a comment why your'e using this URL (it's not obvious that a "slow"
name would be fast). Also, would /download-known-size be more appropriate?

http://codereview.chromium.org/8060042/diff/11002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:45: // Remove the
listener so that we don't call chrome.test.succeed
You can use chrome.test.listenOnce instead.

[benjhayden, 2011-10-18 20:32:56]

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

http://codereview.chromium.org/8060042/diff/11002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:37: {"url":
getURL("slow?0"), "filename": "foo"},
On 2011/10/18 19:59:37, Mihai Parparita wrote:
> Can you add a comment why your'e using this URL (it's not obvious that a
"slow"
> name would be fast). Also, would /download-known-size be more appropriate?

Done.

/download-[un]known-size stalls the request until it receives a parallel request
for /download-finish. I could run an XHR, but it would be confusing unnecessary
overhead.

http://codereview.chromium.org/8060042/diff/11002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:45: // Remove the
listener so that we don't call chrome.test.succeed
On 2011/10/18 19:59:37, Mihai Parparita wrote:
> You can use chrome.test.listenOnce instead.

Done.

[Mihai Parparita -not on Chrome, 2011-10-18 20:39:02]

LGTM

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

http://codereview.chromium.org/8060042/diff/18001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:35: var safe_fast_url =
getURL("slow?0");
This should be called SAFE_FAST_URL.

[benjhayden, 2011-10-20 19:05:44]

I'll submit as soon as the trybots return if there are no further comments.
Thanks!

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

http://codereview.chromium.org/8060042/diff/18001/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/downloads/test.js:35: var safe_fast_url =
getURL("slow?0");
On 2011/10/18 20:39:02, Mihai Parparita wrote:
> This should be called SAFE_FAST_URL.

Done.

[commit-bot: I haz the power, 2011-10-20 19:06:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/8060042/25005

[commit-bot: I haz the power, 2011-10-20 20:20:12]

Change committed as 106560