cc: Move draw params from SetExternalDrawConstraints to OnDraw

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index 9524e41ece9f68a796c393a651aa28dfb5763dc3..78ce57cb728f36082776cb9346d04dde34a4f202 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -9,6 +9,7 @@
 #include <map>
 #include <set>
 
+#include "base/auto_reset.h"
 #include "base/basictypes.h"
 #include "base/containers/hash_tables.h"
 #include "base/containers/small_map.h"
@@ -1257,6 +1258,17 @@ size_t LayerTreeHostImpl::SourceAnimationFrameNumberForTesting() const {
   return fps_counter_->current_frame_number();
 }
 
+void LayerTreeHostImpl::SetExternalViewportForTesting(
+    const gfx::Rect& external_viewport) {
+  external_viewport_ = external_viewport;
+}
+
+void LayerTreeHostImpl::SetResourcelessSoftwareDrawForTesting(
+    bool resourceless_software_draw) {
+  resourceless_software_draw_ = resourceless_software_draw;
+  active_tree_->set_needs_update_draw_properties();
+}
+
 void LayerTreeHostImpl::UpdateTileManagerMemoryPolicy(
     const ManagedMemoryPolicy& policy) {
   if (!resource_pool_)
@@ -1453,60 +1465,32 @@ void LayerTreeHostImpl::SetManagedMemoryPolicy(
     client_->SetNeedsCommitOnImplThread();
 }
 
-void LayerTreeHostImpl::SetExternalDrawConstraints(
-    const gfx::Transform& transform,
-    const gfx::Rect& viewport,
-    const gfx::Rect& clip,
-    const gfx::Rect& viewport_rect_for_tile_priority,
-    const gfx::Transform& transform_for_tile_priority,
-    bool resourceless_software_draw) {
+void LayerTreeHostImpl::SetExternalTilePriorityConstraints(
+    const gfx::Rect& viewport_rect,
+    const gfx::Transform& transform) {

[brianderson, 2015/12/14 20:51:26]

Would it make sense to DCHECK(using_synchronous_renderer_compositor) here? (And
in LayerTreeHostImpl::OnDraw.)

[boliu, 2015/12/16 19:06:41]

This works in production, and so probably good idea long term. But this is
causing a bunch of tests to fail. In theory these don't require
using_synchronous_renderer_compositor, since a bunch of tests has its own
LTHIClient implementations. This DCHECK already exists for
Scheduler::OnDrawForOutputSurface so at least tests using real scheduler is ok.
This patch is already pretty big as it is, so didn't change this.

   gfx::Rect viewport_rect_for_tile_priority_in_view_space;
-  if (!resourceless_software_draw) {
-    gfx::Transform screen_to_view(gfx::Transform::kSkipInitialization);
-    if (transform_for_tile_priority.GetInverse(&screen_to_view)) {
-      // Convert from screen space to view space.
-      viewport_rect_for_tile_priority_in_view_space =
-          MathUtil::ProjectEnclosingClippedRect(
-              screen_to_view, viewport_rect_for_tile_priority);
-    }
+  gfx::Transform screen_to_view(gfx::Transform::kSkipInitialization);
+  if (transform.GetInverse(&screen_to_view)) {
+    // Convert from screen space to view space.
+    viewport_rect_for_tile_priority_in_view_space =
+        MathUtil::ProjectEnclosingClippedRect(screen_to_view, viewport_rect);
   }
 
-  const bool transform_changed = external_transform_ != transform;
-  const bool viewport_changed = external_viewport_ != viewport;
-  const bool clip_changed = external_clip_ != clip;
-  const bool resourceless_software_draw_changed =
-      resourceless_software_draw_ != resourceless_software_draw;
   const bool tile_priority_params_changed =
       viewport_rect_for_tile_priority_ !=
       viewport_rect_for_tile_priority_in_view_space;
 
-  // UpdateDrawProperties does not depend on clip.
-  if (transform_changed || viewport_changed ||
-      resourceless_software_draw_changed || tile_priority_params_changed) {
-    active_tree_->set_needs_update_draw_properties();
-  }
-
-  external_transform_ = transform;
-  external_viewport_ = viewport;
-  external_clip_ = clip;
   viewport_rect_for_tile_priority_ =
       viewport_rect_for_tile_priority_in_view_space;
-  resourceless_software_draw_ = resourceless_software_draw;
 
-  // When not toggling resourceless software draw, need to set redraw for
-  // all changes to draw parameters. Damage will be set externally by Android
-  // WebView for resourceless software draw toggles, so ignored here.
-  const bool draw_params_changed = transform_changed || viewport_changed ||
-                                   clip_changed || tile_priority_params_changed;
-  if (!resourceless_software_draw_changed && draw_params_changed) {
+  if (tile_priority_params_changed) {
+    active_tree_->set_needs_update_draw_properties();
+
+    // Compositor, not OutputSurface, is responsible for setting damage and
+    // triggering redraw for constraint changes.
     SetFullRootLayerDamage();
     SetNeedsRedraw();
   }
-
-  if (resourceless_software_draw_changed) {
-    client_->OnResourcelessSoftareDrawStateChanged(resourceless_software_draw);
-    client_->OnCanDrawStateChanged(CanDraw());
-  }
 }
 
 void LayerTreeHostImpl::SetNeedsRedrawRect(const gfx::Rect& damage_rect) {
@@ -1556,8 +1540,53 @@ void LayerTreeHostImpl::ReclaimResources(const CompositorFrameAck* ack) {
   }
 }
 
-void LayerTreeHostImpl::OnDraw() {
-  client_->OnDrawForOutputSurface();
+void LayerTreeHostImpl::OnDraw(const gfx::Transform& transform,
+                               const gfx::Rect& viewport,
+                               const gfx::Rect& clip,
+                               bool resourceless_software_draw) {
+  DCHECK(!resourceless_software_draw_);
+  const bool transform_changed = external_transform_ != transform;
+  const bool viewport_changed = external_viewport_ != viewport;
+  const bool clip_changed = external_clip_ != clip;
+
+  external_transform_ = transform;
+  external_viewport_ = viewport;
+  external_clip_ = clip;
+
+  {
+    base::AutoReset<bool> resourceless_software_draw_reset(
+        &resourceless_software_draw_, resourceless_software_draw);
+
+    // For resourceless software draw, always set full damage to ensure they
+    // always swap. Otherwise, need to set redraw for any changes to draw
+    // parameters.
+    const bool draw_params_changed =
+        transform_changed || viewport_changed || clip_changed;
+    if (resourceless_software_draw_ || draw_params_changed) {
+      SetFullRootLayerDamage();
+      SetNeedsRedraw();
+    }
+
+    // UpdateDrawProperties does not depend on clip.
+    if (transform_changed || viewport_changed || resourceless_software_draw_) {
+      active_tree_->set_needs_update_draw_properties();
+    }
+
+    if (resourceless_software_draw) {
+      client_->OnCanDrawStateChanged(CanDraw());
+    }
+
+    client_->OnDrawForOutputSurface(resourceless_software_draw_);
+  }
+
+  if (resourceless_software_draw) {

[brianderson, 2015/12/14 20:51:26]

I think it would be easier to understand the flow of this method if there was a
higher level if(resourceless_software_draw) {} else {} block. wdyt?

[boliu, 2015/12/16 19:06:41]

Tried this, result below in this comment. Really dislike duplicating a few
lines, so didn't go with this.

void LayerTreeHostImpl::OnDraw(const gfx::Transform& transform,
                               const gfx::Rect& viewport,
                               const gfx::Rect& clip,
                               bool resourceless_software_draw) {
  DCHECK(!resourceless_software_draw_);
  const bool transform_changed = external_transform_ != transform;
  const bool viewport_changed = external_viewport_ != viewport;
  const bool clip_changed = external_clip_ != clip;

  external_transform_ = transform;
  external_viewport_ = viewport;
  external_clip_ = clip;

  if (!resourceless_software_draw) {
    // Set damage viewport if draw parameters changed.
    if (transform_changed || viewport_changed || clip_changed) {
      SetFullRootLayerDamage();
      SetNeedsRedraw();
    }

    // UpdateDrawProperties does not depend on clip.
    if (transform_changed || viewport_changed) {
      active_tree_->set_needs_update_draw_properties();
    }

    client_->OnDrawForOutputSurface(resourceless_software_draw_);
    return;
  }

  {
    base::AutoReset<bool> resourceless_software_draw_reset(
        &resourceless_software_draw_, resourceless_software_draw);

    // For resourceless software draw, always set full damage to ensure draw
    // always swap.
    SetFullRootLayerDamage();
    SetNeedsRedraw();
    active_tree_->set_needs_update_draw_properties();
    client_->OnCanDrawStateChanged(CanDraw());

    client_->OnDrawForOutputSurface(resourceless_software_draw_);
  }

  active_tree_->set_needs_update_draw_properties();
  client_->OnCanDrawStateChanged(CanDraw());
  // This draw may have reset all damage, which would lead to subsequent
  // incorrect hardware draw, so explicitly set damage for next hardware
  // draw as well.
  SetFullRootLayerDamage();
}

+    active_tree_->set_needs_update_draw_properties();
+    client_->OnCanDrawStateChanged(CanDraw());
+    // This draw may have reset all damage, which would lead to subsequent
+    // incorrect hardware draw, so explicitly set damage for next hardware
+    // draw as well.
+    SetFullRootLayerDamage();
+  }
 }
 
 void LayerTreeHostImpl::OnCanDrawStateChangedForTree() {