Issue 8390002: implement GetProcessExecutablePath() on OpenBSD and start using it

Issue 8390002: implement GetProcessExecutablePath() on OpenBSD and start using it (Closed)

Created:
9 years, 2 months ago by Robert Nagy

Modified:
9 years, 2 months ago

Reviewers:
Mark Mentovai, commit-bot: I haz the power

CC:
chromium-reviews, brettw-cc_chromium.org

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

Description

implement GetProcessExecutablePath() on OpenBSD and start using it also add a way to override the hardcoded path in PathProviderPosix() to ease development BUG= TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107149

Patch Set 1 #

Total comments: 14

Patch Set 2 : fixes for prev. patchset #

Total comments: 2

Patch Set 3 : return the variable instead #

Created: 9 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+24 lines, -3 lines)			Patch
M	base/base_paths_linux.cc	View	1	1 chunk	+5 lines, -1 line	0 comments	Download
M	base/process_util.h	View		1 chunk	+3 lines, -1 line	0 comments	Download
M	base/process_util_openbsd.cc	View	1 2	1 chunk	+16 lines, -1 line	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Mark Mentovai

http://codereview.chromium.org/8390002/diff/1/base/base_paths_linux.cc File base/base_paths_linux.cc (right): http://codereview.chromium.org/8390002/diff/1/base/base_paths_linux.cc#newcode71 base/base_paths_linux.cc:71: if (getenv("CHROME_PATH")) 1. CHROME_PATH could be slightly more descriptive. ...

9 years, 2 months ago (2011-10-25 14:59:19 UTC) #2

Robert Nagy

9 years, 2 months ago (2011-10-25 15:29:38 UTC) #3

http://codereview.chromium.org/8390002/diff/1/base/base_paths_linux.cc
File base/base_paths_linux.cc (right):

http://codereview.chromium.org/8390002/diff/1/base/base_paths_linux.cc#newcode71
base/base_paths_linux.cc:71: if (getenv("CHROME_PATH"))
On 2011/10/25 14:59:19, Mark Mentovai wrote:
> 1. CHROME_PATH could be slightly more descriptive. CHROME_EXE_PATH?
> 
> 2. Don’t getenv() twice. getenv() once, and use the result as both the
> conditional expression and the argument to the FilePath constructor.

Done.

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (left):

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc#ol...
base/process_util_openbsd.cc:50: return
FilePath(std::string("/usr/local/chrome/chrome"));
On 2011/10/25 14:59:19, Mark Mentovai wrote:
> I thought we discussed not hard-coding this in two different locations.
Exactly, that's why I am removing it from here.

It's being removed here because it was wrong to start with and the
GetProcessExecutablePath() function is not used yet.

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc
File base/process_util_openbsd.cc (right):

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc#ne...
base/process_util_openbsd.cc:55: if (sysctl(mib, nitems(mib), NULL, &len, NULL,
0) == -1)
On 2011/10/25 14:59:19, Mark Mentovai wrote:
> What’s nitems? Chrome uses arraysize.

Done.

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc#ne...
base/process_util_openbsd.cc:58: if (sysctl(mib, nitems(mib), &kp, &len, NULL,
0) < 0)
On 2011/10/25 14:59:19, Mark Mentovai wrote:
> Same.

Done.

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc#ne...
base/process_util_openbsd.cc:62: if (!strcmp(kp.p_comm, "chrome"))
On 2011/10/25 14:59:19, Mark Mentovai wrote:
> I think it’s more common in Chrome code to see strcmp being compared to 0 than
> it is to see !strcmp. For strcmp, “returns 0” is a better cognitive model than
> “boolean not”.

Done.

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc#ne...
base/process_util_openbsd.cc:62: if (!strcmp(kp.p_comm, "chrome"))
On 2011/10/25 14:59:19, Mark Mentovai wrote:
> Is p_comm just the original argv[0] on OpenBSD? If someone launches Chrome by
> typing a pathname, will it still be "chrome", or will it contain the full
> pathname?

p_comm won't have a pathname in it, just the name of the binary, and the only
code that calls this function is actually doing a basename on it so it will not
have a path in the end either way. After this it gets compared to
kBrowserProcessExecutableName which is "chrome".

http://codereview.chromium.org/8390002/diff/1/base/process_util_openbsd.cc#ne...
base/process_util_openbsd.cc:63: return FilePath("chrome");
On 2011/10/25 14:59:19, Mark Mentovai wrote:
> Why doesn’t this use the environment variable or implement itself in terms of
> the path service too?
I don't think it's necessary since it gets compared to a basename'd string which
will yield chrome.

Mark Mentovai

http://codereview.chromium.org/8390002/diff/1005/base/process_util_openbsd.cc File base/process_util_openbsd.cc (right): http://codereview.chromium.org/8390002/diff/1005/base/process_util_openbsd.cc#newcode63 base/process_util_openbsd.cc:63: return FilePath("chrome"); OK, so then just return FilePath(kp.p_comm).

9 years, 2 months ago (2011-10-25 16:07:51 UTC) #4

Robert Nagy

9 years, 2 months ago (2011-10-25 16:22:22 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robert.nagy@gmail.com/8390002/5001

9 years, 2 months ago (2011-10-25 16:27:42 UTC) #7

Change committed as 107149

Expand Messages | Collapse Messages