chrome_paths: refactor and sanitize cache directory handling

chrome_paths: refactor and sanitize cache directory handling

Previously we had a bunch of logic in profile_impl.cc that computed
where the cache directory lives.  This change relocates it to
chrome_paths.cc.

Some background on cache directories.
There are two possible places to store cache files:
1) in the user data directory
2) in an OS-specific user cache directory

On Windows, we always pick (1).  On Mac and Linux, we currently use
(2) in some circumstances.  This patch changes both Mac and Linux to
have the same behavior with respect to (2).

The Mac/Linux shared behavior is that if the profile directory
is in the standard location for profiles, we put the cache files
into a matching directory name in the standard system cache directory
(e.g. on Linux if you're using ~/.config/google-chrome, your cache
ends up in ~/.cache/google-chrome; on Mac, the directories are
~/Library/Application Support versus ~/Library/Caches).  If your user
data directory is not in the standard location, we use behavior (1).

The semantic changes of this patch should be:
- On Mac, previously we checked whether the (2) directory had some
  particular subdirectories already when picking which one to use.
  This was removed; which directory is used is solely a question of
  whether the profile directory is in the standard location.
  I think the previous behavior was unpredictable.
- On Linux, previously we only used behavior (2) if you hadn't changed
  your user-data-directory at all.  Now, to match Mac, as long as
  your user-data-dir is in the standard place, you use the system
  cache dir.  So e.g. using ~/.config/foobar puts your cache in
  ~/.cache/foobar.
- On Linux, previously the default cache would end up as directories
  under ~/.cache/google-chrome/; now it ends up as directories under
  ~/.cache/google-chrome/Default/.

(In all instances above, on Linux we continue to obey $XDG_CACHE_HOME.)

BUG=59824
TEST=New test ChromePaths.UserCacheDir

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

[Evan Martin, 2010-11-17 22:26:51]

akalin/mark: mac change
thestig/phajdan: linux change
rvargas: FYI, just if you'd like to comment

This change will change the default location of our cache in some circumstances.
 I don't think this is harmful, as a new cache directory will work ok.  It does
mean we might leave behind some old cache dir cruft, but since it's just cache
files I also think that's probably ok.

[Mark Mentovai, 2010-11-17 22:32:49]

This sounds good on the basis of the CL description. I haven’t read the code.
Yet. But be warned, I will.

[Lei Zhang, 2010-11-18 01:36:55]

Can this be done without changing base/base_paths* ?

The reason I put DIR_USER_CACHE in base/ originally is because I thought the
code would be useful to other projects that used base/, since using the cache
directory is not Chromium specific.

[Evan Martin, 2010-11-18 01:54:47]

On 2010/11/18 01:36:55, Lei Zhang wrote:
> Can this be done without changing base/base_paths* ?
> 
> The reason I put DIR_USER_CACHE in base/ originally is because I thought the
> code would be useful to other projects that used base/, since using the cache
> directory is not Chromium specific.

This is what Pawel probably thought too, when he accidentally caused our cache
corruption bug.  We need a separate cache dir per profile.

Unless you mean you want both DIR_USER_CACHE as well as a separate thing for the
Chrome cache?

[Evan Martin, 2010-11-18 01:55:10]

On 2010/11/18 01:54:47, Evan Martin wrote:
> On 2010/11/18 01:36:55, Lei Zhang wrote:
> > Can this be done without changing base/base_paths* ?
> > 
> > The reason I put DIR_USER_CACHE in base/ originally is because I thought the
> > code would be useful to other projects that used base/, since using the
cache
> > directory is not Chromium specific.
> 
> This is what Pawel probably thought too, when he accidentally caused our cache
> corruption bug.  We need a separate cache dir per profile.
> 
> Unless you mean you want both DIR_USER_CACHE as well as a separate thing for
the
> Chrome cache?

(Part of the reason I knew it was ok to move this from base to chrome is that
there was only one user of it, profile_impl.cc)

[Lei Zhang, 2010-11-18 02:10:35]

On 2010/11/18 01:54:47, Evan Martin wrote:
> On 2010/11/18 01:36:55, Lei Zhang wrote:
> > Can this be done without changing base/base_paths* ?
> > 
> > The reason I put DIR_USER_CACHE in base/ originally is because I thought the
> > code would be useful to other projects that used base/, since using the
cache
> > directory is not Chromium specific.
> 
> This is what Pawel probably thought too, when he accidentally caused our cache
> corruption bug.  We need a separate cache dir per profile.
> 
> Unless you mean you want both DIR_USER_CACHE as well as a separate thing for
the
> Chrome cache?

I think the code that simply figures out where DIR_USER_CACHE is can stay in
base, while all the Chromium specific logic of figuring out when to use
DIR_USER_CACHE and where to put our cache under DIR_USER_CACHE to be in chrome/.

[Lei Zhang, 2010-11-18 02:11:49]

