Move some download tests to content from chrome.

Move some download tests to content from chrome.

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


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

[Randy Smith (Not in Mondays), 2012-08-24 19:32:58]

PTAL.  
Ben: Could you review */browser/download/* and
shell_download_manager_delegate.*?

John: Could you review everything outside of */browser/download/*?  

(Yes, I'm wanting both your eyes on shell_download_manager_delegate.*.)

[Randy Smith (Not in Mondays), 2012-08-24 19:34:12]

On 2012/08/24 19:32:58, rdsmith wrote:
> PTAL.  
> Ben: Could you review */browser/download/* and
> shell_download_manager_delegate.*?
> 
> John: Could you review everything outside of */browser/download/*?  
> 
> (Yes, I'm wanting both your eyes on shell_download_manager_delegate.*.)

I should say: When I have approval and try job runs, I intend to land this in
two passes: A "git cl dcommit" for the new .lib file for which I will not watch
the tree, and after which I'll modify this CL to avoid the new binary file and
send it in through the CQ.  Critiques of this landing plan are also welcome.

[benjhayden, 2012-08-24 20:50:38]

Why do you need the binary file?

https://chromiumcodereview.appspot.com/10867060/diff/1/content/browser/downlo...
File content/browser/download/download_content_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10867060/diff/1/content/browser/downlo...
content/browser/download/download_content_browsertest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
How much of this file is new, and how much copy-pasted?
Do you want to try `git cp .../download_browsertest.cc
.../download_content_browsertest.cc` and then delete code from
download_content_browsertest.cc?

[Randy Smith (Not in Mondays), 2012-08-24 21:03:37]

I need the binary file because MultiDownloads uses it to get a second download
going part way through the first one, and I can only get access to files under
the content/test/data directory (nothing from chrome).  I need MultiDownloads
because it's one of the main tests that calls EnsureNoPendingDownloads.  I can't
simply remove the file at the chrome level because the MockUrlRequestJob is
statically initialized with a root, and it's different for content and chrome. 
There might be some other way to share the file; I didn't see a way, but I
didn't think about it very hard.

(== this cl was a string tug :-}.)

https://chromiumcodereview.appspot.com/10867060/diff/1/content/browser/downlo...
File content/browser/download/download_content_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10867060/diff/1/content/browser/downlo...
content/browser/download/download_content_browsertest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/08/24 20:50:38, benjhayden_chromium wrote:
> How much of this file is new, and how much copy-pasted?
> Do you want to try `git cp .../download_browsertest.cc
> .../download_content_browsertest.cc` and then delete code from
> download_content_browsertest.cc?

Quite a bit was copy-pasted, but with a lot of mods (starting with
Browser->Shell, and continuing).  And just what I did had a lot of tugging on a
string about it.  I thought about trying to write some shared helper routines
(maybe inherit from a content class in chrome) but was a bit reluctant to tug on
a bigger string :-}.

My goals with the CL were a) Enable nuking of EnsureNoPendingDownloads() (next
CL), and b) provide an example of how to write browser tests at the content
layer in the downloads context.  I think this CL achieves those goals, at the
cost of some similar code.  I'm not sure how much I could reduce the duplicate
code without really rewriting things, but if you want I can take a swing at it.

[benjhayden, 2012-08-24 21:11:40]

On 2012/08/24 21:03:37, rdsmith wrote:
> I need the binary file because MultiDownloads uses it to get a second download
> going part way through the first one, and I can only get access to files under
> the content/test/data directory (nothing from chrome).  I need MultiDownloads
> because it's one of the main tests that calls EnsureNoPendingDownloads.  I
can't
> simply remove the file at the chrome level because the MockUrlRequestJob is
> statically initialized with a root, and it's different for content and chrome.

> There might be some other way to share the file; I didn't see a way, but I
> didn't think about it very hard.

Sorry, I should have been clearer.
Why does the file need to be binary? That's the part that is annoying to work
with, both in this CL and in any future CL that might need to change/move/copy
the file. Could it be text instead?

> 
> (== this cl was a string tug :-}.)
> 
>
https://chromiumcodereview.appspot.com/10867060/diff/1/content/browser/downlo...
> File content/browser/download/download_content_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10867060/diff/1/content/browser/downlo...
> content/browser/download/download_content_browsertest.cc:1: // Copyright (c)
> 2012 The Chromium Authors. All rights reserved.
> On 2012/08/24 20:50:38, benjhayden_chromium wrote:
> > How much of this file is new, and how much copy-pasted?
> > Do you want to try `git cp .../download_browsertest.cc
> > .../download_content_browsertest.cc` and then delete code from
> > download_content_browsertest.cc?
> 
> Quite a bit was copy-pasted, but with a lot of mods (starting with
> Browser->Shell, and continuing).  And just what I did had a lot of tugging on
a
> string about it.  I thought about trying to write some shared helper routines
> (maybe inherit from a content class in chrome) but was a bit reluctant to tug
on
> a bigger string :-}.
> 
> My goals with the CL were a) Enable nuking of EnsureNoPendingDownloads() (next
> CL), and b) provide an example of how to write browser tests at the content
> layer in the downloads context.  I think this CL achieves those goals, at the
> cost of some similar code.  I'm not sure how much I could reduce the duplicate
> code without really rewriting things, but if you want I can take a swing at
it.

Sorry, I didn't mean to rewrite it, I just meant to do the git magic to tell
codereview to intelligently present the diffs between the new file and the old
file instead of a green wall of "oh god don't make me grok all that on Friday
afternoon".
I usually copy the file out of the way, git rm it, git copy it from the old
file, then (not git) mv the backup file to the git copied file.

[Randy Smith (Not in Mondays), 2012-08-25 18:34:36]

On 2012/08/24 21:11:40, benjhayden_chromium wrote:
> Sorry, I didn't mean to rewrite it, I just meant to do the git magic to tell
> codereview to intelligently present the diffs between the new file and the old
> file instead of a green wall of "oh god don't make me grok all that on Friday
> afternoon".
> I usually copy the file out of the way, git rm it, git copy it from the old
> file, then (not git) mv the backup file to the git copied file.

I tried this, and it didn't work--git continued to treat it as a new file.  This
doesn't surprise me--my belief is that internally there's nothing special about
"git mv" (there doesn't appear to be a "git cp"); git just uses some internal
difference algorithms to try and figure out if a new file came from somewhere
else.  

What I did instead is to create a fake patchset (PS3) in which the contents of
download_content_browsertest.cc was replaced by the contents of
download_browsertest.cc, so you can look at diffs versus PS3 for changes from
download_browsertest.cc.  As a minor side benefit, I now know the command flag
to override the presubmit checks; they weren't happy about all the chrome
includes :-}.  I'm not sure how useful you'll find it--even with that explicit
base file, it didn't realize that there were specific tests I was cribbing
DownloadCancelled and MultiDownload from.  

I'll take a look at the binary file question, but when I've taken a swing at
this in the past, I've found that it's not very easy, because our sniffing wants
to turn text files into a regular renderer view.  So you need to either make
something sniff as not being text, but allow git to believe it's text (possible,
I think) or explicitly set a mime-type in content-disposition that sniffing
won't override (also possible, but I'd rather not in tests that aren't aimed at
testing content-disposition handling).  But I'll take a swing.

[Randy Smith (Not in Mondays), 2012-08-25 20:20:36]

Somewhat to my surprise, faking out the sniffer worked.  There's still some
problems with the try jobs, but it's probably in reasonable shape for a first
review.  PTAL.

[benjhayden, 2012-08-26 16:19:36]

lgtm

[jam, 2012-08-27 17:14:49]

lgtm

http://codereview.chromium.org/10867060/diff/2004/content/browser/download/do...
File content/browser/download/download_content_browsertest.cc (right):

http://codereview.chromium.org/10867060/diff/2004/content/browser/download/do...
content/browser/download/download_content_browsertest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
nit: files in content don't have "content" in their filenames since it's
redundant (other than things like ContentClient). can you keep this as
download_browsertest.cc? files in chrome, which test the chrome part of
integration with content, do sometimes have "chrome" in their filename if
there's an equivalent file in content, so if you don't want duplicate filenames
you can rename the one in chrome

http://codereview.chromium.org/10867060/diff/2004/content/browser/download/do...
content/browser/download/download_content_browsertest.cc:11: #include
"content/public/browser/browser_context.h"
nit: by convention (i.e. like webkit api), files inside content should include
the impl headers and not the public interfaces

http://codereview.chromium.org/10867060/diff/2004/content/browser/download/do...
content/browser/download/download_content_browsertest.cc:41:
static_cast<ShellDownloadManagerDelegate*>(
nit: indent

http://codereview.chromium.org/10867060/diff/2004/content/browser/download/do...
content/browser/download/download_content_browsertest.cc:57:
content::DownloadTestObserver* CreateWaiter(
nit: here and below, get rid of "content::"

[Randy Smith (Not in Mondays), 2012-08-28 21:09:14]

Believed incorporated all comments; I'll go ahead do try jobs and submit, but
feel free to call out remaining problems if you seen any.

https://chromiumcodereview.appspot.com/10867060/diff/2004/content/browser/dow...
File content/browser/download/download_content_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10867060/diff/2004/content/browser/dow...
content/browser/download/download_content_browsertest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2012/08/27 17:14:49, John Abd-El-Malek wrote:
> nit: files in content don't have "content" in their filenames since it's
> redundant (other than things like ContentClient). can you keep this as
> download_browsertest.cc? files in chrome, which test the chrome part of
> integration with content, do sometimes have "chrome" in their filename if
> there's an equivalent file in content, so if you don't want duplicate
filenames
> you can rename the one in chrome

Done.

https://chromiumcodereview.appspot.com/10867060/diff/2004/content/browser/dow...
content/browser/download/download_content_browsertest.cc:11: #include
"content/public/browser/browser_context.h"
On 2012/08/27 17:14:49, John Abd-El-Malek wrote:
> nit: by convention (i.e. like webkit api), files inside content should include
> the impl headers and not the public interfaces

Done for everything except for download_test_observer.h, which doesn't have a
corresponding impl.h file.

https://chromiumcodereview.appspot.com/10867060/diff/2004/content/browser/dow...
content/browser/download/download_content_browsertest.cc:41:
static_cast<ShellDownloadManagerDelegate*>(
On 2012/08/27 17:14:49, John Abd-El-Malek wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/10867060/diff/2004/content/browser/dow...
content/browser/download/download_content_browsertest.cc:57:
content::DownloadTestObserver* CreateWaiter(
On 2012/08/27 17:14:49, John Abd-El-Malek wrote:
> nit: here and below, get rid of "content::"

Done.

[commit-bot: I haz the power, 2012-08-29 15:47:29]

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

[commit-bot: I haz the power, 2012-08-29 20:06:40]

Try job failure for 10867060-18001 (retry) (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-08-29 20:16:36]

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

[commit-bot: I haz the power, 2012-08-29 21:41:44]

Try job failure for 10867060-18001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-29 21:44:08]

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

[commit-bot: I haz the power, 2012-08-29 23:37:06]

Try job failure for 10867060-18001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-30 15:00:08]

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

[commit-bot: I haz the power, 2012-08-30 17:09:29]

Change committed as 154179