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

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
@@ skipping 25 matching lines @@
 
 const char kOpenFileOption[] = "openFile";
 const char kOpenWritableFileOption[] ="openWritableFile";
 const char kSaveFileOption[] = "saveFile";
 
 namespace file_system = extensions::api::file_system;
 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;
-};
+// Get the user's (optional) home directory via PathService.
+FilePath GetHome() {
+  int home_key = 0;
+#if defined(OS_WIN)
+  home_key = base::DIR_PROFILE;
+#elif defined(OS_POSIX)
+  home_key = base::DIR_HOME;
+#endif
+  FilePath home_path;
+  if (home_key && PathService::Get(home_key, &home_path)) {
+    return home_path;
+  }
+  return FilePath();
+}
 
-const RewritePair g_rewrite_pairs[] = {
-#if defined(OS_WIN)
-  {base::DIR_PROFILE, "~"},
-#elif defined(OS_POSIX)
-  {base::DIR_HOME, "~"},
+// 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
-};
-
-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;
-    }
-  }
-#endif
-
-  return file_path;
 }
 
 bool g_skip_picker_for_test = false;
 FilePath* g_path_to_be_picked_for_test;
 
 bool GetFilePathOfFileEntry(const std::string& filesystem_name,
                             const std::string& filesystem_path,
                             const content::RenderViewHost* render_view_host,
                             FilePath* file_path,
                             std::string* error) {
@@ skipping 459 matching lines @@
 
   if (entry_type == WRITABLE && !HasFileSystemWritePermission()) {
     error_ = kRequiresFileSystemWriteError;
     return false;
   }
 
   return ShowPicker(suggested_name, file_type_info, picker_type, entry_type);
 }
 
 }  // namespace extensions