Mac: Towards moving CALayerTree to the browser proc

Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

Add me as an OWNER for ImageTransportSurfaceOverlayMac.

BUG=604052
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/1d09d3e429a86ade0173092e56772eb080bf0834
Cr-Commit-Position: refs/heads/master@{#388708}

[ccameron, 2016-04-19 21:14:03]

Description was changed from

==========
BUG=604052
==========

to

==========
BUG=604052
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2016-04-19 21:41:14]

Description was changed from

==========
BUG=604052
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

BUG=604052
==========

[ccameron, 2016-04-19 21:41:49]

Description was changed from

==========
Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

BUG=604052
==========

to

==========
Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

Add me as an OWNER for ImageTransportSurfaceOverlayMac.

BUG=604052
==========

[ccameron, 2016-04-19 23:25:26]

ccameron@chromium.org changed reviewers:
+ sievers@chromium.org

[ccameron, 2016-04-19 23:25:26]

ptal. Just moving stuff.

[no sievers, 2016-04-20 00:20:14]

https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/DEPS (right):

https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/DEPS:2: "+gpu/GLES2",
That dependency seems backwards. Can you add the macros in ui/gl/gl_bindings.h
and then include that from ca_layer_tree_mac.mm?

[ccameron, 2016-04-20 07:17:15]

Yeah, if the GL binding include order seems broken, I think we're just at the
tip of the iceberg of that problem.

I'm partial to the gl_bindings_chromium.h solution, if you like that.

https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/DEPS (right):

https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/DEPS:2: "+gpu/GLES2",
On 2016/04/20 00:20:14, sievers wrote:
> That dependency seems backwards. Can you add the macros in ui/gl/gl_bindings.h
> and then include that from ca_layer_tree_mac.mm?

Are you sure? This is used to define GL_CA_LAYER_EDGE_RIGHT_CHROMIUM, which is
then used in cc/output/ca_layer_overlay.cc.

That's where things get disastrous. Because ui/ tends to include  the <GL/gl.h>,
while cc/ tends to include "third_party/khronos/GLES2/gl2ext.h".

Trying to get ui/gl_bindings.h into cc/ seems to have been repeatedly attempted
and failed -- see crbug.com/512833.

After several hours fighting with this, I give up.

Two options
1. Leave things as they are. Once the move to the browser process is complete,
we can just remove the GLES2 dependency, since we no longer need to go through
the command buffer.
2. Create a ui/gl/gl_bindings_chromium.h, which #defines all of the _CHROMIUM
values, and doesn't include any of the system headers. That file could then be
included without fear of conflicts.

Which would you prefer?

[no sievers, 2016-04-20 19:41:56]

On 2016/04/20 07:17:15, ccameron wrote:
> Yeah, if the GL binding include order seems broken, I think we're just at the
> tip of the iceberg of that problem.
> 
> I'm partial to the gl_bindings_chromium.h solution, if you like that.
> 
>
https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
> File ui/accelerated_widget_mac/DEPS (right):
> 
>
https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
> ui/accelerated_widget_mac/DEPS:2: "+gpu/GLES2",
> On 2016/04/20 00:20:14, sievers wrote:
> > That dependency seems backwards. Can you add the macros in
ui/gl/gl_bindings.h
> > and then include that from ca_layer_tree_mac.mm?
> 
> Are you sure? This is used to define GL_CA_LAYER_EDGE_RIGHT_CHROMIUM, which is
> then used in cc/output/ca_layer_overlay.cc.
> 
> That's where things get disastrous. Because ui/ tends to include  the
<GL/gl.h>,
> while cc/ tends to include "third_party/khronos/GLES2/gl2ext.h".
> 
> Trying to get ui/gl_bindings.h into cc/ seems to have been repeatedly
attempted
> and failed -- see crbug.com/512833.
> 
> After several hours fighting with this, I give up.
> 
> Two options
> 1. Leave things as they are. Once the move to the browser process is complete,
> we can just remove the GLES2 dependency, since we no longer need to go through
> the command buffer.
> 2. Create a ui/gl/gl_bindings_chromium.h, which #defines all of the _CHROMIUM
> values, and doesn't include any of the system headers. That file could then be
> included without fear of conflicts.
> 
> Which would you prefer?

Actually you're right. You want to include khronos GLES2 headers, since this
code is going through the GLES2 command buffer.
gl_bindings are for the actual service-side driver bindings.
Desktop GL headers (GL.h) on the other hand should only be used in
desktop-GL-specific implementation files.

So here is the clean way to do it:
If you depend on "//third_party/khronos:khronos_headers", that will in turn add
'../../gpu' as an "include_dir".
Then in the code you can #include <GLES2/gl2extchromium.h> and it doesn't look
weird (and you probably won't need the DEPS change i.e. the hack stays in the
build system).

[ccameron, 2016-04-20 20:25:45]

