Second set adding in Solaris as an OS option

Solaris: Second set adding in Solaris as an OS option

TEST=compiles
BUG=30101
Patch by James Choi <jchoi42@pha.jhu.edu>;.

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

[fbarchard, 2010-02-16 22:42:00]

just looking at ffmpeg.gyp

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

http://codereview.chromium.org/606075/diff/1/10#newcode717
third_party/ffmpeg/ffmpeg.gyp:717: ['OS=="linux" or OS=="freebsd"', {
do you need
or OS=="solaris"
here?

[jchoi42, 2010-02-16 23:52:36]

You're absolutely right. Fixed!

On 2010/02/16 22:42:00, fbarchard wrote:
> just looking at ffmpeg.gyp
> 
> http://codereview.chromium.org/606075/diff/1/10
> File third_party/ffmpeg/ffmpeg.gyp (right):
> 
> http://codereview.chromium.org/606075/diff/1/10#newcode717
> third_party/ffmpeg/ffmpeg.gyp:717: ['OS=="linux" or OS=="freebsd"', {
> do you need
> or OS=="solaris"
> here?

[fbarchard, 2010-02-17 00:03:02]

I've prepared a separate patch for ffmpeg
http://codereview.chromium.org/606076/show
because a number of other changes will applied to that file shortly.

[jchoi42, 2010-02-17 00:19:31]

On 2010/02/17 00:03:02, fbarchard wrote:
> I've prepared a separate patch for ffmpeg
> http://codereview.chromium.org/606076/show
> because a number of other changes will applied to that file shortly.

Sure- taken down. Though you might want to change the description of 606076 --
ffmpeg won't compile *correctly* on Solaris without a few more tweaks. The work
has already been done though (not by me). I can make a new set of patches and CC
you?

[fbarchard, 2010-02-17 00:28:55]

media.gyp lgtm as long as asound is not needed.

if you have further bugs/patches for ffmpeg or media, could you open a new crbug
with details and add the label internals-video

http://codereview.chromium.org/606075/diff/7001/8001
File media/media.gyp (right):

http://codereview.chromium.org/606075/diff/7001/8001#newcode156
media/media.gyp:156: 'libraries': [
I trust you tested this and -lasound is not needed.

[Paweł Hajdan Jr., 2010-02-17 07:27:12]

+Mark Mentovai for gyp comments

http://codereview.chromium.org/606075/diff/9001/10001
File media/media.gyp (right):

http://codereview.chromium.org/606075/diff/9001/10001#newcode152
media/media.gyp:152: ['OS =="solaris"', {
nit: This seems to duplicate the section below. Is it possible to write
OS=="freebsd" or OS=="solaris" instead?

[jchoi42, 2010-02-17 09:27:14]

You're right- I'm caught redhanded with the copypasta XP  

However,the audio system in Solaris/Opensolaris is different from the *BSDs, so
it seemed logical to keep it separate, at least to me. I was thinking in the
future, if extra libraries are needed by BSD/Sol, I didn't want one to break the
other :) 

On 2010/02/17 07:27:12, Paweł Hajdan Jr. wrote:
> +Mark Mentovai for gyp comments
> 
> http://codereview.chromium.org/606075/diff/9001/10001
> File media/media.gyp (right):
> 
> http://codereview.chromium.org/606075/diff/9001/10001#newcode152
> media/media.gyp:152: ['OS =="solaris"', {
> nit: This seems to duplicate the section below. Is it possible to write
> OS=="freebsd" or OS=="solaris" instead?

[Evan Martin, 2010-02-17 14:20:16]

http://codereview.chromium.org/606075/diff/9001/10001
File media/media.gyp (right):

http://codereview.chromium.org/606075/diff/9001/10001#newcode152
media/media.gyp:152: ['OS =="solaris"', {
On 2010/02/17 07:27:12, Paweł Hajdan Jr. wrote:
> nit: This seems to duplicate the section below. Is it possible to write
> OS=="freebsd" or OS=="solaris" instead?

I actually think this entire section of the file is unnecessary -- I refactored
these sorts of broad rules ("only build mac files on mac") into common.gypi, so
we no longer need these.  I will take a shot at cleaning up this file right now.

http://codereview.chromium.org/606075/diff/9001/10002
File webkit/webkit.gyp (right):

http://codereview.chromium.org/606075/diff/9001/10002#newcode402
webkit/webkit.gyp:402: and OS!="solaris"'
Can you use an extra # rather than the continuation?
Minor thing, I know, but continuation lines on comments are pretty weird.

[Evan Martin, 2010-02-17 15:59:34]

http://codereview.chromium.org/608032 FYI

[Mark Mentovai, 2010-02-17 17:44:21]

LG

http://codereview.chromium.org/606075/diff/9001/10001
File media/media.gyp (right):

http://codereview.chromium.org/606075/diff/9001/10001#newcode14
media/media.gyp:14: ['OS!="solaris"', {'sources/': [['exclude', '/solaris/']]}],
Please discuss your changes in this file with Evan Martin.  He's making changes
in here too.

http://codereview.chromium.org/606075/diff/9001/10002
File webkit/webkit.gyp (right):

http://codereview.chromium.org/606075/diff/9001/10002#newcode402
webkit/webkit.gyp:402: and OS!="solaris"'
Evan Martin wrote:
> Can you use an extra # rather than the continuation?
> Minor thing, I know, but continuation lines on comments are pretty weird.

Agree.

[jchoi42, 2010-02-18 03:06:55]

# added, style noted for future reference. Also, good call. I'm totally in favor
of any changes that make Chromium more self-consistent/coherent/elegant; feel
free to make any changes necessary!

On 2010/02/17 17:44:21, Mark Mentovai wrote:
> LG
> 
> http://codereview.chromium.org/606075/diff/9001/10001
> File media/media.gyp (right):
> 
> http://codereview.chromium.org/606075/diff/9001/10001#newcode14
> media/media.gyp:14: ['OS!="solaris"', {'sources/': [['exclude',
'/solaris/']]}],
> Please discuss your changes in this file with Evan Martin.  He's making
changes
> in here too.
> 
> http://codereview.chromium.org/606075/diff/9001/10002
> File webkit/webkit.gyp (right):
> 
> http://codereview.chromium.org/606075/diff/9001/10002#newcode402
> webkit/webkit.gyp:402: and OS!="solaris"'
> Evan Martin wrote:
> > Can you use an extra # rather than the continuation?
> > Minor thing, I know, but continuation lines on comments are pretty weird.
> 
> Agree.

[Evan Martin, 2010-02-18 07:46:10]

Can you sync against trunk and reupload?  I checked in a change to media.gyp
(you'll conflict with it, but I believe the resolution should be that you don't
need any changes to media.gyp).

[jchoi42, 2010-02-18 14:06:42]

Done and done. Thanks!

On 2010/02/18 07:46:10, Evan Martin wrote:
> Can you sync against trunk and reupload?  I checked in a change to media.gyp
> (you'll conflict with it, but I believe the resolution should be that you
don't
> need any changes to media.gyp).