Move DownloadTestObserver and friends down into content.

Move DownloadTestObserver and friends down into content.

This will allow use by tests in content_browsertests.

BUG=None


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

[Randy Smith (Not in Mondays), 2012-08-11 20:01:35]

Ben: Could you review the main body of this test?

Scott: Could you review content/test & content/public/test?  Specifically, I'm
not yet familiar enough with the chrome/content boundary to know if I've done
everything right.  Do I need a CONTENT_EXPORT?

Ben: I need to be careful not to make promises to you :-}.  This is the initial
bit of rope that the string tug on removing EnsureNoPendingDownloads() turned
into.  My proposed replacement involved using SetFileFactoryForTesting, which is
only accessible in content, which requires writing content_browsertests, which
requires moving DTO.  I don't *think* I'm going to need more than three CLs, but
we'll see what happens :-}.

[jam, 2012-08-13 15:59:57]

On 2012/08/11 20:01:35, rdsmith wrote:
> Ben: Could you review the main body of this test?
> 
> Scott: Could you review content/test & content/public/test?  Specifically, I'm
> not yet familiar enough with the chrome/content boundary to know if I've done
> everything right.  Do I need a CONTENT_EXPORT?

did you mean me? (you added me as a reviewer but not Scott, or maybe you added
Scott and he switched to me :) ).

you don't need CONTNET_EXPORT for stuff in content/test (since it's linked into
the test binaries)

> 
> Ben: I need to be careful not to make promises to you :-}.  This is the
initial
> bit of rope that the string tug on removing EnsureNoPendingDownloads() turned
> into.  My proposed replacement involved using SetFileFactoryForTesting, which
is
> only accessible in content, which requires writing content_browsertests, which
> requires moving DTO.  I don't *think* I'm going to need more than three CLs,
but
> we'll see what happens :-}.

[jam, 2012-08-13 16:00:21]

http://codereview.chromium.org/10855116/diff/1/content/public/test/download_t...
File content/public/test/download_test_observer.h (right):

http://codereview.chromium.org/10855116/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.h:32: class DownloadTestObserver :
public content::DownloadManager::Observer,
nit: get rid of content:: everywhere

http://codereview.chromium.org/10855116/diff/1/content/test/download_test_obs...
File content/test/download_test_observer.cc (right):

http://codereview.chromium.org/10855116/diff/1/content/test/download_test_obs...
content/test/download_test_observer.cc:79: content::RunMessageLoop();
ditto

