First cut for a FreeBSD port - much still not working

Added USE_GDK, set when either TOOLKIT_GTK or TOOLKIT_VIEWS is set but
not OS_WIN.

Added USE_X11. Set when OS_LINUX or OS_FREEBSD is set.

Added USE_BASE_DATA_PACK for base::DataPack usage, set for OS_MACOSX,
OS_LINUX or OS_FREEBSD.

Added USE_NSS for ... nss (for crypto). Windows and MacOS use
platform-specific libraries.

All of the above cause slightly odd formulations like:

#if defined(OS_WIN)
...
#elif defined(USE_BASE_DATA_PACK)
...
#endif

Possibly should also define USE_DLL_FOR_DATA, etc? Or something?

Wrapped various references to struct stat64 and stat64() to use struct
stat and stat() for FreeBSD - but a "man stat64" on Linux suggests
that we could do the same thing for at least Linux, too, and perhaps
eliminate the wrapper?

[Amanda Walker, 2009-08-16 20:02:09]

+mark for better gyp review

http://codereview.chromium.org/172032/diff/1/9
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/172032/diff/1/9#newcode12
Line 12: ['OS!="freebsd"', {'sources/': [['exclude', '/linux/']]}],
Will this work?  At first glance, this looks like it's excluding linux subdirs
for everything (!="linux" or !="freebsd")... don't you just want 'OS!="linux"
and OS!="freebsd"' to cover both?

http://codereview.chromium.org/172032/diff/1/9#newcode229
Line 229: 'source_files': []
can this be combined with the linux clause?

[tony, 2009-08-17 17:22:50]

For reference, someone started an open bsd port here (it was never committed):
http://codereview.chromium.org/118469

The main comment I had then was to use OS_BSD in build/build_config.h for now. 
When there are two BSDs, we can start differentiating more finely, but I bet
more code can be shared among BSDs than not.


On 2009/08/16 20:02:09, Amanda Walker wrote:
> +mark for better gyp review
> 
> http://codereview.chromium.org/172032/diff/1/9
> File third_party/ffmpeg/ffmpeg.gyp (right):
> 
> http://codereview.chromium.org/172032/diff/1/9#newcode12
> Line 12: ['OS!="freebsd"', {'sources/': [['exclude', '/linux/']]}],
> Will this work?  At first glance, this looks like it's excluding linux subdirs
> for everything (!="linux" or !="freebsd")... don't you just want 'OS!="linux"
> and OS!="freebsd"' to cover both?
> 
> http://codereview.chromium.org/172032/diff/1/9#newcode229
> Line 229: 'source_files': []
> can this be combined with the linux clause?

[Mark Mentovai, 2009-08-17 20:13:40]

Adding ar all over the place seems conceptually distinct from this change.  Can
you split that out, or explain why you needed it here?

http://codereview.chromium.org/172032/diff/1/4
File app/app.gyp (right):

http://codereview.chromium.org/172032/diff/1/4#newcode234
Line 234: '<(SHARED_INTERMEDIATE_DIR)/app/<(RULE_INPUT_ROOT)_ar.pak',
What's ar and why are we giving it special treatment?

http://codereview.chromium.org/172032/diff/1/11
File base/platform_thread.h (right):

http://codereview.chromium.org/172032/diff/1/11#newcode25
Line 25: #if defined(OS_LINUX) || defined(OS_FREEBSD)
I wonder if cases like this should really fall into a POSIX-but-not-MACOSX
branch.  That really seems right.

http://codereview.chromium.org/172032/diff/1/12
File chrome/chrome.gyp (right):

