download: Remove DownloadManager::MockDownloadOpenForTesting

download: Remove DownloadManager::MockDownloadOpenForTesting

Rename DownloadTestFileChooserObserver to DownloadTestFileActivityObserver.
Implement empty MockDownloadManagerDelegate::OpenDownload in download_test_file_chooser_observer.cc

BUG=171224
TEST=browser_test --gtest_filter="Download*Open"
TBR=benjhayden@chromium.org for chrome/browser/extensions/api/downloads, jcivelli@chromium.org for content/public/test, ben@chromium.org for chrome/browser/ui


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179627

[hashimoto, 2013-01-23 10:10:57]

Please review

[Randy Smith (Not in Mondays), 2013-01-23 21:23:34]

https://codereview.chromium.org/12047039/diff/1/chrome/browser/download/downl...
File chrome/browser/download/download_test_file_chooser_observer.cc (right):

https://codereview.chromium.org/12047039/diff/1/chrome/browser/download/downl...
chrome/browser/download/download_test_file_chooser_observer.cc:53: virtual void
OpenDownload(content::DownloadItem* item) OVERRIDE {}
Is this class being used in DownloadTest.AutoOpen?  I don't see how it would be.
 

Mocking out opening is also not really part of this class' concept.  I think the
right thing is to create a new class in
chrome/browser/download/download_browsertest.cc derived from
ChromeDownloadManagerDelegate that just mocks out Open(), and use it in that
test only.

[hashimoto, 2013-01-24 05:52:30]

https://codereview.chromium.org/12047039/diff/1/chrome/browser/download/downl...
File chrome/browser/download/download_test_file_chooser_observer.cc (right):

https://codereview.chromium.org/12047039/diff/1/chrome/browser/download/downl...
chrome/browser/download/download_test_file_chooser_observer.cc:53: virtual void
OpenDownload(content::DownloadItem* item) OVERRIDE {}
On 2013/01/23 21:23:34, rdsmith wrote:
> Is this class being used in DownloadTest.AutoOpen?  I don't see how it would
be.
Both DownloadTest.AutoOpen and DownloadExtensionTest.DownloadExtensionTest_Open
are using this class by instantiating DownloadTestFileChooserObserver in their
test fixture classes.
> 
> 
> Mocking out opening is also not really part of this class' concept.
Since this class is the only user of
DownloadService::SetDownloadManagerDelegateForTesting, and is responsible to
'mock' user-facing download related behavior, I thought this class is the place
where this empty OpenDownload() should be. (Perhaps, it should be better renamed
to MockDownloadManagerDelegate or something.)

> I think the
> right thing is to create a new class in
> chrome/browser/download/download_browsertest.cc derived from
> ChromeDownloadManagerDelegate that just mocks out Open(), and use it in that
> test only.
Since DownloadTestFileChooserObserver is already setup in
DownloadTest:SetUpOnMainThread for all tests under DownloadTest, I think adding
one empty method here is quite simpler than setting up a new
DownloadManagerDelegate.
The same goes for DownloadExtensionTest.

Thoughts?

[Randy Smith (Not in Mondays), 2013-01-24 18:32:57]

On 2013/01/24 05:52:30, hashimoto wrote:
>
https://codereview.chromium.org/12047039/diff/1/chrome/browser/download/downl...
> File chrome/browser/download/download_test_file_chooser_observer.cc (right):
> 
>
https://codereview.chromium.org/12047039/diff/1/chrome/browser/download/downl...
> chrome/browser/download/download_test_file_chooser_observer.cc:53: virtual
void
> OpenDownload(content::DownloadItem* item) OVERRIDE {}
> On 2013/01/23 21:23:34, rdsmith wrote:
> > Is this class being used in DownloadTest.AutoOpen?  I don't see how it would
> be.
> Both DownloadTest.AutoOpen and
DownloadExtensionTest.DownloadExtensionTest_Open
> are using this class by instantiating DownloadTestFileChooserObserver in their
> test fixture classes.

Ah, I missed that.  And I think I was the one that wrote the class :-} :-{.  

> > Mocking out opening is also not really part of this class' concept.
> Since this class is the only user of
> DownloadService::SetDownloadManagerDelegateForTesting, and is responsible to
> 'mock' user-facing download related behavior, I thought this class is the
place
> where this empty OpenDownload() should be. (Perhaps, it should be better
renamed
> to MockDownloadManagerDelegate or something.)

I think that's probably the right solution.  Would you be willing to rename
DownloadTestFileChooserObserver and MockFileChooserDownloadManagerDelegate? 
Maybe something like DownloadTestExternalOverrideObserver or something like
that?  Alter the comments to indicate that both opening activity and file
choosing activity are being intercepted. 

> 
> > I think the
> > right thing is to create a new class in
> > chrome/browser/download/download_browsertest.cc derived from
> > ChromeDownloadManagerDelegate that just mocks out Open(), and use it in that
> > test only.
> Since DownloadTestFileChooserObserver is already setup in
> DownloadTest:SetUpOnMainThread for all tests under DownloadTest, I think
adding
> one empty method here is quite simpler than setting up a new
> DownloadManagerDelegate.
> The same goes for DownloadExtensionTest.
> 
> Thoughts?

No, you're right.  I just need to reconceptualize what the classes do.  Which I
think suggests a name change :-}.

[hashimoto, 2013-01-25 04:10:25]

New patch set.
PTAL.

DownloadTestFileChooserObserver is renamed to DownloadTestFileActivityObserver.
internal::MockFileChooserDownloadManagerDelegate becomes a internal class of
DownloadTestFileActivityObserver and now simply named 
MockDownloadManagerDelegate.

[Randy Smith (Not in Mondays), 2013-01-25 18:08:14]

LGTM with comment tweak below.  Thanks for doing this!

https://codereview.chromium.org/12047039/diff/11001/chrome/browser/download/d...
File chrome/browser/download/download_test_file_activity_observer.h (right):

https://codereview.chromium.org/12047039/diff/11001/chrome/browser/download/d...
chrome/browser/download/download_test_file_activity_observer.h:12: // Observes
and overrides file related activity for a profile.
Actually, if you can just list the activity (file chooser dialog and opening)
here, I'd appreciate it--I think file related activity is a bit too broad.  For
instance, we're not intercepting show in folder functionality.

[hashimoto, 2013-01-30 10:18:58]

https://codereview.chromium.org/12047039/diff/11001/chrome/browser/download/d...
File chrome/browser/download/download_test_file_activity_observer.h (right):

https://codereview.chromium.org/12047039/diff/11001/chrome/browser/download/d...
chrome/browser/download/download_test_file_activity_observer.h:12: // Observes
and overrides file related activity for a profile.
On 2013/01/25 18:08:14, rdsmith wrote:
> Actually, if you can just list the activity (file chooser dialog and opening)
> here, I'd appreciate it--I think file related activity is a bit too broad. 
For
> instance, we're not intercepting show in folder functionality.

Done.

[hashimoto, 2013-01-30 10:22:34]

TBR-ing owner of each directory for simple renaming changes:
benjhayden@chromium.org for chrome/browser/extensions/api/downloads,
jcivelli@chromium.org for content/public/test, ben@chromium.org for
chrome/browser/ui

[commit-bot: I haz the power, 2013-01-30 10:23:24]

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

[commit-bot: I haz the power, 2013-01-30 13:55:20]

Change committed as 179627