Prettify output from chrome.fileSystem.getDisplayPath for Windows profile directory

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 c6fc2ec98af12f1abcba331b9f16f27e1f3431fc..13588ee7664da7d327d07ccbb9c8a5febda1230d 100644
--- a/chrome/browser/extensions/api/file_system/file_system_api.cc
+++ b/chrome/browser/extensions/api/file_system/file_system_api.cc
@@ -7,6 +7,8 @@
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/path_service.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"
@@ -38,6 +40,34 @@ namespace ChooseFile = file_system::ChooseFile;
 
 namespace {
 
+struct RewritePair {
+  int path_key;
+  const char* output;
+};
+
+RewritePair g_rewrite_pairs[] = {
+#if defined(OS_WIN)
+  {base::DIR_PROFILE, "~"},
+#endif
+  {-1, NULL}
+};
+
+void PrettifyPath(FilePath& file_path) {

[not at google - send to devlin, 2012/07/09 02:07:38]

Except in special circumstances, only pass const-references to functions. If you
need to modify an argument pass a pointer.

Or make this method return the new FilePath.

[thorogood, 2012/07/09 05:12:45]

I've made it return the new FilePath (by value). I've changed the argument to
"const FilePath& file_path", purely for speed.

[not at google - send to devlin, 2012/07/09 05:44:58]

The reason for these const-reference vs non-const-reference vs pointer arguments
isn't because of speed (even though it may make a small difference).

The reason is for readability. Passing non-const references is bad because you
can't tell from the call site whether the value your passing might be modified.
You'd need to look at the function's prototype. We make it a rule to only allow
passing const references so that this question never arises; to modify
something, pass a pointer.

[thorogood, 2012/07/09 05:58:35]

Great, thanks for the write-up.

+  for (int i = 0; g_rewrite_pairs[i].output; ++i) {

[not at google - send to devlin, 2012/07/09 02:07:38]

Use arraysize(g_rewrite_pairs) rather than a sentinel value.

Luckily I think the only compiler that cares about declaring an empty array is
windows, so that should be fine.

[thorogood, 2012/07/09 05:12:45]

Hilariously, I can't use arraysize() on an empty array. I've added a macro'ed
block and a comment.

Although I feel like I should probably wrap this entire code block in the macro
instead. However, I am about to write a follow-up CL adding the unix option here
(it needs some work, as your HOME directory is not already available in the
PathService).

+    FilePath candidate_path;
+    if (!PathService::Get(g_rewrite_pairs[i].path_key, &candidate_path)) {
+      continue;
+    }

[not at google - send to devlin, 2012/07/09 02:07:38]

nit: no {}

[thorogood, 2012/07/09 05:12:45]

Done.

+
+    FilePath output = FilePath::FromUTF8Unsafe(g_rewrite_pairs[i].output);
+    if (candidate_path.AppendRelativePath(file_path, &output)) {
+      DCHECK(!output.IsAbsolute());

[not at google - send to devlin, 2012/07/09 02:07:38]

This check could be done at construction time of PathPrettifier, if you decide
to go with that route.

I actually don't really understand the need for this DCHECK. What is it testing?
Should there be a "continue" if it fails? Perhaps a comment would be
appropriate.

[thorogood, 2012/07/09 05:12:45]

I've added a comment. Basically I'm trying to say that this should never be
absolute, since if it is, that might collide with some other "real" display
path. e.g. you wouldn't use /HOME/ as a fake prefix, since the user might
actually have a folder called "/HOME/". But obviously my comment is a bit
simpler than that explanation.

+      file_path = output;
+      return; // Only prettify the first hit.

[not at google - send to devlin, 2012/07/09 02:07:38]

nit: 2 spaces between ; and //

[thorogood, 2012/07/09 05:12:45]

Done.

+    }
+  }
+}

[not at google - send to devlin, 2012/07/09 02:07:38]

TBH I think this would be more nicely factored as a class (declared in this
file, within the namespace{}), with g_rewrite_pairs a std::map from whatever
that int is to whatever the string means. It would reduce the global surface of
this utility from 3 things to 1.

class PathPrettifier {
 public:
  PathPrettifier() {
#if defined(OS_WIN)
  paths[base::DIR_PROFILE] = "~";
#endif
  }

  void Prettify(FilePath* path) {
    ...
  }

 private:
  std::map<int, std::string> paths_;
};

base::LazyInstance<PathPrettifier> g_lazy_instance = LAZY_INSTANCE_INITIALIZER;

[benwells, 2012/07/09 03:42:45]

FWIW I have no strong preference but prefer it like it is.

[thorogood, 2012/07/09 05:12:45]

I've taken benwells@' comment and left this as-is, but I can see how kalman@'s
approach is also valid.

+
 bool g_skip_picker_for_test = false;
 FilePath* g_path_to_be_picked_for_test;
 
@@ -107,6 +137,7 @@ bool FileSystemGetDisplayPathFunction::RunImpl() {
                               render_view_host_, &file_path, &error_))
     return false;
 
+  PrettifyPath(file_path);
   result_.reset(base::Value::CreateStringValue(file_path.value()));
   return true;
 }