[benjhayden, 2012-08-13 16:35:17]

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_browsertest.cc:350:
content::DownloadTestObserver* CreateWaiter(
using content::DownloadTestObserver?

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_test_file_chooser_observer.cc (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_test_file_chooser_observer.cc:43: virtual void
ChooseDownloadPath(content::DownloadItem* item,
Why is this private?

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_test_file_chooser_observer.h (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_test_file_chooser_observer.h:19: // NOTE:
Again, this overrides the ChromeDownloadManagerDelegate for |profile|.
This looks like a mosquito shotgun solution. Shouldn't there be a more specific
mechanism?

http://codereview.chromium.org/10855116/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:394:
scoped_ptr<content::DownloadTestObserver> observer(
using content::DownloadTestObserver?

[Randy Smith (Not in Mondays), 2012-08-13 17:16:41]

On 2012/08/13 15:59:57, John Abd-El-Malek wrote:
> On 2012/08/11 20:01:35, rdsmith wrote:
> > Ben: Could you review the main body of this test?
> > 
> > Scott: Could you review content/test & content/public/test?  Specifically,
I'm
> > not yet familiar enough with the chrome/content boundary to know if I've
done
> > everything right.  Do I need a CONTENT_EXPORT?
> 
> did you mean me? (you added me as a reviewer but not Scott, or maybe you added
> Scott and he switched to me :) ).

I had originally intended to target you, but found you weren't in the
test/OWNERS files, and switched to Scott.  Except not completely :-}.  Given
that you're in the parent OWNERS directory and it looks like you've already dove
in, I'm happy to switch to you--I'll assume you'll let me know if I need to pull
someone else in.  Specific point along these lines: Despite the somewhat
discouraging text in chrome/OWNERS I'm going assume it's ok to rely on your LGTM
for chrome/chrome_tests.gypi, as I think of that as a rubber stamp.

Thanks!

> 
> you don't need CONTNET_EXPORT for stuff in content/test (since it's linked
into
> the test binaries)
> 
> > 
> > Ben: I need to be careful not to make promises to you :-}.  This is the
> initial
> > bit of rope that the string tug on removing EnsureNoPendingDownloads()
turned
> > into.  My proposed replacement involved using SetFileFactoryForTesting,
which
> is
> > only accessible in content, which requires writing content_browsertests,
which
> > requires moving DTO.  I don't *think* I'm going to need more than three CLs,
> but
> > we'll see what happens :-}.

[Randy Smith (Not in Mondays), 2012-08-13 18:57:49]

PTAL.

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_browsertest.cc (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_browsertest.cc:350:
content::DownloadTestObserver* CreateWaiter(
On 2012/08/13 16:35:18, benjhayden_chromium wrote:
> using content::DownloadTestObserver?

There's been some discussion on chromium-dev discouraging it, and (without
having put a lot of thought into it) I'm gently in that camp--this means that a
code reader doesn't have to go looking through the file to figure out what
namespace a symbol actually is in.  But I don't have strong feelings about it,
especially in test code--if you'd like me to do this enough to ask twice :-},
I'll do it.

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_test_file_chooser_observer.cc (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_test_file_chooser_observer.cc:43: virtual void
ChooseDownloadPath(content::DownloadItem* item,
On 2012/08/13 16:35:18, benjhayden_chromium wrote:
> Why is this private?

I didn't write the code, but I suspect the answer is "because it can be".  The
original is protected rather than private so that it can be overridden by
derived test classes, and this is a derived test class that doesn't have any
overrides, and the who point of this class is overriding this method, so
inheriting from this class (rather than CDMD) and wanting to override
ChooseDownloadPath() doesn't make much sense to me.  

Having said all that, it probably makes sense for it to be protected, just for
consistency with the base class.  My only hesitation is that the CL is just
moving stuff around, rather than changing functionality.  But I'll make the
change.

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
File chrome/browser/download/download_test_file_chooser_observer.h (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/download/downlo...
chrome/browser/download/download_test_file_chooser_observer.h:19: // NOTE:
Again, this overrides the ChromeDownloadManagerDelegate for |profile|.
On 2012/08/13 16:35:18, benjhayden_chromium wrote:
> This looks like a mosquito shotgun solution. Shouldn't there be a more
specific
> mechanism?

Quite possibly; I don't have a strong feeling about it.  This has the advantage
of not requiring any extra interfaces in production code other than the ones we
have, and the disadvantage (called out above) that if you override the CDMD this
way, you can't override it any other way.  Still, I agree that the mosquito
seems a little overpowered by this solution. 

But you're not going to make me rearchitect this test support class just to do a
chrome->content refactor, are you?  :-} :-|

http://codereview.chromium.org/10855116/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10855116/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:394:
scoped_ptr<content::DownloadTestObserver> observer(
On 2012/08/13 16:35:18, benjhayden_chromium wrote:
> using content::DownloadTestObserver?

See comment elsewhere about "using".

http://codereview.chromium.org/10855116/diff/1/content/public/test/download_t...
File content/public/test/download_test_observer.h (right):

http://codereview.chromium.org/10855116/diff/1/content/public/test/download_t...
content/public/test/download_test_observer.h:32: class DownloadTestObserver :
public content::DownloadManager::Observer,
On 2012/08/13 16:00:21, John Abd-El-Malek wrote:
> nit: get rid of content:: everywhere

Done.

http://codereview.chromium.org/10855116/diff/1/content/test/download_test_obs...
File content/test/download_test_observer.cc (right):

http://codereview.chromium.org/10855116/diff/1/content/test/download_test_obs...
content/test/download_test_observer.cc:79: content::RunMessageLoop();
On 2012/08/13 16:00:21, John Abd-El-Malek wrote:
> ditto

Done.

[benjhayden, 2012-08-13 19:05:13]

LGTM
Fair enough on all counts.

[jam, 2012-08-13 20:41:36]

lgtm

[commit-bot: I haz the power, 2012-08-14 14:59:17]

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

[commit-bot: I haz the power, 2012-08-14 17:12:28]

Change committed as 151496