Set OS X cache directory to ~/Library/Caches/[app name]/[profile name]

Set OS X cache directory to ~/Library/Caches/[app name]/[profile name]

- Added implementation of GetUserCacheDirectory() for OS X.
- Added FilePath::GetRelativePath().
- Implemented per-profile cache directories for OS X.

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

[Mark Mentovai, 2009-08-19 12:57:46]

Sending mail so that it shows up in my list.

Is this ready for review?

[Mark Mentovai, 2009-08-19 15:33:22]

Mostly, I've got questions.

1. When someone launches the app with --user-data-dir, where does the cache wind
up being stored?

2. This results in a cache directory at ~/Library/Caches/Chromium/Default/Cache,
right?  (I'm hoping so.)

http://codereview.chromium.org/174053/diff/1008/9
File base/file_path.cc (right):

http://codereview.chromium.org/174053/diff/1008/9#newcode165
Line 165: bool FilePath::GetRelativePath(const FilePath& child,
I don't think we need both GetRelativePath and GetRelativePathInternal, when the
former is just a subset of the latter.  It's fine to allow external callers to
see the version that can tolerate a NULL argument, and in fact I think it's
better to avoid the |*relative_path = FilePath();| and just mandate that the
caller pass in an empty FilePath to avoid the double-construction.

http://codereview.chromium.org/174053/diff/1008/10
File base/file_path.h (right):

http://codereview.chromium.org/174053/diff/1008/10#newcode154
Line 154: // Returns true if IsParent(child) holds and then fills in
I'll be surprised if we wind up needing either of these two methods.  I'll keep
reading to see what you have in mind.

http://codereview.chromium.org/174053/diff/1008/10#newcode261
Line 261: // if NULL and relative_path is assumed to be empty if non-NULL.
filled in if if NULL

http://codereview.chromium.org/174053/diff/1008/12
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/174053/diff/1008/12#newcode538
Line 538: base_cache_path_ = user_cache_path.Append(relative_path);
What's special about our use here that we need to go through these hoops?  Can
we not put an absolute path into base_cache_path_?

http://codereview.chromium.org/174053/diff/1008/13
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/174053/diff/1008/13#newcode87
Line 87: // No concept of a separate cache directory on non-Linux/non-OS X
You can just say "on other systems".  The #ifdef is authoritative, the comment
doesn't need to restate what it says.

http://codereview.chromium.org/174053/diff/1008/15
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/174053/diff/1008/15#newcode39
Line 39: bool GetUserCacheDirectory(FilePath* result) {
Well, this works for GetDefaultCacheDirectory, but it's not quite
GetUserCacheDirectory, right?

[Fred, 2009-08-21 04:36:33]

On 2009/08/19 15:33:22, Mark Mentovai wrote:
> Mostly, I've got questions.
> 
> 1. When someone launches the app with --user-data-dir, where does the cache
wind
> up being stored?
> 
> 2. This results in a cache directory at
~/Library/Caches/Chromium/Default/Cache,
> right?  (I'm hoping so.)

No, it does not get stored in ~/Library/Caches.  Isn't that what we want?  In
the issue, you say:

> If the profile is outside of ~/Library/Application Support, the cache
directory should 
> remain within the profile by default.  This enables profile portability.  So
does the bit 
> about leaving the cache in the profile if it's already found there.

(Of course, if the user passes in --user-data-dir=~/Library/Application Support
then it won't get stored in ~/Library/Caches either, but presumably the user
wants a portable profile.)

> 
> http://codereview.chromium.org/174053/diff/1008/9
> File base/file_path.cc (right):
> 
> http://codereview.chromium.org/174053/diff/1008/9#newcode165
> Line 165: bool FilePath::GetRelativePath(const FilePath& child,
> I don't think we need both GetRelativePath and GetRelativePathInternal, when
the
> former is just a subset of the latter.  It's fine to allow external callers to
> see the version that can tolerate a NULL argument, and in fact I think it's
> better to avoid the |*relative_path = FilePath();| and just mandate that the
> caller pass in an empty FilePath to avoid the double-construction.

Changed GetRelativePathInternal() to AppendRelativePath() and made it public. 
Also removed GetRelativePath().

> http://codereview.chromium.org/174053/diff/1008/10
> File base/file_path.h (right):
> 
> http://codereview.chromium.org/174053/diff/1008/10#newcode154
> Line 154: // Returns true if IsParent(child) holds and then fills in
> I'll be surprised if we wind up needing either of these two methods.  I'll
keep
> reading to see what you have in mind.

I couldn't find any existing code that lets me chop off part of a path and stick
it on another one.

> http://codereview.chromium.org/174053/diff/1008/12#newcode538
> Line 538: base_cache_path_ = user_cache_path.Append(relative_path);
> What's special about our use here that we need to go through these hoops?  Can
> we not put an absolute path into base_cache_path_?

I'm not sure what hoops you mean.  I refrain from touching base_cache_path_
until everything else succeeds, as otherwise the user may end up with
base_cache_path_ pointing to just ~/Library/Cache/Chromium.

> http://codereview.chromium.org/174053/diff/1008/13
> File chrome/common/chrome_paths.cc (right):
> 
> http://codereview.chromium.org/174053/diff/1008/13#newcode87
> Line 87: // No concept of a separate cache directory on non-Linux/non-OS X
> You can just say "on other systems".  The #ifdef is authoritative, the comment
> doesn't need to restate what it says.

Done.

> http://codereview.chromium.org/174053/diff/1008/15
> File chrome/common/chrome_paths_mac.mm (right):
> 
> http://codereview.chromium.org/174053/diff/1008/15#newcode39
> Line 39: bool GetUserCacheDirectory(FilePath* result) {
> Well, this works for GetDefaultCacheDirectory, but it's not quite
> GetUserCacheDirectory, right?

Yes, the name is inaccurate for OS X.  But changing it would be inconsistent
with DIR_USER_CACHE, and changing *that* would be inconsistent with the Linux
version.  So I think we should wait on renaming it until we make the same change
for Linux.  I added a TODO.

I uploaded my most recent changes.  PTAL.

[Fred, 2009-08-22 12:00:39]

For some reason your latest mail isn't in here.  Here it is:

akalin@gmail.com wrote:
>> 2. This results in a cache directory at
>
> ~/Library/Caches/Chromium/Default/Cache,
>>
>> right?  (I'm hoping so.)
>
> No, it does not get stored in ~/Library/Caches.  Isn't that what we
> want?  In the issue, you say:

I meant...

Right now, we'll put the cache at ~/Library/Application
Support/Chromium/Default/Cache.  Under the new regime, do we get a
directory at ~/Library/Caches/Chromium/Default, or
~/Library/Caches/Chromium/Default/Cache?  The latter is preferable, in
case we want to put other cachey things in there by taking them out of
the profile's Default directory.  Basically I want the two to be able
to parallel each other.

It was a different question than the #2, but reading them together was
probably confusing.

> (Of course, if the user passes in --user-data-dir=~/Library/Application
> Support then it won't get stored in ~/Library/Caches either, but
> presumably the user wants a portable profile.)

Hmm.  A few things might be unclear here.  Let's clarify.

I want "--user-data-dir=~/Library/Application Support/Google/Chrome"
to use ~/Library/Caches/Google/Chrome/Cache so that when I ask
Chromium to run with a Chrome profile, it also uses the Chrome cache.

For portability, the suggestion (optional, but nice) would be to
always check the profile to see if it has a Cache directory in it, and
if so, use that as the cache.  (No migration for old profiles, but I
think that's OK.)

For a user-data-dir outside of ~/Library/Application Support, you
should always just use the cache in the profile.

Mark

[Fred, 2009-08-22 12:09:30]

On 2009/08/22 12:00:39, Fred wrote:

> I meant...
> 
> Right now, we'll put the cache at ~/Library/Application
> Support/Chromium/Default/Cache.  Under the new regime, do we get a
> directory at ~/Library/Caches/Chromium/Default, or
> ~/Library/Caches/Chromium/Default/Cache?  The latter is preferable, in
> case we want to put other cachey things in there by taking them out of
> the profile's Default directory.  Basically I want the two to be able
> to parallel each other.

Yes it's the latter.  The fact that we have a media cache too necessitates that.

> Hmm.  A few things might be unclear here.  Let's clarify.
> 
> I want "--user-data-dir=~/Library/Application Support/Google/Chrome"
> to use ~/Library/Caches/Google/Chrome/Cache so that when I ask
> Chromium to run with a Chrome profile, it also uses the Chrome cache.
> 
> For portability, the suggestion (optional, but nice) would be to
> always check the profile to see if it has a Cache directory in it, and
> if so, use that as the cache.  (No migration for old profiles, but I
> think that's OK.)
> 
> For a user-data-dir outside of ~/Library/Application Support, you
> should always just use the cache in the profile.

I think I understand you now.  The latest patch set should do what you want. 
The logic in profile.cc now goes something like the following for OS X: if the
profile directory has neither a 'Cache' or a 'MediaCache' subdirectory and it is
in "~/Library/Application Support", then set the base cache dir to be
"~/Library/Caches/(subdir of Application Support)" (which 'Cache' or
'MediaCache' will be appended too).

The rest of the changes are just to support the above:

- I noticed that DIR_APP_DATA and DIR_LOCAL_APP_DATA weren't implemented for OS
X.  Removed the latter, fixed the comment on the former and implemented it. 
Made GetDefaultUserDataDirectory() just append stuff to DIR_APP_DATA.  Aadded
DIR_CACHE key for OS X.  Also added unittests for OS X path keys.
- Added FilePath::AppendRelativePath() and unittests.
- Refactored some cache directory mangling code in profile.cc.

PTAL.

[Mark Mentovai, 2009-09-15 20:25:41]

I'm profusely sorry for the delay, but I like where this has wound up.  If you
could fix these up and update to make sure that everything still works, I'll
make one final quick pass and I think we'll have a winner.

http://codereview.chromium.org/174053/diff/52/1056
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/174053/diff/52/1056#newcode19
Line 19: bool GetUserDirectory(NSSearchPathDirectory directory, FilePath*
result) {
There isn't an extra level of indentation inside a namespace.

http://codereview.chromium.org/174053/diff/52/1056#newcode19
Line 19: bool GetUserDirectory(NSSearchPathDirectory directory, FilePath*
result) {
I like this function.  Can we take it out of the anonymous namespace, list it in
a header somewhere, and use it some more from chrome/common/chrome_paths_mac.mm?

http://codereview.chromium.org/174053/diff/52/1058
File base/file_path.h (right):

http://codereview.chromium.org/174053/diff/52/1058#newcode155
Line 155: // relative path to child.  For example, if parent holds "/foo/bar",
relative path to child and returns true.

http://codereview.chromium.org/174053/diff/52/1058#newcode158
Line 158: // will hold "/quux/corge/baz/qux".  Otherwise, returns false.
Since the behavior as described here seems kind of weird, it might be more
useful to supply a real-world example using Library/Application Support paths
instead of this made-up stuff.

[Fred, 2009-09-16 12:35:49]

On 2009/09/15 20:25:41, Mark Mentovai wrote:
> I'm profusely sorry for the delay, but I like where this has wound up.  If you
> could fix these up and update to make sure that everything still works, I'll
> make one final quick pass and I think we'll have a winner.
> 
> http://codereview.chromium.org/174053/diff/52/1056
> File base/base_paths_mac.mm (right):
> 
> http://codereview.chromium.org/174053/diff/52/1056#newcode19
> Line 19: bool GetUserDirectory(NSSearchPathDirectory directory, FilePath*
> result) {
> There isn't an extra level of indentation inside a namespace.

Fixed.

> 
> http://codereview.chromium.org/174053/diff/52/1056#newcode19
> Line 19: bool GetUserDirectory(NSSearchPathDirectory directory, FilePath*
> result) {
> I like this function.  Can we take it out of the anonymous namespace, list it
in
> a header somewhere, and use it some more from
chrome/common/chrome_paths_mac.mm?

Yes, but I'd rather do it in a separate patch as it isn't as trivial as I
thought, and I found some other places which could use this function (which I
didn't want to pull into this patch).  I added a TODO, though.

> 
> http://codereview.chromium.org/174053/diff/52/1058
> File base/file_path.h (right):
> 
> http://codereview.chromium.org/174053/diff/52/1058#newcode155
> Line 155: // relative path to child.  For example, if parent holds "/foo/bar",
> relative path to child and returns true.

Fixed.
> 
> http://codereview.chromium.org/174053/diff/52/1058#newcode158
> Line 158: // will hold "/quux/corge/baz/qux".  Otherwise, returns false.
> Since the behavior as described here seems kind of weird, it might be more
> useful to supply a real-world example using Library/Application Support paths
> instead of this made-up stuff.

Done.

Also updated and retested.  PTAL.

[Mark Mentovai, 2009-09-16 18:33:41]

LGTM.

Great!  I'm going to apply this locally and make sure the try bots like it, and
if everything still looks good there, I'll check this in with my suggested
changes.

http://codereview.chromium.org/174053/diff/4001/4003
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/174053/diff/4001/4003#newcode19
Line 19: // TODO(akalin): Export this function somewhere and use it in
Minor style nit.  Leave a blank line after the namespace { line and before the }
 // namespace line.

http://codereview.chromium.org/174053/diff/4001/4003#newcode26
Line 26: NSSearchPathForDirectoriesInDomains(directory, NSUserDomainMask, YES);
Minor style nit.  Continuation lines get indented by four, not two, extra
spaces.

http://codereview.chromium.org/174053/diff/4001/4005
File base/file_path.h (right):

http://codereview.chromium.org/174053/diff/4001/4005#newcode156
Line 156: // holds "/Users/johndoe/Application Support", child holds
Might be nice to put Library in here to make it more real-world-ish, but this is
much easier to follow now with a real example.

[Mark Mentovai, 2009-09-16 18:55:19]

http://codereview.chromium.org/174053/diff/4001/4006
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/174053/diff/4001/4006#newcode530
Line 530: const FilePath base("blah");
On Windows:

C:\b\slave\win\build\src\base\file_path_unittest.cc(530) : error C2664:
'FilePath::FilePath(const FilePath &)' : cannot convert parameter 1 from 'const
char [5]' to 'const FilePath &'
        Reason: cannot convert from 'const char [5]' to 'const FilePath'
        No constructor could take the source type, or constructor overload
resolution was ambiguous

I've changed this to:

  const FilePath base(FPL("base"));

http://codereview.chromium.org/174053/diff/4001/4006#newcode536
Line 536: FilePath result("");
Same error, I've changed this to:

      FilePath result;

[Mark Mentovai, 2009-09-16 19:33:27]

http://codereview.chromium.org/174053/diff/4001/4009
File chrome/common/chrome_paths_mac.mm (right):

http://codereview.chromium.org/174053/diff/4001/4009#newcode19
Line 19: *result = result->Append("Google")->Append("Chrome");
In a Mac build with GOOGLE_CHROME_BUILD defined:

.../chrome/common/chrome_paths_mac.mm: In function ‘bool
chrome::GetDefaultUserDataDirectory(FilePath*)’:
.../chrome/common/chrome_paths_mac.mm:19: error: base operand of ‘->’ has
non-pointer type ‘FilePath’

I'm changing this to:

    *result = result->Append("Google").Append("Chrome");

[Mark Mentovai, 2009-09-16 21:04:13]

Committed at r26387.  Thanks!