On 2016/04/20 19:41:56, sievers wrote:
> On 2016/04/20 07:17:15, ccameron wrote:
> > Yeah, if the GL binding include order seems broken, I think we're just at
the
> > tip of the iceberg of that problem.
> > 
> > I'm partial to the gl_bindings_chromium.h solution, if you like that.
> > 
> >
>
https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
> > File ui/accelerated_widget_mac/DEPS (right):
> > 
> >
>
https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
> > ui/accelerated_widget_mac/DEPS:2: "+gpu/GLES2",
> > On 2016/04/20 00:20:14, sievers wrote:
> > > That dependency seems backwards. Can you add the macros in
> ui/gl/gl_bindings.h
> > > and then include that from ca_layer_tree_mac.mm?
> > 
> > Are you sure? This is used to define GL_CA_LAYER_EDGE_RIGHT_CHROMIUM, which
is
> > then used in cc/output/ca_layer_overlay.cc.
> > 
> > That's where things get disastrous. Because ui/ tends to include  the
> <GL/gl.h>,
> > while cc/ tends to include "third_party/khronos/GLES2/gl2ext.h".
> > 
> > Trying to get ui/gl_bindings.h into cc/ seems to have been repeatedly
> attempted
> > and failed -- see crbug.com/512833.
> > 
> > After several hours fighting with this, I give up.
> > 
> > Two options
> > 1. Leave things as they are. Once the move to the browser process is
complete,
> > we can just remove the GLES2 dependency, since we no longer need to go
through
> > the command buffer.
> > 2. Create a ui/gl/gl_bindings_chromium.h, which #defines all of the
_CHROMIUM
> > values, and doesn't include any of the system headers. That file could then
be
> > included without fear of conflicts.
> > 
> > Which would you prefer?
> 
> Actually you're right. You want to include khronos GLES2 headers, since this
> code is going through the GLES2 command buffer.
> gl_bindings are for the actual service-side driver bindings.
> Desktop GL headers (GL.h) on the other hand should only be used in
> desktop-GL-specific implementation files.
> 
> So here is the clean way to do it:
> If you depend on "//third_party/khronos:khronos_headers", that will in turn
add
> '../../gpu' as an "include_dir".
> Then in the code you can #include <GLES2/gl2extchromium.h> and it doesn't look
> weird (and you probably won't need the DEPS change i.e. the hack stays in the
> build system).

I tried this, but it still wanted the DEPS change to satisfy presubmit.

[no sievers, 2016-04-20 23:39:20]

On 2016/04/20 20:25:45, ccameron wrote:
> On 2016/04/20 19:41:56, sievers wrote:
> > On 2016/04/20 07:17:15, ccameron wrote:
> > > Yeah, if the GL binding include order seems broken, I think we're just at
> the
> > > tip of the iceberg of that problem.
> > > 
> > > I'm partial to the gl_bindings_chromium.h solution, if you like that.
> > > 
> > >
> >
>
https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
> > > File ui/accelerated_widget_mac/DEPS (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1901253002/diff/20001/ui/accelerated_widget_m...
> > > ui/accelerated_widget_mac/DEPS:2: "+gpu/GLES2",
> > > On 2016/04/20 00:20:14, sievers wrote:
> > > > That dependency seems backwards. Can you add the macros in
> > ui/gl/gl_bindings.h
> > > > and then include that from ca_layer_tree_mac.mm?
> > > 
> > > Are you sure? This is used to define GL_CA_LAYER_EDGE_RIGHT_CHROMIUM,
which
> is
> > > then used in cc/output/ca_layer_overlay.cc.
> > > 
> > > That's where things get disastrous. Because ui/ tends to include  the
> > <GL/gl.h>,
> > > while cc/ tends to include "third_party/khronos/GLES2/gl2ext.h".
> > > 
> > > Trying to get ui/gl_bindings.h into cc/ seems to have been repeatedly
> > attempted
> > > and failed -- see crbug.com/512833.
> > > 
> > > After several hours fighting with this, I give up.
> > > 
> > > Two options
> > > 1. Leave things as they are. Once the move to the browser process is
> complete,
> > > we can just remove the GLES2 dependency, since we no longer need to go
> through
> > > the command buffer.
> > > 2. Create a ui/gl/gl_bindings_chromium.h, which #defines all of the
> _CHROMIUM
> > > values, and doesn't include any of the system headers. That file could
then
> be
> > > included without fear of conflicts.
> > > 
> > > Which would you prefer?
> > 
> > Actually you're right. You want to include khronos GLES2 headers, since this
> > code is going through the GLES2 command buffer.
> > gl_bindings are for the actual service-side driver bindings.
> > Desktop GL headers (GL.h) on the other hand should only be used in
> > desktop-GL-specific implementation files.
> > 
> > So here is the clean way to do it:
> > If you depend on "//third_party/khronos:khronos_headers", that will in turn
> add
> > '../../gpu' as an "include_dir".
> > Then in the code you can #include <GLES2/gl2extchromium.h> and it doesn't
look
> > weird (and you probably won't need the DEPS change i.e. the hack stays in
the
> > build system).
> 
> I tried this, but it still wanted the DEPS change to satisfy presubmit.

mind still changing the include to use <GLES2/gl2extchromium.h> though and add
khronos to the deps so at least it's more portable?

[ccameron, 2016-04-20 23:49:54]

Description was changed from

==========
Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

Add me as an OWNER for ImageTransportSurfaceOverlayMac.

BUG=604052
==========

to

==========
Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

Add me as an OWNER for ImageTransportSurfaceOverlayMac.

BUG=604052
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2016-04-20 23:50:49]

