Prettify output from chrome.fileSystem.getDisplayPath for POSIX profile directories

Prettify output from chrome.fileSystem.getDisplayPath for POSIX profile directories

This duplicates review 10693089, except for POSIX. Modifies e.g. "/home/sam/foo" to "~/foo". As a dependency, adds DIR_HOME to PathService for retrieving POSIX home directory.

BUG=135690
TEST=


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

[thorogood, 2012-07-10 06:51:01]

I'll need to find a reviewer for base/, too.

[thorogood, 2012-07-10 06:52:50]

On 2012/07/10 06:51:01, thorogood wrote:
> I'll need to find a reviewer for base/, too.

It's also worth noting that build_config.h fairly explicitly defines that we're
either OS_WIN or OS_POSIX.

[benwells, 2012-07-10 23:57:49]

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/file_system_api.cc:57: for (size_t i =
0; i < arraysize(g_rewrite_pairs); ++i) {
I don't like relying on the fact that OS_POSIX is !OS_WIN. Is this something
which is relied on elsewhere?

E.g. if we support iOS, will this break? Yeah, this code probably won't be in
that port but it doesn't feel good.

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:56: #endif
Again, I don't like relying on OS_POSIX == !OS_WIN

[benwells, 2012-07-11 00:02:03]

https://chromiumcodereview.appspot.com/10700136/diff/1/base/base_paths_posix.cc
File base/base_paths_posix.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/base/base_paths_posix....
base/base_paths_posix.cc:96: *result = FilePath(file_util::GetHomeDir());
Can this just be:
*result = file_util::GetHomeDir();

[Mihai Parparita -not on Chrome, 2012-07-11 00:21:10]

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:64:
"api_test/file_system/get_display_path_prettify")) << message_;
Sorry for not catching this in the other review, but for tests like this one
where you're just checking the result of an ExtensionFunction invocation, it's
less overhead* to do it directly in C++.  There's a bunch of helpers in the
extension_function_test_utils namespace, see
http://code.google.com/p/chromium/source/search?q=%3A%3ARunFunction+file%3Atest
for sample uses.

* both in that the tests will run faster, and in that you don't have to create a
new app/extension manifest, background page, etc. for the test.

[thorogood, 2012-07-11 00:30:24]

https://chromiumcodereview.appspot.com/10700136/diff/1/base/base_paths_posix.cc
File base/base_paths_posix.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/base/base_paths_posix....
base/base_paths_posix.cc:96: *result = FilePath(file_util::GetHomeDir());
On 2012/07/11 00:02:03, benwells wrote:
> Can this just be:
> *result = file_util::GetHomeDir();

Done.

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/file_system_api.cc:57: for (size_t i =
0; i < arraysize(g_rewrite_pairs); ++i) {
On 2012/07/10 23:57:49, benwells wrote:
> I don't like relying on the fact that OS_POSIX is !OS_WIN. Is this something
> which is relied on elsewhere?
> 
> E.g. if we support iOS, will this break? Yeah, this code probably won't be in
> that port but it doesn't feel good.

I've guarded the whole for loop by "#if defined(OS_WIN) || defined(OS_POSIX)" I
feel like this is neater than what I had before.

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:56: #endif
On 2012/07/10 23:57:49, benwells wrote:
> Again, I don't like relying on OS_POSIX == !OS_WIN

I've guarded this test by a define again.

[thorogood, 2012-07-11 04:07:45]

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:64:
"api_test/file_system/get_display_path_prettify")) << message_;
On 2012/07/11 00:21:10, Mihai Parparita wrote:
> Sorry for not catching this in the other review, but for tests like this one
> where you're just checking the result of an ExtensionFunction invocation, it's
> less overhead* to do it directly in C++.  There's a bunch of helpers in the
> extension_function_test_utils namespace, see
>
http://code.google.com/p/chromium/source/search?q=%253A%253ARunFunction+file%...
> for sample uses.
> 
> * both in that the tests will run faster, and in that you don't have to create
a
> new app/extension manifest, background page, etc. for the test.

Thanks for the insight! I've had a play with it, but I'm not sure if this is the
right approach here. The getDisplayPath function only makes sense on an
initialized FileEntry, which we only get from calling another API function which
builds it and registers an isolated filesystem, so I feel like it's still neater
to test via a manifest.

Some of the other tests here that don't depend on other API calls for their
results might be better candidates.

[benwells, 2012-07-11 04:51:45]

lgtm

http://codereview.chromium.org/10700136/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

http://codereview.chromium.org/10700136/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/file_system/file_system_api.cc:57: for (size_t i =
0; i < arraysize(g_rewrite_pairs); ++i) {
On 2012/07/11 00:30:24, thorogood wrote:
> On 2012/07/10 23:57:49, benwells wrote:
> > I don't like relying on the fact that OS_POSIX is !OS_WIN. Is this something
> > which is relied on elsewhere?
> > 
> > E.g. if we support iOS, will this break? Yeah, this code probably won't be
in
> > that port but it doesn't feel good.
> 
> I've guarded the whole for loop by "#if defined(OS_WIN) || defined(OS_POSIX)"
I
> feel like this is neater than what I had before.

Yeah agreed, this is nicer.

[Mihai Parparita -not on Chrome, 2012-07-11 05:32:28]

LGTM

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:64:
"api_test/file_system/get_display_path_prettify")) << message_;
On 2012/07/11 04:07:45, thorogood wrote:
> Thanks for the insight! I've had a play with it, but I'm not sure if this is
the
> right approach here. The getDisplayPath function only makes sense on an
> initialized FileEntry, which we only get from calling another API function
which
> builds it and registers an isolated filesystem, so I feel like it's still
neater
> to test via a manifest.
> 
> Some of the other tests here that don't depend on other API calls for their
> results might be better candidates.

Fair enough.

[thorogood, 2012-07-11 23:39:14]

+jar for base review

[thorogood, 2012-07-20 04:03:22]

On 2012/07/11 23:39:14, thorogood wrote:
> +jar for base review

ping :)

[jar (doing other things), 2012-07-20 18:02:54]

With nit below.... LGTM for base

(I didn't read other code very carefully)

https://chromiumcodereview.appspot.com/10700136/diff/6002/base/base_paths_pos...
File base/base_paths_posix.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/6002/base/base_paths_pos...
base/base_paths_posix.cc:95: case base::DIR_HOME:
nit: consistently use curlies around the body of the case, as was done in the
other cases above.

[thorogood, 2012-07-21 07:39:22]

https://chromiumcodereview.appspot.com/10700136/diff/6002/base/base_paths_pos...
File base/base_paths_posix.cc (right):

https://chromiumcodereview.appspot.com/10700136/diff/6002/base/base_paths_pos...
base/base_paths_posix.cc:95: case base::DIR_HOME:
On 2012/07/20 18:02:54, jar wrote:
> nit: consistently use curlies around the body of the case, as was done in the
> other cases above.

Done.

[commit-bot: I haz the power, 2012-07-21 07:40:40]

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

[commit-bot: I haz the power, 2012-07-21 08:40:58]

Try job failure for 10700136-14001 (retry) on mac_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-22 23:46:48]

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

[commit-bot: I haz the power, 2012-07-23 00:30:23]

Try job failure for 10700136-21001 (retry) on mac_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-23 06:06:09]

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

[commit-bot: I haz the power, 2012-07-23 07:12:53]

Try job failure for 10700136-13002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-23 07:16:15]

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

[commit-bot: I haz the power, 2012-07-23 08:22:46]

Change committed as 147833