Mac: Shift more code into C++ classes from ObjC classes

Mac: Shift more code into C++ classes from ObjC classes

It is difficult to reason about the liftime of ObjC classes, especially
NSViews and CALayers. To simplify verifying their correctness, move the
bulk of the code for the ObjC classes into C++ helper classes (these
classes are already necessary to interface with owning structures).

Make the NSView sub-class BrowserCompositorViewCocoa be owned by
BrowserCompositorViewMacInternal.

Move the bulk of the work in CompositingIOSurfaceLayer to be done by
CompositingIOSurfaceLayerHelper, and mark that the ownership
relationship should be inverted (that isn't feasible at the moment
because we are mid-transition from non-delegated rendering to delegated
rendering).

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

[ccameron, 2014-07-16 21:13:08]

Sorry, this has giant diffs.

Apart from a couple of minor things (moving scoped CA disablers and
early-out-ing from drawing frames when we have no superview in browser comp
view), there's nothing functional in this change.

[miu, 2014-07-16 22:31:56]

I like this code structure much better, and it definitely clarifies/resolves the
tear-down semantics.  Mostly nits:

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

https://codereview.chromium.org/394883007/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:59:
internal_view_.reset(g_recyclable_internal_view.Get().release());
nit: internal_view_ = g_recyclable_internal_view.Get().Pass();

https://codereview.chromium.org/394883007/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:68:
g_recyclable_internal_view.Get().reset(internal_view_.release());
ditto: g_recyclable_internal_view.Get() = internal_view_.Pass();

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

https://codereview.chromium.org/394883007/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_private_mac.mm:56:
compositor_.reset();
nit: Your choice, but you could let C++ destruction order achieve this by having
the compositor_ member declared after the cocoa_view_ member.  The comment is
good, so you'd want to move that to the header file where compositor_ is
declared.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_layer_mac.h (right):

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.h:60:
content::CompositingIOSurfaceLayerClient* client_;
nit: Please make const since client_ is never modified outside of ctor init
list.

  content::CompositingIOSurfaceLayerClient* const client_;

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.h:63:
CompositingIOSurfaceLayer* layer_;
ditto: pointer should be const.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.h:92: BOOL
needs_display_;
IMHO, this state should be moved into the helper class along with the
setAsynchronous:YES call.  (see comments in .mm file)

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_layer_mac.mm (left):

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:70:
needs_display_ = NO;
I think you meant to keep this statement.  But, if you agree with my other
comments, then never mind here.

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

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:56:
did_not_draw_counter_ = 0;
Seems like this should only be set to zero when setAnynchronous:YES is called. 
Here is fine, but redundant.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:59: if
(did_not_draw_counter_ > 30)
Consider making this:

  if (did_not_draw_counter_ == 30)

...so the [layer_ setAsynchronous:NO] call is not made redundantly.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:156: if (![self
isAsynchronous])
This should be moved into the helper.  That way, all the functionality around
toggling asynchronous mode (including "needs display" tracking) is encapsulated
in the same class; and this makes it clearer exactly which events trigger the
transition.

[ccameron, 2014-07-17 02:30:02]

Thanks! Updated.

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

https://codereview.chromium.org/394883007/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:59:
internal_view_.reset(g_recyclable_internal_view.Get().release());
On 2014/07/16 22:31:55, miu wrote:
> nit: internal_view_ = g_recyclable_internal_view.Get().Pass();

Done.

https://codereview.chromium.org/394883007/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_mac.mm:68:
g_recyclable_internal_view.Get().reset(internal_view_.release());
On 2014/07/16 22:31:55, miu wrote:
> ditto: g_recyclable_internal_view.Get() = internal_view_.Pass();

Done.

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

https://codereview.chromium.org/394883007/diff/20001/content/browser/composit...
content/browser/compositor/browser_compositor_view_private_mac.mm:56:
compositor_.reset();
On 2014/07/16 22:31:55, miu wrote:
> nit: Your choice, but you could let C++ destruction order achieve this by
having
> the compositor_ member declared after the cocoa_view_ member.  The comment is
> good, so you'd want to move that to the header file where compositor_ is
> declared.

Moved this to the header variable ordering.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_layer_mac.h (right):

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.h:60:
content::CompositingIOSurfaceLayerClient* client_;
On 2014/07/16 22:31:55, miu wrote:
> nit: Please make const since client_ is never modified outside of ctor init
> list.
> 
>   content::CompositingIOSurfaceLayerClient* const client_;

Done.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.h:63:
CompositingIOSurfaceLayer* layer_;
On 2014/07/16 22:31:55, miu wrote:
> ditto: pointer should be const.

Done.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.h:92: BOOL
needs_display_;
On 2014/07/16 22:31:55, miu wrote:
> IMHO, this state should be moved into the helper class along with the
> setAsynchronous:YES call.  (see comments in .mm file)

Moved.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_layer_mac.mm (left):

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:70:
needs_display_ = NO;
On 2014/07/16 22:31:56, miu wrote:
> I think you meant to keep this statement.  But, if you agree with my other
> comments, then never mind here.

Moved it to the C++ class.

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

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:56:
did_not_draw_counter_ = 0;
On 2014/07/16 22:31:56, miu wrote:
> Seems like this should only be set to zero when setAnynchronous:YES is called.

> Here is fine, but redundant.

This is a bit easier to reason about correctness, if, yes, redundant.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:59: if
(did_not_draw_counter_ > 30)
On 2014/07/16 22:31:55, miu wrote:
> Consider making this:
> 
>   if (did_not_draw_counter_ == 30)
> 
> ...so the [layer_ setAsynchronous:NO] call is not made redundantly.

Good idea -- done.

https://codereview.chromium.org/394883007/diff/20001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.mm:156: if (![self
isAsynchronous])
On 2014/07/16 22:31:55, miu wrote:
> This should be moved into the helper.  That way, all the functionality around
> toggling asynchronous mode (including "needs display" tracking) is
encapsulated
> in the same class; and this makes it clearer exactly which events trigger the
> transition.

You're right -- moved it to the helper.

[miu, 2014-07-17 06:12:56]

lgtm

https://codereview.chromium.org/394883007/diff/80001/content/browser/renderer...
File content/browser/renderer_host/compositing_iosurface_layer_mac.h (right):

https://codereview.chromium.org/394883007/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositing_iosurface_layer_mac.h:74: BOOL
needs_display_;
s/BOOL/bool/

[ccameron, 2014-07-18 08:33:37]

Thanks!

[ccameron, 2014-07-18 08:33:41]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-07-18 08:34:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/394883007/120001

[commit-bot: I haz the power, 2014-07-18 12:08:22]

Change committed as 284063