views: Remove View::PaintCommon, merge it into View::Paint.

ui/views/view.cc

Index: ui/views/view.cc
diff --git a/ui/views/view.cc b/ui/views/view.cc
index e67e13ed193e444c9e1a3885bb221156248bfd5d..e1906c2305afb8ff1ea55fb301c3412923c24679 100644
--- a/ui/views/view.cc
+++ b/ui/views/view.cc
@@ -775,56 +775,76 @@ void View::Paint(gfx::Canvas* canvas, const CullSet& cull_set) {
 
   gfx::ScopedCanvas scoped_canvas(canvas);
 
-  // Paint this View and its children, setting the clip rect to the bounds
-  // of this View and translating the origin to the local bounds' top left
-  // point.
-  //
-  // Note that the X (or left) position we pass to ClipRectInt takes into
-  // consideration whether or not the view uses a right-to-left layout so that
-  // we paint our view in its mirrored position if need be.
-  gfx::Rect clip_rect = bounds();
-  clip_rect.Inset(clip_insets_);
-  if (parent_)
-    clip_rect.set_x(parent_->GetMirroredXForRect(clip_rect));
-  canvas->ClipRect(clip_rect);
-  if (canvas->IsClipEmpty())
-    return;
-
-  // Non-empty clip, translate the graphics such that 0,0 corresponds to where
-  // this view is located (related to its parent).
-  canvas->Translate(GetMirroredPosition().OffsetFromOrigin());
-  canvas->Transform(GetTransform());
-
-  // If we are a paint root, we need to construct our own CullSet object for
-  // propagation to our children.
-  if (IsPaintRoot()) {
-    if (!bounds_tree_)
-      bounds_tree_.reset(new BoundsTree(2, 5));
+  // If the view is backed by a layer, it should paint with itself as the origin
+  // rather than relative to its parent.
+  if (!layer()) {
+    // Set the clip rect to the bounds of this View and translating the origin
+    // to the local bounds' top left point.
+    //
+    // Note that the X (or left) position we pass to ClipRectInt takes into
+    // consideration whether or not the view uses a right-to-left layout so that
+    // we paint our view in its mirrored position if need be.
+    gfx::Rect clip_rect = bounds();
+    clip_rect.Inset(clip_insets_);
+    if (parent_)
+      clip_rect.set_x(parent_->GetMirroredXForRect(clip_rect));
+    canvas->ClipRect(clip_rect);
+    if (canvas->IsClipEmpty())
+      return;
 
-    // Recompute our bounds tree as needed.
-    UpdateRootBounds(bounds_tree_.get(), gfx::Vector2d());
+    // Non-empty clip, translate the graphics such that 0,0 corresponds to where
+    // this view is located (related to its parent).
+    canvas->Translate(GetMirroredPosition().OffsetFromOrigin());
+    canvas->Transform(GetTransform());
+  }
 
-    // Grab the clip rect from the supplied canvas to use as the query rect.
-    gfx::Rect canvas_bounds;
-    if (!canvas->GetClipBounds(&canvas_bounds)) {
-      NOTREACHED() << "Failed to get clip bounds from the canvas!";
-      return;
+  {
+    // If the View we are about to paint requested the canvas to be flipped, we
+    // should change the transform appropriately.
+    // The canvas mirroring is undone once the View is done painting so that we
+    // don't pass the canvas with the mirrored transform to Views that didn't
+    // request the canvas to be flipped.
+    gfx::ScopedCanvas scoped(canvas);
+    if (FlipCanvasOnPaintForRTLUI()) {
+      canvas->Translate(gfx::Vector2d(width(), 0));
+      canvas->Scale(-1, 1);
     }
 
-    // Now query our bounds_tree_ for a set of damaged views that intersect
-    // our canvas bounds.
-    scoped_ptr<base::hash_set<intptr_t>> damaged_views(
-        new base::hash_set<intptr_t>());
-    bounds_tree_->AppendIntersectingRecords(canvas_bounds, damaged_views.get());
-    // Construct a CullSet to wrap the damaged views set, it will delete it
-    // for us on scope exit.
-    CullSet paint_root_cull_set(damaged_views.Pass());
-    // Paint all descendents using our new cull set.
-    PaintCommon(canvas, paint_root_cull_set);
-  } else {
-    // Not a paint root, so we can proceed as normal.
-    PaintCommon(canvas, cull_set);
+    // Delegate painting the contents of the View to the virtual OnPaint method.
+    OnPaint(canvas);
+  }
+
+  // If the view is not a paint root, it should come with a CullSet already
+  // constructed for use.
+  // TODO(danakj): However, if it is a paint root but is also backed by a
+  // layer(), we are unable to construct a CullSet because the bounds() do not
+  // match the position we will be painting at and UpdateRootBounds() does the

[sky, 2015/03/25 22:55:05]

I don't understand, bounds in the cullset are relative to the paint root.

[danakj, 2015/03/25 22:57:11]

I didn't dig too deep, but when I remove the ||layer() the white line disappears
under the active window in the ash taskbar. My comment might be wrong... how far
do you want me to dig here? This was meant to preserve behaviour for now.

I think the problem is when the PaintRoot() != this but layer() is true. Then
the bounds are relative to some PaintRoot() != |this|, but the painting is done
relative to |this|.

+  // wrong thing.
+  if (!IsPaintRoot() || layer()) {
+    PaintChildren(canvas, cull_set);
+    return;
   }
+
+  // We are a paint root, so we construct our own CullSet object for propagation
+  // to our children.
+  if (!bounds_tree_)
+    bounds_tree_.reset(new BoundsTree(2, 5));
+
+  // Recompute our bounds tree as needed.
+  UpdateRootBounds(bounds_tree_.get(), gfx::Vector2d());
+
+  // Grab the clip rect from the supplied canvas to use as the query rect.
+  gfx::Rect canvas_bounds;
+  bool got_clip = canvas->GetClipBounds(&canvas_bounds);
+  DCHECK(got_clip) << "Failed to get clip bounds from the canvas!";
+
+  // Now query our bounds_tree_ for a set of damaged views that intersect
+  // our canvas bounds.
+  scoped_ptr<base::hash_set<intptr_t>> damaged_views(
+      new base::hash_set<intptr_t>);
+  bounds_tree_->AppendIntersectingRecords(canvas_bounds, damaged_views.get());
+  CullSet paint_root_cull_set(damaged_views.Pass());
+  PaintChildren(canvas, paint_root_cull_set);
 }
 
 void View::set_background(Background* b) {
@@ -1464,11 +1484,11 @@ void View::UpdateChildLayerBounds(const gfx::Vector2d& offset) {
 }
 
 void View::OnPaintLayer(gfx::Canvas* canvas) {
-  if (!layer() || !layer()->fills_bounds_opaquely())
+  if (!layer()->fills_bounds_opaquely())

[danakj, 2015/03/25 20:52:36]

I'm feeling more confident about this, so I removed the !layer() check here.
This is the LayerDelegate, it's only called by ui::Layer, so there must be such
a layer(). Bots should show us if tests expect something else.. but codesearch
suggests it won't.

[sky, 2015/03/25 22:55:05]

Agreed. If we wanted something it should be a DCHECK(layer()). But I'm fine with
removing entirely.

     canvas->DrawColor(SK_ColorBLACK, SkXfermode::kClear_Mode);
   if (!visible_)
     return;
-  PaintCommon(canvas, CullSet());
+  Paint(canvas, CullSet());
 }
 
 void View::OnDelegatedFrameDamage(
@@ -1751,25 +1771,6 @@ void View::SchedulePaintBoundsChanged(SchedulePaintType type) {
   }
 }
 
-void View::PaintCommon(gfx::Canvas* canvas, const CullSet& cull_set) {
-  {
-    // If the View we are about to paint requested the canvas to be flipped, we
-    // should change the transform appropriately.
-    // The canvas mirroring is undone once the View is done painting so that we
-    // don't pass the canvas with the mirrored transform to Views that didn't
-    // request the canvas to be flipped.
-    gfx::ScopedCanvas scoped(canvas);
-    if (FlipCanvasOnPaintForRTLUI()) {
-      canvas->Translate(gfx::Vector2d(width(), 0));
-      canvas->Scale(-1, 1);
-    }
-
-    OnPaint(canvas);
-  }
-
-  PaintChildren(canvas, cull_set);
-}
-
 // Tree operations -------------------------------------------------------------
 
 void View::DoRemoveChildView(View* view,