Create an AcceleratedWidgetMac

Create an AcceleratedWidgetMac

Split BrowserCompositorCALayerTreeMac into two parts. One part is the
ui::Compositor bits of the structure, and the other is the
AcceleratedWidgetMac which has the gfx::AcceleratedWidget bits of the
structure.

Update BrowserCompositorViewMac to explicitly hold these two parts, the
ui::Compositor and the AcceleratedWidgetMac, and rename it to
BrowserCompositorMac.

Make AcceleratedWidgetMacNSView provide the link to bind an
AcceleratedWidgetMac to draw the contents of an NSView. An
AcceleratedWidgetMac may be bound to multiple NSViews in sequence.

Update the recycling mechanism of BrowserCompositorMac to use this
binding mechanism.

Remove some of the few dependencies that AcceleratedWidgetMac would have
on content by making the callers for GotAcceleratedFrame specify a
callback for when the frame displays (in addition to the callback that
the client has, as the client may be far away from the caller).

The next steps after this are to move the AcceleratedWidgetMac to ui,
and then to typedef ui::AcceleratedWidgetMac* to gfx::AcceleratedWidget
for Mac.

BUG=424058
Committed: https://crrev.com/b9f5521acd4912794b6e3040dc5c9a59d0013f04
Cr-Commit-Position: refs/heads/master@{#305188}

[ccameron, 2014-11-20 04:34:00]

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

[ccameron, 2014-11-20 04:35:55]

So, this would be the first step. The next one would be to move
AcceleratedWidgetMac and its friends to ui::

We could make gfx::AcceleratedWidget actually *be* ui::AcceleratedWidgetMac on
Mac, or we could keep this as a helper (I originally named the structure
AcceleratedWidgetHelperMac, but that was a mouthful and wasn't pushing this
direction as clearly).

Would this be useful?

[ccameron, 2014-11-20 04:37:20]

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h (right):

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:35: // a time,
though).
Oh, I was thinking to call this AcceleratedWidgetViewMac instead of "client" --
would that be more clear?

[tapted, 2014-11-20 05:38:54]

Looks good I think. There's a few more dependencies (the old CL was
http://crrev.com/656033002 if it helps) - many were just unused headers. But I
should be able to work with this if we can get approval to put it somewhere
ui/views can get to it.. 

Comments are mostly nits. also maybe `TODO() rename
browser_compositor_ca_layer_tree_mac.h` for a follow-up (or do that now - the
affected files are probably already being touched in this CL.

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h (right):

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:35: // a time,
though).
On 2014/11/20 04:37:20, ccameron1 wrote:
> Oh, I was thinking to call this AcceleratedWidgetViewMac instead of "client"
--
> would that be more clear?

Hm. Both have merit. I was leaning towards `Client`. Since `View` gets
overloaded somewhat. e.g. would the client typically own a separate [NS]View
rather than be one itself?  `Container` also crossed my mind.

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:52: : public
IOSurfaceLayerClient {
nit: pull up

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.mm (right):

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.mm:76:
AcceleratedWidgetMacClient* client) {
nit: pull up

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:137:
base::Closure accelerated_frame_drawn_callback_;
could these be combined? i.e. just do

has_unacknowledged_accelerated_frame ==
!accelerated_frame_drawn_callback_.is_null()

and accelerated_frame_drawn_callback_.Reset() in ::AcknowledgeAcceleratedFrame()

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:25: ui::Compositor*
compositor();
nit: inline these?

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:33: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:10: #include
"content/browser/gpu/gpu_process_host_ui_shim.h"
is this unused in this file now?

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:12: #include
"content/common/gpu/gpu_messages.h"
this might be unused too

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:44: if
(g_recyclable_browser_compositor.Get())
Maybe DCHECK(!*compositor) - some minor protection against an AcquireCompositor
not being balanced

(Or if you like this kind of stuff, a step further, with something like

/// .h ///

#include "base/macros.h"

class ui::Compositor;
namespace content{

class AcceleratedWidgetMac;
class BrowserCompositorMac;
ScopedBrowserCompositorMac {
 public:
  ScopedBrowserCompositorMac();
  ~ScopedBrowserCompositorMac();
 
  ui::Compositor* compositor();
  AcceleratedWidgetMac* accelerated_widget_mac();

 private:
  BrowserCompositorMac* handle;

  DISALLOW..
};
}

/// .cc ///

class BrowserCompositorMac {
 friend class ScopedBrowserCompositorMac;
...
};

ScopedBrowserCompositorMac::ScopedBrowserCompositorMac() {
  BrowserCompositorMac::Acquire..(&handle);
}

ScopedBrowserCompositorMac::ScopedBrowserCompositorMac() {
  BrowserCompositorMac::Release..(&handle);
}

ui::Compositor* ScopedBrowserCompositorMac::compositor {
  return handle->compositor_;
}

etc.

https://codereview.chromium.org/745453002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:526: new
BrowserCompositorMacPlaceholder),
nit: pull up?

[ccameron, 2014-11-20 21:29:28]

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

[ccameron, 2014-11-20 21:33:51]

Thanks! Updated, and adding piman & danakj.

This addresses the issue that gfx::AcceleratedWidget is a lie on Mac. It breaks
up and renames structures so that we have a content::AcceleratedWidgetMac, which
is "what we really want in a gfx::AcceleratedWidget".

I left all the files in-place here so that the diffs were manageable and history
would be preserved. The next change would be to throw
content::AcceleratedWidgetMac (and friends) into ui/, and change the typedef for
gfx::AcceleratedWidget to be a ui::AcceleratedWidgetMac* on Mac.

I think that having made this distinction clear in my original ÜC implementation
is what's causing all of these issues.

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h (right):

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:35: // a time,
though).
On 2014/11/20 05:38:53, tapted wrote:
> On 2014/11/20 04:37:20, ccameron1 wrote:
> > Oh, I was thinking to call this AcceleratedWidgetViewMac instead of "client"
> --
> > would that be more clear?
> 
> Hm. Both have merit. I was leaning towards `Client`. Since `View` gets
> overloaded somewhat. e.g. would the client typically own a separate [NS]View
> rather than be one itself?  `Container` also crossed my mind. 

