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

content/browser/renderer_host/compositing_iosurface_layer_mac.mm

Index: content/browser/renderer_host/compositing_iosurface_layer_mac.mm
diff --git a/content/browser/renderer_host/compositing_iosurface_layer_mac.mm b/content/browser/renderer_host/compositing_iosurface_layer_mac.mm
index 040d8f9fb8e1de21351a3ddcd044286c1bc633a4..724e56c9ff7dc292c433b46c3e3a0e763e84784d 100644
--- a/content/browser/renderer_host/compositing_iosurface_layer_mac.mm
+++ b/content/browser/renderer_host/compositing_iosurface_layer_mac.mm
@@ -17,29 +17,90 @@
 #include "ui/gfx/size_conversions.h"
 #include "ui/gl/gpu_switching_manager.h"
 
-@interface CompositingIOSurfaceLayer(Private)
-- (void)immediatelyForceDisplayAndAck;
-- (void)ackPendingFrame:(bool)success;
-- (void)timerFired;
-@end
+////////////////////////////////////////////////////////////////////////////////
+// CompositingIOSurfaceLayerHelper
 
 namespace content {
 
-// The base::DelayTimer needs a C++ class to operate on, rather than Objective C
-// class. This helper class provides a bridge between the two.
-class CompositingIOSurfaceLayerHelper {
- public:
-  CompositingIOSurfaceLayerHelper(CompositingIOSurfaceLayer* layer)
-      : layer_(layer) {}
-  void TimerFired() {
-    [layer_ timerFired];
+CompositingIOSurfaceLayerHelper::CompositingIOSurfaceLayerHelper(
+    CompositingIOSurfaceLayerClient* client,
+    CompositingIOSurfaceLayer* layer)
+        : client_(client),
+          layer_(layer),
+          has_pending_frame_(false),
+          did_not_draw_counter_(0),
+          timer_(
+              FROM_HERE,
+              base::TimeDelta::FromSeconds(1) / 6,
+              this,
+              &CompositingIOSurfaceLayerHelper::TimerFired) {}
+
+CompositingIOSurfaceLayerHelper::~CompositingIOSurfaceLayerHelper() {
+  // Any acks that were waiting on this layer to draw will not occur, so ack
+  // them now to prevent blocking the renderer.
+  AckPendingFrame(true);
+}
+
+void CompositingIOSurfaceLayerHelper::GotNewFrame() {
+  has_pending_frame_ = true;
+  timer_.Reset();
+  // A trace value of 2 indicates that there is a pending swap ack. See
+  // canDrawInCGLContext for other value meanings.
+  TRACE_COUNTER_ID1("browser", "PendingSwapAck", this, 2);
+}
+
+void CompositingIOSurfaceLayerHelper::CanDrawCalled(bool needs_display) {
+  // If we return NO 30 times in a row, switch to being synchronous to avoid
+  // burning CPU cycles on this callback.
+  if (needs_display) {
+    did_not_draw_counter_ = 0;

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

Seems like this should only be set to zero when setAnynchronous:YES is called. 
Here is fine, but redundant.

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

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

+  } else {
+    did_not_draw_counter_ += 1;
+    if (did_not_draw_counter_ > 30)

[miu, 2014/07/16 22:31:55]

Consider making this:

  if (did_not_draw_counter_ == 30)

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

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

Good idea -- done.

+      [layer_ setAsynchronous:NO];
   }
- private:
-  CompositingIOSurfaceLayer* layer_;
-};
+
+  // Add an instantaneous blip to the PendingSwapAck state to indicate
+  // that CoreAnimation asked if a frame is ready. A blip up to to 3 (usually
+  // from 2, indicating that a swap ack is pending) indicates that we
+  // requested a draw. A blip up to 1 (usually from 0, indicating there is no
+  // pending swap ack) indicates that we did not request a draw. This would
+  // be more natural to do with a tracing pseudo-thread
+  // http://crbug.com/366300
+  TRACE_COUNTER_ID1("browser", "PendingSwapAck", this, needs_display ? 3 : 1);
+  TRACE_COUNTER_ID1("browser", "PendingSwapAck", this,
+                    has_pending_frame_ ? 2 : 0);
+}
+
+void CompositingIOSurfaceLayerHelper::AckPendingFrame(bool success) {
+  if (!has_pending_frame_)
+    return;
+  has_pending_frame_ = false;
+  client_->AcceleratedLayerDidDrawFrame(success);
+  // A trace value of 0 indicates that there is no longer a pending swap ack.
+  TRACE_COUNTER_ID1("browser", "PendingSwapAck", this, 0);
+}
+
+void CompositingIOSurfaceLayerHelper::ImmediatelyForceDisplayAndAck() {
+  [layer_ setNeedsDisplay];
+  [layer_ displayIfNeeded];
+
+  // Calls to setNeedsDisplay can sometimes be ignored, especially if issued
+  // rapidly (e.g, with vsync off). This is unacceptable because the failure
+  // to ack a single frame will hang the renderer. Ensure that the renderer
+  // not be blocked by lying and claiming that we drew the frame.
+  AckPendingFrame(true);
+}
+
+void CompositingIOSurfaceLayerHelper::TimerFired() {
+  ImmediatelyForceDisplayAndAck();
+}
 
 }  // namespace content
 
+////////////////////////////////////////////////////////////////////////////////
+// CompositingIOSurfaceLayer
+
 @implementation CompositingIOSurfaceLayer
 
 - (content::CompositingIOSurfaceMac*)iosurface {
@@ -55,21 +116,12 @@ class CompositingIOSurfaceLayerHelper {
         withScaleFactor:(float)scale_factor
              withClient:(content::CompositingIOSurfaceLayerClient*)client {
   if (self = [super init]) {
-    iosurface_ = iosurface;
-    client_ = client;
-    helper_.reset(new content::CompositingIOSurfaceLayerHelper(self));
-    timer_.reset(new base::DelayTimer<content::CompositingIOSurfaceLayerHelper>(
-        FROM_HERE,
-        base::TimeDelta::FromSeconds(1) / 6,
-        helper_.get(),
-        &content::CompositingIOSurfaceLayerHelper::TimerFired));
+    helper_.reset(new content::CompositingIOSurfaceLayerHelper(client, self));
 
+    iosurface_ = iosurface;
     context_ = content::CompositingIOSurfaceContext::Get(
         content::CompositingIOSurfaceContext::kCALayerContextWindowNumber);
     DCHECK(context_);
-    needs_display_ = NO;

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

I think you meant to keep this statement.  But, if you agree with my other
comments, then never mind here.

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

Moved it to the C++ class.

-    has_pending_frame_ = NO;
-    did_not_draw_counter_ = 0;
 
     [self setBackgroundColor:CGColorGetConstantColor(kCGColorWhite)];
     [self setAnchorPoint:CGPointMake(0, 0)];
@@ -83,25 +135,22 @@ class CompositingIOSurfaceLayerHelper {
   return self;
 }
 
+- (void)dealloc {
+  DCHECK(!helper_);
+  [super dealloc];
+}
+
 - (void)resetClient {
-  // Any acks that were waiting on this layer to draw will not occur, so ack
-  // them now to prevent blocking the renderer.
-  [self ackPendingFrame:true];
-  client_ = NULL;
+  helper_.reset();
 }
 
 - (void)gotNewFrame {
-  has_pending_frame_ = YES;
-  timer_->Reset();
-
-  // A trace value of 2 indicates that there is a pending swap ack. See
-  // canDrawInCGLContext for other value meanings.
-  TRACE_COUNTER_ID1("browser", "PendingSwapAck", self, 2);
+  helper_->GotNewFrame();
 
   if (context_ && context_->is_vsync_disabled()) {
     // If vsync is disabled, draw immediately and don't bother trying to use
     // the isAsynchronous property to ensure smooth animation.
-    [self immediatelyForceDisplayAndAck];
+    helper_->ImmediatelyForceDisplayAndAck();
   } else {
     needs_display_ = YES;
     if (![self isAsynchronous])

[miu, 2014/07/16 22:31:55]

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]

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

@@ -109,34 +158,6 @@ class CompositingIOSurfaceLayerHelper {
   }
 }
 
-// Private methods:
-
-- (void)immediatelyForceDisplayAndAck {
-  [self setNeedsDisplay];
-  [self displayIfNeeded];
-
-  // Calls to setNeedsDisplay can sometimes be ignored, especially if issued
-  // rapidly (e.g, with vsync off). This is unacceptable because the failure
-  // to ack a single frame will hang the renderer. Ensure that the renderer
-  // not be blocked by lying and claiming that we drew the frame.
-  [self ackPendingFrame:true];
-}
-
-- (void)ackPendingFrame:(bool)success {
-  if (!has_pending_frame_)
-    return;
-
-  TRACE_COUNTER_ID1("browser", "PendingSwapAck", self, 0);
-  has_pending_frame_ = NO;
-  if (client_)
-    client_->AcceleratedLayerDidDrawFrame(success);
-}
-
-- (void)timerFired {
-  if (has_pending_frame_)
-    [self immediatelyForceDisplayAndAck];
-}
-
 // The remaining methods implement the CAOpenGLLayer interface.
 
 - (CGLPixelFormatObj)copyCGLPixelFormatForDisplayMask:(uint32_t)mask {
@@ -160,27 +181,8 @@ class CompositingIOSurfaceLayerHelper {
                 pixelFormat:(CGLPixelFormatObj)pixelFormat
                forLayerTime:(CFTimeInterval)timeInterval
                 displayTime:(const CVTimeStamp*)timeStamp {
-  // Add an instantaneous blip to the PendingSwapAck state to indicate
-  // that CoreAnimation asked if a frame is ready. A blip up to to 3 (usually
-  // from 2, indicating that a swap ack is pending) indicates that we requested
-  // a draw. A blip up to 1 (usually from 0, indicating there is no pending swap
-  // ack) indicates that we did not request a draw. This would be more natural
-  // to do with a tracing pseudo-thread
-  // http://crbug.com/366300
-  TRACE_COUNTER_ID1("browser", "PendingSwapAck", self, needs_display_ ? 3 : 1);
-  TRACE_COUNTER_ID1("browser", "PendingSwapAck", self,
-                    has_pending_frame_ ? 2 : 0);
-
-  // If we return NO 30 times in a row, switch to being synchronous to avoid
-  // burning CPU cycles on this callback.
-  if (needs_display_) {
-    did_not_draw_counter_ = 0;
-  } else {
-    did_not_draw_counter_ += 1;
-    if (did_not_draw_counter_ > 30)
-      [self setAsynchronous:NO];
-  }
-
+  if (helper_)
+    helper_->CanDrawCalled(needs_display_);
   return needs_display_;
 }
 
@@ -211,7 +213,8 @@ class CompositingIOSurfaceLayerHelper {
   bool draw_succeeded = iosurface_->DrawIOSurface(
       context_, window_rect, window_scale_factor);
 
-  [self ackPendingFrame:draw_succeeded];
+  if (helper_)
+    helper_->AckPendingFrame(draw_succeeded);
   needs_display_ = NO;
 
   [super drawInCGLContext:glContext