Prettify Mac names, rewrite getDisplayPath API call to focus on $HOME

This updates the getDisplayPath fileSystem API call to focus more on prettifying the user's home directory: this seems to be the main obvious use of function for now (it's previous extensibility was probably needless).

Additionally, this adds code specifically to prettify Mac path names, which have various possibilities for prettifying through localization, but are singly exposed via the LSCopyDisplayNameForRef call.

Basically, on a Mac, folders that end with ".localized" or ".app" can be localized due to their contents (e.g., they may have a name for English, and another for German). Additionally, folders directly within your home directory may be localized by your system language settings (again, perhaps you're set to non-English). These folders do not change their actual name on disk, but within Finder/etc, they appear differently.

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

[thorogood, 2012-08-03 07:21:38]

Hi Ben,

The test is a bit of a mess, but the rest of this code looks good and has been
tested manually. PTAL - thanks!

[benwells, 2012-08-06 01:59:14]

http://chromiumcodereview.appspot.com/10823157/diff/5001/base/mac/foundation_...
File base/mac/foundation_util.h (right):

http://chromiumcodereview.appspot.com/10823157/diff/5001/base/mac/foundation_...
base/mac/foundation_util.h:90: void GetDisplayNameForPath(const FilePath& path,
std::string* result);
This will need to be BASE_EXPORT to work with the component build.

[thorogood, 2012-09-03 07:20:23]

thakis: base/mac
benwells: chrome/browser/extensions

Thanks! :)

https://chromiumcodereview.appspot.com/10823157/diff/5001/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/5001/base/mac/foundation...
base/mac/foundation_util.h:90: void GetDisplayNameForPath(const FilePath& path,
std::string* result);
On 2012/08/06 01:59:14, benwells wrote:
> This will need to be BASE_EXPORT to work with the component build.

Done.