Yeah, view is getting too much use -- I just called it
AcceleratedWidgetMacNSView -- that seems unambiguous enough.

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:52: : public
IOSurfaceLayerClient {
On 2014/11/20 05:38:53, tapted wrote:
> nit: pull up

Done.

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.mm (right):

https://codereview.chromium.org/745453002/diff/1/content/browser/compositor/b...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.mm:76:
AcceleratedWidgetMacClient* client) {
On 2014/11/20 05:38:53, tapted wrote:
> nit: pull up

Done.

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.h (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.h:137:
base::Closure accelerated_frame_drawn_callback_;
On 2014/11/20 05:38:53, tapted wrote:
> could these be combined? i.e. just do
> 
> has_unacknowledged_accelerated_frame ==
> !accelerated_frame_drawn_callback_.is_null()
> 
> and accelerated_frame_drawn_callback_.Reset() in
::AcknowledgeAcceleratedFrame()

Done.

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:25: ui::Compositor*
compositor();
On 2014/11/20 05:38:54, tapted wrote:
> nit: inline these?

Done.

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:33: };
On 2014/11/20 05:38:54, tapted wrote:
> nit: DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:10: #include
"content/browser/gpu/gpu_process_host_ui_shim.h"
On 2014/11/20 05:38:54, tapted wrote:
> is this unused in this file now?

Yup, gone.

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:12: #include
"content/common/gpu/gpu_messages.h"
On 2014/11/20 05:38:54, tapted wrote:
> this might be unused too

Done.

https://codereview.chromium.org/745453002/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:44: if
(g_recyclable_browser_compositor.Get())
On 2014/11/20 05:38:54, tapted wrote:
> Maybe DCHECK(!*compositor) - some minor protection against an
AcquireCompositor
> not being balanced
> 
> (Or if you like this kind of stuff, a step further, with something like ...

Good point. Changed this to have a ::Create and ::Recycle method, and then have
the RWHVMac hold an ordinary scoped_ptr.

https://codereview.chromium.org/745453002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:526: new
BrowserCompositorMacPlaceholder),
On 2014/11/20 05:38:54, tapted wrote:
> nit: pull up?

Done.

[tapted, 2014-11-20 23:18:07]

lgtm - I'll try basing the ui/views and in-process changes on this today to see
how things look

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_ca_layer_tree_mac.mm (right):

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_ca_layer_tree_mac.mm:328: const {
nit: pull up

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:20: static
BrowserCompositorMac* Create();
You can enforce the lifetime dealie by returning a scoped_ptr here

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:23: static void
Recycle(BrowserCompositorMac* compositor);
and taking a scoped_ptr here (then see below)

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:48:
BrowserCompositorMac* compositor) {
nit: pull up

https://codereview.chromium.org/745453002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:599:
browser_compositor_.reset(BrowserCompositorMac::Create());
with the scoped_ptr arg tweaks, this would become

browser_compositor_ = BrowserCompositorMac::Create();

https://codereview.chromium.org/745453002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:642:
BrowserCompositorMac::Recycle(browser_compositor_.release());
and .release() -> .Pass()

[piman, 2014-11-21 00:15:48]

LGTM

[ccameron, 2014-11-21 01:32:41]

Thanks!

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.h (right):

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:20: static
BrowserCompositorMac* Create();
On 2014/11/20 23:18:07, tapted wrote:
> You can enforce the lifetime dealie by returning a scoped_ptr here

(hides head in shame). Done.

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.h:23: static void
Recycle(BrowserCompositorMac* compositor);
On 2014/11/20 23:18:07, tapted wrote:
> and taking a scoped_ptr here (then see below)

(hides head further in shame). Done.

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
File content/browser/compositor/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/40001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:48:
BrowserCompositorMac* compositor) {
On 2014/11/20 23:18:07, tapted wrote:
> nit: pull up

(doesn't quite fit anymore)

https://codereview.chromium.org/745453002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/745453002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:599:
browser_compositor_.reset(BrowserCompositorMac::Create());
On 2014/11/20 23:18:07, tapted wrote:
> with the scoped_ptr arg tweaks, this would become
> 
> browser_compositor_ = BrowserCompositorMac::Create();

Done.

https://codereview.chromium.org/745453002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_mac.mm:642:
BrowserCompositorMac::Recycle(browser_compositor_.release());
On 2014/11/20 23:18:07, tapted wrote:
> and .release() -> .Pass()

Done.

[ccameron, 2014-11-21 01:33:35]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-11-21 01:35:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/745453002/60001

[commit-bot: I haz the power, 2014-11-21 02:05:26]

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

[commit-bot: I haz the power, 2014-11-21 02:05:27]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ccameron, 2014-11-21 08:42:37]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-11-21 08:43:33]

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

[commit-bot: I haz the power, 2014-11-21 09:32:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-21 09:33:43]

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