x11: Split the gfx_x11 target into its own gyp file.

x11: Split the gfx_x11 target into its own gyp file.

Add a gfx_x11.gyp in //ui/gfx/x/ and define the gfx_x11 target in there,
instead of defining it conditionally in //ui/gfx/gfx.gyp. This allows
including this code from particular components even when X11 is turned
off.

BUG=361137
R=piman@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269804

[sadrul, 2014-05-09 21:35:22]

piman@ for content/content_tests.gyp

sky@ for the rest (the main changes are in //ui/gfx/gfx.gyp and
//ui/gfx/x/gfx_x11.gyp)

[piman, 2014-05-09 21:54:16]

https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp
File ui/gfx/x/gfx_x11.gyp (right):

https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp#new...
ui/gfx/x/gfx_x11.gyp:24: '-lXi',
Why not adding a dependency on system.gyp:x11 like it was before?

[sadrul, 2014-05-09 22:11:14]

https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp
File ui/gfx/x/gfx_x11.gyp (right):

https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp#new...
ui/gfx/x/gfx_x11.gyp:24: '-lXi',
On 2014/05/09 21:54:17, piman wrote:
> Why not adding a dependency on system.gyp:x11 like it was before?

system.gyp:x11 target is also defined conditionally in system.gyp. So we are
unable to depend on that target when use_x11 is set to off.

If we move the x11 target out of system.gyp into its own separate gyp (e.g.
build/linux/x11.gyp), then this would not be a problem. I can do that if you
think that would be better than this.

[piman, 2014-05-09 22:12:54]

On 2014/05/09 22:11:14, sadrul wrote:
> https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp
> File ui/gfx/x/gfx_x11.gyp (right):
> 
>
https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp#new...
> ui/gfx/x/gfx_x11.gyp:24: '-lXi',
> On 2014/05/09 21:54:17, piman wrote:
> > Why not adding a dependency on system.gyp:x11 like it was before?
> 
> system.gyp:x11 target is also defined conditionally in system.gyp. So we are
> unable to depend on that target when use_x11 is set to off.
> 
> If we move the x11 target out of system.gyp into its own separate gyp (e.g.
> build/linux/x11.gyp), then this would not be a problem. I can do that if you
> think that would be better than this.

I don't understand, why would we use the x11 infra if use_x11 is set to off?

[sadrul, 2014-05-09 22:18:54]

On 2014/05/09 22:12:54, piman wrote:
> On 2014/05/09 22:11:14, sadrul wrote:
> > https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp
> > File ui/gfx/x/gfx_x11.gyp (right):
> > 
> >
>
https://codereview.chromium.org/275273002/diff/20001/ui/gfx/x/gfx_x11.gyp#new...
> > ui/gfx/x/gfx_x11.gyp:24: '-lXi',
> > On 2014/05/09 21:54:17, piman wrote:
> > > Why not adding a dependency on system.gyp:x11 like it was before?
> > 
> > system.gyp:x11 target is also defined conditionally in system.gyp. So we are
> > unable to depend on that target when use_x11 is set to off.
> > 
> > If we move the x11 target out of system.gyp into its own separate gyp (e.g.
> > build/linux/x11.gyp), then this would not be a problem. I can do that if you
> > think that would be better than this.
> 
> I don't understand, why would we use the x11 infra if use_x11 is set to off?

We need this for the X11 backend for ozone.

Ozone by default sets use_x11 to off. But to be able to build and test on the
desktop, we are working on an X11 implementation of the ozone platform. So we
want to re-use some of the X11 code we currently use for linux-aura/chromeos for
gfx and events. For the normal ozone build, we do a build of content-shell with
use_x11 = 0. But we still want to be able to build the basic x11-related code
for the x11 ozone-impl. (The larger in-progress CL for this is at
https://codereview.chromium.org/275263002/)

[piman, 2014-05-09 22:23:11]

On Fri, May 9, 2014 at 3:18 PM, <sadrul@chromium.org> wrote:

> On 2014/05/09 22:12:54, piman wrote:
>
>> On 2014/05/09 22:11:14, sadrul wrote:
>> > https://codereview.chromium.org/275273002/diff/20001/ui/
>> gfx/x/gfx_x11.gyp
>> > File ui/gfx/x/gfx_x11.gyp (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/275273002/diff/20001/ui/
> gfx/x/gfx_x11.gyp#newcode24
>
>> > ui/gfx/x/gfx_x11.gyp:24: '-lXi',
>> > On 2014/05/09 21:54:17, piman wrote:
>> > > Why not adding a dependency on system.gyp:x11 like it was before?
>> >
>> > system.gyp:x11 target is also defined conditionally in system.gyp. So
>> we are
>> > unable to depend on that target when use_x11 is set to off.
>> >
>> > If we move the x11 target out of system.gyp into its own separate gyp
>> (e.g.
>> > build/linux/x11.gyp), then this would not be a problem. I can do that
>> if you
>> > think that would be better than this.
>>
>
>  I don't understand, why would we use the x11 infra if use_x11 is set to
>> off?
>>
>
> We need this for the X11 backend for ozone.
>
> Ozone by default sets use_x11 to off. But to be able to build and test on
> the
> desktop, we are working on an X11 implementation of the ozone platform. So
> we
> want to re-use some of the X11 code we currently use for
> linux-aura/chromeos for
> gfx and events. For the normal ozone build, we do a build of content-shell
> with
> use_x11 = 0. But we still want to be able to build the basic x11-related
> code
> for the x11 ozone-impl. (The larger in-progress CL for this is at
> https://codereview.chromium.org/275263002/)
>

How about defining a use_ozone_x11, and gate both system.gyp:x11 and this
on 'use_x11==1 or use_ozone_x11==1'?


>
> https://codereview.chromium.org/275273002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-05-09 22:37:47]

On 2014/05/09 22:23:11, piman wrote:
> On Fri, May 9, 2014 at 3:18 PM, <mailto:sadrul@chromium.org> wrote:
> 
> > On 2014/05/09 22:12:54, piman wrote:
> >
> >> On 2014/05/09 22:11:14, sadrul wrote:
> >> > https://codereview.chromium.org/275273002/diff/20001/ui/
> >> gfx/x/gfx_x11.gyp
> >> > File ui/gfx/x/gfx_x11.gyp (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/275273002/diff/20001/ui/
> > gfx/x/gfx_x11.gyp#newcode24
> >
> >> > ui/gfx/x/gfx_x11.gyp:24: '-lXi',
> >> > On 2014/05/09 21:54:17, piman wrote:
> >> > > Why not adding a dependency on system.gyp:x11 like it was before?
> >> >
> >> > system.gyp:x11 target is also defined conditionally in system.gyp. So
> >> we are
> >> > unable to depend on that target when use_x11 is set to off.
> >> >
> >> > If we move the x11 target out of system.gyp into its own separate gyp
> >> (e.g.
> >> > build/linux/x11.gyp), then this would not be a problem. I can do that
> >> if you
> >> > think that would be better than this.
> >>
> >
> >  I don't understand, why would we use the x11 infra if use_x11 is set to
> >> off?
> >>
> >
> > We need this for the X11 backend for ozone.
> >
> > Ozone by default sets use_x11 to off. But to be able to build and test on
> > the
> > desktop, we are working on an X11 implementation of the ozone platform. So
> > we
> > want to re-use some of the X11 code we currently use for
> > linux-aura/chromeos for
> > gfx and events. For the normal ozone build, we do a build of content-shell
> > with
> > use_x11 = 0. But we still want to be able to build the basic x11-related
> > code
> > for the x11 ozone-impl. (The larger in-progress CL for this is at
> > https://codereview.chromium.org/275263002/)
> >
> 
> How about defining a use_ozone_x11, and gate both system.gyp:x11 and this
> on 'use_x11==1 or use_ozone_x11==1'?

Done! (to keep with the currently used format, I named the variable
ozone_platform_ozonex, and used that)

[piman, 2014-05-09 22:42:51]

https://codereview.chromium.org/275273002/diff/40001/ui/gfx/x/gfx_x11.gyp
File ui/gfx/x/gfx_x11.gyp (right):

https://codereview.chromium.org/275273002/diff/40001/ui/gfx/x/gfx_x11.gyp#new...
ui/gfx/x/gfx_x11.gyp:8: 'use_x11': 1,
do we need this? seems wrong.

[sadrul, 2014-05-09 23:00:22]

https://codereview.chromium.org/275273002/diff/40001/ui/gfx/x/gfx_x11.gyp
File ui/gfx/x/gfx_x11.gyp (right):

https://codereview.chromium.org/275273002/diff/40001/ui/gfx/x/gfx_x11.gyp#new...
ui/gfx/x/gfx_x11.gyp:8: 'use_x11': 1,
On 2014/05/09 22:42:52, piman wrote:
> do we need this? seems wrong.

This unfortunately is necessary because otherwise all the x11_ files in the
sources list get excluded because of the exclusion rules in filename_rules.gypi
(we cannot check the ozone_platform_ozonex variable for that, because we do want
to exclude the x11 files from the normal build)

[piman, 2014-05-09 23:07:38]

On Fri, May 9, 2014 at 4:00 PM, <sadrul@chromium.org> wrote:

>
> https://codereview.chromium.org/275273002/diff/40001/ui/gfx/x/gfx_x11.gyp
> File ui/gfx/x/gfx_x11.gyp (right):
>
> https://codereview.chromium.org/275273002/diff/40001/ui/
> gfx/x/gfx_x11.gyp#newcode8
> ui/gfx/x/gfx_x11.gyp:8: 'use_x11': 1,
> On 2014/05/09 22:42:52, piman wrote:
>
>> do we need this? seems wrong.
>>
>
> This unfortunately is necessary because otherwise all the x11_ files in
> the sources list get excluded because of the exclusion rules in
> filename_rules.gypi (we cannot check the ozone_platform_ozonex variable
> for that, because we do want to exclude the x11 files from the normal
> build)
>

I think includes override excludes in gyp, so we could include * to cancel
the excludes.
If not, can you please add a comment about why we're doing this?


>
> https://codereview.chromium.org/275273002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-05-09 23:17:40]

On 2014/05/09 23:07:38, piman wrote:
> On Fri, May 9, 2014 at 4:00 PM, <mailto:sadrul@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/275273002/diff/40001/ui/gfx/x/gfx_x11.gyp
> > File ui/gfx/x/gfx_x11.gyp (right):
> >
> > https://codereview.chromium.org/275273002/diff/40001/ui/
> > gfx/x/gfx_x11.gyp#newcode8
> > ui/gfx/x/gfx_x11.gyp:8: 'use_x11': 1,
> > On 2014/05/09 22:42:52, piman wrote:
> >
> >> do we need this? seems wrong.
> >>
> >
> > This unfortunately is necessary because otherwise all the x11_ files in
> > the sources list get excluded because of the exclusion rules in
> > filename_rules.gypi (we cannot check the ozone_platform_ozonex variable
> > for that, because we do want to exclude the x11 files from the normal
> > build)
> >
> 
> I think includes override excludes in gyp, so we could include * to cancel
> the excludes.

This didn't seem to work. I tried:

diff --git a/ui/gfx/x/gfx_x11.gyp b/ui/gfx/x/gfx_x11.gyp
index 7d94648..8a7829e 100644
--- a/ui/gfx/x/gfx_x11.gyp
+++ b/ui/gfx/x/gfx_x11.gyp
@@ -5,7 +5,6 @@
 { 
   'variables': {
     'chromium_code': 1,
-    'use_x11': 1,
   },
   
   'targets': [
@@ -19,15 +18,15 @@
       'defines': [
         'GFX_IMPLEMENTATION',
       ],
-      'sources': [
-        'x11_atom_cache.cc',
-        'x11_atom_cache.h',
-        'x11_connection.cc',
-        'x11_connection.h',
-        'x11_error_tracker.cc',
-        'x11_error_tracker.h',
-        'x11_types.cc',
-        'x11_types.h',
+      'sources/': [
+        ['include', 'x11_atom_cache.cc'],
+        ['include', 'x11_atom_cache.h'],
+        ['include', 'x11_connection.cc'],
+        ['include', 'x11_connection.h'],
+        ['include', 'x11_error_tracker.cc'],
+        ['include', 'x11_error_tracker.h'],
+        ['include', 'x11_types.cc'],
+        ['include', 'x11_types.h'],
       ],
     },
   ]

and

diff --git a/ui/gfx/x/gfx_x11.gyp b/ui/gfx/x/gfx_x11.gyp
index 7d94648..049db87 100644
--- a/ui/gfx/x/gfx_x11.gyp
+++ b/ui/gfx/x/gfx_x11.gyp
@@ -5,7 +5,6 @@
 {
   'variables': {
     'chromium_code': 1,
-    'use_x11': 1,
   },
 
   'targets': [
@@ -29,6 +28,16 @@
         'x11_types.cc',
         'x11_types.h',
       ],
+      'sources/': [
+        ['include', 'x11_atom_cache.cc'],
+        ['include', 'x11_atom_cache.h'],
+        ['include', 'x11_connection.cc'],
+        ['include', 'x11_connection.h'],
+        ['include', 'x11_error_tracker.cc'],
+        ['include', 'x11_error_tracker.h'],
+        ['include', 'x11_types.cc'],
+        ['include', 'x11_types.h'],
+      ],
     },
   ]
 }


> If not, can you please add a comment about why we're doing this?

Done.

[piman, 2014-05-09 23:22:44]

LGTM then

[sky, 2014-05-12 15:26:47]

LGTM

[sadrul, 2014-05-12 17:31:51]

Committed patchset #4 manually as r269804 (presubmit successful).