Mac: Clean up ifdefs in output surface

Mac: Clean up ifdefs in output surface

Break off a separate sub-class of OutputSurface for all Mac-specific
things.

BUG=604052
Committed: https://crrev.com/7767885d70c56aa4b31d0c51623ac54fc7903ff6
Cr-Commit-Position: refs/heads/master@{#397299}

[ccameron, 2016-05-26 18:44:30]

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

[ccameron, 2016-05-26 18:44:30]

There are too many OutputSurface sub-classes, and too many ifdefs in them ...
this makes one problem marginally worse, and another much better.

I'm also hoping that when we complete the move of CoreAnimation presentation to
the browser, we'll be able to kill off all of the "suspended for recycle"
nonsense.

[danakj, 2016-05-26 21:33:51]

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

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/browser_compositor_output_surface.h:74: virtual void
SetSurfaceSuspendedForRecycle(bool suspended);
why not abstract?

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

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface_mac.h:12: class
GpuBrowserCompositorOutputSurfaceMac
GpuSurfacelessBrowserCompositorOutputSurfaceMac ? (lol)

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface_mac.h:25: void
SetSurfaceSuspendedForRecycle(bool suspended) override;
This is from BrowserCompositorOutputSurface, not cc::OutputSurface.

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

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface_mac.mm:27:
base::CommandLine::ForCurrentProcess()->HasSwitch(
I think I'd rather GPTF did the command line checking stuff, and just gave bools
or whatever to the output surface to act on.

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:452:
compositor->task_runner().get(), std::move(validator)));
nullptr for the validator?

[ccameron, 2016-05-26 22:46:16]

Thanks! Updated.

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

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/browser_compositor_output_surface.h:74: virtual void
SetSurfaceSuspendedForRecycle(bool suspended);
On 2016/05/26 21:33:51, danakj wrote:
> why not abstract?

I wanted to get rid of all of the #if defined(OS_MACOSX) in
GpuBrowserCompositorOutputSurface. I changed this back.

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

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface_mac.h:12: class
GpuBrowserCompositorOutputSurfaceMac
On 2016/05/26 21:33:51, danakj wrote:
> GpuSurfacelessBrowserCompositorOutputSurfaceMac ? (lol)

I was contemplating this -- I've changed it to GpuOutputSurfaceMac instead :D

This way it matches SoftwareOutputDeviceMac, too.

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface_mac.h:25: void
SetSurfaceSuspendedForRecycle(bool suspended) override;
On 2016/05/26 21:33:51, danakj wrote:
> This is from BrowserCompositorOutputSurface, not cc::OutputSurface.

Oops! Moved.

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

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_browser_compositor_output_surface_mac.mm:27:
base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/05/26 21:33:51, danakj wrote:
> I think I'd rather GPTF did the command line checking stuff, and just gave
bools
> or whatever to the output surface to act on.

Sounds fair -- I've moved this & friends back to GPTF.

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/2018603002/diff/1/content/browser/compositor/...
content/browser/compositor/gpu_process_transport_factory.cc:452:
compositor->task_runner().get(), std::move(validator)));
On 2016/05/26 21:33:51, danakj wrote:
> nullptr for the validator?

Oh no ... I didn't realize that was !defined(OS_MACOSX). Fixed.

[ccameron, 2016-05-31 22:20:55]

Post-vacation ping

[danakj, 2016-06-01 18:11:04]

This is wonderful, few comments, LGTM overall.

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

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Why is this a .mm and not a .cc?

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:27: context,
move

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:29: vsync_manager,
move

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h
(right):

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h:39:
protected:
can you leave the member variables protected?

i'm not sure why we're trying to hide the overrides here at all, the class is an
output surface and is meant to be used as one. Can you just make the methods
public? Is that sufficient?

[danakj, 2016-06-01 18:11:22]

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h
(right):

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h:39:
protected:
On 2016/06/01 18:11:04, danakj wrote:
> can you leave the member variables protected?

i mean, private.

> i'm not sure why we're trying to hide the overrides here at all, the class is
an
> output surface and is meant to be used as one. Can you just make the methods
> public? Is that sufficient?

[ccameron, 2016-06-01 19:44:23]

Thanks!

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

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/06/01 18:11:04, danakj wrote:
> Why is this a .mm and not a .cc?

We keep track of the GPU-process-hosted CALayers in ui::AcceleratedWidgetMac ...
next patch is to move that here. Those APIs are all ObjC.

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:27: context,
On 2016/06/01 18:11:04, danakj wrote:
> move

Done.

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_output_surface_mac.mm:29: vsync_manager,
On 2016/06/01 18:11:04, danakj wrote:
> move

Done.

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
File
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h
(right):

https://codereview.chromium.org/2018603002/diff/20001/content/browser/composi...
content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h:39:
protected:
On 2016/06/01 18:11:22, danakj wrote:
> On 2016/06/01 18:11:04, danakj wrote:
> > can you leave the member variables protected?
> 
> i mean, private.
> 
> > i'm not sure why we're trying to hide the overrides here at all, the class
is
> an
> > output surface and is meant to be used as one. Can you just make the methods
> > public? Is that sufficient?
> 

Yeah, didn't make sense to me. Moved the private down instead.

[danakj, 2016-06-01 19:45:02]

LGTM

[ccameron, 2016-06-01 21:54:22]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-06-01 21:55:03]

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

[commit-bot: I haz the power, 2016-06-02 01:51:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 01:51:05]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2016-06-02 02:51:18]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-06-02 02:51:47]

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

[commit-bot: I haz the power, 2016-06-02 03:35:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 03:35:08]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2016-06-02 05:07:52]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-06-02 05:08:04]

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

[commit-bot: I haz the power, 2016-06-02 05:48:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-02 05:49:32]

Description was changed from

==========
Mac: Clean up ifdefs in output surface

Break off a separate sub-class of OutputSurface for all Mac-specific
things.

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

to

==========
Mac: Clean up ifdefs in output surface

Break off a separate sub-class of OutputSurface for all Mac-specific
things.

BUG=604052

Committed: https://crrev.com/7767885d70c56aa4b31d0c51623ac54fc7903ff6
Cr-Commit-Position: refs/heads/master@{#397299}
==========

[commit-bot: I haz the power, 2016-06-02 05:49:34]

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