http://codereview.chromium.org/172032/diff/1/12#newcode2930
Line 2930: ['OS=="linux" or OS=="freebsd"', {
Same comment applies here.  This will get cumbersome, we'll probably want a
better way to pick these.

http://codereview.chromium.org/172032/diff/1/3
File media/media.gyp (right):

http://codereview.chromium.org/172032/diff/1/3#newcode14
Line 14: ['OS!="linux" and OS!="freebsd"', {'sources/': [['exclude',
'/linux/']]}],
Does this imply that "linux" is the wrong name for the directory?  Should we
move it?  If so, either move it in this change, or (probably better) put a TODO
here and do a followup immediately after this lands.

http://codereview.chromium.org/172032/diff/1/9
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/172032/diff/1/9#newcode12
Line 12: ['OS!="freebsd"', {'sources/': [['exclude', '/linux/']]}],
This is not correct.

http://codereview.chromium.org/172032/diff/1/6
File third_party/libevent/libevent.gyp (right):

http://codereview.chromium.org/172032/diff/1/6#newcode44
Line 44: [ 'OS == "freebsd"',   { 'sources': [ 'kqueue.c' ],
Weird spacing before the {.

http://codereview.chromium.org/172032/diff/1/6#newcode45
Line 45: 'include_dirs': [ 'mac' ] } ],
This implies that mac is a bad name for the directory.

http://codereview.chromium.org/172032/diff/1/6#newcode46
Line 46: 
Blank line is not helpful here.

http://codereview.chromium.org/172032/diff/1/8
File third_party/libxml/libxml.gyp (right):

http://codereview.chromium.org/172032/diff/1/8#newcode9
Line 9: ['OS=="freebsd"', {'os_include': 'linux'}],
Probably ought to be combined with the previous condition, and the same thing
about directory naming applies.

http://codereview.chromium.org/172032/diff/1/7
File third_party/libxslt/libxslt.gyp (right):

http://codereview.chromium.org/172032/diff/1/7#newcode9
Line 9: ['OS=="freebsd"', {'os_include': 'linux'}],
...

(I won't list these individually any more.)

[brettw, 2009-08-17 20:32:25]

I'm very scared of this.

We have hundreds of places where OS_LINUX appears in our code. How many of them
do you think will be different for FreeBSD?

I really don't want to see #if defined(OS_LINUX) || defined(OS_FREEBSD)
everywhere.

[Ben Laurie (Chromium), 2009-08-17 20:38:07]

Fresh patch coming soon, but some comments in the meantime...

On Mon, Aug 17, 2009 at 9:13 PM, <mark@chromium.org> wrote:
> Adding ar all over the place seems conceptually distinct from this
> change. =A0Can you split that out, or explain why you needed it here?

ar is needed because the i18n stuff is broken in a way that -j hides.
If you build without -j, then ar is the first i18n target, and so
causes all the rest of them to get built, and thus avoids the
breakage. If you build with -j, the ones that happen to be explicitly
mentioned get built soon enough that all the unmentioned ones are in
place before things fall over.

In other words, ar is a kludge that stands in for the whole list of
i18n targets. Hence the comments about auto-dependencies.

This could indeed be a standalone change, but since it is a kludge
anyway, should it be?

>
>
> http://codereview.chromium.org/172032/diff/1/4
> File app/app.gyp (right):
>
> http://codereview.chromium.org/172032/diff/1/4#newcode234
> Line 234: '<(SHARED_INTERMEDIATE_DIR)/app/<(RULE_INPUT_ROOT)_ar.pak',
> What's ar and why are we giving it special treatment?
>
> http://codereview.chromium.org/172032/diff/1/11
> File base/platform_thread.h (right):
>
> http://codereview.chromium.org/172032/diff/1/11#newcode25
> Line 25: #if defined(OS_LINUX) || defined(OS_FREEBSD)
> I wonder if cases like this should really fall into a
> POSIX-but-not-MACOSX branch. =A0That really seems right.
>
> http://codereview.chromium.org/172032/diff/1/12
> File chrome/chrome.gyp (right):
>
> http://codereview.chromium.org/172032/diff/1/12#newcode2930
> Line 2930: ['OS=3D=3D"linux" or OS=3D=3D"freebsd"', {
> Same comment applies here. =A0This will get cumbersome, we'll probably
> want a better way to pick these.
>
> http://codereview.chromium.org/172032/diff/1/3
> File media/media.gyp (right):
>
> http://codereview.chromium.org/172032/diff/1/3#newcode14
> Line 14: ['OS!=3D"linux" and OS!=3D"freebsd"', {'sources/': [['exclude',
> '/linux/']]}],
> Does this imply that "linux" is the wrong name for the directory?
> Should we move it? =A0If so, either move it in this change, or (probably
> better) put a TODO here and do a followup immediately after this lands.

Agreed, a TODO is needed - this is more a guess that the linux stuff
will probably be close to what is needed than a definitive decision
that it is exactly right.

>
> http://codereview.chromium.org/172032/diff/1/9
> File third_party/ffmpeg/ffmpeg.gyp (right):
>
> http://codereview.chromium.org/172032/diff/1/9#newcode12
> Line 12: ['OS!=3D"freebsd"', {'sources/': [['exclude', '/linux/']]}],
> This is not correct.
>
> http://codereview.chromium.org/172032/diff/1/6
> File third_party/libevent/libevent.gyp (right):
>
> http://codereview.chromium.org/172032/diff/1/6#newcode44
> Line 44: [ 'OS =3D=3D "freebsd"', =A0 { 'sources': [ 'kqueue.c' ],
> Weird spacing before the {.
>
> http://codereview.chromium.org/172032/diff/1/6#newcode45
> Line 45: 'include_dirs': [ 'mac' ] } ],
> This implies that mac is a bad name for the directory.
>
> http://codereview.chromium.org/172032/diff/1/6#newcode46
> Line 46:
> Blank line is not helpful here.
>
> http://codereview.chromium.org/172032/diff/1/8
> File third_party/libxml/libxml.gyp (right):
>
> http://codereview.chromium.org/172032/diff/1/8#newcode9
> Line 9: ['OS=3D=3D"freebsd"', {'os_include': 'linux'}],
> Probably ought to be combined with the previous condition, and the same
> thing about directory naming applies.
>
> http://codereview.chromium.org/172032/diff/1/7
> File third_party/libxslt/libxslt.gyp (right):
>
> http://codereview.chromium.org/172032/diff/1/7#newcode9
> Line 9: ['OS=3D=3D"freebsd"', {'os_include': 'linux'}],
> ...
>
> (I won't list these individually any more.)
>
> http://codereview.chromium.org/172032
>

[brettw, 2009-08-17 20:38:37]

Also, this seems like something significant we should get team consensus on
whether and how to add it rather than just checking in patches.

[Ben Laurie (Chromium), 2009-08-17 20:41:27]

On Mon, Aug 17, 2009 at 9:32 PM, <brettw@chromium.org> wrote:
> I'm very scared of this.
>
> We have hundreds of places where OS_LINUX appears in our code. How many
> of them do you think will be different for FreeBSD?
>
> I really don't want to see #if defined(OS_LINUX) || defined(OS_FREEBSD)
> everywhere.

Likewise. Its hard to assess without finishing the job. Which is going
to take some time. What do you suggest?

My inclination would be to let them accumulate for a while and then
see what the pattern is.


>
> http://codereview.chromium.org/172032
>

[brettw, 2009-08-17 20:51:39]

If we're going to keep a FreeBSD port, I think we should either convert all
OS_LINUX to OS_UNIX which defines both LINUX and BSD, or say BSD also counts as
LINUX and check in those places where we actually want to differentiate FreeBSD
and Linux.

I think there will only be about 2 places where Linux and *BSD will differ. I
personally therefore prefer calling everything LINUX so that the additional
overhead is on the not-supported BSD people to then add their own conditions as
needed.

[Amanda Walker (Google), 2009-08-17 20:56:57]

That's what Ben appeared to be doing, actually.

--Amanda

On Mon, Aug 17, 2009 at 4:51 PM, <brettw@chromium.org> wrote:
> If we're going to keep a FreeBSD port, I think we should either convert
> all OS_LINUX to OS_UNIX which defines both LINUX and BSD, or say BSD
> also counts as LINUX and check in those places where we actually want to
> differentiate FreeBSD and Linux.
>
> I think there will only be about 2 places where Linux and *BSD will
> differ. I personally therefore prefer calling everything LINUX so that
> the additional overhead is on the not-supported BSD people to then add
> their own conditions as needed.
>
> http://codereview.chromium.org/172032
>



-- 
"Portability is generally the result of advance planning rather than trench
warfare involving #ifdef" -- Henry Spencer (1992)

[Ben Goodger (Google), 2009-08-17 21:11:21]

In which case, a CL desc explaining this, or an email to chromium-dev before
mystery patches show up is a good idea.

-Ben

On 2009/08/17 20:56:57, Amanda Walker (Google) wrote:
> That's what Ben appeared to be doing, actually.
> 
> --Amanda
> 
> On Mon, Aug 17, 2009 at 4:51 PM, <mailto:brettw@chromium.org> wrote:
> > If we're going to keep a FreeBSD port, I think we should either convert
> > all OS_LINUX to OS_UNIX which defines both LINUX and BSD, or say BSD
> > also counts as LINUX and check in those places where we actually want to
> > differentiate FreeBSD and Linux.
> >
> > I think there will only be about 2 places where Linux and *BSD will
> > differ. I personally therefore prefer calling everything LINUX so that
> > the additional overhead is on the not-supported BSD people to then add
> > their own conditions as needed.
> >
> > http://codereview.chromium.org/172032
> >
> 
> 
> 
> -- 
> "Portability is generally the result of advance planning rather than trench
> warfare involving #ifdef" -- Henry Spencer (1992)

[Ben Laurie (Chromium), 2009-08-17 21:24:22]

http://codereview.chromium.org/172032/diff/1/12
File chrome/chrome.gyp (right):

http://codereview.chromium.org/172032/diff/1/12#newcode2930
Line 2930: ['OS=="linux" or OS=="freebsd"', {
On 2009/08/17 20:13:40, Mark Mentovai wrote:
> Same comment applies here.  This will get cumbersome, we'll probably want a
> better way to pick these.

For this, we'd need to introduce a new variable. OS_FAMILY, perhaps?

If so, what's the value? "posix_not_macosx"?

If not, what?

http://codereview.chromium.org/172032/diff/1/9
File third_party/ffmpeg/ffmpeg.gyp (right):

http://codereview.chromium.org/172032/diff/1/9#newcode12
Line 12: ['OS!="freebsd"', {'sources/': [['exclude', '/linux/']]}],
On 2009/08/16 20:02:09, Amanda Walker wrote:
> Will this work?  At first glance, this looks like it's excluding linux subdirs
> for everything (!="linux" or !="freebsd")... don't you just want 'OS!="linux"
> and OS!="freebsd"' to cover both?

Done.

http://codereview.chromium.org/172032/diff/1/9#newcode229
Line 229: 'source_files': []
On 2009/08/16 20:02:09, Amanda Walker wrote:
> can this be combined with the linux clause?

Done.

http://codereview.chromium.org/172032/diff/1/6
File third_party/libevent/libevent.gyp (right):

http://codereview.chromium.org/172032/diff/1/6#newcode44
Line 44: [ 'OS == "freebsd"',   { 'sources': [ 'kqueue.c' ],
On 2009/08/17 20:13:40, Mark Mentovai wrote:
> Weird spacing before the {.

Done.

http://codereview.chromium.org/172032/diff/1/6#newcode45
Line 45: 'include_dirs': [ 'mac' ] } ],
On 2009/08/17 20:13:40, Mark Mentovai wrote:
> This implies that mac is a bad name for the directory.

In this case, I think this is clear. So, what should it be called? kqueue,
perhaps?

http://codereview.chromium.org/172032/diff/1/6#newcode46
Line 46: 
On 2009/08/17 20:13:40, Mark Mentovai wrote:
> Blank line is not helpful here.

Done.

http://codereview.chromium.org/172032/diff/1/8
File third_party/libxml/libxml.gyp (right):

http://codereview.chromium.org/172032/diff/1/8#newcode9
Line 9: ['OS=="freebsd"', {'os_include': 'linux'}],
On 2009/08/17 20:13:40, Mark Mentovai wrote:
> Probably ought to be combined with the previous condition, and the same thing
> about directory naming applies.

Done.

http://codereview.chromium.org/172032/diff/1/7
File third_party/libxslt/libxslt.gyp (right):

http://codereview.chromium.org/172032/diff/1/7#newcode9
Line 9: ['OS=="freebsd"', {'os_include': 'linux'}],
On 2009/08/17 20:13:40, Mark Mentovai wrote:
> ...
> 
> (I won't list these individually any more.)

Done.

[Ben Laurie (Chromium), 2009-08-19 14:13:17]

Updated!

http://codereview.chromium.org/172032

[Amanda Walker, 2009-08-19 15:44:32]

gyp changes look much better.  For compile flags, I'd rather have a pattern of
"test for OS_MACOSX and then OS_POSIX" than the OS_POSIX_NOT_MACOSX flag.

http://codereview.chromium.org/172032/diff/35/45
File base/platform_thread.h (right):

http://codereview.chromium.org/172032/diff/35/45#newcode25
Line 25: #if defined(OS_POSIX_NOT_MACOSX)
I'd rather remove this and reorder the #ifs to be OS_WIN/OS_MACOSX/OS_POSIX
unless we expect to have widespread need for OS_POSIX_NOT_MACOSX.  Too many
nested #ifs and flags that encode combinations don't make the code any more
readable.

http://codereview.chromium.org/172032/diff/35/44
File build/build_config.h (right):

http://codereview.chromium.org/172032/diff/35/44#newcode43
Line 43: #define OS_POSIX_NOT_MACOSX 1
Encoding booleans into #defines doesn't really save anything.  See my related
comment for platform_thread.h.

http://codereview.chromium.org/172032/diff/35/36
File media/base/media_posix.cc (right):

http://codereview.chromium.org/172032/diff/35/36#newcode24
Line 24: #elif defined(OS_POSIX_NOT_MACOSX)
This can be just OS_POSIX

[brettw, 2009-08-19 17:00:11]

Have you addressed me & Ben G.'s concerns? (Post to chromium-dev for how we want
to do this and write a good CL description.)

[Ben Laurie (Chromium), 2009-08-19 18:28:42]

Updated.

Post to -dev coming shortly.

CL description once people are happy with state of ifdefs.

[wtc, 2009-08-27 17:25:54]

I reviewed the non-GUI changes to base.

http://codereview.chromium.org/172032/diff/3383/2309
File base/base_paths.h (right):

http://codereview.chromium.org/172032/diff/3383/2309#newcode17
Line 17: #include "base/base_paths_linux.h"
Perhaps this file should be reamed base_paths_posix.h and
you can use #elif defined(OS_POSIX)?  Would need to be
careful about testing defined(OS_MACOSX) first.

http://codereview.chromium.org/172032/diff/3383/2311
File base/file_util_posix.cc (right):

http://codereview.chromium.org/172032/diff/3383/2311#newcode91
Line 91: typedef struct stat64 stat_wrapper;
Typo: stat_wrapper => stat_wrapper_t

I suspect that you'll also need to wrap the off64_t type.
But perhaps because we use int64, e.g.,

  struct FileInfo {
    // The size of the file in bytes.  Undefined when is_directory is true.
    int64 size;
    ...

this need hasn't arisen.

http://codereview.chromium.org/172032/diff/3383/2279
File base/platform_thread.h (right):

http://codereview.chromium.org/172032/diff/3383/2279#newcode28
Line 28: #elif defined(OS_POSIX)
We already tested defined(OS_POSIX) on line 22.

I think the OS_LINUX and OS_MACOSX cases in the original
code should be deleted.  It's dead code (suppressed by
the defined(OS_POSIX) test on line 22).

http://codereview.chromium.org/172032/diff/3383/2310
File base/process_util.h (right):

http://codereview.chromium.org/172032/diff/3383/2310#newcode16
Line 16: #elif defined(OS_LINUX) || defined(OS_FREEBSD)
May want to consider
  #elif defined(OS_POSIX) && !defined(OS_MACOSX)

This would require a corresponding change at lines 276-281
below.  Would need to test defined(OS_MACOSX) before
defined(OS_POSIX).

http://codereview.chromium.org/172032/diff/3383/2308
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/172032/diff/3383/2308#newcode72
Line 72: // FIXME(benl): I have no idea how to get this
See if this code works:

http://www.freebsd.org/cgi/cvsweb.cgi/ports/devel/nspr/files/patch-.._pr_src_...

http://codereview.chromium.org/172032/diff/3383/2314
File base/third_party/nspr/prcpucfg.h (right):

http://codereview.chromium.org/172032/diff/3383/2314#newcode38
Line 38: #include "base/third_party/nspr/prcpucfg_linux.h"
Please add a "base/third_party/nspr/prcpucfg_freebsd.h"
file for FreeBSD.  You can get it by downloading and
unpacking the NSPR 4.8 source tarball:
https://ftp.mozilla.org/pub/mozilla.org/mozilla.org/nspr/releases/v4.8/src/ns...
and use mozilla/nsprpub/pr/include/md/_freebsd.cfg (need
to rename it prcpucfg_freebsd.h).

http://codereview.chromium.org/172032/diff/3383/2313
File base/time_posix.cc (right):

http://codereview.chromium.org/172032/diff/3383/2313#newcode163
Line 163: || defined(OS_FREEBSD)
Nit: put || at the end of the previous line.  We usually
put operators at end of lines when breaking long expressions.

[sky, 2009-08-27 18:35:15]

http://codereview.chromium.org/172032/diff/2330/2339
File build/build_config.h (right):

http://codereview.chromium.org/172032/diff/2330/2339#newcode19
Line 19: # define OS_MACOSX 1
I don't believe we generally indent defines like this, it certainly makes me
queasy. Please don't do it.

[brettw, 2009-08-27 19:23:59]

I agree with Scott with the spaces. I can see why people do that, but we haven't
done it anywhere so I would like to stay consistent.

USE_GDK is misleading because TOOLKIT_VIEWS on Linux uses both GTK and GDK.

Darin suggested to me a better approach that is the only thing I've heard that
I'm happy with.

These defines should generally match the file extension or directory name that
we would add to the file. This makes it nice in that when you have too many
defines, you know to make a new file, and you can basically keep the naming
consistent.

So I suggest using USE_X11 to mean the intersection of GTK and Views+GTK rather
than adding a new define. What do you think about this?

[Ben Laurie (Chromium), 2009-08-27 19:44:21]

On Thu, Aug 27, 2009 at 6:25 PM, <wtc@chromium.org> wrote:
> I reviewed the non-GUI changes to base.
>
>
> http://codereview.chromium.org/172032/diff/3383/2309
> File base/base_paths.h (right):
>
> http://codereview.chromium.org/172032/diff/3383/2309#newcode17
> Line 17: #include "base/base_paths_linux.h"
> Perhaps this file should be reamed base_paths_posix.h and
> you can use #elif defined(OS_POSIX)? =A0Would need to be
> careful about testing defined(OS_MACOSX) first.

As previously stated, I wanted to avoid this kind of renaming for now,
because it might be reversed when we get more working.

> http://codereview.chromium.org/172032/diff/3383/2311
> File base/file_util_posix.cc (right):
>
> http://codereview.chromium.org/172032/diff/3383/2311#newcode91
> Line 91: typedef struct stat64 stat_wrapper;
> Typo: stat_wrapper =3D> stat_wrapper_t

Done.

> I suspect that you'll also need to wrap the off64_t type.
> But perhaps because we use int64, e.g.,
>
> =A0struct FileInfo {
> =A0 =A0// The size of the file in bytes. =A0Undefined when is_directory i=
s
> true.
> =A0 =A0int64 size;
> =A0 =A0...
>
> this need hasn't arisen.

It hasn't so far.

> http://codereview.chromium.org/172032/diff/3383/2279
> File base/platform_thread.h (right):
>
> http://codereview.chromium.org/172032/diff/3383/2279#newcode28
> Line 28: #elif defined(OS_POSIX)
> We already tested defined(OS_POSIX) on line 22.

Ooops.

>
> I think the OS_LINUX and OS_MACOSX cases in the original
> code should be deleted. =A0It's dead code (suppressed by
> the defined(OS_POSIX) test on line 22).

You are misreading the nesting (why don't we indent #if blocks?)

> http://codereview.chromium.org/172032/diff/3383/2310
> File base/process_util.h (right):
>
> http://codereview.chromium.org/172032/diff/3383/2310#newcode16
> Line 16: #elif defined(OS_LINUX) || defined(OS_FREEBSD)
> May want to consider
> =A0#elif defined(OS_POSIX) && !defined(OS_MACOSX)

According to man dirent on FreeBSD, dirent (for example) is not POSIX.

> This would require a corresponding change at lines 276-281
> below. =A0Would need to test defined(OS_MACOSX) before
> defined(OS_POSIX).
>
> http://codereview.chromium.org/172032/diff/3383/2308
> File base/sys_info_posix.cc (right):
>
> http://codereview.chromium.org/172032/diff/3383/2308#newcode72
> Line 72: // FIXME(benl): I have no idea how to get this
> See if this code works:
>
> http://www.freebsd.org/cgi/cvsweb.cgi/ports/devel/nspr/files/patch-.._pr_=
src_misc_prsystem.c

Nice, but this worries me because its in bytes and its only 32 bits long.

> http://codereview.chromium.org/172032/diff/3383/2314
> File base/third_party/nspr/prcpucfg.h (right):
>
> http://codereview.chromium.org/172032/diff/3383/2314#newcode38
> Line 38: #include "base/third_party/nspr/prcpucfg_linux.h"
> Please add a "base/third_party/nspr/prcpucfg_freebsd.h"
> file for FreeBSD. =A0You can get it by downloading and
> unpacking the NSPR 4.8 source tarball:
> https://ftp.mozilla.org/pub/mozilla.org/mozilla.org/nspr/releases/v4.8/sr=
c/nspr-4.8.tar.gz
> and use mozilla/nsprpub/pr/include/md/_freebsd.cfg (need
> to rename it prcpucfg_freebsd.h).

Done.

>
> http://codereview.chromium.org/172032/diff/3383/2313
> File base/time_posix.cc (right):
>
> http://codereview.chromium.org/172032/diff/3383/2313#newcode163
> Line 163: || defined(OS_FREEBSD)
> Nit: put || at the end of the previous line. =A0We usually
> put operators at end of lines when breaking long expressions.

Done.

>
> http://codereview.chromium.org/172032
>

[Ben Laurie (Chromium), 2009-08-27 19:48:00]

On Thu, Aug 27, 2009 at 7:35 PM, <sky@chromium.org> wrote:
>
> http://codereview.chromium.org/172032/diff/2330/2339
> File build/build_config.h (right):
>
> http://codereview.chromium.org/172032/diff/2330/2339#newcode19
> Line 19: # define OS_MACOSX 1
> I don't believe we generally indent defines like this, it certainly
> makes me queasy. Please don't do it.

Happy not to, I did it because I was confused. I should note that
Wen-Teh made an incorrect comment because he was confused by #if
nesting, though, so it seem a shame not to do something.

>
> http://codereview.chromium.org/172032
>

[wtc, 2009-08-27 19:56:39]

http://codereview.chromium.org/172032/diff/3383/2310
File base/process_util.h (right):

http://codereview.chromium.org/172032/diff/3383/2310#newcode17
Line 17: #include <dirent.h>
<dirent.h> is in Single Unix Specification 3:
http://www.opengroup.org/onlinepubs/009695399/basedefs/dirent.h.html

Our OS_POSIX macro means Unix and Unix-like systems.
It doesn't mean just POSIX.

[Ben Laurie (Chromium), 2009-08-27 23:16:56]

On Thu, Aug 27, 2009 at 8:23 PM, <brettw@chromium.org> wrote:
> I agree with Scott with the spaces. I can see why people do that, but we
> haven't done it anywhere so I would like to stay consistent.
>
> USE_GDK is misleading because TOOLKIT_VIEWS on Linux uses both GTK and
> GDK.
>
> Darin suggested to me a better approach that is the only thing I've
> heard that I'm happy with.
>
> These defines should generally match the file extension or directory
> name that we would add to the file. This makes it nice in that when you
> have too many defines, you know to make a new file, and you can
> basically keep the naming consistent.
>
> So I suggest using USE_X11 to mean the intersection of GTK and Views+GTK
> rather than adding a new define. What do you think about this?

Sure. To be clear you mean just drop USE_GDK and use USE_X11 throughout?

>
> http://codereview.chromium.org/172032
>

[Ben Laurie (Chromium), 2009-08-28 06:25:52]

Updated.

[brettw, 2009-08-28 16:42:44]

http://codereview.chromium.org/172032/diff/7029/7069
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/172032/diff/7029/7069#newcode122
Line 122: targets[i].target = static_cast<gchar *>(malloc(iter->first.size() +
1));
We normally don't add the space before the * here.

http://codereview.chromium.org/172032/diff/7029/7068
File base/file_util_posix.cc (right):

http://codereview.chromium.org/172032/diff/7029/7068#newcode87
Line 87: static int stat_wrapper(const char *path, stat_wrapper_t *sb) {
Please give this a Google-style function name. I don't really like "wrapper",
how about something like CallStat?

http://codereview.chromium.org/172032/diff/7029/7039
File base/platform_thread.h (right):

http://codereview.chromium.org/172032/diff/7029/7039#newcode22
Line 22: #elif defined(OS_POSIX)
Does FreeBSD fall under Posix here? I don't really understand this change or
when the "else" clause would get hit.

http://codereview.chromium.org/172032/diff/7029/7065
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/172032/diff/7029/7065#newcode72
Line 72: // FIXME(benl): I have no idea how to get this
Can you insert NOTIMPLEMENTED() in here which will give a warning?

[Ben Laurie (Chromium), 2009-08-28 17:54:59]

http://codereview.chromium.org/172032/diff/7029/7069
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/172032/diff/7029/7069#newcode122
Line 122: targets[i].target = static_cast<gchar *>(malloc(iter->first.size() +
1));
On 2009/08/28 16:42:44, brettw wrote:
> We normally don't add the space before the * here.

Done.

http://codereview.chromium.org/172032/diff/7029/7068
File base/file_util_posix.cc (right):

http://codereview.chromium.org/172032/diff/7029/7068#newcode87
Line 87: static int stat_wrapper(const char *path, stat_wrapper_t *sb) {
On 2009/08/28 16:42:44, brettw wrote:
> Please give this a Google-style function name. I don't really like "wrapper",
> how about something like CallStat?

Done.

http://codereview.chromium.org/172032/diff/7029/7039
File base/platform_thread.h (right):

http://codereview.chromium.org/172032/diff/7029/7039#newcode22
Line 22: #elif defined(OS_POSIX)
On 2009/08/28 16:42:44, brettw wrote:
> Does FreeBSD fall under Posix here?

Yes.

> I don't really understand this change or
> when the "else" clause would get hit.

If you mean the one on line 28, then it hits when it's POSIX but not MACOSX
(i.e. it _is_ LINUX or FREEBSD).

I've added a comment to clarify.

http://codereview.chromium.org/172032/diff/7029/7065
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/172032/diff/7029/7065#newcode72
Line 72: // FIXME(benl): I have no idea how to get this
On 2009/08/28 16:42:44, brettw wrote:
> Can you insert NOTIMPLEMENTED() in here which will give a warning?

Done.

[Ben Laurie (Chromium), 2009-08-28 17:55:53]

Updated.

[wtc, 2009-08-28 18:31:52]

Ben,

I reviewed the following files in Patch Set 14.  They look
good to me.  You may want to split them into a separate CL
and check them in first.

base/base_paths.h
base/native_library.h
base/platform_thread.h
base/port.h
base/process_util.h
base/process_util_posix.cc
base/sys_info_posix.cc
base/third_party/nspr/prcpucfg.h
base/third_party/nspr/prcpucfg_freebsd.h
base/third_party/nspr/prtime.cc
base/time_posix.cc

Note: base/crypto/signature_verifier.h also
looks good, but it requires build/build_config.h.

http://codereview.chromium.org/172032/diff/2409/2446
File base/base_paths.h (right):

http://codereview.chromium.org/172032/diff/2409/2446#newcode16
Line 16: #elif defined(OS_LINUX) || defined(OS_FREEBSD)
base_paths_linux.cc contains this call:

  readlink("/proc/self/exe", bin_dir, PATH_MAX)

Please make sure "/proc/self/exe" exists and works the same
way on FreeBSD.

http://codereview.chromium.org/172032/diff/2409/2437
File base/port.h (right):

http://codereview.chromium.org/172032/diff/2409/2437#newcode50
Line 50: #elif defined(OS_LINUX) || defined(OS_MACOSX) || defined(OS_FREEBSD)
I would just use #else here.  Things should break if
API_CALL isn't defined correctly...

http://codereview.chromium.org/172032/diff/2409/2447
File base/process_util.h (right):

http://codereview.chromium.org/172032/diff/2409/2447#newcode16
Line 16: #elif defined(OS_POSIX) && !defined(OS_MACOSX)
We probably can move the forward declaration on lines 53-55
here, like this:

#elif defined(OS_MACOSX)
struct kinfo_proc;
#elif defined(OS_POSIX)
#include <dirent.h>
#include <limits.h>
#include <sys/types.h>
#endif

It's strange why the forward declaration of struct
kinfo_proc is needed because we use kinfo_proc in a
std::vector, so we should need kinfo_proc's definition.

http://codereview.chromium.org/172032/diff/2409/2445
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/172032/diff/2409/2445#newcode74
Line 74: return 1024*1024*1024;
Would it be better to return 0?

[Ben Laurie (Chromium), 2009-08-29 18:57:55]

wtc: thanks for that, excellent suggestion.

I have moved those files here: http://codereview.chromium.org/180037

The next update of this patch will not contain them.

http://codereview.chromium.org/172032/diff/2409/2446
File base/base_paths.h (right):

http://codereview.chromium.org/172032/diff/2409/2446#newcode16
Line 16: #elif defined(OS_LINUX) || defined(OS_FREEBSD)
On 2009/08/28 18:31:52, wtc wrote:
> base_paths_linux.cc contains this call:
> 
>   readlink("/proc/self/exe", bin_dir, PATH_MAX)
> 
> Please make sure "/proc/self/exe" exists and works the same
> way on FreeBSD.

Good catch. FreeBSD does not. I have fixed it.

http://codereview.chromium.org/172032/diff/2409/2437
File base/port.h (right):

http://codereview.chromium.org/172032/diff/2409/2437#newcode50
Line 50: #elif defined(OS_LINUX) || defined(OS_MACOSX) || defined(OS_FREEBSD)
On 2009/08/28 18:31:52, wtc wrote:
> I would just use #else here.  Things should break if
> API_CALL isn't defined correctly...

Done.

http://codereview.chromium.org/172032/diff/2409/2447
File base/process_util.h (right):

http://codereview.chromium.org/172032/diff/2409/2447#newcode16
Line 16: #elif defined(OS_POSIX) && !defined(OS_MACOSX)
On 2009/08/28 18:31:52, wtc wrote:
> We probably can move the forward declaration on lines 53-55
> here, like this:
> 
> #elif defined(OS_MACOSX)
> struct kinfo_proc;
> #elif defined(OS_POSIX)
> #include <dirent.h>
> #include <limits.h>
> #include <sys/types.h>
> #endif
> 
> It's strange why the forward declaration of struct
> kinfo_proc is needed because we use kinfo_proc in a
> std::vector, so we should need kinfo_proc's definition.

Done, but I agree that this must be redundant - I've added a FIXME. Perhaps
someone with a Mac dev platform could test that removing it completely is
harmless?

http://codereview.chromium.org/172032/diff/2409/2445
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/172032/diff/2409/2445#newcode74
Line 74: return 1024*1024*1024;
On 2009/08/28 18:31:52, wtc wrote:
> Would it be better to return 0?

"better" in the sense that its more likely to break something and force us to
work this out, yes.

Done.

[Ben Laurie (Chromium), 2009-08-29 19:00:46]

Updated.

[wtc, 2009-08-29 19:09:30]

http://codereview.chromium.org/172032/diff/2409/2447
File base/process_util.h (right):

http://codereview.chromium.org/172032/diff/2409/2447#newcode16
Line 16: #elif defined(OS_POSIX) && !defined(OS_MACOSX)
On 2009/08/29 18:57:55, benl wrote:
>
> Done, but I agree that this must be redundant - I've added a FIXME. Perhaps
> someone with a Mac dev platform could test that removing it completely is
> harmless?

I tested this on a Mac.  For reasons I don't understand,
the std::vector<kinfo_proc> kinfo_procs_ member on line
277 below just needs a forward declaration of struct
kinfo_proc.  The definition of that struct is not
necessary.  struct kinfo_proc is defined in
<sys/sysctl.h>, so I guess the intention here is
to use the forward declaration to replace a header
include, which is recommended by our style guide.

http://codereview.chromium.org/172032/diff/2409/2445
File base/sys_info_posix.cc (right):

http://codereview.chromium.org/172032/diff/2409/2445#newcode74
Line 74: return 1024*1024*1024;
On 2009/08/29 18:57:55, benl wrote:
>
> "better" in the sense that its more likely to break something and force us to
> work this out, yes.

Actually, I meant that our code may be taking a
conservative action when it doesn't know the amount
of physical memory, and that may be better than
an action based on an incorrect amount of physical
memory.

[Amanda Walker, 2009-09-01 20:59:24]

LGTM.  The revised #ifdef approach is much more readable.

[Amanda Walker, 2009-09-01 22:03:07]

LGTM, one optional nit :-).

http://codereview.chromium.org/172032/diff/7076/6085
File base/clipboard.h (right):

http://codereview.chromium.org/172032/diff/7076/6085#newcode170
Line 170: #if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_FREEBSD)
Since the TODO is Mac-specific, "#if !defined(OS_MACOSX) is OK here.

[Ben Laurie (Chromium), 2009-09-02 22:11:24]

On Wed, Sep 2, 2009 at 10:11 PM, <thestig@chromium.org> wrote:
>
> http://codereview.chromium.org/172032/diff/7076/6061
> File chrome/chrome.gyp (right):
>
> http://codereview.chromium.org/172032/diff/7076/6061#newcode257
> Line 257: '<(grit_out_dir)/<(RULE_INPUT_ROOT)_ar.pak',
> After this, you'd need to add _bg.pak and so on. I fixed this in r25213.

Yeah, this is just a proxy for the correct solution :-)

>
> http://codereview.chromium.org/172032
>

[Nico, 2009-09-07 18:32:15]

http://codereview.chromium.org/172032/diff/14001/14026
File base/base.gyp (right):

http://codereview.chromium.org/172032/diff/14001/14026#newcode401
Line 401: 'conditions': [
Since this is a python dict, this 'conditions' (silently) overwrites the other
entry for 'conditions' above, which means the 'chromeos==1' above won't be
evaluated and hence _chromeos\.cc$ above won't cause sys_info_chromeos.cc won't
be added on chromeos, which causes the linker failures that broke the build.

Mark, is there any way gyp could warn (or error) when a dict key is present
multiple times?

[Ben Laurie (Chromium), 2009-09-13 16:29:08]

On 2009/09/07 18:32:15, Nico wrote:
> http://codereview.chromium.org/172032/diff/14001/14026
> File base/base.gyp (right):
> 
> http://codereview.chromium.org/172032/diff/14001/14026#newcode401
> Line 401: 'conditions': [
> Since this is a python dict, this 'conditions' (silently) overwrites the other
> entry for 'conditions' above, which means the 'chromeos==1' above won't be
> evaluated and hence _chromeos\.cc$ above won't cause sys_info_chromeos.cc
won't
> be added on chromeos, which causes the linker failures that broke the build.
> 
> Mark, is there any way gyp could warn (or error) when a dict key is present
> multiple times?

You can see code to do this here: http://codereview.chromium.org/202059

[Ben Laurie (Chromium), 2009-09-13 17:01:51]

This was committed at r25599.

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