Canvas2DLayerBridge: cleanups

This CL address multiple cleanups I found during
crrev.com/c/1452720:

- Removes unused member variables.
- Makes IsValid() non-const: const for no reason;
 but makes some member vars const.
- Prefers early return: fights arrow code.
- Removes unnecessary {}.
- Collapses if()+if() conditions.
- Removes unnecessary enum and uses constexpr next
 to the only use site: makes it clearer.

No new functionality intended.

Test:webkit_unit_tests --gtest_filter=Canvas2DLayerBridgeTest*:CanvasRenderingContext2DTest*

Bug: 927103
Change-Id: Ic2e6e26e3792d186a5d3f7671f0c7021315478a4
Reviewed-on: https://chromium-review.googlesource.com/c/1452618
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#629611}

third_party/blink/renderer/platform/graphics/canvas_2d_layer_bridge.cc

@@ -50,21 +50,12 @@
 #include "third_party/skia/include/core/SkData.h"
 #include "third_party/skia/include/core/SkSurface.h"
 
-namespace {
-enum {
-  InvalidMailboxIndex = -1,
-  MaxCanvasAnimationBacklog = 2,  // Make sure the the GPU is never more than
-                                  // two animation frames behind.
-};
-}  // namespace
-
 namespace blink {
 
 Canvas2DLayerBridge::Canvas2DLayerBridge(const IntSize& size,
                                          AccelerationMode acceleration_mode,
                                          const CanvasColorParams& color_params)
     : logger_(std::make_unique<Logger>()),
-      bytes_allocated_(0),
       have_recorded_draw_commands_(false),
       is_hidden_(false),
       is_deferral_enabled_(true),
@@ -86,33 +77,30 @@ Canvas2DLayerBridge::Canvas2DLayerBridge(const IntSize& size,
     recorder_->getRecordingCanvas()->clear(
         color_params_.GetOpacityMode() == kOpaque ? SK_ColorBLACK
                                                   : SK_ColorTRANSPARENT);
-    DidDraw(FloatRect(FloatPoint(0, 0), FloatSize(size_)));
+    DidDraw(FloatRect(0.f, 0.f, size_.Width(), size_.Height()));
   }
 }
 
 Canvas2DLayerBridge::~Canvas2DLayerBridge() {
   UMA_HISTOGRAM_BOOLEAN("Blink.Canvas.2DLayerBridgeIsDeferred",
                         is_deferral_enabled_);
-
   if (IsHibernating())
     logger_->ReportHibernationEvent(kHibernationEndedWithTeardown);
   ResetResourceProvider();
 
-  if (layer_ && acceleration_mode_ != kDisableAcceleration) {
+  if (!layer_)
+    return;
+
+  if (acceleration_mode_ != kDisableAcceleration) {
     GraphicsLayer::UnregisterContentsLayer(layer_.get());
     layer_->ClearTexture();
-    // Orphaning the layer is required to trigger the recration of a new layer
+    // Orphaning the layer is required to trigger the recreation of a new layer
     // in the case where destruction is caused by a canvas resize. Test:
     // virtual/gpu/fast/canvas/canvas-resize-after-paint-without-layout.html
     layer_->RemoveFromParent();
   }
-
-  DCHECK(!bytes_allocated_);
-
-  if (layer_) {
   layer_->ClearClient();
   layer_ = nullptr;
-  }
 }
 
 void Canvas2DLayerBridge::StartRecording() {
@@ -124,21 +112,15 @@ void Canvas2DLayerBridge::StartRecording() {
   // and clip.
   canvas->save();
 
-  if (resource_host_) {
+  if (resource_host_)
     resource_host_->RestoreCanvasMatrixClipStack(canvas);
-  }
 
   recording_pixel_count_ = 0;
 }
 
-void Canvas2DLayerBridge::SetLoggerForTesting(std::unique_ptr<Logger> logger) {
-  logger_ = std::move(logger);
-}
-
 void Canvas2DLayerBridge::ResetResourceProvider() {
-  if (resource_host_) {
+  if (resource_host_)
     resource_host_->ReplaceResourceProvider(nullptr);
-  }
 }
 
 bool Canvas2DLayerBridge::ShouldAccelerate(AccelerationHint hint) const {
@@ -162,8 +144,9 @@ bool Canvas2DLayerBridge::ShouldAccelerate(AccelerationHint hint) const {
   if (accelerate && (!context_provider_wrapper ||
                      context_provider_wrapper->ContextProvider()
                              ->ContextGL()
-                             ->GetGraphicsResetStatusKHR() != GL_NO_ERROR))
+                             ->GetGraphicsResetStatusKHR() != GL_NO_ERROR)) {
     accelerate = false;
+  }
   return accelerate;
 }
 
@@ -304,12 +287,14 @@ CanvasResourceProvider* Canvas2DLayerBridge::GetOrCreateResourceProvider(
   AccelerationHint adjusted_hint =
       want_acceleration ? kPreferAcceleration : kPreferNoAcceleration;
 
-  // We call Impl directly here, to allow HTMLCanvasElement to call us
-  // in GetOrCreateCanvasResourceProvider.
+  // We call GetOrCreateCanvasResourceProviderImpl directly here to prevent a
+  // circular callstack from HTMLCanvasElement.
   resource_provider =
       resource_host_->GetOrCreateCanvasResourceProviderImpl(adjusted_hint);
+  if (!resource_provider)
+    return nullptr;
 
-  if (resource_provider && IsAccelerated() && !layer_) {
+  if (IsAccelerated() && !layer_) {
     layer_ = cc::TextureLayer::CreateForMailbox(this);
     layer_->SetIsDrawable(true);
     layer_->SetContentsOpaque(ColorParams().GetOpacityMode() == kOpaque);
@@ -319,7 +304,9 @@ CanvasResourceProvider* Canvas2DLayerBridge::GetOrCreateResourceProvider(
     GraphicsLayer::RegisterContentsLayer(layer_.get());
   }
 
-  if (resource_provider && IsHibernating()) {
+  if (!IsHibernating())
+    return resource_provider;
+
   if (resource_provider->IsAccelerated()) {
     logger_->ReportHibernationEvent(kHibernationEndedNormally);
   } else {
@@ -341,27 +328,22 @@ CanvasResourceProvider* Canvas2DLayerBridge::GetOrCreateResourceProvider(
   hibernation_image_.reset();
 
   if (resource_host_) {
-      if (!is_deferral_enabled_) {
-        resource_host_->RestoreCanvasMatrixClipStack(
-            resource_provider->Canvas());
-      }
+    if (!is_deferral_enabled_)
+      resource_host_->RestoreCanvasMatrixClipStack(resource_provider->Canvas());
 
     // shouldBeDirectComposited() may have changed.
     resource_host_->SetNeedsCompositingUpdate();
   }
-  }
-
   return resource_provider;
 }
 
 cc::PaintCanvas* Canvas2DLayerBridge::Canvas() {
   DCHECK(resource_host_);
-  if (!is_deferral_enabled_) {
+  if (is_deferral_enabled_)
+    return recorder_->getRecordingCanvas();
   if (GetOrCreateResourceProvider())
     return ResourceProvider()->Canvas();
   return nullptr;
-  }
-  return recorder_->getRecordingCanvas();
 }
 
 void Canvas2DLayerBridge::DisableDeferral(DisableDeferralReason reason) {
@@ -450,9 +432,8 @@ void Canvas2DLayerBridge::SetIsHidden(bool hidden) {
       resource_host_->ReplaceResourceProvider(std::move(old_resource_provider));
     }
   }
-  if (!IsHidden() && IsHibernating()) {
+  if (!IsHidden() && IsHibernating())
     GetOrCreateResourceProvider();  // Rude awakening
-  }
 }
 
 void Canvas2DLayerBridge::DrawFullImage(const cc::PaintImage& image) {
@@ -466,6 +447,7 @@ bool Canvas2DLayerBridge::WritePixels(const SkImageInfo& orig_info,
                                       int y) {
   if (!GetOrCreateResourceProvider())
     return false;
+
   if (x <= 0 && y <= 0 && x + orig_info.width() >= size_.Width() &&
       y + orig_info.height() >= size_.Height()) {
     SkipQueuedDrawCommands();
@@ -477,7 +459,6 @@ bool Canvas2DLayerBridge::WritePixels(const SkImageInfo& orig_info,
 
   ResourceProvider()->WritePixels(orig_info, pixels, row_bytes, x, y);
   DidDraw(FloatRect(x, y, orig_info.width(), orig_info.height()));
-
   return true;
 }
 
@@ -488,14 +469,14 @@ void Canvas2DLayerBridge::SkipQueuedDrawCommands() {
     have_recorded_draw_commands_ = false;
   }
 
-  if (is_deferral_enabled_) {
-    if (rate_limiter_)
+  if (is_deferral_enabled_ && rate_limiter_)
     rate_limiter_->Reset();
-  }
 }
 
 void Canvas2DLayerBridge::FlushRecording() {
-  if (have_recorded_draw_commands_ && GetOrCreateResourceProvider()) {
+  if (!have_recorded_draw_commands_ || !GetOrCreateResourceProvider())
+    return;
+
   TRACE_EVENT0("cc", "Canvas2DLayerBridge::flushRecording");
 
   cc::PaintCanvas* canvas = ResourceProvider()->Canvas();
@@ -505,7 +486,7 @@ void Canvas2DLayerBridge::FlushRecording() {
   }
 
   // Rastering the recording would have locked images, since we've flushed
-    // all recorded ops, we should relase all locked images as well.
+  // all recorded ops, we should release all locked images as well.
   // A new null check on the resource provider is necessary just in case
   // the playback crashed the context.
   if (GetOrCreateResourceProvider())
@@ -514,11 +495,10 @@ void Canvas2DLayerBridge::FlushRecording() {
   if (is_deferral_enabled_)
     StartRecording();
   have_recorded_draw_commands_ = false;
-  }
 }
 
-bool Canvas2DLayerBridge::IsValid() const {
-  return const_cast<Canvas2DLayerBridge*>(this)->CheckResourceProviderValid();
+bool Canvas2DLayerBridge::IsValid() {
+  return CheckResourceProviderValid();
 }
 
 bool Canvas2DLayerBridge::CheckResourceProviderValid() {
@@ -582,12 +562,10 @@ bool Canvas2DLayerBridge::PrepareTransferableResource(
   DCHECK(layer_);  // This explodes if FinalizeFrame() was not called.
 
   frames_since_last_commit_ = 0;
-  if (rate_limiter_) {
+  if (rate_limiter_)
     rate_limiter_->Reset();
-  }
 
-  // if hibernating but not hidden, we want to wake up from
-  // hibernation
+  // If hibernating but not hidden, we want to wake up from hibernation.
   if ((IsHibernating() || software_rendering_while_hidden_) && IsHidden())
     return false;
 
@@ -620,7 +598,9 @@ cc::Layer* Canvas2DLayerBridge::Layer() {
 void Canvas2DLayerBridge::DidDraw(const FloatRect& rect) {
   if (snapshot_state_ == kDidAcquireSnapshot)
     snapshot_state_ = kDrawnToAfterSnapshot;
-  if (is_deferral_enabled_) {
+  if (!is_deferral_enabled_)
+    return;
+
   have_recorded_draw_commands_ = true;
   IntRect pixel_bounds = EnclosingIntRect(rect);
   base::CheckedNumeric<int> pixel_bounds_size = pixel_bounds.Width();
@@ -637,10 +617,8 @@ void Canvas2DLayerBridge::DidDraw(const FloatRect& rect) {
     DisableDeferral(kDisableDeferralReasonExpensiveOverdrawHeuristic);
     return;
   }
-    if (recording_pixel_count_.ValueOrDie() >= threshold_size.ValueOrDie()) {
+  if (recording_pixel_count_.ValueOrDie() >= threshold_size.ValueOrDie())
     DisableDeferral(kDisableDeferralReasonExpensiveOverdrawHeuristic);
-    }
-  }
 }
 
 void Canvas2DLayerBridge::FinalizeFrame() {
@@ -652,20 +630,18 @@ void Canvas2DLayerBridge::FinalizeFrame() {
     return;
 
   ++frames_since_last_commit_;
-
   if (frames_since_last_commit_ >= 2) {
     ResourceProvider()->FlushSkia();
-    if (IsAccelerated()) {
-      if (!rate_limiter_) {
+    if (IsAccelerated() && !rate_limiter_) {
+      // Make sure the GPU is never more than two animation frames behind.
+      constexpr unsigned kMaxCanvasAnimationBacklog = 2;
       rate_limiter_ =
-            SharedContextRateLimiter::Create(MaxCanvasAnimationBacklog);
-      }
+          SharedContextRateLimiter::Create(kMaxCanvasAnimationBacklog);
     }
   }
 
-  if (rate_limiter_) {
+  if (rate_limiter_)
     rate_limiter_->Tick();
-  }
 }
 
 void Canvas2DLayerBridge::DoPaintInvalidation(const FloatRect& dirty_rect) {

third_party/blink/renderer/platform/graphics/canvas_2d_layer_bridge.h

@@ -27,6 +27,7 @@
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_CANVAS_2D_LAYER_BRIDGE_H_
 
 #include <memory>
+#include <utility>
 
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
@@ -109,7 +110,7 @@ class PLATFORM_EXPORT Canvas2DLayerBridge : public cc::TextureLayerClient {
   virtual bool IsAccelerated() const;
 
   cc::PaintCanvas* Canvas();
-  bool IsValid() const;
+  bool IsValid();
   bool WritePixels(const SkImageInfo&,
                    const void* pixels,
                    size_t row_bytes,
@@ -158,13 +159,19 @@ class PLATFORM_EXPORT Canvas2DLayerBridge : public cc::TextureLayerClient {
     virtual ~Logger() = default;
   };
 
-  void SetLoggerForTesting(std::unique_ptr<Logger>);
+  void SetLoggerForTesting(std::unique_ptr<Logger> logger) {
+    logger_ = std::move(logger);
+  }
   CanvasResourceProvider* GetOrCreateResourceProvider(
       AccelerationHint = kPreferAcceleration);
   CanvasResourceProvider* ResourceProvider() const;
   void FlushRecording();
 
  private:
+  friend class Canvas2DLayerBridgeTest;
+  friend class CanvasRenderingContext2DTest;
+  friend class HTMLCanvasPainterTestForCAP;
+
   bool IsHidden() { return is_hidden_; }
   bool CheckResourceProviderValid();
   void ResetResourceProvider();
@@ -179,24 +186,19 @@ class PLATFORM_EXPORT Canvas2DLayerBridge : public cc::TextureLayerClient {
   scoped_refptr<cc::TextureLayer> layer_;
   std::unique_ptr<SharedContextRateLimiter> rate_limiter_;
   std::unique_ptr<Logger> logger_;
-  int msaa_sample_count_;
   int frames_since_last_commit_ = 0;
-  size_t bytes_allocated_;
   bool have_recorded_draw_commands_;
   bool is_hidden_;
+  // See the implementation of DisableDeferral() for more information.
   bool is_deferral_enabled_;
   bool software_rendering_while_hidden_;
   bool hibernation_scheduled_ = false;
   bool dont_use_idle_scheduling_for_testing_ = false;
   bool context_lost_ = false;
 
-  friend class Canvas2DLayerBridgeTest;
-  friend class CanvasRenderingContext2DTest;
-  friend class HTMLCanvasPainterTestForCAP;
-
-  AccelerationMode acceleration_mode_;
-  CanvasColorParams color_params_;
-  IntSize size_;
+  const AccelerationMode acceleration_mode_;
+  const CanvasColorParams color_params_;
+  const IntSize size_;
   base::CheckedNumeric<int> recording_pixel_count_;
 
   enum SnapshotState {