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

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

Modifies the getDisplayPath to allow for replacing specific prefixes with short, relative path names. Enables this for Windows-only for now, replacing the full path to the user's profile directory with "HOME/".

BUG=135690
TEST=


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

[thorogood, 2012-07-06 00:07:51]

Trybot now passes. :)

[benwells, 2012-07-06 00:34:35]

Looking good, just a few comments / questions ....

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

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:42: /*struct
RewritePair {
We should uncomment this and use to simplify g_rewrite_pairs below.

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:52:
{base::DIR_PROFILE, "HOME"},
Can we make HOME a bit ... prettier?

I'm open to ideas ... I like "~", but something like "Sam Thorogood" would be
consistent with Windows Explorer.

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:60: if
(!PathService::Get(g_rewrite_pairs[i].path_key, &candidate_path)) {
Should failure here be an error?

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/test/data/ex...
File
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js
(right):

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/test/data/ex...
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js:14:
chrome.test.assertTrue(path.indexOf("HOME\\") == 0);
When supported on other platforms we will want this tested there too, so best to
leave out the path seperator. Just checking we start with "HOME" (or whatever we
choose) is good enough.

[thorogood, 2012-07-06 06:34:16]

Sorry for the delay.

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

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:42: /*struct
RewritePair {
On 2012/07/06 00:34:36, benwells wrote:
> We should uncomment this and use to simplify g_rewrite_pairs below.

Done. Although this was mostly left over; I actually intended it to be
completely an anonymous inline struct :)

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:52:
{base::DIR_PROFILE, "HOME"},
On 2012/07/06 00:34:36, benwells wrote:
> Can we make HOME a bit ... prettier?
> 
> I'm open to ideas ... I like "~", but something like "Sam Thorogood" would be
> consistent with Windows Explorer.

Probably - I'm not sure of a consistent way to get the user's login name right
now. I will look.

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:60: if
(!PathService::Get(g_rewrite_pairs[i].path_key, &candidate_path)) {
On 2012/07/06 00:34:36, benwells wrote:
> Should failure here be an error?

Probably not. Get() returns false if it's just an empty result (as well as for
in error cases); and maybe DIR_PROFILE (or other fields we add later) might not
be available some places.

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/test/data/ex...
File
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js
(right):

https://chromiumcodereview.appspot.com/10693089/diff/2005/chrome/test/data/ex...
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js:14:
chrome.test.assertTrue(path.indexOf("HOME\\") == 0);
On 2012/07/06 00:34:36, benwells wrote:
> When supported on other platforms we will want this tested there too, so best
to
> leave out the path seperator. Just checking we start with "HOME" (or whatever
we
> choose) is good enough.

Done, left as HOME for now

[thorogood, 2012-07-09 01:15:54]

I've updated the code to use "~" as the prefix.

[benwells, 2012-07-09 01:35:25]

lgtm, adding kalman who is an owner

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

I'm not too fussed about the class thing in file_system_api.cc, but consider it.

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

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:55: void
PrettifyPath(FilePath& file_path) {
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.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:56: for (int i = 0;
g_rewrite_pairs[i].output; ++i) {
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.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:60: }
nit: no {}

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:64:
DCHECK(!output.IsAbsolute());
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.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:66: return; // Only
prettify the first hit.
nit: 2 spaces between ; and //

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:69: }
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;

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/test/data/e...
File
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js
(right):

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/test/data/e...
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js:14:
chrome.test.assertTrue(path.indexOf("~") == 0);
incorporate this check into the other two conditions?

path.indexOf("~/" == 0  // POSIX
etc

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/test/data/e...
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js:15:
chrome.test.assertTrue(path.indexOf("/") == 1 || // POSIX
nit: 2 spaces between the || and the //, same on next line.

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

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

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:69: }
On 2012/07/09 02:07:38, kalman wrote:
> 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;

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

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

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

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:55: void
PrettifyPath(FilePath& file_path) {
On 2012/07/09 02:07:38, kalman wrote:
> 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.

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

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:56: for (int i = 0;
g_rewrite_pairs[i].output; ++i) {
On 2012/07/09 02:07:38, kalman wrote:
> 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.

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).

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:60: }
On 2012/07/09 02:07:38, kalman wrote:
> nit: no {}

Done.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:64:
DCHECK(!output.IsAbsolute());
On 2012/07/09 02:07:38, kalman wrote:
> 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.

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.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:66: return; // Only
prettify the first hit.
On 2012/07/09 02:07:38, kalman wrote:
> nit: 2 spaces between ; and //

Done.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:69: }
I've taken benwells@' comment and left this as-is, but I can see how kalman@'s
approach is also valid.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/test/data/e...
File
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js
(right):

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/test/data/e...
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js:14:
chrome.test.assertTrue(path.indexOf("~") == 0);
On 2012/07/09 02:07:38, kalman wrote:
> incorporate this check into the other two conditions?
> 
> path.indexOf("~/" == 0  // POSIX
> etc

Done.

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/test/data/e...
chrome/test/data/extensions/api_test/file_system/get_display_path_prettify/test.js:15:
chrome.test.assertTrue(path.indexOf("/") == 1 || // POSIX
On 2012/07/09 02:07:38, kalman wrote:
> nit: 2 spaces between the || and the //, same on next line.

Done.

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

lgtm

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

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:55: void
PrettifyPath(FilePath& file_path) {
On 2012/07/09 05:12:45, thorogood wrote:
> On 2012/07/09 02:07:38, kalman wrote:
> > 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.
> 
> I've made it return the new FilePath (by value). I've changed the argument to
> "const FilePath& file_path", purely for speed.

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.

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

https://chromiumcodereview.appspot.com/10693089/diff/5013/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:60: size_t size =
0;
haha

https://chromiumcodereview.appspot.com/10693089/diff/5013/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:74: return output; 
// Only prettify the first hit.
this comment unnecessary, I think.

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

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

https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
chrome/browser/extensions/api/file_system/file_system_api.cc:55: void
PrettifyPath(FilePath& file_path) {
On 2012/07/09 05:44:58, kalman wrote:
> On 2012/07/09 05:12:45, thorogood wrote:
> > On 2012/07/09 02:07:38, kalman wrote:
> > > 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.
> > 
> > I've made it return the new FilePath (by value). I've changed the argument
to
> > "const FilePath& file_path", purely for speed.
> 
> 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.

Great, thanks for the write-up.

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

https://chromiumcodereview.appspot.com/10693089/diff/5013/chrome/browser/exte...
chrome/browser/extensions/api/file_system/file_system_api.cc:74: return output; 
// Only prettify the first hit.
On 2012/07/09 05:44:58, kalman wrote:
> this comment unnecessary, I think.

Done.

[commit-bot: I haz the power, 2012-07-09 05:58:48]

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

[thorogood, 2012-07-09 05:58:51]

On 2012/07/09 05:58:35, thorogood wrote:
>
https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
> File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
> chrome/browser/extensions/api/file_system/file_system_api.cc:55: void
> PrettifyPath(FilePath& file_path) {
> On 2012/07/09 05:44:58, kalman wrote:
> > On 2012/07/09 05:12:45, thorogood wrote:
> > > On 2012/07/09 02:07:38, kalman wrote:
> > > > 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.
> > > 
> > > I've made it return the new FilePath (by value). I've changed the argument
> to
> > > "const FilePath& file_path", purely for speed.
> > 
> > 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.
> 
> Great, thanks for the write-up.
> 
>
https://chromiumcodereview.appspot.com/10693089/diff/5013/chrome/browser/exte...
> File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10693089/diff/5013/chrome/browser/exte...
> chrome/browser/extensions/api/file_system/file_system_api.cc:74: return
output; 
> // Only prettify the first hit.
> On 2012/07/09 05:44:58, kalman wrote:
> > this comment unnecessary, I think.
> 
> Done.

[thorogood, 2012-07-09 07:29:20]

On 2012/07/09 05:58:51, thorogood wrote:
> On 2012/07/09 05:58:35, thorogood wrote:
> >
>
https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
> > File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10693089/diff/14002/chrome/browser/ext...
> > chrome/browser/extensions/api/file_system/file_system_api.cc:55: void
> > PrettifyPath(FilePath& file_path) {
> > On 2012/07/09 05:44:58, kalman wrote:
> > > On 2012/07/09 05:12:45, thorogood wrote:
> > > > On 2012/07/09 02:07:38, kalman wrote:
> > > > > 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.
> > > > 
> > > > I've made it return the new FilePath (by value). I've changed the
argument
> > to
> > > > "const FilePath& file_path", purely for speed.
> > > 
> > > 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.
> > 
> > Great, thanks for the write-up.
> > 
> >
>
https://chromiumcodereview.appspot.com/10693089/diff/5013/chrome/browser/exte...
> > File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10693089/diff/5013/chrome/browser/exte...
> > chrome/browser/extensions/api/file_system/file_system_api.cc:74: return
> output; 
> > // Only prettify the first hit.
> > On 2012/07/09 05:44:58, kalman wrote:
> > > this comment unnecessary, I think.
> > 
> > Done.

[commit-bot: I haz the power, 2012-07-09 07:29:25]

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

[commit-bot: I haz the power, 2012-07-09 08:37:19]

Change committed as 145650