Tweak PathProviderPosix's FILE_EXE and FILE_MODULE handling on FreeBSD.

Tweak PathProviderPosix's FILE_EXE and FILE_MODULE handling on FreeBSD.

BUG=none
TEST=none

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

[grt (UTC plus 2), 2011-10-20 19:50:14]

Reviewers:

jar: base/base_paths_linux.cc
ananta: chrome/default_plugin/*, chrome_frame/*
tony: webkit/glue/webkitplatformsupport_impl.cc

Thanks.

[tony, 2011-10-20 20:24:53]

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
File chrome/default_plugin/plugin_impl_gtk.cc (right):

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
chrome/default_plugin/plugin_impl_gtk.cc:43: if (mime_type == NULL || *mime_type
== L'\0') {
Why L?

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
File chrome/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
chrome/default_plugin/plugin_impl_win.cc:75: if (mime_type == NULL || *mime_type
== '0') {
Missing \

[grt (UTC plus 2), 2011-10-20 20:52:31]

Thanks for the good eyes.  PTAL.

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
File chrome/default_plugin/plugin_impl_gtk.cc (right):

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
chrome/default_plugin/plugin_impl_gtk.cc:43: if (mime_type == NULL || *mime_type
== L'\0') {
On 2011/10/20 20:24:54, tony wrote:
> Why L?

Because I goofed.

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
File chrome/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/8361011/diff/1/chrome/default_plugin/plugin_im...
chrome/default_plugin/plugin_impl_win.cc:75: if (mime_type == NULL || *mime_type
== '0') {
On 2011/10/20 20:24:54, tony wrote:
> Missing \

My mission for today is to prove the value of code reviews.  Mission complete.

[tony, 2011-10-20 20:56:05]

LGTM

[wtc, 2011-10-20 21:41:32]

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc
File base/base_paths_linux.cc (right):

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc#new...
base/base_paths_linux.cc:54: if (error < 0 || length == 0 || *bin_dir == '\0') {

The *bin_dir == '\0' test may not be necessary.  It's not
clear if systcl() null-terminates the bin_dir buffer.  See
also my comment below.

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc#new...
base/base_paths_linux.cc:58: bin_dir[strlen(bin_dir)] = 0;

This statement needs work.

If bin_dir is null-terminated, this statement is a no-op.

If bin_dir is not null-terminated, we can't call strlen(bin_dir).
We should do this instead:
      bin_dir[length] = '\0';

Without knowing how sysctl() works on FreeBSD, I don't know
how to fix this.

[jar (doing other things), 2011-10-20 22:00:37]

This is sad, and more than amazing.


LGTM

[wtc, 2011-10-20 22:15:34]

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc
File base/base_paths_linux.cc (right):

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc#new...
base/base_paths_linux.cc:58: bin_dir[strlen(bin_dir)] = 0;

The FreeBSD sysctl(3) man page does not say whether it
returns a string null-terminated:
http://fuse4bsd.creo.hu/localcgi/man-cgi.cgi?sysctl+3

That means sysctl very likely null-terminates the string,
otherwise it needs to be documented in the man page.

Another evidence of null termination is in the gdb source
code:
http://opensource.apple.com/source/gdb/gdb-954/src/gdb/fbsd-nat.c

So let's just remove this statement.

[grt (UTC plus 2), 2011-10-21 13:51:33]

wtc: PTAL.

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc
File base/base_paths_linux.cc (right):

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc#new...
base/base_paths_linux.cc:54: if (error < 0 || length == 0 || *bin_dir == '\0') {
On 2011/10/20 21:41:32, wtc wrote:
> 
> The *bin_dir == '\0' test may not be necessary.  It's not
> clear if systcl() null-terminates the bin_dir buffer.  See
> also my comment below.

Since sysctl includes the string terminator in the length (see my comment
below), *bin_dir == '\0' is equivalent to length == 1.  So this is testing:
1. Did sysctl return an error?
2. Did sysctl return no data?
3. Did sysctl return an empty string?

This is equivalent to: if (error < 0 || length <= 1).

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc#new...
base/base_paths_linux.cc:58: bin_dir[strlen(bin_dir)] = 0;
On 2011/10/20 22:15:34, wtc wrote:
> 
> The FreeBSD sysctl(3) man page does not say whether it
> returns a string null-terminated:
> http://fuse4bsd.creo.hu/localcgi/man-cgi.cgi?sysctl+3
> 
> That means sysctl very likely null-terminates the string,
> otherwise it needs to be documented in the man page.
> 
> Another evidence of null termination is in the gdb source
> code:
> http://opensource.apple.com/source/gdb/gdb-954/src/gdb/fbsd-nat.c
> 
> So let's just remove this statement.

I checked the FreeBSD source: the string is terminated and the returned length
includes the terminator (this, at least, is consistent with the man page), so
removing this is safe.  Done.

http://codereview.chromium.org/8361011/diff/5001/base/base_paths_linux.cc#new...
base/base_paths_linux.cc:59: *result = FilePath(bin_dir);
Since |length| is the length of bin_dir plus terminator, we can create the
std::string given to FilePath without yet another call to strlen; see my change.

[jar (doing other things), 2011-10-21 16:55:07]

Quite amazingly (to me), Will Chan verified that gcc optimizes:

(strlen(foo) == 0)

into:

(!*foo)

I was amazed, because there are potential changes in "side effects" to such an
optimization (the first can crash, by visiting invalid memory).  I can
understand the justification that strlen() is not traditionally used for
crashing :-), so the optimization seems to help support the user's intent :-).

I would agree with wtc, that the comparison of strlen with 0 is easier to read
(and is as nice as having an empty() function).  While the alternative you
provide is generally faster (and I'd be embarrassed to write the strlen
variant), but the faster variant certainly makes me pause to figure out what it
means.  

If (much to my surprise) other compilers also optimize this, then I'll be forced
to recant and believe that the stupid use of strlen is actually better (same
speed, more readable).

Perhaps the nicest all around solution would have been to change:

strlen(foo) == 0

into something like:

is_empty(foo)

We could then have the speed you a a million C programmers want, and yet have
better readability.  You could probably add such a function to our strings.h
header file IMO.

[grt (UTC plus 2), 2011-10-21 18:17:44]

On 2011/10/21 16:55:07, jar wrote:
> Quite amazingly (to me), Will Chan verified that gcc optimizes:
> 
> (strlen(foo) == 0)
> 
> into:
> 
> (!*foo)
> 
> I was amazed, because there are potential changes in "side effects" to such an
> optimization (the first can crash, by visiting invalid memory).  I can
> understand the justification that strlen() is not traditionally used for
> crashing :-), so the optimization seems to help support the user's intent :-).
> 
> I would agree with wtc, that the comparison of strlen with 0 is easier to read
> (and is as nice as having an empty() function).  While the alternative you
> provide is generally faster (and I'd be embarrassed to write the strlen
> variant), but the faster variant certainly makes me pause to figure out what
it
> means.  
> 
> If (much to my surprise) other compilers also optimize this, then I'll be
forced
> to recant and believe that the stupid use of strlen is actually better (same
> speed, more readable).
> 
> Perhaps the nicest all around solution would have been to change:
> 
> strlen(foo) == 0
> 
> into something like:
> 
> is_empty(foo)
> 
> We could then have the speed you a a million C programmers want, and yet have
> better readability.  You could probably add such a function to our strings.h
> header file IMO.

The strlen thing bugs me less knowing that it's optimized away.  I generally
don't like to rely on the compiler for things like that, but maybe that's just
me.  I think is_empty() would be hard to discover, so it's likely to not be used
beyond this commit unless I also put in a presubmit check (which is probably
also called for for the *foo = '\0' thing).  That doesn't seem to be the best
use of my time, so I'll just back off on this change unless someone on the
review list chimes up.

Thanks,

Greg

[wtc, 2011-10-21 19:18:19]

Patch Set 3 LGTM.

grt: thanks for checking the FreeBSD source code.  Didn't
mean to make you do that :-)

http://codereview.chromium.org/8361011/diff/6003/webkit/glue/webkitplatformsu...
File webkit/glue/webkitplatformsupport_impl.cc (left):

http://codereview.chromium.org/8361011/diff/6003/webkit/glue/webkitplatformsu...
webkit/glue/webkitplatformsupport_impl.cc:427: if (!strlen(name))

Some strlen implementations may "helpfully" allow strlen(NULL)
to return 0.  I hope we don't pass a null |name| to
WebKitPlatformSupportImpl::loadResource.

[ananta, 2011-10-22 17:46:17]

Default plugin changes LGTM

[grt (UTC plus 2), 2011-10-24 13:29:07]

I've reverted the bulk changes of strlen(foo) == 0 -> *foo == '\0' since
compilers seem to do that for us.  What remains is what I believe to be better
code for FreeBSD's use of sysctl to get the path to the binary (in path provider
code for FILE_EXE and FILE_MODULE), and a NULL check in WebKit glue.  PTAL:

wtc: general code
tony: webkit/glue OWNERS
jar: base OWNERS

[jar (doing other things), 2011-10-24 16:11:05]

LGTM

[tony, 2011-10-24 17:28:01]

LGTM

[wtc, 2011-10-25 00:07:56]

Patch Set 4 LGTM.

Did anyone object to the strlen(foo) == 0 -> *foo == '\0' changes?

http://codereview.chromium.org/8361011/diff/12001/webkit/glue/webkitplatforms...
File webkit/glue/webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/8361011/diff/12001/webkit/glue/webkitplatforms...
webkit/glue/webkitplatformsupport_impl.cc:427: if (name == NULL ||
!strlen(name))

I would exclude this change.  It may be wrong to pass a null
|name| argument to this function, and it may be better to crash
in that case.

[grt (UTC plus 2), 2011-10-25 13:21:50]

On 2011/10/25 00:07:56, wtc wrote:
> Patch Set 4 LGTM.
> 
> Did anyone object to the strlen(foo) == 0 -> *foo == '\0' changes?

@jar felt that, because our compilers optimize away the strlen, "the stupid use
of strlen is actually better (same speed, more readable)."  I don't object
strongly enough to push back, so I reverted that part of the change.

I'm going to land what's left of this CL now since it's received enough LGTMs.

Thanks for the reviews, gentlemen.

http://codereview.chromium.org/8361011/diff/12001/webkit/glue/webkitplatforms...
File webkit/glue/webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/8361011/diff/12001/webkit/glue/webkitplatforms...
webkit/glue/webkitplatformsupport_impl.cc:427: if (name == NULL ||
!strlen(name))
On 2011/10/25 00:07:56, wtc wrote:
> 
> I would exclude this change.  It may be wrong to pass a null
> |name| argument to this function, and it may be better to crash
> in that case.

Done.

[commit-bot: I haz the power, 2011-10-25 13:22:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/8361011/12004

[commit-bot: I haz the power, 2011-10-25 14:36:58]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2011-10-25 15:09:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/8361011/12004

[commit-bot: I haz the power, 2011-10-25 16:22:28]

Change committed as 107134

[wtc, 2011-10-25 17:42:30]

http://codereview.chromium.org/8361011/diff/12001/webkit/glue/webkitplatforms...
File webkit/glue/webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/8361011/diff/12001/webkit/glue/webkitplatforms...
webkit/glue/webkitplatformsupport_impl.cc:427: if (name == NULL ||
!strlen(name))

Just FYI: my quick experiments showed that strlen(NULL) crashes
correctly on the three major platforms (Linux, Mac, Windows).