New issue
Advanced search Search tips

Issue 789390 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 26
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocked on:
issue 785687

Blocking:
issue 591099



Sign in to add a comment

Fix layout test failures for fragment box for SPAN

Project Member Reported by yosin@chromium.org, Nov 29 2017

Issue description

This is a tracking bug when we generate fragment box for SPAN[1].


[1] http://crrev.com/c/790177 Create NGPhysicalFragment for SPAN element
 

Comment 1 by yosin@chromium.org, Nov 29 2017

Blockedon: 785687
Blocking: 591099
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/813190a7045c2226aa6f1a4c9156888038bd9c48

commit 813190a7045c2226aa6f1a4c9156888038bd9c48
Author: Yoshifumi Inoue <yosin@chromium.org>
Date: Fri Dec 01 05:51:41 2017

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}
[modify] https://crrev.com/813190a7045c2226aa6f1a4c9156888038bd9c48/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/813190a7045c2226aa6f1a4c9156888038bd9c48/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d32934233b5f4470896217439d39922cefc99cf6

commit d32934233b5f4470896217439d39922cefc99cf6
Author: Koji Ishii <kojii@chromium.org>
Date: Tue Dec 05 11:44:56 2017

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

This reverts commit 813190a7045c2226aa6f1a4c9156888038bd9c48.

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: Koji Ishii <kojii@chromium.org>
Reviewed-by: yosin (OOO Dec 11 to Jan 8) <yosin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521669}
[modify] https://crrev.com/d32934233b5f4470896217439d39922cefc99cf6/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/d32934233b5f4470896217439d39922cefc99cf6/third_party/WebKit/Source/core/layout/LayoutInlineTest.cpp
[modify] https://crrev.com/d32934233b5f4470896217439d39922cefc99cf6/third_party/WebKit/Source/core/layout/ng/inline/ng_inline_layout_algorithm.cc

Status: Fixed (was: Available)

Sign in to add a comment