[MediaGalleries] Mock the app data dir on Windows/Mac

[MediaGalleries] Mock the app data dir on Windows/Mac

This fixes a local problem where if the test machine has iTunes installed,
some of the tests assume it won't be found and so fail.

R=thestig@chromium.org
BUG=None

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

[Lei Zhang, 2013-06-18 19:57:17]

Can you write a dummy test that doesn't use EnsureMediaDirectoriesExists and
make sure it's not using the overridden path from here? If it is, then that
means EnsureMediaDirectoriesExists should save the original path before
overriding, and restore it in the dtor.

https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/media_galleries_test_util.cc:97:
PathService::OverrideAndCreateIfNeeded(
You can just use PathService::Override(), which is the same as
OverrideAndCreateIfNeeded(...., true).

https://codereview.chromium.org/17389007/diff/1/chrome/browser/storage_monito...
File chrome/browser/storage_monitor/test_storage_monitor_win.h (right):

https://codereview.chromium.org/17389007/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/test_storage_monitor_win.h:34: virtual Receiver*
receiver() const OVERRIDE;
This is suppose to be part of the CL? Just want to make sure.

[Greg Billock, 2013-06-19 05:13:15]

https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/media_galleries_test_util.cc:97:
PathService::OverrideAndCreateIfNeeded(
On 2013/06/18 19:57:17, Lei Zhang wrote:
> You can just use PathService::Override(), which is the same as
> OverrideAndCreateIfNeeded(...., true).

Switched to using ScopedPathOverride, but was having all kinds of trouble with
my win checkout to get it tested, so I haven't uploaded it yet. It ends up
looking quite different. (SPO uses Override(), which I also figured out is doing
this.)

https://codereview.chromium.org/17389007/diff/1/chrome/browser/storage_monito...
File chrome/browser/storage_monitor/test_storage_monitor_win.h (right):

https://codereview.chromium.org/17389007/diff/1/chrome/browser/storage_monito...
chrome/browser/storage_monitor/test_storage_monitor_win.h:34: virtual Receiver*
receiver() const OVERRIDE;
On 2013/06/18 19:57:17, Lei Zhang wrote:
> This is suppose to be part of the CL? Just want to make sure.

I think so. It's mostly internal fixture plumbing as well, so I put it in this
cl rather than the other. Doesn't make much difference.

[Lei Zhang, 2013-06-19 05:18:54]

https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
chrome/browser/media_galleries/media_galleries_test_util.cc:97:
PathService::OverrideAndCreateIfNeeded(
On 2013/06/19 05:13:15, Greg Billock wrote:
> On 2013/06/18 19:57:17, Lei Zhang wrote:
> > You can just use PathService::Override(), which is the same as
> > OverrideAndCreateIfNeeded(...., true).
> 
> Switched to using ScopedPathOverride, but was having all kinds of trouble with
> my win checkout to get it tested, so I haven't uploaded it yet. It ends up
> looking quite different. (SPO uses Override(), which I also figured out is
doing
> this.)

Doh, ping me when there's a patch set 2 and I'll take a look.

[Greg Billock, 2013-06-19 15:27:03]

On 2013/06/19 05:18:54, Lei Zhang wrote:
>
https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
> File chrome/browser/media_galleries/media_galleries_test_util.cc (right):
> 
>
https://codereview.chromium.org/17389007/diff/1/chrome/browser/media_gallerie...
> chrome/browser/media_galleries/media_galleries_test_util.cc:97:
> PathService::OverrideAndCreateIfNeeded(
> On 2013/06/19 05:13:15, Greg Billock wrote:
> > On 2013/06/18 19:57:17, Lei Zhang wrote:
> > > You can just use PathService::Override(), which is the same as
> > > OverrideAndCreateIfNeeded(...., true).
> > 
> > Switched to using ScopedPathOverride, but was having all kinds of trouble
with
> > my win checkout to get it tested, so I haven't uploaded it yet. It ends up
> > looking quite different. (SPO uses Override(), which I also figured out is
> doing
> > this.)
> 
> Doh, ping me when there's a patch set 2 and I'll take a look.

Ping'd

[Lei Zhang, 2013-06-19 21:06:49]

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:74: #endif
Can we keep the #else, so Chrome and Android don't try to compile code they'll
never use?

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:76:
DCHECK(fake_dir_.CreateUniqueTempDir());
Can't do things in a DCHECK.

Let's just leave the ASSERT_TRUE()s we had before. This code is only used for
testing.

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.h (right):

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.h:38:
scoped_ptr<base::ScopedPathOverride> appdir_override_;
You may want to mention why we are doing this. Otherwise someone is going to
read this and raise an eyebrow.

[Greg Billock, 2013-06-20 05:24:52]

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.cc (right):

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:74: #endif
On 2013/06/19 21:06:50, Lei Zhang wrote:
> Can we keep the #else, so Chrome and Android don't try to compile code they'll
> never use?

Done.

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.cc:76:
DCHECK(fake_dir_.CreateUniqueTempDir());
On 2013/06/19 21:06:50, Lei Zhang wrote:
> Can't do things in a DCHECK.
> 
> Let's just leave the ASSERT_TRUE()s we had before. This code is only used for
> testing.

You're right. I always do this. It can't be ASSERT_TRUE because it's in a
constuctor, but I'll fix it.

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.h (right):

https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.h:38:
scoped_ptr<base::ScopedPathOverride> appdir_override_;
On 2013/06/19 21:06:50, Lei Zhang wrote:
> You may want to mention why we are doing this. Otherwise someone is going to
> read this and raise an eyebrow.

There's an explanation in the cc file.

[Lei Zhang, 2013-06-20 06:03:46]

On 2013/06/20 05:24:52, Greg Billock wrote:
https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
> chrome/browser/media_galleries/media_galleries_test_util.cc:76:
> DCHECK(fake_dir_.CreateUniqueTempDir());
> On 2013/06/19 21:06:50, Lei Zhang wrote:
> > Can't do things in a DCHECK.
> > 
> > Let's just leave the ASSERT_TRUE()s we had before. This code is only used
for
> > testing.
> 
> You're right. I always do this. It can't be ASSERT_TRUE because it's in a
> constuctor, but I'll fix it.

Right, which is why we had Init() before.

In the previous code, if the temp dir creation fails, then the test that tried
to use this class fails, in both debug and release mode. Then gtest can continue
running the next test.

With the new code, if the temp dir creation fails, then the test will crash and
no more tests will run. In release mode, it'll end up creating directories in
$PWD, which may not get deleted at the end of the test..

[Greg Billock, 2013-06-20 15:31:01]

On 2013/06/20 06:03:46, Lei Zhang wrote:
> On 2013/06/20 05:24:52, Greg Billock wrote:
>
https://codereview.chromium.org/17389007/diff/11002/chrome/browser/media_gall...
> > chrome/browser/media_galleries/media_galleries_test_util.cc:76:
> > DCHECK(fake_dir_.CreateUniqueTempDir());
> > On 2013/06/19 21:06:50, Lei Zhang wrote:
> > > Can't do things in a DCHECK.
> > > 
> > > Let's just leave the ASSERT_TRUE()s we had before. This code is only used
> for
> > > testing.
> > 
> > You're right. I always do this. It can't be ASSERT_TRUE because it's in a
> > constuctor, but I'll fix it.
> 
> Right, which is why we had Init() before.
> 
> In the previous code, if the temp dir creation fails, then the test that tried
> to use this class fails, in both debug and release mode. Then gtest can
continue
> running the next test.
> 
> With the new code, if the temp dir creation fails, then the test will crash
and
> no more tests will run. In release mode, it'll end up creating directories in
> $PWD, which may not get deleted at the end of the test..

Yeah, you're right. That's better. I'll use that.

[Lei Zhang, 2013-06-20 22:10:18]

lgtm

https://codereview.chromium.org/17389007/diff/30001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.h (right):

https://codereview.chromium.org/17389007/diff/30001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.h:30: virtual
~EnsureMediaDirectoriesExists();
not virtual

[Greg Billock, 2013-06-20 23:30:56]

https://codereview.chromium.org/17389007/diff/30001/chrome/browser/media_gall...
File chrome/browser/media_galleries/media_galleries_test_util.h (right):

https://codereview.chromium.org/17389007/diff/30001/chrome/browser/media_gall...
chrome/browser/media_galleries/media_galleries_test_util.h:30: virtual
~EnsureMediaDirectoriesExists();
On 2013/06/20 22:10:18, Lei Zhang wrote:
> not virtual

Done.

[commit-bot: I haz the power, 2013-06-20 23:39:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/17389007/37001

[commit-bot: I haz the power, 2013-06-21 09:53:10]

Change committed as 207777