[benwells, 2012-09-04 04:15:00]

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.h:90: BASE_EXPORT std::string
GetDisplayNameForPath(const FilePath& path);
Is this the display name for the whole path, or just for the base name?

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.mm:134: if (!FSRefFromPath(path.value(), &ref)) {
Nit: No need for braces around the body of these three ifs.

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.mm:144: NSObjectRelease((void *) str);
I have little idea about objective c memory but I am surprised by this for two
reasons:
1. I thought these things were garbage collected in objective c
2. If need to release, why not in the LSCopyDisplayNameForRef succeeding case?

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (left):

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:52: struct
RewritePair {
How are you planning on doing MyDocuments etc. now that the RewritePairs are
gone? Windows has bazillions more users than Mac so would be good to see this
soon.

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:7: #if
defined(OS_MACOSX)
Move OS specific includes to after the rest
(http://dev.chromium.org/developers/coding-style)

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:89: display_path =
display_path.Append(display_name);
Having two appends here feels strange. If you get display names (i.e. not just
base names) for the path more than once, does it mean it will have both of these
in the returned path? Or does GetDisplayNameForPath only return a prettified
base name?

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:93: if
(display_path.IsAbsolute() && home_path == actual_path)
Why is the check to IsAbsolute needed here? Can home_path be == actual_path more
than once?

If not this function could be made slightly easier to comprehend by moving this
check to just after actual_path = actual_path.Append, and making it continue if
it replaces display_path with ~.

[thorogood, 2012-09-04 05:57:33]

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.h:90: BASE_EXPORT std::string
GetDisplayNameForPath(const FilePath& path);
On 2012/09/04 04:15:01, benwells wrote:
> Is this the display name for the whole path, or just for the base name?

Just the base name. I've updated the comment to clarify.

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.mm:134: if (!FSRefFromPath(path.value(), &ref)) {
On 2012/09/04 04:15:01, benwells wrote:
> Nit: No need for braces around the body of these three ifs.

Done.

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.mm:144: NSObjectRelease((void *) str);
On 2012/09/04 04:15:01, benwells wrote:
> I have little idea about objective c memory but I am surprised by this for two
> reasons:
> 1. I thought these things were garbage collected in objective c

I believe it's optional. I don't believe we do that in Chrome. Additionally,
note that FSRef is not a pointer (it's an opaque struct), and we don't need to
release it.

> 2. If need to release, why not in the LSCopyDisplayNameForRef succeeding case?

LSCopyDisplayNameForRef returns 0 on success. We need to release it as specified
by its contract:
https://developer.apple.com/library/mac/documentation/Carbon/Reference/Launch...

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (left):

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:52: struct
RewritePair {
On 2012/09/04 04:15:01, benwells wrote:
> How are you planning on doing MyDocuments etc. now that the RewritePairs are
> gone? Windows has bazillions more users than Mac so would be good to see this
> soon.

Yup, I'm happy to do that too, but I realised that we are going to have to be
platform-specific for this: the RewritePair idiom does not work at all for Mac.
It might work for Windows, but I'd reintroduce it in that CL.

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:7: #if
defined(OS_MACOSX)
On 2012/09/04 04:15:01, benwells wrote:
> Move OS specific includes to after the rest
> (http://dev.chromium.org/developers/coding-style)

Done, thanks.

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:89: display_path =
display_path.Append(display_name);
On 2012/09/04 04:15:01, benwells wrote:
> Having two appends here feels strange. If you get display names (i.e. not just
> base names) for the path more than once, does it mean it will have both of
these
> in the returned path? Or does GetDisplayNameForPath only return a prettified
> base name?

GetDisplayNameForPath only returns a prettified base name. Since any component
of the path may be localized/prettified, then I think stepping through with two
paths - the real one and the virtual one - makes sense.

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:93: if
(display_path.IsAbsolute() && home_path == actual_path)
On 2012/09/04 04:15:01, benwells wrote:
> Why is the check to IsAbsolute needed here? Can home_path be == actual_path
more
> than once?
> 
> If not this function could be made slightly easier to comprehend by moving
this
> check to just after actual_path = actual_path.Append, and making it continue
if
> it replaces display_path with ~.

It was mostly a short-circuit; once you find $HOME, your path will never be
absolute, so this does a (faster) bool check rather than a (still pretty fast)
string comparison.

I've moved it to the top of the loop as you've suggested.

[benwells, 2012-09-04 06:38:11]

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.h:90: BASE_EXPORT std::string
GetDisplayNameForPath(const FilePath& path);
On 2012/09/04 05:57:33, thorogood wrote:
> On 2012/09/04 04:15:01, benwells wrote:
> > Is this the display name for the whole path, or just for the base name?
> 
> Just the base name. I've updated the comment to clarify.

More importantly can you change the name of the function?

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.mm:144: NSObjectRelease((void *) str);
On 2012/09/04 05:57:33, thorogood wrote:
> On 2012/09/04 04:15:01, benwells wrote:
> > I have little idea about objective c memory but I am surprised by this for
two
> > reasons:
> > 1. I thought these things were garbage collected in objective c
> 
> I believe it's optional. I don't believe we do that in Chrome. Additionally,
> note that FSRef is not a pointer (it's an opaque struct), and we don't need to
> release it.
> 
> > 2. If need to release, why not in the LSCopyDisplayNameForRef succeeding
case?
> 
> LSCopyDisplayNameForRef returns 0 on success. We need to release it as
specified
> by its contract:
>
https://developer.apple.com/library/mac/documentation/Carbon/Reference/Launch...

OK cool, sounds like you've looked into it.

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:89: display_path =
display_path.Append(display_name);
On 2012/09/04 05:57:33, thorogood wrote:
> On 2012/09/04 04:15:01, benwells wrote:
> > Having two appends here feels strange. If you get display names (i.e. not
just
> > base names) for the path more than once, does it mean it will have both of
> these
> > in the returned path? Or does GetDisplayNameForPath only return a prettified
> > base name?
> 
> GetDisplayNameForPath only returns a prettified base name. Since any component
> of the path may be localized/prettified, then I think stepping through with
two
> paths - the real one and the virtual one - makes sense.

Makes sense now, this is only confusing because of the name of the
GetDisplayNameForPath function which implies it is a name for the whole path.

https://chromiumcodereview.appspot.com/10823157/diff/16001/base/mac/foundatio...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/16001/base/mac/foundatio...
base/mac/foundation_util.mm:138: if (LSCopyDisplayNameForRef(&ref, &str))
add != noErr to make it clear this is a failure

[thorogood, 2012-09-04 06:45:43]

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/base/mac/foundation...
base/mac/foundation_util.h:90: BASE_EXPORT std::string
GetDisplayNameForPath(const FilePath& path);
On 2012/09/04 06:38:11, benwells wrote:
> On 2012/09/04 05:57:33, thorogood wrote:
> > On 2012/09/04 04:15:01, benwells wrote:
> > > Is this the display name for the whole path, or just for the base name?
> > 
> > Just the base name. I've updated the comment to clarify.
> 
> More importantly can you change the name of the function?

I've changed it to GetDisplayBaseNameForPath.

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/9001/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:89: display_path =
display_path.Append(display_name);
On 2012/09/04 06:38:11, benwells wrote:
> On 2012/09/04 05:57:33, thorogood wrote:
> > On 2012/09/04 04:15:01, benwells wrote:
> > > Having two appends here feels strange. If you get display names (i.e. not
> just
> > > base names) for the path more than once, does it mean it will have both of
> > these
> > > in the returned path? Or does GetDisplayNameForPath only return a
prettified
> > > base name?
> > 
> > GetDisplayNameForPath only returns a prettified base name. Since any
component
> > of the path may be localized/prettified, then I think stepping through with
> two
> > paths - the real one and the virtual one - makes sense.
> 
> Makes sense now, this is only confusing because of the name of the
> GetDisplayNameForPath function which implies it is a name for the whole path.

Updated this bit for the new method call.

https://chromiumcodereview.appspot.com/10823157/diff/16001/base/mac/foundatio...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/16001/base/mac/foundatio...
base/mac/foundation_util.mm:138: if (LSCopyDisplayNameForRef(&ref, &str))
On 2012/09/04 06:38:11, benwells wrote:
> add != noErr to make it clear this is a failure

Done.

[Nico, 2012-09-04 14:09:37]

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.h:89: // If the path is not localized, this will just
return the base name.
Why is this in base? It looks like it's only called in a single file, could it
be a static function in that file instead?

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.mm:134: if (!FSRefFromPath(path.value(), &ref))
Can you use a CFURL instead of a FSRef?

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.mm:142: NSObjectRelease((void *) str);
CFRelease

https://chromiumcodereview.appspot.com/10823157/diff/12003/chrome/browser/ext...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:98:
DCHECK(actual_path == source_path);
DCHECK_EQ

[thorogood, 2012-09-06 00:16:53]

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.h:89: // If the path is not localized, this will just
return the base name.
On 2012/09/04 14:09:37, Nico wrote:
> Why is this in base? It looks like it's only called in a single file, could it
> be a static function in that file instead?

Yes, it could! I have no strong opinions, but I believe this is a useful method
that meshes well with calls like "GetUserDirectory", which are all about user
folders such as Downloads, Desktop, etc..., and are most likely going to be
localized by the system.

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.mm:134: if (!FSRefFromPath(path.value(), &ref))
On 2012/09/04 14:09:37, Nico wrote:
> Can you use a CFURL instead of a FSRef?

It seems like I could, e.g.: [NSURL fileURLWithPath:path_string];

Is there a reason that would be preferred?

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.mm:142: NSObjectRelease((void *) str);
On 2012/09/04 14:09:37, Nico wrote:
> CFRelease

Done.

https://chromiumcodereview.appspot.com/10823157/diff/12003/chrome/browser/ext...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:98:
DCHECK(actual_path == source_path);
On 2012/09/04 14:09:37, Nico wrote:
> DCHECK_EQ

This seems to try to convert one of the arguments implicitly to a std::string.
I've used DCHECK_EQ with each of the path's value()s to solve this.

[Nico, 2012-09-06 00:22:28]

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.h:89: // If the path is not localized, this will just
return the base name.
On 2012/09/06 00:16:53, thorogood wrote:
> On 2012/09/04 14:09:37, Nico wrote:
> > Why is this in base? It looks like it's only called in a single file, could
it
> > be a static function in that file instead?
> 
> Yes, it could! I have no strong opinions, but I believe this is a useful
method
> that meshes well with calls like "GetUserDirectory", which are all about user
> folders such as Downloads, Desktop, etc..., and are most likely going to be
> localized by the system.

I think we're trying to not add things to base that could conceivably be useful
elsewhere down the road. We're trying to keep base/ smallish.

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.mm:134: if (!FSRefFromPath(path.value(), &ref))
On 2012/09/06 00:16:53, thorogood wrote:
> On 2012/09/04 14:09:37, Nico wrote:
> > Can you use a CFURL instead of a FSRef?
> 
> It seems like I could, e.g.: [NSURL fileURLWithPath:path_string];
> 
> Is there a reason that would be preferred?

FSRef is a very old type, and NSURL is what Apple seems to be advocating these
days. Since it's the same amount of code, I'd opt for the one less likely to
disappear in the medium future :-)

[thorogood, 2012-09-06 04:42:37]

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.h (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.h:89: // If the path is not localized, this will just
return the base name.
On 2012/09/06 00:22:28, Nico wrote:
> On 2012/09/06 00:16:53, thorogood wrote:
> > On 2012/09/04 14:09:37, Nico wrote:
> > > Why is this in base? It looks like it's only called in a single file,
could
> it
> > > be a static function in that file instead?
> > 
> > Yes, it could! I have no strong opinions, but I believe this is a useful
> method
> > that meshes well with calls like "GetUserDirectory", which are all about
user
> > folders such as Downloads, Desktop, etc..., and are most likely going to be
> > localized by the system.
> 
> I think we're trying to not add things to base that could conceivably be
useful
> elsewhere down the road. We're trying to keep base/ smallish.

I've put this back into file_system_api.cc. Could you still PTAL from the Mac
point of view?

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
File base/mac/foundation_util.mm (right):

https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
base/mac/foundation_util.mm:134: if (!FSRefFromPath(path.value(), &ref))
On 2012/09/06 00:22:28, Nico wrote:
> On 2012/09/06 00:16:53, thorogood wrote:
> > On 2012/09/04 14:09:37, Nico wrote:
> > > Can you use a CFURL instead of a FSRef?
> > 
> > It seems like I could, e.g.: [NSURL fileURLWithPath:path_string];
> > 
> > Is there a reason that would be preferred?
> 
> FSRef is a very old type, and NSURL is what Apple seems to be advocating these
> days. Since it's the same amount of code, I'd opt for the one less likely to
> disappear in the medium future :-)

Turns out it's now reasonably more verbose, both the create method and now since
I've got to own the object (whereas FSRef was just an opaque struct. Done
anyway, let me know what you think.

An alternative I see is I could probably write a NSURLFromPath method, which at
least makes this a bit easier to do (I've just copied my version from
native_library_mac.mm:32).

[Nico, 2012-09-06 05:21:38]

On 2012/09/06 04:42:37, thorogood wrote:
>
https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
> File base/mac/foundation_util.h (right):
> 
>
https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
> base/mac/foundation_util.h:89: // If the path is not localized, this will just
> return the base name.
> On 2012/09/06 00:22:28, Nico wrote:
> > On 2012/09/06 00:16:53, thorogood wrote:
> > > On 2012/09/04 14:09:37, Nico wrote:
> > > > Why is this in base? It looks like it's only called in a single file,
> could
> > it
> > > > be a static function in that file instead?
> > > 
> > > Yes, it could! I have no strong opinions, but I believe this is a useful
> > method
> > > that meshes well with calls like "GetUserDirectory", which are all about
> user
> > > folders such as Downloads, Desktop, etc..., and are most likely going to
be
> > > localized by the system.
> > 
> > I think we're trying to not add things to base that could conceivably be
> useful
> > elsewhere down the road. We're trying to keep base/ smallish.
> 
> I've put this back into file_system_api.cc. Could you still PTAL from the Mac
> point of view?
> 
>
https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
> File base/mac/foundation_util.mm (right):
> 
>
https://chromiumcodereview.appspot.com/10823157/diff/12003/base/mac/foundatio...
> base/mac/foundation_util.mm:134: if (!FSRefFromPath(path.value(), &ref))
> On 2012/09/06 00:22:28, Nico wrote:
> > On 2012/09/06 00:16:53, thorogood wrote:
> > > On 2012/09/04 14:09:37, Nico wrote:
> > > > Can you use a CFURL instead of a FSRef?
> > > 
> > > It seems like I could, e.g.: [NSURL fileURLWithPath:path_string];
> > > 
> > > Is there a reason that would be preferred?
> > 
> > FSRef is a very old type, and NSURL is what Apple seems to be advocating
these
> > days. Since it's the same amount of code, I'd opt for the one less likely to
> > disappear in the medium future :-)
> 
> Turns out it's now reasonably more verbose, both the create method and now
since
> I've got to own the object (whereas FSRef was just an opaque struct. Done
> anyway, let me know what you think.
> 
> An alternative I see is I could probably write a NSURLFromPath method, which
at
> least makes this a bit easier to do (I've just copied my version from
> native_library_mac.mm:32).

Mac bits lgtm. Someone should look at the extension bits too.

[thorogood, 2012-09-06 08:35:01]

Hey Ben, as discussed in-person, I've broken apart the OSX parts vs. the other
parts.

A bigger alternative is that we could also put this stuff into a helper .cc just
for platforms with cool implementations.

e.g.;
file_system_apiutil.cc does PrettifyPath for non-Mac
file_system_apiutil_mac.cc does PrettifyPath for Mac only, and we could add
_win.cc, etc... as we go along.

[thorogood, 2012-09-10 15:01:24]

On 2012/09/06 08:35:01, thorogood wrote:
> Hey Ben, as discussed in-person, I've broken apart the OSX parts vs. the other
> parts.
> 
> A bigger alternative is that we could also put this stuff into a helper .cc
just
> for platforms with cool implementations.
> 
> e.g.;
> file_system_apiutil.cc does PrettifyPath for non-Mac
> file_system_apiutil_mac.cc does PrettifyPath for Mac only, and we could add
> _win.cc, etc... as we go along.

By the way, it turns out that on Windows Vista and above, Windows works
basically the same way as on a Mac: it provides SHGetLocalizedName, which
functionally acts like LSCopyDisplayNameForURL. (Previously, folders were
physically localized).

It may be appropriate to yet again re-think this, and make this type of solution
the approach for most platforms, and just assume this 'sort' of method exists
everywhere. But this CL is still fine for now, I think.

[benwells, 2012-09-12 02:32:24]

lgtm with tiny nits

https://chromiumcodereview.appspot.com/10823157/diff/24002/chrome/browser/ext...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/24002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:130: #endif //
defined(OS_MACOSX)
Nit: two spaces before // (and above)

[commit-bot: I haz the power, 2012-09-13 02:49:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/24004

[thorogood, 2012-09-13 02:49:39]

https://chromiumcodereview.appspot.com/10823157/diff/24002/chrome/browser/ext...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10823157/diff/24002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:130: #endif //
defined(OS_MACOSX)
On 2012/09/12 02:32:24, benwells wrote:
> Nit: two spaces before // (and above)

Done.

[commit-bot: I haz the power, 2012-09-13 03:12:53]

Try job failure for 10823157-24004 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-09-13 04:37:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/22009

[commit-bot: I haz the power, 2012-09-13 04:39:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/9005

[commit-bot: I haz the power, 2012-09-13 06:14:56]

Try job failure for 10823157-9005 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-13 06:22:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/9005

[commit-bot: I haz the power, 2012-09-13 08:01:37]

Try job failure for 10823157-9005 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-09-13 22:38:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/22013

[commit-bot: I haz the power, 2012-09-13 22:44:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/23016

[commit-bot: I haz the power, 2012-09-13 22:59:21]

Retried try job too often for step(s) crypto_unittests, unit_tests,
cacheinvalidation_unittests, remoting_unittests, jingle_unittests,
nacl_integration, ipc_tests, interactive_ui_tests, browser_tests, net_unittests,
content_unittests, gpu_unittests, base_unittests, sync_integration_tests,
media_unittests, safe_browsing_tests, content_browsertests, sql_unittests,
printing_unittests, sync_unit_tests, check_deps

[commit-bot: I haz the power, 2012-09-14 04:19:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/23018

[commit-bot: I haz the power, 2012-09-14 04:23:26]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-09-16 00:21:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/17011

[commit-bot: I haz the power, 2012-09-16 01:19:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/14020

[commit-bot: I haz the power, 2012-09-16 03:08:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-16 03:11:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10823157/10032

[commit-bot: I haz the power, 2012-09-16 07:24:41]

Change committed as 157043