Add external extensions json source in proper mac location.

Add external extensions json source in proper mac location.

The old path will be deprecated once developers have migrated.

BUG=67203
TEST=FileUtilTest.IsPathControledByAdmin

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

[Sam Kerner (Chrome), 2011-08-24 22:09:13]

Added Antony to review changes to extensions.
Added Evan to review changes to base/

[Evan Martin, 2011-08-24 22:38:53]

base code looks good, just nitpicking

http://codereview.chromium.org/7718021/diff/4001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util.h#newcode662
base/file_util.h:662: #if defined (OS_POSIX)
no space after defined

http://codereview.chromium.org/7718021/diff/4001/base/file_util.h#newcode667
base/file_util.h:667: BASE_EXPORT bool IsPathControlledByAdmin(const FilePath&
path);
this one needs doc comments too

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:947: // parent of |path|.
These comments belong in the header.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:954: 
Is the double-newline here intentional?

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:960: LOG(ERROR) << "Failed to get information on path "
<< path.value();
Use PLOG() for functions like this that use errno

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:980: const FilePath
kFileSystemRoot(FILE_PATH_LITERAL("/"));
Since this is in POSIX code, you can use "/" without FILE_PATH_LITERAL around it

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:113: #if defined (OS_POSIX)
no space after defined

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:114: void ChangePosixFilePermissions(const FilePath&
path,
can you add doc comments?

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:127: ASSERT_EQ(0, chmod(path.value().c_str(),
new_mode_bits));
ASSERT doesn't owrk in helper functions unless you coupel it with extra code. 
see
http://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Using_Assertions_...

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:129: #endif  // defined (OS_POSIX)
no space after defined

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:1841: // using a user id we control.
This comment is good.  I'd rename the test to say it's testing
IsPathControlledByUser

[asargent_no_longer_on_chrome, 2011-08-24 22:55:09]

chrome/* code LGTM

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_extension_loader.cc (right):

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:106:
prefs_->size());
Optional suggestion - do we want to also measure the count of extensions using
the new location? Then we could see the ratio of deprecated to new over time.

[Sam Kerner (Chrome), 2011-08-25 14:04:37]

http://codereview.chromium.org/7718021/diff/4001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util.h#newcode662
base/file_util.h:662: #if defined (OS_POSIX)
On 2011/08/24 22:38:53, Evan Martin wrote:
> no space after defined

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util.h#newcode667
base/file_util.h:667: BASE_EXPORT bool IsPathControlledByAdmin(const FilePath&
path);
On 2011/08/24 22:38:53, Evan Martin wrote:
> this one needs doc comments too

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:947: // parent of |path|.
On 2011/08/24 22:38:53, Evan Martin wrote:
> These comments belong in the header.

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:954: 
On 2011/08/24 22:38:53, Evan Martin wrote:
> Is the double-newline here intentional?

No.  Removed.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:960: LOG(ERROR) << "Failed to get information on path "
<< path.value();
On 2011/08/24 22:38:53, Evan Martin wrote:
> Use PLOG() for functions like this that use errno

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:980: const FilePath
kFileSystemRoot(FILE_PATH_LITERAL("/"));
On 2011/08/24 22:38:53, Evan Martin wrote:
> Since this is in POSIX code, you can use "/" without FILE_PATH_LITERAL around
it

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:113: #if defined (OS_POSIX)
On 2011/08/24 22:38:53, Evan Martin wrote:
> no space after defined

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:114: void ChangePosixFilePermissions(const FilePath&
path,
On 2011/08/24 22:38:53, Evan Martin wrote:
> can you add doc comments?

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:127: ASSERT_EQ(0, chmod(path.value().c_str(),
new_mode_bits));
On 2011/08/24 22:38:53, Evan Martin wrote:
> ASSERT doesn't owrk in helper functions unless you coupel it with extra code. 
> see
>
http://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Using_Assertions_...

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:129: #endif  // defined (OS_POSIX)
On 2011/08/24 22:38:53, Evan Martin wrote:
> no space after defined

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:1841: // using a user id we control.
On 2011/08/24 22:38:53, Evan Martin wrote:
> This comment is good.  I'd rename the test to say it's testing
> IsPathControlledByUser

Done.

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_extension_loader.cc (right):

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:106:
prefs_->size());
On 2011/08/24 22:55:09, Antony Sargent wrote:
> Optional suggestion - do we want to also measure the count of extensions using
> the new location? Then we could see the ratio of deprecated to new over time. 

Good point.  Done.

[TVL, 2011-08-25 14:07:27]

few comments.

+ Mark - Thoughts on the groups questions especially.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:951: if (path == base)
do you need to sanity check that base is the prefix of path?  also, what happens
if you get "" for base?

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:955: if (!IsPathControlledByUser(base, path.DirName(),
owner_uid))
could this be done with a loop to avoid the recursion?  not sure how large of
paths you'll get, but if it's stuff within a users profile, seems like it could
be pretty deep.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:1924: #endif  // defined(OS_POSIX)
i tend to include tests of bad inputs to make sure the code doesn't crash. 
reverse base and path, empty value, no overlapping.  Never know what can go
wrong in the code that calls this to feed it garbage.

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_extension_loader.cc (right):

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:103: // automatic
migration methods we implement.
fyi - if folks were modifying the bundle link this, they were breaking the code
signature on the app.  But nothing on the OS really checks this.

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc#new...
base/file_util_posix.cc:958: if (stat_info.st_uid != owner_uid) {
what happens if any segment is a link?  does that provide a vector to still let
another user control things?

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc#new...
base/file_util_posix.cc:969: return true;
the group permissions could still be a issue, but I'm not completely sure how to
deal with that.  Hardcoding a check for the admin group might be ok here.

[Mark Mentovai, 2011-08-25 14:15:48]

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc#new...
base/file_util_posix.cc:946: if (path == base)
This scheme seems to provide absolutely no way to verify that / doesn’t have a
preposterous mode, like 0777.

[Sam Kerner (Chrome), 2011-08-25 14:29:57]

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/external_pref_extension_loader.cc (right):

http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/external_pref_extension_loader.cc:103: // automatic
migration methods we implement.
On 2011/08/25 14:07:28, TVL wrote:
> fyi - if folks were modifying the bundle link this, they were breaking the
code
> signature on the app.  But nothing on the OS really checks this.

  Mark mentioned that in the bug.  In order to figure out how we should migrate
away from the current setup, I would like to know more about this.  I think
someone mentioned that requests to read from the keychain would have extra
prompts.  Will these prompts happen every time while the file exists, or every
time the file is altered?

[TVL, 2011-08-25 14:32:02]

On 2011/08/25 14:29:57, Sam Kerner (Chrome) wrote:
>
http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/external_pref_extension_loader.cc (right):
> 
>
http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
> chrome/browser/extensions/external_pref_extension_loader.cc:103: // automatic
> migration methods we implement.
> On 2011/08/25 14:07:28, TVL wrote:
> > fyi - if folks were modifying the bundle link this, they were breaking the
> code
> > signature on the app.  But nothing on the OS really checks this.
> 
>   Mark mentioned that in the bug.  In order to figure out how we should
migrate
> away from the current setup, I would like to know more about this.  I think
> someone mentioned that requests to read from the keychain would have extra
> prompts.  Will these prompts happen every time while the file exists, or every
> time the file is altered?

Every time the keychain is read and the signature doesn't match (unless the user
clicks "Always Allow", in which case they probably stop until the binary is
changed again)

[Mark Mentovai, 2011-08-25 14:52:36]

Our binaries update like once a week. The Keychain prompts are
horrible and show up in multiples. “Always allow” only works until the
app is updated again.

The rule is: don’t touch anything inside the app, ever.

We may add our own validation that spews loud ugly messages to the log
if anyone does touch stuff inside the app.

On Thu, Aug 25, 2011 at 10:32 AM,  <thomasvl@chromium.org> wrote:
> On 2011/08/25 14:29:57, Sam Kerner (Chrome) wrote:
>
>
http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
>>
>> File chrome/browser/extensions/external_pref_extension_loader.cc (right):
>
>
>
http://codereview.chromium.org/7718021/diff/4001/chrome/browser/extensions/ex...
>>
>> chrome/browser/extensions/external_pref_extension_loader.cc:103: //
>> automatic
>> migration methods we implement.
>> On 2011/08/25 14:07:28, TVL wrote:
>> > fyi - if folks were modifying the bundle link this, they were breaking
>> > the
>> code
>> > signature on the app.  But nothing on the OS really checks this.
>
>>   Mark mentioned that in the bug.  In order to figure out how we should
>
> migrate
>>
>> away from the current setup, I would like to know more about this.  I
>> think
>> someone mentioned that requests to read from the keychain would have extra
>> prompts.  Will these prompts happen every time while the file exists, or
>> every
>> time the file is altered?
>
> Every time the keychain is read and the signature doesn't match (unless the
> user
> clicks "Always Allow", in which case they probably stop until the binary is
> changed again)
>
>
> http://codereview.chromium.org/7718021/
>

[Sam Kerner (Chrome), 2011-08-26 19:59:15]

Ready for another look.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:951: if (path == base)
On 2011/08/25 14:07:28, TVL wrote:
> do you need to sanity check that base is the prefix of path?  also, what
happens
> if you get "" for base?

Sanity checks added.  Unit test IsPathControlledByUserTest.BadPaths now covers
both cases.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_posix.cc#newc...
base/file_util_posix.cc:955: if (!IsPathControlledByUser(base, path.DirName(),
owner_uid))
On 2011/08/25 14:07:28, TVL wrote:
> could this be done with a loop to avoid the recursion?  not sure how large of
> paths you'll get, but if it's stuff within a users profile, seems like it
could
> be pretty deep.

Done.

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/7718021/diff/4001/base/file_util_unittest.cc#n...
base/file_util_unittest.cc:1924: #endif  // defined(OS_POSIX)
On 2011/08/25 14:07:28, TVL wrote:
> i tend to include tests of bad inputs to make sure the code doesn't crash. 
> reverse base and path, empty value, no overlapping.  Never know what can go
> wrong in the code that calls this to feed it garbage.

Lots of tests added, including all these cases.

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc#new...
base/file_util_posix.cc:946: if (path == base)
On 2011/08/25 14:15:49, Mark Mentovai wrote:
> This scheme seems to provide absolutely no way to verify that / doesn’t have a
> preposterous mode, like 0777.

Changed the test to include the base path, sop that '/' must pass the tests like
any other directory.

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc#new...
base/file_util_posix.cc:958: if (stat_info.st_uid != owner_uid) {
On 2011/08/25 14:07:28, TVL wrote:
> what happens if any segment is a link?  does that provide a vector to still
let
> another user control things?

Good point.  Links are now forbidden.

http://codereview.chromium.org/7718021/diff/11001/base/file_util_posix.cc#new...
base/file_util_posix.cc:969: return true;
On 2011/08/25 14:07:28, TVL wrote:
> the group permissions could still be a issue, but I'm not completely sure how
to
> deal with that.  Hardcoding a check for the admin group might be ok here.

Hardcoded check for the group named "admin".

[TVL, 2011-08-26 20:17:14]

http://codereview.chromium.org/7718021/diff/16001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/16001/base/file_util_posix.cc#new...
base/file_util_posix.cc:1001: ib = base_components.begin();
this is also in the loop setup

http://codereview.chromium.org/7718021/diff/16001/base/file_util_posix.cc#new...
base/file_util_posix.cc:1016: }
FilePath has IsParent()

[Sam Kerner (Chrome), 2011-08-29 14:19:48]

http://codereview.chromium.org/7718021/diff/16001/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/16001/base/file_util_posix.cc#new...
base/file_util_posix.cc:1001: ib = base_components.begin();
On 2011/08/26 20:17:15, TVL wrote:
> this is also in the loop setup

Done.

http://codereview.chromium.org/7718021/diff/16001/base/file_util_posix.cc#new...
base/file_util_posix.cc:1016: }
On 2011/08/26 20:17:15, TVL wrote:
> FilePath has IsParent()

Used it to make an early-out at the top of the function.  I still need the loop
to get the non-common part of the path, burt I made the if()s into CHECK()s.

[Sam Kerner (Chrome), 2011-08-30 14:32:38]

Antony and Evan,
   Both the base and extensions code have changed enough that they may need
another look.  Your call.

Sam

[TVL, 2011-08-30 15:32:40]

lgtm with one small fixup (sorry for not noticing that before)

http://codereview.chromium.org/7718021/diff/21002/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/7718021/diff/21002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:331: FILE_PATH_LITERAL("/Library/Application
Support/Google/Chrome"));
This should probably use
base::mac::GetLocalDirectory(NSApplicationSupportDirectory, ...) (in
foundation_utils) and then append "Google" and "Chrome" to that.

[asargent_no_longer_on_chrome, 2011-08-30 16:51:28]

still LGTM

[Evan Martin, 2011-08-30 17:26:38]

http://codereview.chromium.org/7718021/diff/21002/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/7718021/diff/21002/base/file_util.h#newcode666
base/file_util.h:666: // * Are owned by a specific user and group.
Does it matter that you check the group, when you don't check whether it's
group-modifiable?

http://codereview.chromium.org/7718021/diff/21002/base/file_util.h#newcode679
base/file_util.h:679: // "admin", are not writable by all users, and contain no
symbolic links.
What is "admin" here?  It is not true that my root directory is controlled by
that user.

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:953: bool SpecificPathControlledByUser(const FilePath&
path,
Should be in the anon namespace if it's not part of the file API.

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:971: << " is owned by the wrong user.";
It seems you're reporting it as an error when these conditions aren't met.  That
seems stronger than a function "IsFooBar" is promising.  Like if the function
was named "IsMonitor20InchesWide" it'd be surprising if it starting printing out
errors when the monitor wasn't 20 inches wide.

Maybe a name like VerifyPathControlledByUser() would be more appropriate?

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:982: LOG(ERROR) << "Path "<< path.value()
space before <<

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:1036: base::ThreadRestrictions::AssertIOAllowed();
Thanks for doing this!

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:1038: struct group *groupRecord =
getgrnam(kAdminGroupName);
group_record, not groupRecord

http://codereview.chromium.org/7718021/diff/21002/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/7718021/diff/21002/base/file_util_unittest.cc#...
base/file_util_unittest.cc:1864: CreateTextFile(text_file_, L"This text file has
some text in it.");
Are your tests vulnerable to the umask?  I wonder if it's worth doing some
initial sanitization here to ensure the directories have the permissions you
expect.  (I see below you do that in one of the tests, but I worry that e.g. the
BadPaths test will pass for the wrong reason.)

http://codereview.chromium.org/7718021/diff/21002/base/file_util_unittest.cc#...
base/file_util_unittest.cc:1916:
ASSERT_TRUE(file_util::CreateSymbolicLink(sub_dir_, link_to_sub_dir_))
Heh, the trailing underscore bled into the directory name here.  (I don't think
it matters, but I also don't think it was intentional...)

[Sam Kerner (Chrome), 2011-09-16 18:12:59]

http://codereview.chromium.org/7718021/diff/21002/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/7718021/diff/21002/base/file_util.h#newcode666
base/file_util.h:666: // * Are owned by a specific user and group.
On 2011/08/30 17:26:38, Evan Martin wrote:
> Does it matter that you check the group, when you don't check whether it's
> group-modifiable?

I considered not checking group membership if the group can't write.  I chose
not to do this, because there is no legitimate reason any group other than admin
should own /Library/... paths on mac os, and admin can write to everything under
/Library/.  If some future user needs it we could add it, but loosening the test
makes it more complex, and it will not be exercised outside unit tests.

http://codereview.chromium.org/7718021/diff/21002/base/file_util.h#newcode679
base/file_util.h:679: // "admin", are not writable by all users, and contain no
symbolic links.
On 2011/08/30 17:26:38, Evan Martin wrote:
> What is "admin" here?  It is not true that my root directory is controlled by
> that user.

Updated to make it clear that this function is mac-specific.  Put it in #if
defined(OS_MACOSX).

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc
File base/file_util_posix.cc (right):

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:953: bool SpecificPathControlledByUser(const FilePath&
path,
On 2011/08/30 17:26:38, Evan Martin wrote:
> Should be in the anon namespace if it's not part of the file API.

Done.

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:971: << " is owned by the wrong user.";
On 2011/08/30 17:26:38, Evan Martin wrote:
> It seems you're reporting it as an error when these conditions aren't met. 
That
> seems stronger than a function "IsFooBar" is promising.  Like if the function
> was named "IsMonitor20InchesWide" it'd be surprising if it starting printing
out
> errors when the monitor wasn't 20 inches wide.
> 
> Maybe a name like VerifyPathControlledByUser() would be more appropriate?

Done.

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:982: LOG(ERROR) << "Path "<< path.value()
On 2011/08/30 17:26:38, Evan Martin wrote:
> space before <<

Done.

http://codereview.chromium.org/7718021/diff/21002/base/file_util_posix.cc#new...
base/file_util_posix.cc:1038: struct group *groupRecord =
getgrnam(kAdminGroupName);
On 2011/08/30 17:26:38, Evan Martin wrote:
> group_record, not groupRecord

Done.

http://codereview.chromium.org/7718021/diff/21002/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/7718021/diff/21002/base/file_util_unittest.cc#...
base/file_util_unittest.cc:1864: CreateTextFile(text_file_, L"This text file has
some text in it.");
On 2011/08/30 17:26:38, Evan Martin wrote:
> Are your tests vulnerable to the umask?  I wonder if it's worth doing some
> initial sanitization here to ensure the directories have the permissions you
> expect.  (I see below you do that in one of the tests, but I worry that e.g.
the
> BadPaths test will pass for the wrong reason.)

I explicitly set the permissions bits here.

Each test that has a failing case has another check that a path without the
cause of the failure passes.  The final EXPECT in BadPaths is an example.  This
way, if some issue with the test environment makes all paths fail, the test will
detect that something is wrong.

http://codereview.chromium.org/7718021/diff/21002/base/file_util_unittest.cc#...
base/file_util_unittest.cc:1916:
ASSERT_TRUE(file_util::CreateSymbolicLink(sub_dir_, link_to_sub_dir_))
On 2011/08/30 17:26:38, Evan Martin wrote:
> Heh, the trailing underscore bled into the directory name here.  (I don't
think
> it matters, but I also don't think it was intentional...)

Search and replace was a bit too effective.  Fixed.

http://codereview.chromium.org/7718021/diff/21002/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

http://codereview.chromium.org/7718021/diff/21002/chrome/common/chrome_paths....
chrome/common/chrome_paths.cc:331: FILE_PATH_LITERAL("/Library/Application
Support/Google/Chrome"));
On 2011/08/30 15:32:40, TVL wrote:
> This should probably use
> base::mac::GetLocalDirectory(NSApplicationSupportDirectory, ...) (in
> foundation_utils) and then append "Google" and "Chrome" to that.

Done.

[Evan Martin, 2011-09-21 18:15:24]

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h#newcode52
base/file_util.h:52: 
Probably didn't mean for so much whitespace here and below.

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h#newcode60
base/file_util.h:60: const FilePath* path,
Why is this a pointer?  If it being NULL has a meaning, please document that in
the main comment.  ...but it looks like the .cc file has a different prototype,
so maybe this function isn't used (?)

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h#newcode82
base/file_util.h:82: // Functions that operate purely on a path string w/o
touching the filesystem:
It looks like this file might be organized into categories.  I wonder if you
could find a more appropriate place for your addition than at the top.

[Sam Kerner (Chrome), 2011-09-21 18:57:06]

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h#newcode52
base/file_util.h:52: 
On 2011/09/21 18:15:24, Evan Martin wrote:
> Probably didn't mean for so much whitespace here and below.

Trybots have been failing to patch changes to this file, so I have been playing
with whitespace and position of this code to appease them.  Removed.

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h#newcode60
base/file_util.h:60: const FilePath* path,
On 2011/09/21 18:15:24, Evan Martin wrote:
> Why is this a pointer?  If it being NULL has a meaning, please document that
in
> the main comment.  ...but it looks like the .cc file has a different
prototype,
> so maybe this function isn't used (?)

Fixed.

http://codereview.chromium.org/7718021/diff/40014/base/file_util.h#newcode82
base/file_util.h:82: // Functions that operate purely on a path string w/o
touching the filesystem:
On 2011/09/21 18:15:24, Evan Martin wrote:
> It looks like this file might be organized into categories.  I wonder if you
> could find a more appropriate place for your addition than at the top.

Put them with functions that touch the filesystem.  Put them above classes,
which are all at the end of the file.

[Evan Martin, 2011-09-21 18:57:46]

base LGTM