On 2010/11/18 02:10:35, Lei Zhang wrote:
> On 2010/11/18 01:54:47, Evan Martin wrote:
> > On 2010/11/18 01:36:55, Lei Zhang wrote:
> > > Can this be done without changing base/base_paths* ?
> > > 
> > > The reason I put DIR_USER_CACHE in base/ originally is because I thought
the
> > > code would be useful to other projects that used base/, since using the
> cache
> > > directory is not Chromium specific.
> > 
> > This is what Pawel probably thought too, when he accidentally caused our
cache
> > corruption bug.  We need a separate cache dir per profile.
> > 
> > Unless you mean you want both DIR_USER_CACHE as well as a separate thing for
> the
> > Chrome cache?
> 
> I think the code that simply figures out where DIR_USER_CACHE is can stay in
> base, while all the Chromium specific logic of figuring out when to use
> DIR_USER_CACHE and where to put our cache under DIR_USER_CACHE to be in
chrome/.

Arg, grammar fail. s/to be in chrome/should be in chrome/

[Paweł Hajdan Jr., 2010-11-18 10:39:57]

LGTM

[Mark Mentovai, 2010-11-18 16:54:54]

LGTM

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_internal.h (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_internal.h:27: // Note that we create subdirectories
from this directory, like
Eh, do you HAVE to say “we?” Can’t you say “this creates?” After all, this
(GetUserCacheDirectory) is what does the creating, not “us” (Evan and Mark?)

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_mac.mm:64: FilePath system_relative_dir = cache_dir;
Do you need this copy? Why don’t you just move the GetUserDirectory call right
here and have it fill in system_relative_dir?

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_unittest.cc (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_unittest.cc:18: FilePath homedir =
FilePath(getenv("HOME"));
Why not file_util::GetHomeDir() as in non-Mac POSIX?

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_unittest.cc:24: // Verify that a custom user data dir
doesn't get a custom cache dir.
This block is the same for Mac and non-Mac POSIX, maybe put it outside of the
#if.

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_unittest.cc:36:
ASSERT_TRUE(chrome::GetUserCacheDirectory(user_data_dir, &cache_dir));
For that matter, maybe you can put this line outside of the #if, and the next
one too if you refactor a tiny bit to have an expected_cache_dir string inside
the #if. Then the #ifs would just contain a couple of basic variable assignments
and the meat of the test would be shared.

[Mark Mentovai, 2010-11-18 16:55:57]

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_linux.cc:46: FilePath system_relative_dir =
cache_dir;
By the way, “why the copy?” applies here too.

[Evan Martin, 2010-11-18 18:13:07]

Ok, PTAL.

I reinstated base::DIR_CACHE (but I renamed it slightly -- I'm scared of people
thinking that base::DIR_USER_CACHE is the Chrome cache directory, because it
looks like DIR_USER_DATA or something)

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_internal.h (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_internal.h:27: // Note that we create subdirectories
from this directory, like
On 2010/11/18 16:54:54, Mark Mentovai wrote:
> Eh, do you HAVE to say “we?” Can’t you say “this creates?” After all, this
> (GetUserCacheDirectory) is what does the creating, not “us” (Evan and
Mark?)

The caller ends up creating those directories.  But I know your hatred of "we",
so I'll rephrase.

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_mac.mm:64: FilePath system_relative_dir = cache_dir;
On 2010/11/18 16:54:54, Mark Mentovai wrote:
> Do you need this copy? Why don’t you just move the GetUserDirectory call right
> here and have it fill in system_relative_dir?

I found the usage of AppendRelativePath kind of hard to follow, so I thought the
extra naming would help.  I'll try a different approach.

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_unittest.cc (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_unittest.cc:18: FilePath homedir =
FilePath(getenv("HOME"));
On 2010/11/18 16:54:54, Mark Mentovai wrote:
> Why not file_util::GetHomeDir() as in non-Mac POSIX?

We only have a Linux impl.  (If $HOME is unset you have to go to more obscure
logic.)  It's what I tried first.  :\

I'm not too happy about this, I was hoping you had a better suggestion for it. 
I'm not sure how magical ~/Library/Caches is on mac, like whether that ~ is
always getenv(HOME).

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_unittest.cc:36:
ASSERT_TRUE(chrome::GetUserCacheDirectory(user_data_dir, &cache_dir));
On 2010/11/18 16:54:54, Mark Mentovai wrote:
> For that matter, maybe you can put this line outside of the #if, and the next
> one too if you refactor a tiny bit to have an expected_cache_dir string inside
> the #if. Then the #ifs would just contain a couple of basic variable
assignments
> and the meat of the test would be shared.

Great idea, I've now rewritten the test to be much shorter.

[Mark Mentovai, 2010-11-18 19:11:48]

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_unittest.cc (right):

http://codereview.chromium.org/5123004/diff/14001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_unittest.cc:18: FilePath homedir =
FilePath(getenv("HOME"));
Evan Martin wrote:
> On 2010/11/18 16:54:54, Mark Mentovai wrote:
> > Why not file_util::GetHomeDir() as in non-Mac POSIX?
> 
> We only have a Linux impl.  (If $HOME is unset you have to go to more obscure
> logic.)  It's what I tried first.  :\
> 
> I'm not too happy about this, I was hoping you had a better suggestion for it.

I looked at base/file_util.h and saw that GetHomeDir wasn’t decorated inside a
per-OS #if, so I figured it was safe. Let’s see when that happened…

Uh-oh!

r45380 | evan@chromium.org | 2010-04-22 19:28:05 -0400 (Thu, 22 Apr 2010)

I can give you a quick Mac implementation if you’d like. The right answer is to
call NSHomeDirectory() (and, for us, take the -fileSystemRepresentation and
convert it to a FilePath).

> I'm not sure how magical ~/Library/Caches is on mac, like whether that ~ is
> always getenv(HOME).

Although I’ve never seen a case where Caches wasn’t in ~/Library, we should be
defensive.

http://codereview.chromium.org/5123004/diff/34001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/5123004/diff/34001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_mac.mm:60: FilePath cache_dir = cache_dir;
Definitely not.

[Evan Martin, 2010-11-18 19:18:54]

On 2010/11/18 19:11:48, Mark Mentovai wrote:
> I can give you a quick Mac implementation if you’d like.

I hate adding APIs to base just to satisfy a test corner case.  I would prefer
to remove GetHomeDir on the Linux side if I could...

> The right answer is to
> call NSHomeDirectory() (and, for us, take the -fileSystemRepresentation and
> convert it to a FilePath).

Since the test is verifying that we handle the special Library/Caches dir
properly, so perhaps I should just use PathService to fetch these directly? 
Going just halfway to use NSHomeDirectory but then not use the API to get the
full paths doesn't make a lot of sense to me...

On the other hand, redundantly naming these paths explicitly might help catch
regressions if the PathService code broke for whatever reason.

Tell me what you would like, and I can make it work.

[Evan Martin, 2010-11-18 19:19:06]

http://codereview.chromium.org/5123004/diff/34001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/5123004/diff/34001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_mac.mm:60: FilePath cache_dir = cache_dir;
On 2010/11/18 19:11:48, Mark Mentovai wrote:
> Definitely not.

Aw, c'mon, it's tautologically true!


*embarrassed face*

[Mark Mentovai, 2010-11-18 19:36:49]

Evan Martin wrote:
> Since the test is verifying that we handle the special Library/Caches dir
> properly, so perhaps I should just use PathService to fetch these directly?
> Going just halfway to use NSHomeDirectory but then not use the API to get the
> full paths doesn't make a lot of sense to me...
>
> On the other hand, redundantly naming these paths explicitly might help catch
> regressions if the PathService code broke for whatever reason.
>
> Tell me what you would like, and I can make it work.

I guess we should use the path service, then. It gives a better
end-to-end test, and since the “what’s my cache?” call winds up
depending on the path service anyway, it doesn’t introduce a new
dependency.

[Evan Martin, 2010-11-22 23:16:13]

On 2010/11/18 19:36:49, Mark Mentovai wrote:
> I guess we should use the path service, then. It gives a better

Refactored to use path service (see diffs from patch set 8).  PTAL.

[akalin, 2010-11-22 23:22:44]

http://codereview.chromium.org/5123004/diff/42001/chrome/browser/profile_impl.cc
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/5123004/diff/42001/chrome/browser/profile_impl...
chrome/browser/profile_impl.cc:279: if (!chrome::GetUserCacheDirectory(path_,
&base_cache_path_))
All implementations of GetUserCacheDirectory() already set the output dir to the
input profile dir and return true for the default case; why not make
GetUserCacheDirectory return void, or change the implementations to return
'false' as the default case?

[Evan Martin, 2010-11-22 23:31:38]

OK, PTAL

[akalin, 2010-11-22 23:54:20]

LGTM

On 2010/11/22 23:31:38, Evan Martin wrote:
> OK, PTAL

[akalin, 2010-11-22 23:54:28]

http://codereview.chromium.org/5123004/diff/50001/chrome/common/chrome_paths_...
File chrome/common/chrome_paths_unittest.cc (right):

http://codereview.chromium.org/5123004/diff/50001/chrome/common/chrome_paths_...
chrome/common/chrome_paths_unittest.cc:15: // Test the behavior of
chrome::GetUserCacheDirectory.
May be worth testing that the function remains a no-op on Windows

[Evan Martin, 2010-11-23 18:57:52]

PTAL

[akalin, 2010-11-23 19:08:11]

LGTM

On 2010/11/23 18:57:52, Evan Martin wrote:
> PTAL