Commit d3293423 authored by Koji Ishii's avatar Koji Ishii Committed by Commit Bot

Revert "Create NGPhysicalFragment for every SPAN element for...

Revert "Create NGPhysicalFragment for every SPAN element for LayoutNGPaintFragments runtime feature enabled"

This reverts commit 813190a7.

Reason for revert: It turned out that this change regressed
387 tests when NGPaintFragment is enabled. Local test here:
https://chromium-review.googlesource.com/c/chromium/src/+/802840

The assumption was that this change fixes several failures
without any additional failures (after bidi was fixed in CL:802673.)

Since the assumption didn't stand, revert this once and we'll
revisit again when we have more insights on what's happening.

Original change's description:
> Create NGPhysicalFragment for every SPAN element for LayoutNGPaintFragments runtime feature enabled
> 
> This patch changes |ShouldCreateBoxFragment()| to return |true| for producing
> box fragments for plain SPAN when |LayoutNGPaintFragments| runtime feature
> enabled, to locate |NGPhysicalFragment| associated to |LayoutInlinline| for
> computing bounding box and other kinds rects.
> 
> Without this patch, we need to traverse fragment tree rooted by
> |LayoutBlockFlow| containing |LayoutInline|.
> 
> In legacy layout tree, we don't generate |InlineBox| for |LayoutInline| for
> performance optimization and called "Culled InlineBox". To compute rects in
> legacy layout tree, we traverse layout tree and line boxes in |LayoutInline|.
> This code makes |LayoutInline| implementation complicated. Note: There is a
> flag disabling "Culled InlineBox" as |AlwaysCreateLineBoxesForLayoutInline|.
> 
> Once LayoutNG is stable enough to have profiling, we investigate whether
> generating fragment for every SPAN causes performance regression or not.
> 
> 
> [1] http://crrev.com/c/763188: ONE [LayoutNG] Utilize DCHECK(CanUseInlineBox())
> in core/layout/
> 
> Bug: 789390
> Change-Id: Ife87fc9ee487397891ae4515d1bcaea1afcf9c04
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
> Reviewed-on: https://chromium-review.googlesource.com/790177
> Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
> Reviewed-by: Koji Ishii <kojii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#520865}

TBR=yosin@chromium.org,kojii@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 789390
Change-Id: I7cdbb2ca7ab9b07718a2ce6b7f998f4b38803f3b
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/807426
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: default avatarKoji Ishii <kojii@chromium.org>
Reviewed-by: default avataryosin (OOO Dec 11 to Jan 8) <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521669}
parent a5bf8e8a
...@@ -584,9 +584,6 @@ crbug.com/635619 [ Mac ] virtual/layout_ng/fast/block/margin-collapse/block-insi ...@@ -584,9 +584,6 @@ crbug.com/635619 [ Mac ] virtual/layout_ng/fast/block/margin-collapse/block-insi
# ====== LayoutNG,LayoutNGPaintFragments ====== # ====== LayoutNG,LayoutNGPaintFragments ======
# expected is wrong
crbug.com/789390 virtual/layout_ng_paint/fast/inline/vertical-align-text-inherit.html [ Failure ]
### Crash site: ContainerNode.cpp ### Crash site: ContainerNode.cpp
crbug.com/714962 virtual/layout_ng_paint/fast/inline/inline-with-empty-inline-children.html [ Crash Failure ] crbug.com/714962 virtual/layout_ng_paint/fast/inline/inline-with-empty-inline-children.html [ Crash Failure ]
......
...@@ -35,7 +35,9 @@ TEST_P(ParameterizedLayoutInlineTest, LinesBoundingBox) { ...@@ -35,7 +35,9 @@ TEST_P(ParameterizedLayoutInlineTest, LinesBoundingBox) {
LoadAhem(); LoadAhem();
SetBodyInnerHTML( SetBodyInnerHTML(
"<style>" "<style>"
"* { font-family: Ahem; font-size: 13px; }" "html { font-family: Ahem; font-size: 13px; }"
// LayoutNG requires box decorations at this moment. crbug.com/789390
"span { background-color: yellow; }"
".vertical { writing-mode: vertical-rl; }" ".vertical { writing-mode: vertical-rl; }"
"</style>" "</style>"
"<p><span id=ltr1>abc<br>xyz</span></p>" "<p><span id=ltr1>abc<br>xyz</span></p>"
......
...@@ -37,17 +37,7 @@ namespace { ...@@ -37,17 +37,7 @@ namespace {
inline bool ShouldCreateBoxFragment(const NGInlineItem& item, inline bool ShouldCreateBoxFragment(const NGInlineItem& item,
const NGInlineItemResult& item_result) { const NGInlineItemResult& item_result) {
// Note: When you introduce more conditions, please make sure we have
// box fragments for plain SPAN to avoid using culled InlineBox which
// requires traversing layout tree to find |InlineBox| in SPAN, e.g.
// <div><span>foo</span></div>.
// In legacy layout tree, we don't have |InlineBox| for plain SPAN if
// |LayoutObject::AlwaysCreateLineBoxes()| is false.
// See |LayoutBlockFlow::LayoutInlineChildren()| for conditions for
// enabling |AlwaysCreateLineBoxes()|.
DCHECK(item.Style()); DCHECK(item.Style());
if (RuntimeEnabledFeatures::LayoutNGPaintFragmentsEnabled())
return true;
const ComputedStyle& style = *item.Style(); const ComputedStyle& style = *item.Style();
// TODO(kojii): We might need more conditions to create box fragments. // TODO(kojii): We might need more conditions to create box fragments.
return style.HasBoxDecorationBackground() || style.HasOutline() || return style.HasBoxDecorationBackground() || style.HasOutline() ||
......
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