Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1280543005:
Refactor LayoutInline::addOutlineRects to avoid tricky logic to avoid dups (Closed)
|
Created:
5 years, 4 months ago by Xianzhu Modified:
5 years, 4 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRefactor LayoutInline::addOutlineRects to avoid tricky logic to avoid dups
Previously LayoutInline::addOutlineRects() checks if the incoming rects
vector is empty to determine if it should add outline rects for line
boxes, to avoid duplicated outline rects for line boxes if the ancestor
has already added the line boxes. The check is implicit and fragile.
Separate LayoutInline::addOutlineRectsForChildrenAndContinuations() to
make the intent explicit.
Also separate LayoutInline::addOutlineForContinuations() for use by
https://codereview.chromium.org/1278543002/
BUG=506669
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200274
Patch Set 1 #
Total comments: 7
Patch Set 2 : Change comments about calling addOutlineRectsForChildrenAndContinuations #Patch Set 3 : Change comments about calling addOutlineRectsForChildrenAndContinuations #
Total comments: 2
Patch Set 4 : Rephrase comments #
Messages
Total messages: 23 (3 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... File Source/core/layout/LayoutInline.cpp (left): https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... Source/core/layout/LayoutInline.cpp:1384: if (rects.isEmpty()) { I don't quite get yet why the new version avoids this duplication problem. Can you explain in more detail?
https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... File Source/core/layout/LayoutInline.cpp (left): https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... Source/core/layout/LayoutInline.cpp:1384: if (rects.isEmpty()) { On 2015/08/07 17:08:40, chrishtr wrote: > I don't quite get yet why the new version avoids this duplication problem. Can > you explain in more detail? The duplication problem is about LayoutBlocks with inline children. Previously the call tree was: LayoutBlock::addOutlineRects 1 add rects for line boxes 2 LayoutBoxModelObject::addOutlineRectsForNormalChildren 2.1 LayoutBoxModelObject::addOutlineRectsForDescendant 2.1.1 LayoutInline::addOutlineRects At 2.1.1 we checked if the rects is empty. With this CL, 2.1.1 becomes LayoutInline::addOutlineRectsForChildrenAndContinuations() which doesn't add rects for line boxes.
On 2015/08/07 at 17:18:49, wangxianzhu wrote: > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > File Source/core/layout/LayoutInline.cpp (left): > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > Source/core/layout/LayoutInline.cpp:1384: if (rects.isEmpty()) { > On 2015/08/07 17:08:40, chrishtr wrote: > > I don't quite get yet why the new version avoids this duplication problem. Can > > you explain in more detail? > > The duplication problem is about LayoutBlocks with inline children. Previously the call tree was: > LayoutBlock::addOutlineRects > 1 add rects for line boxes > 2 LayoutBoxModelObject::addOutlineRectsForNormalChildren > 2.1 LayoutBoxModelObject::addOutlineRectsForDescendant > 2.1.1 LayoutInline::addOutlineRects > At 2.1.1 we checked if the rects is empty. > > With this CL, 2.1.1 becomes LayoutInline::addOutlineRectsForChildrenAndContinuations() which doesn't add rects for line boxes. So previously 2.1.1 was checking for duplicate work already done in step 1?
On 2015/08/07 22:46:41, chrishtr wrote: > On 2015/08/07 at 17:18:49, wangxianzhu wrote: > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > > File Source/core/layout/LayoutInline.cpp (left): > > > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > > Source/core/layout/LayoutInline.cpp:1384: if (rects.isEmpty()) { > > On 2015/08/07 17:08:40, chrishtr wrote: > > > I don't quite get yet why the new version avoids this duplication problem. > Can > > > you explain in more detail? > > > > The duplication problem is about LayoutBlocks with inline children. Previously > the call tree was: > > LayoutBlock::addOutlineRects > > 1 add rects for line boxes > > 2 LayoutBoxModelObject::addOutlineRectsForNormalChildren > > 2.1 LayoutBoxModelObject::addOutlineRectsForDescendant > > 2.1.1 LayoutInline::addOutlineRects > > At 2.1.1 we checked if the rects is empty. > > > > With this CL, 2.1.1 becomes > LayoutInline::addOutlineRectsForChildrenAndContinuations() which doesn't add > rects for line boxes. > > So previously 2.1.1 was checking for duplicate work already done in step 1? Yes.
On 2015/08/07 at 22:56:49, wangxianzhu wrote: > On 2015/08/07 22:46:41, chrishtr wrote: > > On 2015/08/07 at 17:18:49, wangxianzhu wrote: > > > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > > > File Source/core/layout/LayoutInline.cpp (left): > > > > > > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > > > Source/core/layout/LayoutInline.cpp:1384: if (rects.isEmpty()) { > > > On 2015/08/07 17:08:40, chrishtr wrote: > > > > I don't quite get yet why the new version avoids this duplication problem. > > Can > > > > you explain in more detail? > > > > > > The duplication problem is about LayoutBlocks with inline children. Previously > > the call tree was: > > > LayoutBlock::addOutlineRects > > > 1 add rects for line boxes > > > 2 LayoutBoxModelObject::addOutlineRectsForNormalChildren > > > 2.1 LayoutBoxModelObject::addOutlineRectsForDescendant > > > 2.1.1 LayoutInline::addOutlineRects > > > At 2.1.1 we checked if the rects is empty. > > > > > > With this CL, 2.1.1 becomes > > LayoutInline::addOutlineRectsForChildrenAndContinuations() which doesn't add > > rects for line boxes. > > > > So previously 2.1.1 was checking for duplicate work already done in step 1? > > Yes. But LayoutInline::addOutlineRects calls LayoutInline::addOutlineRectsForChildrenAndContinuations so why do we even need 2.1.1 at all now? Still confused.
On 2015/08/07 23:00:43, chrishtr wrote: > On 2015/08/07 at 22:56:49, wangxianzhu wrote: > > On 2015/08/07 22:46:41, chrishtr wrote: > > > On 2015/08/07 at 17:18:49, wangxianzhu wrote: > > > > > > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > > > > File Source/core/layout/LayoutInline.cpp (left): > > > > > > > > > > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutIn... > > > > Source/core/layout/LayoutInline.cpp:1384: if (rects.isEmpty()) { > > > > On 2015/08/07 17:08:40, chrishtr wrote: > > > > > I don't quite get yet why the new version avoids this duplication > problem. > > > Can > > > > > you explain in more detail? > > > > > > > > The duplication problem is about LayoutBlocks with inline children. > Previously > > > the call tree was: > > > > LayoutBlock::addOutlineRects > > > > 1 add rects for line boxes > > > > 2 LayoutBoxModelObject::addOutlineRectsForNormalChildren > > > > 2.1 LayoutBoxModelObject::addOutlineRectsForDescendant > > > > 2.1.1 LayoutInline::addOutlineRects > > > > At 2.1.1 we checked if the rects is empty. > > > > > > > > With this CL, 2.1.1 becomes > > > LayoutInline::addOutlineRectsForChildrenAndContinuations() which doesn't add > > > rects for line boxes. > > > > > > So previously 2.1.1 was checking for duplicate work already done in step 1? > > > > Yes. > > But LayoutInline::addOutlineRects calls > LayoutInline::addOutlineRectsForChildrenAndContinuations so why do we even need > 2.1.1 at all now? Still confused. 2.1.1 is now replaced with LayoutInline::addOutlineRectsForChildrenAndContinuations(). Now - LayoutInline::addOutlineRects() always add outline rects for line boxes, children and continuations. It is called when the LayoutInline itself needs to get outline rects. - LayoutInline::addOutlineRectsForChildrenAndContinuations() doesn't add outline rects for line boxes. It is called when we have already added outline rects for line boxes for some ancestor of the LayoutInline. Previously we call LayoutInline::addOutlineRects() in both of the above cases, and check rects.isEmpty() to distinguish them. This is tricky and might be incorrect in some corner cases.
https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlock.cpp:2600: if (inlineElementContinuation) Can this code just be deleted, and then the special casing in LayoutBoxModelObject::addOutlineRectsForDescendant is gone?
https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlock.cpp:2600: if (inlineElementContinuation) On 2015/08/07 23:35:31, chrishtr wrote: > Can this code just be deleted, and then the special casing in > LayoutBoxModelObject::addOutlineRectsForDescendant is gone? They are for different cases. You can treat the addOutlineRects a two-dimensional action: - parent-children dimension - and continuation dimension. The code path (and special case) in LayoutBoxModelObject::addOutlineRectsForDescendant() is in parent-children dimension, where we don't want descendant LayoutInline to add duplicated line boxes. This 'inlineElementContinuation' code is executed in continuation dimension as LayoutInline::addOutlineRects() -> (same 'this') LayoutInline::addOutlineRectsForChildrenAndContinuation() -> (block continuation) LayoutBlock::addOutlineRects() -> (inline continuation) LayoutInline::addOutlineRects() Here the last LayoutInline is not a descendant of any previous objects which have added outline rects, so we call addOutlineRects() to let it add all outline rects of it.
https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... File Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... Source/core/layout/LayoutBlock.cpp:2600: if (inlineElementContinuation) On 2015/08/07 at 23:56:23, Xianzhu wrote: > On 2015/08/07 23:35:31, chrishtr wrote: > > Can this code just be deleted, and then the special casing in > > LayoutBoxModelObject::addOutlineRectsForDescendant is gone? > > They are for different cases. You can treat the addOutlineRects a two-dimensional action: > - parent-children dimension > - and continuation dimension. > > The code path (and special case) in LayoutBoxModelObject::addOutlineRectsForDescendant() is in parent-children dimension, where we don't want descendant LayoutInline to add duplicated line boxes. > > This 'inlineElementContinuation' code is executed in continuation dimension as > LayoutInline::addOutlineRects() > -> (same 'this') LayoutInline::addOutlineRectsForChildrenAndContinuation() > -> (block continuation) LayoutBlock::addOutlineRects() > -> (inline continuation) LayoutInline::addOutlineRects() > Here the last LayoutInline is not a descendant of any previous objects which have added outline rects, so we call addOutlineRects() to let it add all outline rects of it. Then I don't understand where the potential duplication of outline rects is coming from. I thought this was the place where outline rects were being generated that duplicated things and necessitated the special case in LayoutBoxModelObject::addOutlineRectsForDescendant(). I guess not? Where do the duplicates come from?
On 2015/08/08 02:42:00, chrishtr wrote: > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... > File Source/core/layout/LayoutBlock.cpp (right): > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... > Source/core/layout/LayoutBlock.cpp:2600: if (inlineElementContinuation) > On 2015/08/07 at 23:56:23, Xianzhu wrote: > > On 2015/08/07 23:35:31, chrishtr wrote: > > > Can this code just be deleted, and then the special casing in > > > LayoutBoxModelObject::addOutlineRectsForDescendant is gone? > > > > They are for different cases. You can treat the addOutlineRects a > two-dimensional action: > > - parent-children dimension > > - and continuation dimension. > > > > The code path (and special case) in > LayoutBoxModelObject::addOutlineRectsForDescendant() is in parent-children > dimension, where we don't want descendant LayoutInline to add duplicated line > boxes. > > > > This 'inlineElementContinuation' code is executed in continuation dimension as > > LayoutInline::addOutlineRects() > > -> (same 'this') LayoutInline::addOutlineRectsForChildrenAndContinuation() > > -> (block continuation) LayoutBlock::addOutlineRects() > > -> (inline continuation) LayoutInline::addOutlineRects() > > Here the last LayoutInline is not a descendant of any previous objects which > have added outline rects, so we call addOutlineRects() to let it add all outline > rects of it. > > Then I don't understand where the potential duplication of outline rects is > coming from. I thought this was the place where outline rects were being > generated that duplicated things and necessitated the special case in > LayoutBoxModelObject::addOutlineRectsForDescendant(). I guess not? Where do the > duplicates come from? The duplicates come from line boxes which are in both LayoutBlockFlow with inline children and descendant LayoutInlines. For the example in #4, without dup-prevention, rects of line boxes will be added twice in 1 and 2.1.1. Dup-prevention logic was added in https://codereview.chromium.org/1083403005. This CL changes how we prevent the dups. Previously we use "if (rects.isEmpty())" in LayoutInline::addOutlineRects to guess whether the call is from ancestor which is tricky. With this CL we explicitly call LayoutInline::addOutlineRectsForChildrenAndContinuations() from LayoutBoxModelObject::addOutlineRectsForDescendant() for the case.
On 2015/08/10 16:04:30, Xianzhu wrote: > On 2015/08/08 02:42:00, chrishtr wrote: > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... > > File Source/core/layout/LayoutBlock.cpp (right): > > > > > https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBl... > > Source/core/layout/LayoutBlock.cpp:2600: if (inlineElementContinuation) > > On 2015/08/07 at 23:56:23, Xianzhu wrote: > > > On 2015/08/07 23:35:31, chrishtr wrote: > > > > Can this code just be deleted, and then the special casing in > > > > LayoutBoxModelObject::addOutlineRectsForDescendant is gone? > > > > > > They are for different cases. You can treat the addOutlineRects a > > two-dimensional action: > > > - parent-children dimension > > > - and continuation dimension. > > > > > > The code path (and special case) in > > LayoutBoxModelObject::addOutlineRectsForDescendant() is in parent-children > > dimension, where we don't want descendant LayoutInline to add duplicated line > > boxes. > > > > > > This 'inlineElementContinuation' code is executed in continuation dimension > as > > > LayoutInline::addOutlineRects() > > > -> (same 'this') LayoutInline::addOutlineRectsForChildrenAndContinuation() > > > -> (block continuation) LayoutBlock::addOutlineRects() > > > -> (inline continuation) LayoutInline::addOutlineRects() > > > Here the last LayoutInline is not a descendant of any previous objects which > > have added outline rects, so we call addOutlineRects() to let it add all > outline > > rects of it. > > > > Then I don't understand where the potential duplication of outline rects is > > coming from. I thought this was the place where outline rects were being > > generated that duplicated things and necessitated the special case in > > LayoutBoxModelObject::addOutlineRectsForDescendant(). I guess not? Where do > the > > duplicates come from? > > The duplicates come from line boxes which are in both LayoutBlockFlow with > inline children and descendant LayoutInlines. For the example in #4, without > dup-prevention, rects of line boxes will be added twice in 1 and 2.1.1. > Dup-prevention logic was added in https://codereview.chromium.org/1083403005. > > This CL changes how we prevent the dups. Previously we use "if > (rects.isEmpty())" in LayoutInline::addOutlineRects to guess whether the call is > from ancestor which is tricky. With this CL we explicitly call > LayoutInline::addOutlineRectsForChildrenAndContinuations() from > LayoutBoxModelObject::addOutlineRectsForDescendant() for the case. P.S. Duplicates of line boxes occur in parent-children dimension only, not in continuations dimension, so LayoutBlock::addOutlineRects() calls LayoutInline::addOutlineRects() for the inlineElementContinuation.
https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBo... File Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBo... Source/core/layout/LayoutBoxModelObject.cpp:418: // Some ancestor of the LayoutInline has already added outline rects for line boxes, Add to this: // The rects are computed by an ancestor as an optimization to avoid extra computation for line boxes. // In particular, LayoutBlock adds rects for its RootOutlineBoxes, which include the bounds of this LayoutInline.
https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBo... File Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/1280543005/diff/1/Source/core/layout/LayoutBo... Source/core/layout/LayoutBoxModelObject.cpp:418: // Some ancestor of the LayoutInline has already added outline rects for line boxes, On 2015/08/10 20:14:47, chrishtr wrote: > Add to this: > > // The rects are computed by an ancestor as an optimization to avoid extra > computation for line boxes. > // In particular, LayoutBlock adds rects for its RootOutlineBoxes, which include > the bounds of this LayoutInline. Done.
lgtm https://codereview.chromium.org/1280543005/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/1280543005/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBoxModelObject.cpp:419: // line boxes, so descendants don't need to add line boxes again. In particular, LayoutBlock LayoutBlock is not necessarily the root entry point into addOutlineRects, there is code that calls directly into the LayoutInline version. So rephrase to indicate this, maybe: "For example, if the parent is a LayoutBlock, it adds rects..."
https://codereview.chromium.org/1280543005/diff/40001/Source/core/layout/Layo... File Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/1280543005/diff/40001/Source/core/layout/Layo... Source/core/layout/LayoutBoxModelObject.cpp:419: // line boxes, so descendants don't need to add line boxes again. In particular, LayoutBlock On 2015/08/10 21:54:10, chrishtr wrote: > LayoutBlock is not necessarily the root entry point into addOutlineRects, there > is code that calls directly into the LayoutInline version. So rephrase to > indicate this, maybe: > > "For example, if the parent is a LayoutBlock, it adds rects..." Done.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1280543005/#ps60001 (title: "Rephrase comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1280543005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1280543005/60001
lgtm
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200274 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
