Make the gyp files more cross-platform across the Unices

Make use of the new cross-platform POSIX defines toolkit_uses_gtk, os_posix, and use_x11 for the gyp files.  For lists of source files that use a mix of POSIX and Gtk APIs, toolkit_uses_gtk was given precedence.  I also added Solaris to the remaining grit files.

Patch by ruben (chromium@hybridsource.org).

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

[ruben, 2011-05-13 06:21:58]

Patch for gyp files using the new cross-Unix defines.  Evan is primary reviewer,
input welcome from Tony and Peter.

[tony, 2011-05-16 19:25:11]

In general, I would probably use 'os_posix==1 and OS!="mac"' more and
toolkit_uses_gtk==1 less.  I admit that the conditions are confusing because we
mix gtk and other dependencies together.  I guess I would rather be overly broad
rather than have toolkit_uses_gtk imply other dependencies.

Anyway, if I didn't comment on it, I don't feel strongly about it so you needed
worry about it.

http://codereview.chromium.org/7011032/diff/1/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

http://codereview.chromium.org/7011032/diff/1/chrome/chrome_exe.gypi#newcode153
chrome/chrome_exe.gypi:153: ['use_x11==1', {
Nit: This might be better as os_posix==1 and OS!="mac".

http://codereview.chromium.org/7011032/diff/1/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7011032/diff/1/chrome/chrome_tests.gypi#newcod...
chrome/chrome_tests.gypi:3657: ['toolkit_uses_gtk==1', {
This also looks like os_posix==1 and OS!="mac".

http://codereview.chromium.org/7011032/diff/1/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/7011032/diff/1/content/content_browser.gypi#ne...
content/content_browser.gypi:364: ['toolkit_uses_gtk==1', {
Here too: os_posix==1 and OS!="mac"

http://codereview.chromium.org/7011032/diff/1/ui/gfx/compositor/compositor.gyp
File ui/gfx/compositor/compositor.gyp (right):

http://codereview.chromium.org/7011032/diff/1/ui/gfx/compositor/compositor.gy...
ui/gfx/compositor/compositor.gyp:11: ['os_posix==1 and OS!="mac"', {'sources/':
[
Can you fix the line wrapping while you're here?

http://codereview.chromium.org/7011032/diff/1/ui/gfx/surface/surface.gyp
File ui/gfx/surface/surface.gyp (right):

http://codereview.chromium.org/7011032/diff/1/ui/gfx/surface/surface.gyp#newc...
ui/gfx/surface/surface.gyp:13: ['toolkit_uses_gtk==1', {'sources/': [
This line wrapping is also a bit awkward.

http://codereview.chromium.org/7011032/diff/1/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/7011032/diff/1/ui/ui.gyp#newcode17
ui/ui.gyp:17: ['toolkit_uses_gtk==1', {'sources/': [
wrapping

[ruben, 2011-05-16 22:59:04]

On 2011/05/16 19:25:11, tony wrote:
> In general, I would probably use 'os_posix==1 and OS!="mac"' more and
> toolkit_uses_gtk==1 less.  I admit that the conditions are confusing because
we
> mix gtk and other dependencies together.  I guess I would rather be overly
broad
> rather than have toolkit_uses_gtk imply other dependencies.
Pretty arbitrary at this point since they're interchangeable right now: I made
the changes you asked for.
 
>
http://codereview.chromium.org/7011032/diff/1/chrome/chrome_exe.gypi#newcode153
> chrome/chrome_exe.gypi:153: ['use_x11==1', {
> Nit: This might be better as os_posix==1 and OS!="mac".
Fixed.

>
http://codereview.chromium.org/7011032/diff/1/chrome/chrome_tests.gypi#newcod...
> chrome/chrome_tests.gypi:3657: ['toolkit_uses_gtk==1', {
> This also looks like os_posix==1 and OS!="mac".
Fixed.
 
>
http://codereview.chromium.org/7011032/diff/1/content/content_browser.gypi#ne...
> content/content_browser.gypi:364: ['toolkit_uses_gtk==1', {
> Here too: os_posix==1 and OS!="mac"
This one could go either way for me, but I changed it.
 
>
http://codereview.chromium.org/7011032/diff/1/ui/gfx/compositor/compositor.gy...
> ui/gfx/compositor/compositor.gyp:11: ['os_posix==1 and OS!="mac"',
{'sources/':
> [
> Can you fix the line wrapping while you're here?
Changed it but not sure what you had in mind, let me know if it's good.
 
>
http://codereview.chromium.org/7011032/diff/1/ui/gfx/surface/surface.gyp#newc...
> ui/gfx/surface/surface.gyp:13: ['toolkit_uses_gtk==1', {'sources/': [
> This line wrapping is also a bit awkward.
Same as above. 

> http://codereview.chromium.org/7011032/diff/1/ui/ui.gyp#newcode17
> ui/ui.gyp:17: ['toolkit_uses_gtk==1', {'sources/': [
> wrapping
Same as last two.

[tony, 2011-05-16 23:00:32]

Evan, do you want to take a look?

http://codereview.chromium.org/7011032/diff/5002/ui/gfx/compositor/compositor...
File ui/gfx/compositor/compositor.gyp (right):

http://codereview.chromium.org/7011032/diff/5002/ui/gfx/compositor/compositor...
ui/gfx/compositor/compositor.gyp:12: 'sources/': [['include', '_(gl)\\.cc$'],]
Yes, this is what I meant.  Thanks!

[Evan Stade, 2011-05-17 00:46:12]

so the two statements toolkit_uses_gtk vs. os_posix and !os_mac which do the
same thing? I think this is going to be confusing. I don't think the distinction
(use toolkit_uses_gtk when the target depends on gtk) will hold up for long,
because if someone needs to add a gtk dependency to a target that's currently
marked 'posix and not mac', they probably aren't going to be conscious of the
fact they're supposed to change the define to the synonymous toolkit_uses_gtk.

But that's just my input. I don't feel strongly enough to stand in the way of
this patch.

http://codereview.chromium.org/7011032/diff/5002/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/7011032/diff/5002/chrome/chrome_common.gypi#ne...
chrome/chrome_common.gypi:259: ['toolkit_uses_gtk==1', {
wow, we are really inconsistent about putting spaces around operators in gyp
files. But I think we do want them as per the python style guidelines.

http://codereview.chromium.org/7011032/diff/5002/chrome/chrome_common.gypi#ne...
chrome/chrome_common.gypi:390: [ 'os_posix==1 and OS!="mac"', {
I guess to be consistent there should be no space between [ and ' (here and
elsewhere)

http://codereview.chromium.org/7011032/diff/5002/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/7011032/diff/5002/content/content_browser.gypi...
content/content_browser.gypi:367: '../build/linux/system.gyp:gtk',
so now I am confused, I thought you would have put toolkit_uses_gtk for this one
due to this dependency?

[ruben, 2011-05-17 01:11:24]

On 2011/05/17 00:46:12, Evan Stade wrote:
> so the two statements toolkit_uses_gtk vs. os_posix and !os_mac which do the
> same thing? I think this is going to be confusing. I don't think the
distinction
> (use toolkit_uses_gtk when the target depends on gtk) will hold up for long,
> because if someone needs to add a gtk dependency to a target that's currently
> marked 'posix and not mac', they probably aren't going to be conscious of the
> fact they're supposed to change the define to the synonymous toolkit_uses_gtk.
Won't matter either way in practice, as both are currently logically equivalent
and only used for better naming.  Until someone uses Gtk on a platform that
isn't non-mac POSIX, these changes are cosmetic for the 3-4 supported platforms.

>
http://codereview.chromium.org/7011032/diff/5002/chrome/chrome_common.gypi#ne...
> chrome/chrome_common.gypi:259: ['toolkit_uses_gtk==1', {
> wow, we are really inconsistent about putting spaces around operators in gyp
> files. But I think we do want them as per the python style guidelines.
I find that no spaces is much more common, but both forms are used.  I can add
spaces to this patch but there's a lot more gyp files to fix then, so maybe
better to do it as a separate automatically-generated patch.

>
http://codereview.chromium.org/7011032/diff/5002/chrome/chrome_common.gypi#ne...
> chrome/chrome_common.gypi:390: [ 'os_posix==1 and OS!="mac"', {
> I guess to be consistent there should be no space between [ and ' (here and
> elsewhere)
Fixed, here and two other locations.

>
http://codereview.chromium.org/7011032/diff/5002/content/content_browser.gypi...
> content/content_browser.gypi:367: '../build/linux/system.gyp:gtk',
> so now I am confused, I thought you would have put toolkit_uses_gtk for this
one
> due to this dependency?
I thought so too, but I think Tony feels the other dependencies outweigh this
one.  This is so subjective and arbitrary, I honestly don't care what's used (I
didn't define a toolkit_uses_gtk in my original Unix patch, instead using
something similar to os_posix).  I'll use whatever you two decide is better, let
me know.

[tony, 2011-05-17 16:37:24]

I agree that the distinction is arbitrary right now, but 
a) It matches the code (we have the same flags in build/build_config.h)
b) We have other cases like this such as WCHAR_T_IS_UTF16/WCHAR_T_IS_UTF32 or
COMPILER_GCC/COMPILER_MSVC both of which are really the same as Win/non-win.

If we really wanted to be correct, we would split some of the conditions into
two conditions.  One for uses_toolkit_gtk that only has the gtk dependency and
one for posix but not mac that includes the other dependencies, but I'm not sure
it's worth the effort right now.

On 2011/05/17 01:11:24, ruben wrote:
http://codereview.chromium.org/7011032/diff/5002/content/content_browser.gypi...
> > content/content_browser.gypi:367: '../build/linux/system.gyp:gtk',
> > so now I am confused, I thought you would have put toolkit_uses_gtk for this
> one
> > due to this dependency?
> I thought so too, but I think Tony feels the other dependencies outweigh this
> one.  This is so subjective and arbitrary, I honestly don't care what's used
(I
> didn't define a toolkit_uses_gtk in my original Unix patch, instead using
> something similar to os_posix).  I'll use whatever you two decide is better,
let
> me know.

Either way is fine with me.  My reasoning for 'os_posix==1 and OS!="mac"' is
that most of the stuff is not related to gtk (dbus, x11, fontconfig).  You could
split it into two conditions, but I don't think it's worth it.

[ruben, 2011-05-17 22:06:23]

On 2011/05/17 16:37:24, tony wrote:
> I agree that the distinction is arbitrary right now, but 
> a) It matches the code (we have the same flags in build/build_config.h)
> b) We have other cases like this such as WCHAR_T_IS_UTF16/WCHAR_T_IS_UTF32 or
> COMPILER_GCC/COMPILER_MSVC both of which are really the same as Win/non-win.
The problem is that ifdefs in the source are usually covering things more
fine-grained and specific than the stuff in gyp files.

> If we really wanted to be correct, we would split some of the conditions into
> two conditions.  One for uses_toolkit_gtk that only has the gtk dependency and
> one for posix but not mac that includes the other dependencies, but I'm not
sure
> it's worth the effort right now.
Little point in doing this till someone actually uses the Gtk code on a
non-POSIX platform (Views don't count as those have the toolkit_views define
anyway), which I don't see happening anytime soon. ;)
 
>
http://codereview.chromium.org/7011032/diff/5002/content/content_browser.gypi...
> > > content/content_browser.gypi:367: '../build/linux/system.gyp:gtk',
> Either way is fine with me.  My reasoning for 'os_posix==1 and OS!="mac"' is
> that most of the stuff is not related to gtk (dbus, x11, fontconfig).  You
could
> split it into two conditions, but I don't think it's worth it.
OK, I went back to toolkit_uses_gtk then.  I'm using the new gyp defines with a
very simple precedence rule: toolkit_uses_gtk automatically gets precedence over
the other two defines, and use_x11 gets precedence over os_posix and !mac. That
means any hint of Gtk in a cluster of POSIX files and I apply toolkit_uses_gtk,
so that there's no subjective weighing of whether it's more POSIX or Gtk.  At
some point they can be split up into separate sections, when there's an actual
need for it.

[Evan Stade, 2011-05-18 20:55:11]

LGTM

re: whitespace, I'd make sure to use it for new stuff and we can worry about
updating existing misuses later

[ruben, 2011-05-18 23:57:17]

On 2011/05/18 20:55:11, Evan Stade wrote:
> LGTM
> 
> re: whitespace, I'd make sure to use it for new stuff and we can worry about
> updating existing misuses later
I updated the patch to add the whitespace around the equality operators.  I'll
try committing now.

[commit-bot: I haz the power, 2011-05-19 00:01:22]

Try job failure for 7011032-14001 on win:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[ruben, 2011-05-19 01:14:05]

On 2011/05/19 00:01:22, commit-bot wrote:
> Try job failure for 7011032-14001 on win:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Flake?  I can't make any sense of it and I don't have a windows box to test on.

[tony, 2011-05-19 17:03:37]

http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/27462

Failure from the fixed patch.

http://codereview.chromium.org/7011032/diff/14001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7011032/diff/14001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:3715: ['os_posix == 1 and OS!=="mac"', {
The trybots were complaining about this !==.  I've patched it in a local
checkout and I'm running through the try bots now.

[tony, 2011-05-19 19:12:40]

I'm sending to the try bots again.  This seems to be working locally for me.

[tony, 2011-05-19 21:49:59]

Committed in r85979.