[Mac] if someone overrides FILE_EXE/DIR_EXE in the path service, it can cause...

[Mac] if someone overrides FILE_EXE/DIR_EXE in the path service, it can cause the calculation for DIR_SOURCE_ROOT to go wrong.  Avoid this by not using the value from the path service.

BUG=52918
TEST=green tree

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

[Mark Mentovai, 2010-08-24 20:03:04]

http://codereview.chromium.org/3176036/diff/1/2
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/3176036/diff/1/2#newcode19
base/base_paths_mac.mm:19: std::string GetNSExecutablePath(void) {
What’s (void) about? This isn’t C!

http://codereview.chromium.org/3176036/diff/1/2#newcode20
base/base_paths_mac.mm:20: std::string executable_path;
Don’t declare this until used. Put it right before the WriteInto call.

http://codereview.chromium.org/3176036/diff/1/2#newcode26
base/base_paths_mac.mm:26: DCHECK_GT(executable_length, 1u);
Technically, this should be DCHECK_GE(…, 1u) or DCHECK_GT(…, 0u).

http://codereview.chromium.org/3176036/diff/1/2#newcode27
base/base_paths_mac.mm:27: char* executable_cstr = WriteInto(&executable_path,
executable_length);
Suggest calling it |executable_path_c| so that it’s clear that it’s based on
executable_path and not something different.

http://codereview.chromium.org/3176036/diff/1/2#newcode40
base/base_paths_mac.mm:40: std::string cur;
cur is only used in the FILE_EXE/FILE_MODULE case. Just make that case set
result itself. Every other case sets result itself and returns true (or some
other value).

http://codereview.chromium.org/3176036/diff/1/2#newcode44
base/base_paths_mac.mm:44: cur = GetNSExecutablePath();
If GetNSExecutablePath() returns empty, this function should return false.

http://codereview.chromium.org/3176036/diff/1/2#newcode53
base/base_paths_mac.mm:53: *result = FilePath(GetNSExecutablePath()).DirName();
If GetNSExecutablePath() returns empty, this function should return false.

http://codereview.chromium.org/3176036/diff/1/2#newcode70
base/base_paths_mac.mm:70: *result = FilePath(cur);
When getting rid of cur, you can get rid fo these two lines.

[Paweł Hajdan Jr., 2010-08-24 20:12:19]

After fixing those and Mark's comments, LGTM. Thanks for working on this.

http://codereview.chromium.org/3176036/diff/1/2
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/3176036/diff/1/2#newcode29
base/base_paths_mac.mm:29: DCHECK_EQ(rv, 0);
If we're checking a return value, we should pass it to the caller. I'd prefer
the function signature to be bool GetNSExecutablePath(FilePath* path)
WARN_UNUSED_RESULT. WARN_UNUSED_RESULT is defined in base/compiler_specific.h.

http://codereview.chromium.org/3176036/diff/1/2#newcode35
base/base_paths_mac.mm:35: } // namespace
nit: Two spaces between } and // namespace.

[TVL, 2010-08-24 20:24:57]

http://codereview.chromium.org/3176036/diff/1/2
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/3176036/diff/1/2#newcode19
base/base_paths_mac.mm:19: std::string GetNSExecutablePath(void) {
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> What’s (void) about? This isn’t C!

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode20
base/base_paths_mac.mm:20: std::string executable_path;
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> Don’t declare this until used. Put it right before the WriteInto call.

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode26
base/base_paths_mac.mm:26: DCHECK_GT(executable_length, 1u);
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> Technically, this should be DCHECK_GE(…, 1u) or DCHECK_GT(…, 0u).

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode27
base/base_paths_mac.mm:27: char* executable_cstr = WriteInto(&executable_path,
executable_length);
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> Suggest calling it |executable_path_c| so that it’s clear that it’s based on
> executable_path and not something different.

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode29
base/base_paths_mac.mm:29: DCHECK_EQ(rv, 0);
On 2010/08/24 20:12:20, Paweł Hajdan Jr. wrote:
> If we're checking a return value, we should pass it to the caller. I'd prefer
> the function signature to be bool GetNSExecutablePath(FilePath* path)
> WARN_UNUSED_RESULT. WARN_UNUSED_RESULT is defined in base/compiler_specific.h.

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode35
base/base_paths_mac.mm:35: } // namespace
On 2010/08/24 20:12:20, Paweł Hajdan Jr. wrote:
> nit: Two spaces between } and // namespace.

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode40
base/base_paths_mac.mm:40: std::string cur;
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> cur is only used in the FILE_EXE/FILE_MODULE case. Just make that case set
> result itself. Every other case sets result itself and returns true (or some
> other value).

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode44
base/base_paths_mac.mm:44: cur = GetNSExecutablePath();
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> If GetNSExecutablePath() returns empty, this function should return false.

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode53
base/base_paths_mac.mm:53: *result = FilePath(GetNSExecutablePath()).DirName();
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> If GetNSExecutablePath() returns empty, this function should return false.

Done.

http://codereview.chromium.org/3176036/diff/1/2#newcode70
base/base_paths_mac.mm:70: *result = FilePath(cur);
On 2010/08/24 20:03:05, Mark Mentovai wrote:
> When getting rid of cur, you can get rid fo these two lines.

Done.

[Mark Mentovai, 2010-08-24 20:32:19]

LGTM

http://codereview.chromium.org/3176036/diff/8001/9001
File base/base_paths_mac.mm (right):

http://codereview.chromium.org/3176036/diff/8001/9001#newcode25
base/base_paths_mac.mm:25: uint32_t executable_length = 0;
DCHECK(path);

http://codereview.chromium.org/3176036/diff/8001/9001#newcode60
base/base_paths_mac.mm:60: //
src/xcodebuild/{Debug|Release}/Chromium.app/Contents/MacOS/Chromium.
Make this 80 by getting rid of the period.