Sure -- updated.

[no sievers, 2016-04-21 00:07:21]

lgtm

[ccameron, 2016-04-21 00:14:36]

On 2016/04/21 00:07:21, sievers wrote:
> lgtm

Hmm, the updates to BUILD.gn don't pass checks
  ERROR at //ui/accelerated_widget_mac/BUILD.gn:30:5: Item type does not match.
      "//third_party/khronos:khronos_headers",
      ^--------------------------------------
  The type of //third_party/khronos:khronos_headers
  here is a target but was previously seen as a config.

  The most common cause is that the label of a config was put in the
  in the deps section of a target (or vice-versa).
  See //ui/gl/BUILD.gn:124:22: 
    public_configs = [ "//third_party/khronos:khronos_headers" ]
                       ^--------------------------------------

Removed it in the updated patch

[no sievers, 2016-04-21 00:19:37]

On 2016/04/21 00:14:36, ccameron wrote:
> On 2016/04/21 00:07:21, sievers wrote:
> > lgtm
> 
> Hmm, the updates to BUILD.gn don't pass checks
>   ERROR at //ui/accelerated_widget_mac/BUILD.gn:30:5: Item type does not
match.
>       "//third_party/khronos:khronos_headers",
>       ^--------------------------------------
>   The type of //third_party/khronos:khronos_headers
>   here is a target but was previously seen as a config.
> 
>   The most common cause is that the label of a config was put in the
>   in the deps section of a target (or vice-versa).
>   See //ui/gl/BUILD.gn:124:22: 
>     public_configs = [ "//third_party/khronos:khronos_headers" ]
>                        ^--------------------------------------
> 
> Removed it in the updated patch

gpu/command_buffer/client/BUILD.gn does this:

all_dependent_configs = [ "//third_party/khronos:khronos_headers" ]

[no sievers, 2016-04-21 00:23:23]

On 2016/04/21 00:19:37, sievers wrote:
> On 2016/04/21 00:14:36, ccameron wrote:
> > On 2016/04/21 00:07:21, sievers wrote:
> > > lgtm
> > 
> > Hmm, the updates to BUILD.gn don't pass checks
> >   ERROR at //ui/accelerated_widget_mac/BUILD.gn:30:5: Item type does not
> match.
> >       "//third_party/khronos:khronos_headers",
> >       ^--------------------------------------
> >   The type of //third_party/khronos:khronos_headers
> >   here is a target but was previously seen as a config.
> > 
> >   The most common cause is that the label of a config was put in the
> >   in the deps section of a target (or vice-versa).
> >   See //ui/gl/BUILD.gn:124:22: 
> >     public_configs = [ "//third_party/khronos:khronos_headers" ]
> >                        ^--------------------------------------
> > 
> > Removed it in the updated patch
> 
> gpu/command_buffer/client/BUILD.gn does this:
> 
> all_dependent_configs = [ "//third_party/khronos:khronos_headers" ]

Ok that doesn't help though, I see.
Yea I don't know what a 'config' is in GN.

[ccameron, 2016-04-21 00:24:41]

Patchset #6 (id:100001) has been deleted

[ccameron, 2016-04-21 06:13:45]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-04-21 06:13:45]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1901253002/#ps80001
(title: "Remove BUILD.gn")

[commit-bot: I haz the power, 2016-04-21 06:13:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1901253002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1901253002/80001

[commit-bot: I haz the power, 2016-04-21 07:21:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-22 19:32:07]

Description was changed from

==========
Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

Add me as an OWNER for ImageTransportSurfaceOverlayMac.

BUG=604052
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Mac: Towards moving CALayerTree to the browser proc

We currently construct the CALayerTree in the browser process,
then shuffle it over to the GPU process.

This moves the code over to being next to AcceleratedWidgetMac,
which is what will be hosting the CALayerTree when it moves to
the browser process.

Add me as an OWNER for ImageTransportSurfaceOverlayMac.

BUG=604052
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/1d09d3e429a86ade0173092e56772eb080bf0834
Cr-Commit-Position: refs/heads/master@{#388708}
==========

[commit-bot: I haz the power, 2016-04-22 19:32:08]

Patchset 5 (id:??) landed as
https://crrev.com/1d09d3e429a86ade0173092e56772eb080bf0834
Cr-Commit-Position: refs/heads/master@{#388708}