Skip to content

GitLab

T
tangled
Commit 8f50572b authored by Xianzhu Wang's avatar Xianzhu Wang Committed by Commit Bot
Browse files
Options

[SPv175+] Revert "Don't output OverflowClip if InnerBorderRadiusClip already has the same rect" 

This reverts commit c422bd5a
(reviewed on crrev.com/c/934923).

The CL was to reduce number of paint property nodes and paint operations
for objects with border-radius and overflow-clip and the clip rects are
the same.

Revert because:
- Border radius with overflow clip is rare;
- Hiding ObjectPaintProperties::OverflowClip() and exposing
  OverflowOrInnerBorderRadiusClip() was confusing, and I would not like
  to hide OverflowClip() again after crrev.com/c/1026941;
- The relationship between omitting OverflowClip and clip for hit
  testing is complex and tricky. Sometimes omitting OverflowClip causes
  full subtree property update when we just need a clip for hit
  testing. The cost is much bigger than saving just one paint property
  node and 3 paint operations.

Cluster-telemetry shows only 0.39% increase of paint op count with this
CL: https://ct.skia.org/results/cluster-telemetry/tasks/chromium_perf_runs/wangxianzhu-20180506194029/html/index.html

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I7c25a727d2f0bfa52dda4151e817ac71d5feb5c7
Reviewed-on: https://chromium-review.googlesource.com/1046152
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: default avatarPhilip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556614}
parent 198a0615
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 134 additions and 225 deletions
third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html deleted 100644 → 0
View file @ 198a0615
<!DOCTYPE html>
<div style="overflow: hidden; border-radius: 4px;">
<div style="border: 1px solid black; border-radius: 4px 0px 0px 4px; background: lightgray; width: 20px; height: 20px">
</div>
</div>
third_party/blink/renderer/core/paint/box_clipper_base.cc
View file @ 8f50572b
......@@ -24,12 +24,11 @@ void BoxClipperBase::InitializeScopedClipProperty(
if (!fragment)
return;
const auto* properties = fragment->PaintProperties();
if (!properties || !properties->OverflowOrInnerBorderRadiusClip())
if (!properties || !properties->OverflowClip())
return;
scoped_clip_property_.emplace(paint_info.context.GetPaintController(),
properties->OverflowOrInnerBorderRadiusClip(),
client,
properties->OverflowClip(), client,
paint_info.DisplayItemTypeForClipping());
}
......
third_party/blink/renderer/core/paint/find_properties_needing_update.h
View file @ 8f50572b
......@@ -189,8 +189,8 @@ class FindObjectPropertiesNeedingUpdateScope {
DCHECK_OBJECT_PROPERTY_EQ(object_,
original_properties_->InnerBorderRadiusClip(),
object_properties->InnerBorderRadiusClip());
DCHECK_OBJECT_PROPERTY_EQ(object_, OverflowClip(*original_properties_),
OverflowClip(*object_properties));
DCHECK_OBJECT_PROPERTY_EQ(object_, original_properties_->OverflowClip(),
object_properties->OverflowClip());
DCHECK_OBJECT_PROPERTY_EQ(object_, original_properties_->Perspective(),
object_properties->Perspective());
DCHECK_OBJECT_PROPERTY_EQ(
......
third_party/blink/renderer/core/paint/fragment_data.cc
View file @ 8f50572b
......@@ -71,8 +71,10 @@ const ClipPaintPropertyNode* FragmentData::PreClip() const {
const ClipPaintPropertyNode* FragmentData::PostOverflowClip() const {
if (const auto* properties = PaintProperties()) {
if (const auto* clip = properties->OverflowOrInnerBorderRadiusClip())
return clip;
if (properties->OverflowClip())
return properties->OverflowClip();
if (properties->InnerBorderRadiusClip())
return properties->InnerBorderRadiusClip();
}
return LocalBorderBoxProperties().Clip();
}
......
third_party/blink/renderer/core/paint/object_paint_properties.h
View file @ 8f50572b
......@@ -147,10 +147,6 @@ class CORE_EXPORT ObjectPaintProperties {
const ClipPaintPropertyNode* OverflowClip() const {
return overflow_clip_.get();
}
const ClipPaintPropertyNode* OverflowOrInnerBorderRadiusClip() const {
return overflow_clip_ ? overflow_clip_.get()
: inner_border_radius_clip_.get();
}
// The following clear* functions return true if the property tree structure
// changes (an existing node was deleted), and false otherwise. See the
......@@ -333,9 +329,6 @@ class CORE_EXPORT ObjectPaintProperties {
#endif
private:
friend const ClipPaintPropertyNode* OverflowClip(
const ObjectPaintProperties&);
ObjectPaintProperties() = default;
// Return true if the property tree structure changes (an existing node was
......@@ -391,15 +384,6 @@ class CORE_EXPORT ObjectPaintProperties {
DISALLOW_COPY_AND_ASSIGN(ObjectPaintProperties);
};
// ObjectPaintProperties doesn't provide public OverflowClip() method to avoid
// misuse because in most cases we want OverflowOrInnerBorderRadiusClip().
// This function returns the actual OverflowClip for actual requirements of
// mere OverflowClip, e.g. PaintPropertyTreePrinter, tests etc.
inline const ClipPaintPropertyNode* OverflowClip(
const ObjectPaintProperties& properties) {
return properties.overflow_clip_.get();
}
} // namespace blink
#endif // THIRD_PARTY_BLINK_RENDERER_CORE_PAINT_OBJECT_PAINT_PROPERTIES_H_
third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
View file @ 8f50572b
......@@ -1271,30 +1271,19 @@ void FragmentPaintPropertyTreeBuilder::UpdateOverflowClip() {
viewport_container.Viewport()));
}
bool should_create_overflow_clip = true;
if (auto* border_radius_clip = properties_->InnerBorderRadiusClip()) {
if (border_radius_clip->ClipRect().Rect() == state.clip_rect.Rect() &&
(state.clip_rect_excluding_overlay_scrollbars &&
state.clip_rect == *state.clip_rect_excluding_overlay_scrollbars))
should_create_overflow_clip = false;
}
if (should_create_overflow_clip) {
const ClipPaintPropertyNode* existing = properties_->OverflowClip();
bool equal_ignoring_hit_test_rects =
!!existing &&
existing->EqualIgnoringHitTestRects(context_.current.clip, state);
OnUpdateClip(properties_->UpdateOverflowClip(context_.current.clip,
std::move(state)),
equal_ignoring_hit_test_rects);
} else {
OnClearClip(properties_->ClearOverflowClip());
}
const ClipPaintPropertyNode* existing = properties_->OverflowClip();
bool equal_ignoring_hit_test_rects =
!!existing &&
existing->EqualIgnoringHitTestRects(context_.current.clip, state);
OnUpdateClip(properties_->UpdateOverflowClip(context_.current.clip,
std::move(state)),
equal_ignoring_hit_test_rects);
} else {
OnClearClip(properties_->ClearOverflowClip());
}
}
if (auto* overflow_clip = OverflowClip(*properties_))
if (auto* overflow_clip = properties_->OverflowClip())
context_.current.clip = overflow_clip;
}
......@@ -1778,8 +1767,7 @@ void FragmentPaintPropertyTreeBuilder::SetNeedsPaintPropertyUpdateIfNeeded() {
const LayoutBox& box = ToLayoutBox(object_);
if (NeedsOverflowClip(box)) {
bool had_overflow_clip =
properties_ && properties_->OverflowOrInnerBorderRadiusClip();
bool had_overflow_clip = properties_ && properties_->OverflowClip();
if (had_overflow_clip == CanOmitOverflowClip(box))
box.GetMutableForPainting().SetNeedsPaintPropertyUpdate();
}
......
third_party/blink/renderer/core/paint/paint_property_tree_printer.cc
View file @ 8f50572b
......@@ -101,7 +101,7 @@ class PropertyTreePrinterTraits<ClipPaintPropertyNode> {
printer.AddNode(properties.CssClipFixedPosition());
printer.AddNode(properties.OverflowControlsClip());
printer.AddNode(properties.InnerBorderRadiusClip());
printer.AddNode(OverflowClip(properties));
printer.AddNode(properties.OverflowClip());
}
};
......@@ -183,7 +183,7 @@ void UpdateDebugNames(const LayoutObject& object,
object);
SetDebugName(properties.InnerBorderRadiusClip(), "InnerBorderRadiusClip",
object);
SetDebugName(OverflowClip(properties), "OverflowClip", object);
SetDebugName(properties.OverflowClip(), "OverflowClip", object);
SetDebugName(properties.Effect(), "Effect", object);
SetDebugName(properties.Filter(), "Filter", object);
SetDebugName(properties.Mask(), "Mask", object);
......
third_party/blink/renderer/core/paint/paint_property_tree_update_tests.cc
View file @ 8f50572b
......@@ -468,14 +468,14 @@ TEST_P(PaintPropertyTreeUpdateTest, ClipChangesUpdateOverflowClip) {
div->setAttribute(HTMLNames::styleAttr, "display:inline-block; width:7px;");
GetDocument().View()->UpdateAllLifecyclePhases();
auto* clip_properties =
OverflowClip(*div->GetLayoutObject()->FirstFragment().PaintProperties());
div->GetLayoutObject()->FirstFragment().PaintProperties()->OverflowClip();
EXPECT_EQ(FloatRect(0, 0, 7, 0), clip_properties->ClipRect().Rect());
// Width changes should update the overflow clip.
div->setAttribute(HTMLNames::styleAttr, "display:inline-block; width:7px;");
GetDocument().View()->UpdateAllLifecyclePhases();
clip_properties =
OverflowClip(*div->GetLayoutObject()->FirstFragment().PaintProperties());
div->GetLayoutObject()->FirstFragment().PaintProperties()->OverflowClip();
EXPECT_EQ(FloatRect(0, 0, 7, 0), clip_properties->ClipRect().Rect());
div->setAttribute(HTMLNames::styleAttr, "display:inline-block; width:9px;");
GetDocument().View()->UpdateAllLifecyclePhases();
......@@ -487,7 +487,7 @@ TEST_P(PaintPropertyTreeUpdateTest, ClipChangesUpdateOverflowClip) {
"display:inline-block; width:7px; padding-right:3px;");
GetDocument().View()->UpdateAllLifecyclePhases();
clip_properties =
OverflowClip(*div->GetLayoutObject()->FirstFragment().PaintProperties());
div->GetLayoutObject()->FirstFragment().PaintProperties()->OverflowClip();
EXPECT_EQ(FloatRect(0, 0, 10, 0), clip_properties->ClipRect().Rect());
div->setAttribute(HTMLNames::styleAttr,
"display:inline-block; width:8px; padding-right:2px;");
......@@ -503,7 +503,7 @@ TEST_P(PaintPropertyTreeUpdateTest, ClipChangesUpdateOverflowClip) {
div->setAttribute(HTMLNames::styleAttr, "border-right:3px solid red;");
GetDocument().View()->UpdateAllLifecyclePhases();
clip_properties =
OverflowClip(*div->GetLayoutObject()->FirstFragment().PaintProperties());
div->GetLayoutObject()->FirstFragment().PaintProperties()->OverflowClip();
EXPECT_EQ(FloatRect(0, 0, 797, 0), clip_properties->ClipRect().Rect());
div->setAttribute(HTMLNames::styleAttr, "border-right:5px solid red;");
GetDocument().View()->UpdateAllLifecyclePhases();
......@@ -513,13 +513,15 @@ TEST_P(PaintPropertyTreeUpdateTest, ClipChangesUpdateOverflowClip) {
div->setAttribute(HTMLNames::styleAttr, "overflow:hidden;");
GetDocument().View()->UpdateAllLifecyclePhases();
clip_properties =
OverflowClip(*div->GetLayoutObject()->FirstFragment().PaintProperties());
div->GetLayoutObject()->FirstFragment().PaintProperties()->OverflowClip();
EXPECT_EQ(FloatRect(0, 0, 800, 0), clip_properties->ClipRect().Rect());
div->setAttribute(HTMLNames::styleAttr, "overflow:visible;");
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_TRUE(!div->GetLayoutObject()->FirstFragment().PaintProperties() ||
!OverflowClip(
*div->GetLayoutObject()->FirstFragment().PaintProperties()));
!div->GetLayoutObject()
->FirstFragment()
.PaintProperties()
->OverflowClip());
}
TEST_P(PaintPropertyTreeUpdateTest, ContainPaintChangesUpdateOverflowClip) {
......@@ -535,14 +537,16 @@ TEST_P(PaintPropertyTreeUpdateTest, ContainPaintChangesUpdateOverflowClip) {
GetDocument().View()->UpdateAllLifecyclePhases();
auto* div = GetDocument().getElementById("div");
auto* properties =
OverflowClip(*div->GetLayoutObject()->FirstFragment().PaintProperties());
div->GetLayoutObject()->FirstFragment().PaintProperties()->OverflowClip();
EXPECT_EQ(FloatRect(0, 0, 7, 6), properties->ClipRect().Rect());
div->setAttribute(HTMLNames::styleAttr, "");
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_TRUE(!div->GetLayoutObject()->FirstFragment().PaintProperties() ||
!OverflowClip(
*div->GetLayoutObject()->FirstFragment().PaintProperties()));
!div->GetLayoutObject()
->FirstFragment()
.PaintProperties()
->OverflowClip());
}
// A basic sanity check for over-invalidation of paint properties.
......@@ -796,7 +800,7 @@ TEST_P(PaintPropertyTreeUpdateTest, ScrollbarWidthChange) {
auto* container = GetLayoutObjectByElementId("container");
auto* overflow_clip =
OverflowClip(*container->FirstFragment().PaintProperties());
container->FirstFragment().PaintProperties()->OverflowClip();
EXPECT_EQ(FloatSize(80, 80), overflow_clip->ClipRect().Rect().Size());
auto* new_style = GetDocument().CreateRawElement(HTMLNames::styleTag);
......@@ -805,7 +809,7 @@ TEST_P(PaintPropertyTreeUpdateTest, ScrollbarWidthChange) {
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_EQ(overflow_clip,
OverflowClip(*container->FirstFragment().PaintProperties()));
container->FirstFragment().PaintProperties()->OverflowClip());
EXPECT_EQ(FloatSize(60, 60), overflow_clip->ClipRect().Rect().Size());
}
......@@ -836,12 +840,12 @@ TEST_P(PaintPropertyTreeUpdateTest, MenuListControlClipChange) {
)HTML");
auto* select = GetLayoutObjectByElementId("select");
EXPECT_NE(nullptr, OverflowClip(*select->FirstFragment().PaintProperties()));
EXPECT_NE(nullptr, select->FirstFragment().PaintProperties()->OverflowClip());
// Should not assert in FindPropertiesNeedingUpdate.
ToHTMLSelectElement(select->GetNode())->setSelectedIndex(1);
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_NE(nullptr, OverflowClip(*select->FirstFragment().PaintProperties()));
EXPECT_NE(nullptr, select->FirstFragment().PaintProperties()->OverflowClip());
}
TEST_P(PaintPropertyTreeUpdateTest, BoxAddRemoveMask) {
......@@ -1091,7 +1095,7 @@ TEST_P(PaintPropertyTreeUpdateTest, SVGViewportContainerOverflowChange) {
const auto* properties = PaintPropertiesForElement("target");
ASSERT_NE(nullptr, properties);
EXPECT_EQ(FloatRect(0, 0, 30, 40),
OverflowClip(*properties)->ClipRect().Rect());
properties->OverflowClip()->ClipRect().Rect());
GetDocument().getElementById("target")->setAttribute("overflow", "visible");
GetDocument().View()->UpdateAllLifecyclePhases();
......@@ -1102,7 +1106,7 @@ TEST_P(PaintPropertyTreeUpdateTest, SVGViewportContainerOverflowChange) {
properties = PaintPropertiesForElement("target");
ASSERT_NE(nullptr, properties);
EXPECT_EQ(FloatRect(0, 0, 30, 40),
OverflowClip(*properties)->ClipRect().Rect());
properties->OverflowClip()->ClipRect().Rect());
}
TEST_P(PaintPropertyTreeUpdateTest, SVGForeignObjectOverflowChange) {
......@@ -1117,7 +1121,7 @@ TEST_P(PaintPropertyTreeUpdateTest, SVGForeignObjectOverflowChange) {
const auto* properties = PaintPropertiesForElement("target");
ASSERT_NE(nullptr, properties);
EXPECT_EQ(FloatRect(10, 20, 30, 40),
OverflowClip(*properties)->ClipRect().Rect());
properties->OverflowClip()->ClipRect().Rect());
GetDocument().getElementById("target")->setAttribute("overflow", "visible");
GetDocument().View()->UpdateAllLifecyclePhases();
......@@ -1128,7 +1132,7 @@ TEST_P(PaintPropertyTreeUpdateTest, SVGForeignObjectOverflowChange) {
properties = PaintPropertiesForElement("target");
ASSERT_NE(nullptr, properties);
EXPECT_EQ(FloatRect(10, 20, 30, 40),
OverflowClip(*properties)->ClipRect().Rect());
properties->OverflowClip()->ClipRect().Rect());
}
TEST_P(PaintPropertyTreeBuilderTest, OmitOverflowClipOnSelectionChange) {
......@@ -1138,15 +1142,15 @@ TEST_P(PaintPropertyTreeBuilderTest, OmitOverflowClipOnSelectionChange) {
</div>
)HTML");
EXPECT_FALSE(OverflowClip(*PaintPropertiesForElement("target")));
EXPECT_FALSE(PaintPropertiesForElement("target")->OverflowClip());
GetDocument().GetFrame()->Selection().SelectAll();
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_TRUE(OverflowClip(*PaintPropertiesForElement("target")));
EXPECT_TRUE(PaintPropertiesForElement("target")->OverflowClip());
GetDocument().GetFrame()->Selection().Clear();
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_FALSE(OverflowClip(*PaintPropertiesForElement("target")));
EXPECT_FALSE(PaintPropertiesForElement("target")->OverflowClip());
}
TEST_P(PaintPropertyTreeBuilderTest, OmitOverflowClipOnCaretChange) {
......@@ -1159,15 +1163,15 @@ TEST_P(PaintPropertyTreeBuilderTest, OmitOverflowClipOnCaretChange) {
GetDocument().GetPage()->GetFocusController().SetActive(true);
GetDocument().GetPage()->GetFocusController().SetFocused(true);
auto* target = GetDocument().getElementById("target");
EXPECT_FALSE(OverflowClip(*PaintPropertiesForElement("target")));
EXPECT_FALSE(PaintPropertiesForElement("target")->OverflowClip());
target->focus();
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_TRUE(OverflowClip(*PaintPropertiesForElement("target")));
EXPECT_TRUE(PaintPropertiesForElement("target")->OverflowClip());
target->blur();
GetDocument().View()->UpdateAllLifecyclePhases();
EXPECT_FALSE(OverflowClip(*PaintPropertiesForElement("target")));
EXPECT_FALSE(PaintPropertiesForElement("target")->OverflowClip());
}
} // namespace blink
third_party/blink/renderer/core/paint/svg_container_painter.cc
View file @ 8f50572b
......@@ -58,11 +58,11 @@ void SVGContainerPainter::Paint(const PaintInfo& paint_info) {
// TODO(crbug.com/814815): The condition should be a DCHECK, but for now
// we may paint the object for filters during PrePaint before the
// properties are ready.
if (properties && properties->OverflowOrInnerBorderRadiusClip()) {
if (properties && properties->OverflowClip()) {
scoped_paint_chunk_properties.emplace(
paint_info.context.GetPaintController(),
properties->OverflowOrInnerBorderRadiusClip(),
layout_svg_container_, paint_info.DisplayItemTypeForClipping());
properties->OverflowClip(), layout_svg_container_,
paint_info.DisplayItemTypeForClipping());
}
} else {
FloatRect viewport =
......
third_party/blink/renderer/core/paint/view_painter_test.cc
View file @ 8f50572b
......@@ -130,7 +130,7 @@ TEST_P(ViewPainterTest, DocumentBackgroundWithScroll) {
const auto* properties = GetLayoutView().FirstFragment().PaintProperties();
if (RuntimeEnabledFeatures::RootLayerScrollingEnabled()) {
EXPECT_EQ(properties->ScrollTranslation(), tree_state.Transform());
EXPECT_EQ(OverflowClip(*properties), tree_state.Clip());
EXPECT_EQ(properties->OverflowClip(), tree_state.Clip());
} else {
EXPECT_EQ(nullptr, properties);
const auto* frame_view = GetDocument().View();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment