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

chrome/browser/extensions/api/file_system/file_system_api.cc

Index: chrome/browser/extensions/api/file_system/file_system_api.cc
diff --git a/chrome/browser/extensions/api/file_system/file_system_api.cc b/chrome/browser/extensions/api/file_system/file_system_api.cc
index 915da20379831171ffdeb5904a5685d5116a6609..e90946ccfe7158c16d26de6fb8bec2553090fd88 100644
--- a/chrome/browser/extensions/api/file_system/file_system_api.cc
+++ b/chrome/browser/extensions/api/file_system/file_system_api.cc
@@ -4,6 +4,10 @@
 
 #include "chrome/browser/extensions/api/file_system/file_system_api.h"
 
+#if defined(OS_MACOSX)

[benwells, 2012/09/04 04:15:01]

Move OS specific includes to after the rest
(http://dev.chromium.org/developers/coding-style)

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

Done, thanks.

+#include "base/mac/foundation_util.h"
+#endif
+
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
@@ -49,38 +53,55 @@ namespace ChooseFile = file_system::ChooseFile;
 
 namespace {
 
-struct RewritePair {

[benwells, 2012/09/04 04:15:01]

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.

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

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.

-  int path_key;
-  const char* output;
-};
-
-const RewritePair g_rewrite_pairs[] = {
+// Get the user's (optional) home directory via PathService.
+FilePath GetHome() {
+  int home_key = 0;
 #if defined(OS_WIN)
-  {base::DIR_PROFILE, "~"},
+  home_key = base::DIR_PROFILE;
 #elif defined(OS_POSIX)
-  {base::DIR_HOME, "~"},
+  home_key = base::DIR_HOME;
 #endif
-};
+  FilePath home_path;
+  if (home_key && PathService::Get(home_key, &home_path)) {
+    return home_path;
+  }
+  return FilePath();
+}
 
-FilePath PrettifyPath(const FilePath& file_path) {
-#if defined(OS_WIN) || defined(OS_POSIX)
-  for (size_t i = 0; i < arraysize(g_rewrite_pairs); ++i) {
-    FilePath candidate_path;
-    if (!PathService::Get(g_rewrite_pairs[i].path_key, &candidate_path))
-      continue;  // We don't DCHECK this value, as Get will return false even
-                 // if the path_key gives a blank string as a result.
-
-    FilePath output = FilePath::FromUTF8Unsafe(g_rewrite_pairs[i].output);
-    if (candidate_path.AppendRelativePath(file_path, &output)) {
-      // The output path must not be absolute, as it might collide with the
-      // real filesystem.
-      DCHECK(!output.IsAbsolute());
-      return output;
-    }
+// Prettifies |source_path|. If the path is inside the user's home directory,
+// then replace the home directory component with "~".
+FilePath PrettifyPath(const FilePath& source_path) {
+  DCHECK(source_path.IsAbsolute());
+  FilePath home_path = GetHome();
+
+#if defined(OS_MACOSX)
+  // Break down the incoming path into components, and grab the display name
+  // for every component. This will match app bundles, ".localized" folders,
+  // and localized subfolders of the user's home directory.
+  std::vector<FilePath::StringType> components;
+  source_path.GetComponents(&components);
+  FilePath display_path = FilePath(components[0]);
+  FilePath actual_path = display_path;
+  for (std::vector<FilePath::StringType>::iterator i = components.begin() + 1;
+       i != components.end(); ++i) {
+    actual_path = actual_path.Append(*i);
+    std::string display_name = base::mac::GetDisplayNameForPath(actual_path);
+    display_path = display_path.Append(display_name);

[benwells, 2012/09/04 04:15:01]

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?

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

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.

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

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.

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

Updated this bit for the new method call.

+
+    // If the display path is absolute, we haven't replaced the home directory
+    // subcomponents with "~" yet.
+    if (display_path.IsAbsolute() && home_path == actual_path)

[benwells, 2012/09/04 04:15:01]

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]

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.

+      display_path = FilePath("~");
   }
+  DCHECK(actual_path == source_path);
+  return display_path;
+#else
+  // If found, replace the leading home directory with "~".
+  FilePath display_path("~");
+  if (!home_path.empty() && home_path.AppendRelativePath(source_path, &output))
+    return display_path;
+  return source_path;
 #endif
-
-  return file_path;
 }
 
 bool g_skip_picker_for_test = false;