Use xdg_user_dir_lookup to lookup directories.

Add xdg-user-dirs to chrome/third_party.

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

[Evan Stade, 2009-02-26 01:26:54]

http://codereview.chromium.org/27186/diff/1002/1003
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/1002/1003#newcode13
Line 13: char* xdg_user_dir_lookup(const char *type);
you should check in and include the header

http://codereview.chromium.org/27186/diff/1002/1003#newcode37
Line 37: FilePath GetStandardDirectory(const char* env_name, const char*
fallback_dir) {
probably can change the name of this function to GetXDGDirectory (I don't think
we plan to use it for any non-XDG specified directory)

http://codereview.chromium.org/27186/diff/1002/1003#newcode40
Line 40: const char* const xdg_dirs[] = { "DOWNLOAD",
put each of these on a new line indented 4 spaces and put a comma after the last
element

http://codereview.chromium.org/27186/diff/1002/1003#newcode50
Line 50: for (unsigned int i = 0; i < sizeof(xdg_dirs) / sizeof(xdg_dirs[0]);
i++) {
For the record, this should be:

for (size_t i = 0; i < ARRAYSIZE_UNSAGE(xdg_dirs); ++i)

but, I would get rid of it altogether, and simply have the callers use
GetXDGUserDirectory directly.

[Lei Zhang, 2009-02-26 01:57:52]

http://codereview.chromium.org/27186/diff/1002/1003
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/1002/1003#newcode13
Line 13: char* xdg_user_dir_lookup(const char *type);
On 2009/02/26 01:26:54, estade wrote:
> you should check in and include the header

Done.

http://codereview.chromium.org/27186/diff/1002/1003#newcode37
Line 37: FilePath GetStandardDirectory(const char* env_name, const char*
fallback_dir) {
On 2009/02/26 01:26:54, estade wrote:
> probably can change the name of this function to GetXDGDirectory (I don't
think
> we plan to use it for any non-XDG specified directory)

Done.

http://codereview.chromium.org/27186/diff/1002/1003#newcode50
Line 50: for (unsigned int i = 0; i < sizeof(xdg_dirs) / sizeof(xdg_dirs[0]);
i++) {
On 2009/02/26 01:26:54, estade wrote:
> For the record, this should be:
> 
> for (size_t i = 0; i < ARRAYSIZE_UNSAGE(xdg_dirs); ++i)
> 
> but, I would get rid of it altogether, and simply have the callers use
> GetXDGUserDirectory directly.
> 
> 

Removed.

[Evan Martin, 2009-02-26 02:04:50]

http://codereview.chromium.org/27186/diff/12/1009
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/12/1009#newcode20
Line 20: return FilePath("/tmp/");
Why not g_get_home_dir()?  That looks up in /etc/passwd or whatever if HOME is
unset.

http://codereview.chromium.org/27186/diff/12/1009#newcode27
Line 27: char* xdg_dir = xdg_user_dir_lookup(env_name);
Should we cache the output of this?  I hate to think of it grunging around in
the file system every time someone asks for a directory at run time.

[Evan Stade, 2009-02-26 02:05:30]

http://codereview.chromium.org/27186/diff/12/1009
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/12/1009#newcode20
Line 20: return FilePath("/tmp/");
PathService::Get(base::DIR_TEMP, &rv)

http://codereview.chromium.org/27186/diff/12/1009#newcode26
Line 26: FilePath rv;
declare this inside the if block

http://codereview.chromium.org/27186/diff/12/1009#newcode37
Line 37: FilePath rv;
unused variable

http://codereview.chromium.org/27186/diff/12/1009#newcode38
Line 38: const char* env_value;
condense this line with the next 2 (the empty line and the assignment)

[Lei Zhang, 2009-02-26 02:19:44]

http://codereview.chromium.org/27186/diff/12/1009
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/12/1009#newcode20
Line 20: return FilePath("/tmp/");
On 2009/02/26 02:04:50, Evan Martin wrote:
> Why not g_get_home_dir()?  That looks up in /etc/passwd or whatever if HOME is
> unset.

Interesting, I can add it back in if we don't mind depending on glib. I suppose
we can always ifdef it out should someone ever get a qt port going.

http://codereview.chromium.org/27186/diff/12/1009#newcode20
Line 20: return FilePath("/tmp/");
On 2009/02/26 02:05:30, estade wrote:
> PathService::Get(base::DIR_TEMP, &rv)

Ok. I'll keep /tmp as a last resort.

http://codereview.chromium.org/27186/diff/12/1009#newcode26
Line 26: FilePath rv;
On 2009/02/26 02:05:30, estade wrote:
> declare this inside the if block

Ok.

http://codereview.chromium.org/27186/diff/12/1009#newcode27
Line 27: char* xdg_dir = xdg_user_dir_lookup(env_name);
On 2009/02/26 02:04:50, Evan Martin wrote:
> Should we cache the output of this?  I hate to think of it grunging around in
> the file system every time someone asks for a directory at run time.

Can we use PathService for this or do we need to roll our own caching mechanism?

http://codereview.chromium.org/27186/diff/12/1009#newcode37
Line 37: FilePath rv;
On 2009/02/26 02:05:30, estade wrote:
> unused variable

Done and done.

[Evan Stade, 2009-02-26 02:23:23]

http://codereview.chromium.org/27186/diff/21/22
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/21/22#newcode49
Line 49: FilePath GetXDGDirectory(const char* env_name, const char*
fallback_dir) {
somehow you lost the comment that explained how this function works.

[Evan Martin, 2009-02-26 02:33:26]

http://codereview.chromium.org/27186/diff/12/1009
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/12/1009#newcode27
Line 27: char* xdg_dir = xdg_user_dir_lookup(env_name);
On 2009/02/26 02:19:44, Lei Zhang wrote:
> On 2009/02/26 02:04:50, Evan Martin wrote:
> > Should we cache the output of this?  I hate to think of it grunging around
in
> > the file system every time someone asks for a directory at run time.
> 
> Can we use PathService for this or do we need to roll our own caching
mechanism?

That's a good point.  PathService already caches.

[Lei Zhang, 2009-02-26 02:47:51]

On 2009/02/26 02:33:26, Evan Martin wrote:
> http://codereview.chromium.org/27186/diff/12/1009
> File chrome/common/chrome_paths_linux.cc (right):
> 
> http://codereview.chromium.org/27186/diff/12/1009#newcode27
> Line 27: char* xdg_dir = xdg_user_dir_lookup(env_name);
> On 2009/02/26 02:19:44, Lei Zhang wrote:
> > On 2009/02/26 02:04:50, Evan Martin wrote:
> > > Should we cache the output of this?  I hate to think of it grunging around
> in
> > > the file system every time someone asks for a directory at run time.
> > 
> > Can we use PathService for this or do we need to roll our own caching
> mechanism?
> 
> That's a good point.  PathService already caches.

Yep, chrome_paths.cc caches the results we return. Please review patch set 6.

[Evan Stade, 2009-02-26 18:47:02]

LG with a couple nits

http://codereview.chromium.org/27186/diff/1037/34
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/1037/34#newcode9
Line 9: #include <string.h>
do you still need these headers?

http://codereview.chromium.org/27186/diff/1037/34#newcode41
Line 41: FilePath rv = FilePath(xdg_dir);
typically our style is 

FilePath rv(xdg_dir);

but I'm not sure how important that is to follow.

http://codereview.chromium.org/27186/diff/1037/35
File chrome/common/common.scons (right):

http://codereview.chromium.org/27186/diff/1037/35#newcode287
Line 287: '$CHROME_DIR/third_party/xdg_user_dirs/xdg_user_dir_lookup.cc',
this looks funny to me... but, I know nothing of scons.

[Lei Zhang, 2009-02-26 19:09:40]

http://codereview.chromium.org/27186/diff/1037/34
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/27186/diff/1037/34#newcode9
Line 9: #include <string.h>
On 2009/02/26 18:47:03, estade wrote:
> do you still need these headers?

Still need stdlib for getenv(), removed string.h.

http://codereview.chromium.org/27186/diff/1037/34#newcode41
Line 41: FilePath rv = FilePath(xdg_dir);
On 2009/02/26 18:47:03, estade wrote:
> typically our style is 
> 
> FilePath rv(xdg_dir);
> 
> but I'm not sure how important that is to follow.

Not used to that, but ok. Also changed FilePath config_dir while I'm at it.

http://codereview.chromium.org/27186/diff/1037/35
File chrome/common/common.scons (right):

http://codereview.chromium.org/27186/diff/1037/35#newcode287
Line 287: '$CHROME_DIR/third_party/xdg_user_dirs/xdg_user_dir_lookup.cc',
On 2009/02/26 18:47:03, estade wrote:
> this looks funny to me... but, I know nothing of scons.

This is ok. I doubt we need a separate scons file for xdg_user_dirs, which we
won't use anywhere else.