Mac: Move remote layer use to GpuOutputSurfaceMac

Mac: Move remote layer use to GpuOutputSurfaceMac

Move the logic for managing remote layers from AcceleratedWidgetMac
to GpuOutputSurfaceMac. Change AcceleratedWidgetMac's interface to take
CALayers for new content, not remote handles (CAContextIDs).

In future patches, GpuOutputSurfaceMac will instantiate a
CALayerTreeCoordinatorMac locally (as opposed to in the GPU process),
and will present the resulting CALayers directly.

BUG=604052
Committed: https://crrev.com/898bf6cab1519ef567ecfc0dd07d3fef9b941247
Cr-Commit-Position: refs/heads/master@{#397646}

[ccameron, 2016-05-26 23:08:06]

ccameron@chromium.org changed reviewers:
+ mark@chromium.org, tapted@chromium.org

[ccameron, 2016-05-26 23:08:06]

Adding tapted@ for ui::AcceleratedWidgetMac.

Adding mark@ for base/mac.

[tapted, 2016-05-27 00:33:20]

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_output_surface_mac.h (right):

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.h:12: class
GpuOutputSurfaceMac
Is there an upstream patch? (this file doesn't seem to exist for me in
origin/master..)

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.h:39: class RemoteLayers;
struct? (otherwise my brain sees "class" and wants underscores on the data
members :p)

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_output_surface_mac.mm (right):

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.mm:18: class
GpuOutputSurfaceMac::GpuOutputSurfaceMac::RemoteLayers {
I think one of the GpuOutputSurfaceMac:: isn't needed

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.mm:44:
base::scoped_nsobject<CALayerHost> content_layer;
nit: blank line before

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/software_output_device_mac.mm:153: if
(!update_vsync_callback_.is_null())
does this need to be outside the if (widget) block to ensure something gets an
ACK?

https://codereview.chromium.org/2018793002/diff/1/ui/accelerated_widget_mac/a...
File ui/accelerated_widget_mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/2018793002/diff/1/ui/accelerated_widget_mac/a...
ui/accelerated_widget_mac/accelerated_widget_mac.h:47: #if defined(__OBJC__)
With AcceleratedWidgetMacGotFrame(..) removed from this file is it now just .mm
files that #include this? (can these guards (and the one above over #imports) be
removed?

[Mark Mentovai, 2016-05-27 19:45:49]

LGTM in base

[ccameron, 2016-06-03 02:43:01]

Thanks -- updated

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_output_surface_mac.h (right):

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.h:12: class
GpuOutputSurfaceMac
On 2016/05/27 00:33:19, tapted wrote:
> Is there an upstream patch? (this file doesn't seem to exist for me in
> origin/master..)

Yes -- it has landed now.

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.h:39: class RemoteLayers;
On 2016/05/27 00:33:19, tapted wrote:
> struct? (otherwise my brain sees "class" and wants underscores on the data
> members :p)

Done.

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_output_surface_mac.mm (right):

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.mm:18: class
GpuOutputSurfaceMac::GpuOutputSurfaceMac::RemoteLayers {
On 2016/05/27 00:33:19, tapted wrote:
> I think one of the GpuOutputSurfaceMac:: isn't needed

I'm amazed that compiles. Fixed.

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_output_surface_mac.mm:44:
base::scoped_nsobject<CALayerHost> content_layer;
On 2016/05/27 00:33:19, tapted wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/2018793002/diff/1/content/browser/compositor/...
content/browser/compositor/software_output_device_mac.mm:153: if
(!update_vsync_callback_.is_null())
On 2016/05/27 00:33:19, tapted wrote:
> does this need to be outside the if (widget) block to ensure something gets an
> ACK?

This is just updating vsync info, so it doesn't need to run every frame (in
practice every few seconds is sufficient).

https://codereview.chromium.org/2018793002/diff/1/ui/accelerated_widget_mac/a...
File ui/accelerated_widget_mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/2018793002/diff/1/ui/accelerated_widget_mac/a...
ui/accelerated_widget_mac/accelerated_widget_mac.h:47: #if defined(__OBJC__)
On 2016/05/27 00:33:19, tapted wrote:
> With AcceleratedWidgetMacGotFrame(..) removed from this file is it now just
.mm
> files that #include this? (can these guards (and the one above over #imports)
be
> removed?

Yes!! (had to change a reference or two to be a forward decl).

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_output_surface_mac.mm (right):

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:91:
base::scoped_nsobject<CALayer>(
I left out the base change here, since a larger change was needed in
https://codereview.chromium.org/2028823003/

[tapted, 2016-06-03 03:10:47]

lgtm % nits

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_output_surface_mac.mm (right):

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:91:
base::scoped_nsobject<CALayer>(
nit: I think the preferred way to do this is

base::scoped_nsobject<CALayer>(remote_layers_->content_layer,
base::scoped_policy::RETAIN)

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:94:
base::scoped_nsobject<CALayer>(
ditto

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

https://codereview.chromium.org/2018793002/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.h:30: class
AcceleratedWidgetMac;
nit: remove

[ccameron, 2016-06-03 05:37:21]

Thanks!

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
File content/browser/compositor/gpu_output_surface_mac.mm (right):

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:91:
base::scoped_nsobject<CALayer>(
On 2016/06/03 03:10:46, tapted wrote:
> nit: I think the preferred way to do this is
> 
> base::scoped_nsobject<CALayer>(remote_layers_->content_layer,
> base::scoped_policy::RETAIN)

Good point -- done.

https://codereview.chromium.org/2018793002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:94:
base::scoped_nsobject<CALayer>(
On 2016/06/03 03:10:47, tapted wrote:
> ditto

Done.

[ccameron, 2016-06-03 05:37:25]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-06-03 05:37:25]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2016-06-03 05:37:25]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/2018793002/#ps40001
(title: "Review feedback")

[ccameron, 2016-06-03 05:37:26]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-06-03 05:37:54]

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

[commit-bot: I haz the power, 2016-06-03 06:14:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-03 06:15:54]

Description was changed from

==========
Mac: Move remote layer use to GpuOutputSurfaceMac

Move the logic for managing remote layers from AcceleratedWidgetMac
to GpuOutputSurfaceMac. Change AcceleratedWidgetMac's interface to take
CALayers for new content, not remote handles (CAContextIDs).

In future patches, GpuOutputSurfaceMac will instantiate a
CALayerTreeCoordinatorMac locally (as opposed to in the GPU process),
and will present the resulting CALayers directly.

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

to

==========
Mac: Move remote layer use to GpuOutputSurfaceMac

Move the logic for managing remote layers from AcceleratedWidgetMac
to GpuOutputSurfaceMac. Change AcceleratedWidgetMac's interface to take
CALayers for new content, not remote handles (CAContextIDs).

In future patches, GpuOutputSurfaceMac will instantiate a
CALayerTreeCoordinatorMac locally (as opposed to in the GPU process),
and will present the resulting CALayers directly.

BUG=604052

Committed: https://crrev.com/898bf6cab1519ef567ecfc0dd07d3fef9b941247
Cr-Commit-Position: refs/heads/master@{#397646}
==========

[commit-bot: I haz the power, 2016-06-03 06:15:56]

Patchset 3 (id:??) landed as
https://crrev.com/898bf6cab1519ef567ecfc0dd07d3fef9b941247
Cr-Commit-Position: refs/heads/master@{#397646}