LayoutMenuList::ControlClipRect may depend on size of children

third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp

Index: third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
index 206782a6454c44ec37bdf59615489fea93970959..2a5b8cb4a5a944b726e609cce8f75923b3db6e19 100644
--- a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
@@ -790,7 +790,7 @@ void PaintPropertyTreeBuilder::UpdateScrollbarPaintOffset(
   }
 }
 
-static bool NeedsOverflowScroll(const LayoutObject& object) {
+static bool NeedsOverflowClip(const LayoutObject& object) {
   return object.IsBox() && ToLayoutBox(object).ShouldClipOverflow();
 }
 
@@ -799,11 +799,9 @@ void PaintPropertyTreeBuilder::UpdateOverflowClip(
     PaintPropertyTreeBuilderFragmentContext& context,
     bool& force_subtree_update) {
   if (object.NeedsPaintPropertyUpdate() || force_subtree_update) {
-    if (NeedsOverflowScroll(object)) {
+    if (NeedsOverflowClip(object)) {
       const LayoutBox& box = ToLayoutBox(object);
-      LayoutRect clip_rect;
-      clip_rect =
-          LayoutRect(box.OverflowClipRect(context.current.paint_offset));
+      LayoutRect clip_rect = box.OverflowClipRect(context.current.paint_offset);
 
       auto& properties = *object.GetMutableForPainting().PaintProperties();
       const auto* current_clip = context.current.clip;
@@ -1154,27 +1152,43 @@ void PaintPropertyTreeBuilder::UpdateForObjectLocationAndSize(
   if (!object.IsBox())
     return;
   const LayoutBox& box = ToLayoutBox(object);
-  if (box.Size() == box.PreviousSize())
-    return;
+  bool needs_paint_property_update = false;
+
+  // The overflow clip rect may change even if this box's size didn't change,
+  // e.g. LayoutMenuList::ControlClipRect() may depend on size of children.
+  // Check for change of overflow clip rect to ensure paint property update.

[pdr., 2017/05/12 23:20:30]

In other areas of code we mark the object as needing a paint property update
when the control clip changes, rather than detecting the change here. Is that
possible in this case?

[Xianzhu, 2017/05/13 00:16:02]

ControlClipRect() is calculated from other properties and it's hard to detect
its change immediately after the calculated value changes. For example,
LayoutMenuList::ControlClipRect() depends on child geometries. For that to work
we would have to detect child geometry changes, which would need complex and
tricky logic.

We used to call SetNeedsPaintPropertyUpdate() from LayoutBox::SizeChanged() and
it just happened to work because LayoutMenuList::SizeChanged() is called even
when it size doesn't change. That method also caused unnecessary paint property
updates on unchanged layout.

For now for paint property updates caused by geometry changes, we rely on
MayNeedsPaintInvalidation() to reach the object during PrePaint, and call this
function regardless of NeedsPaintPropertyUpdate() and
SetNeedsPaintPropertyUpdate() when needed. 

Previously overflow clip changes were detected by size change, but for the bug
case, overflow clip changes without size change. This CL moves overflow clip
case out of the size change condition.

[pdr., 2017/05/13 02:05:09]

Is this just needed for ControlClipRect or is it needed for all OverflowClips?

It looks like LayoutMenuList manages it's children directly so it may not be
that hard to detect changes to ControlClipRect. If it's not too hard, I would
like to avoid adding branches to a central function like
UpdateForObjectLocationAndSize for a rare object like LayoutMenuList. What about
putting this in LayoutMenuList::AdjustInnerStyle:
// The control clip paint property depends on the inner content
// geometry (see: ControlClipRect).
SetNeedsPaintPropertyUpdate();

[Xianzhu, 2017/05/13 04:55:06]

SetNeedsPaintPropertyUpdate() in AdjustInnerStyle() can fix the bug, but it
seems not to catch all cases of ControlClipRect change. For example,
LayoutMenuList::AddChild() will add child under inner_block_ which may affect
inner_block_'s ContentSize() and LayoutMenuList's ControlClipRect(). Also
LayoutMenuList is just an example. I'm not sure if there have been or will be
other cases like that.

UpdateForObjectLocationAndSize() is a general solution for updating paint
property caused by geometry changes. This CL moves the overflow clip case out of
the box.Size() != box.PreviousSize() condition for any cases of overflow clip
change.

Actually if we could caught all the conditions needing paint property update
before PrePaint, we would not need UpdateForObjectLocationAndSize() at all. I
think we need it because we can't efficiently detect geometry changes before
PrePaint (considering cases that geometry changed forth and back resulting no
actual change in one layout stage). This was one of the reasons that we
developed repaint-after-layout. 

+  bool had_overflow_clip =
+      box.PaintProperties() && box.PaintProperties()->OverflowClip();
+  if (NeedsOverflowClip(box) != had_overflow_clip) {
+    needs_paint_property_update = true;
+  } else if (had_overflow_clip &&
+             box.PaintProperties()->OverflowClip()->ClipRect().Rect() !=
+                 FloatRect(
+                     box.OverflowClipRect(context.current.paint_offset))) {
+    needs_paint_property_update = true;
+  }
 
-  // CSS mask and clip-path comes with an implicit clip to the border box.
-  // Currently only SPv2 generate and take advantage of those.
-  const bool box_generates_property_nodes_for_mask_and_clip_path =
-      RuntimeEnabledFeatures::slimmingPaintV2Enabled() &&
-      (box.HasMask() || box.HasClipPath());
-  // The overflow clip paint property depends on the border box rect through
-  // overflowClipRect(). The border box rect's size equals the frame rect's
-  // size so we trigger a paint property update when the frame rect changes.
-  if (box.ShouldClipOverflow() ||
+  if (!needs_paint_property_update && box.Size() != box.PreviousSize()) {
+    if (box.HasClip()) {
       // The used value of CSS clip may depend on size of the box, e.g. for
       // clip: rect(auto auto auto -5px).
-      box.HasClip() ||
+      needs_paint_property_update = true;
+    } else if (box.StyleRef().HasTransform() ||
+               box.StyleRef().HasPerspective()) {
       // Relative lengths (e.g., percentage values) in transform, perspective,
       // transform-origin, and perspective-origin can depend on the size of the
       // frame rect, so force a property update if it changes. TODO(pdr): We
       // only need to update properties if there are relative lengths.
-      box.StyleRef().HasTransform() || box.StyleRef().HasPerspective() ||
-      box_generates_property_nodes_for_mask_and_clip_path)
+      needs_paint_property_update = true;
+    } else if (RuntimeEnabledFeatures::slimmingPaintV2Enabled() &&
+               (box.HasMask() || box.HasClipPath())) {
+      // CSS mask and clip-path comes with an implicit clip to the border box.
+      // Currently only SPv2 generate and take advantage of those.
+      needs_paint_property_update = true;
+    }
+  }
+
+  if (needs_paint_property_update)
     box.GetMutableForPainting().SetNeedsPaintPropertyUpdate();
 }
 
@@ -1185,7 +1199,7 @@ void PaintPropertyTreeBuilder::UpdatePaintProperties(
       NeedsPaintOffsetTranslation(object) || NeedsTransform(object) ||
       NeedsEffect(object) || NeedsTransformForNonRootSVG(object) ||
       NeedsFilter(object) || NeedsCssClip(object) ||
-      NeedsScrollbarPaintOffset(object) || NeedsOverflowScroll(object) ||
+      NeedsScrollbarPaintOffset(object) || NeedsOverflowClip(object) ||
       NeedsPerspective(object) || NeedsSVGLocalToBorderBoxTransform(object) ||
       NeedsScrollTranslation(object) || NeedsCssClipFixedPosition(object);