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

ui/views/view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #define _USE_MATH_DEFINES // For VC++ to get M_PI. This has to be first.
 
 #include "ui/views/view.h"
 
 #include <algorithm>
 #include <cmath>
@@ skipping 757 matching lines @@
     return;
   // The cull_set may allow us to skip painting without canvas construction or
   // even canvas rect intersection.
   if (!cull_set.ShouldPaint(this))
     return;
 
   TRACE_EVENT1("views", "View::Paint", "class", GetClassName());
 
   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;
+  // 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;
 
-  // 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());
+    // 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));
-
-    // 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;
-    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) {
   background_.reset(b);
 }
 
 void View::SetBorder(scoped_ptr<Border> b) { border_ = b.Pass(); }
 
 ui::ThemeProvider* View::GetThemeProvider() const {
   const Widget* widget = GetWidget();
@@ skipping 619 matching lines @@
   } else {
     for (int i = 0, count = child_count(); i < count; ++i) {
       View* child = child_at(i);
       child->UpdateChildLayerBounds(
           offset + gfx::Vector2d(child->GetMirroredX(), child->y()));
     }
   }
 }
 
 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(
     const gfx::Rect& damage_rect_in_dip) {
 }
 
 void View::OnDeviceScaleFactorChanged(float device_scale_factor) {
   snap_layer_to_pixel_boundary_ =
       (device_scale_factor - std::floor(device_scale_factor)) != 0.0f;
   SnapLayerToPixelBoundary();
@@ skipping 262 matching lines @@
     SchedulePaint();
   } else if (parent_ && type == SCHEDULE_PAINT_SIZE_SAME) {
     // The compositor doesn't Draw() until something on screen changes, so
     // if our position changes but nothing is being animated on screen, then
     // tell the compositor to redraw the scene. We know layer() exists due to
     // the above if clause.
     layer()->ScheduleDraw();
   }
 }
 
-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,
                              bool update_focus_cycle,
                              bool update_tool_tip,
                              bool delete_removed_view,
                              View* new_parent) {
   DCHECK(view);
 
   const Views::iterator i(std::find(children_.begin(), children_.end(), view));
@@ skipping 691 matching lines @@
   // Message the RootView to do the drag and drop. That way if we're removed
   // the RootView can detect it and avoid calling us back.
   gfx::Point widget_location(event.location());
   ConvertPointToWidget(this, &widget_location);
   widget->RunShellDrag(this, data, widget_location, drag_operations, source);
   // WARNING: we may have been deleted.
   return true;
 }
 
 }  // namespace views