Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
767283005:
Take continuation into consideration when calculating absoluteClippedOverflowRect. (Closed)
|
Created:
6 years ago by c.shu Modified:
5 years, 11 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionTake continuation into consideration when calculating absoluteClippedOverflowRect.
The root issue is not related to spatial navigation but RenderInline. When calculating absoluteClippedOverflowRect, we should also take the RenderObjects in the same continuation flow into account.
In details, when there's a DIV inside an Anchor, the Anchor element's renderer, RenderInline, is being split into multiple RenderBlocks. The RenderText, which is originally a child of the RenderInline, becomes a sibling of it, and both of them are now children of the Anonymous RenderBlock. The original algorithm that calculates the rect of RenderInline only goes through it's child list, which will not work in this case. We have to explicitly go through all renderObjects for the continuation flow related to the RenderInline to get the right rect.
BUG=438367
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188780
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address reviewers' comments #
Total comments: 4
Patch Set 3 : Address review comments and rebaseline #
Messages
Total messages: 25 (6 generated)
c.shu@samsung.com changed reviewers: + tkent@chromium.org
The root issue is not related to spatial navigation but RenderInline. When calculating absoluteClippedOverflowRect, we should also take the RenderObjects in the same continuation flow into account.
pdr@chromium.org changed reviewers: + pdr@chromium.org
Just a quick drive-by review. https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1023: return clippedOverflowRectForPaintInvalidation(view()); It looks like clippedOverflowRectForPaintInvalidation already has code for continuations. Does it not work? https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.h (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.h:99: LayoutRect absoluteClippedOverflowRect() const override; Should this be virtual? Also final? https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderObject.h:864: virtual LayoutRect absoluteClippedOverflowRect() const; Can you just update RenderInline::clippedOverflowRectForPaintInvalidation instead of making this virtual?
https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1023: return clippedOverflowRectForPaintInvalidation(view()); On 2014/12/02 21:56:21, pdr wrote: > It looks like clippedOverflowRectForPaintInvalidation already has code for > continuations. Does it not work? The continuation code in clippedOverflowRectForPaintInvalidation only does some additional calculation for outlineSize. https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.h (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.h:99: LayoutRect absoluteClippedOverflowRect() const override; On 2014/12/02 21:56:21, pdr wrote: > Should this be virtual? Also final? sounds good. thanks. https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderObject.h:864: virtual LayoutRect absoluteClippedOverflowRect() const; On 2014/12/02 21:56:21, pdr wrote: > Can you just update RenderInline::clippedOverflowRectForPaintInvalidation > instead of making this virtual? I put the code in absoluteClippedOverflowRect for performance consideration. clippedOverflowRectForPaintInvalidation is called by many other places. Putting the code in here would have many duplications in calculation and cause performance regression in rendering, I believe. Another way to avoid making absoluteClippedOverflowRect virtual is to check if isRenderInline in the base. What do you think?
> Another way to avoid making absoluteClippedOverflowRect virtual is to check if > isRenderInline in the base. What do you think? I take this back. I still need a virtual function somewhere. :)
tkent@chromium.org changed reviewers: - tkent@chromium.org
pdr@chromium.org changed reviewers: + leviw@chromium.org
On 2014/12/03 at 21:42:32, pdr wrote: > pdr@chromium.org changed reviewers: > + leviw@chromium.org leviw, would you be able to review this one?
https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1023: return clippedOverflowRectForPaintInvalidation(view()); On 2014/12/02 at 22:16:31, c.shu wrote: > On 2014/12/02 21:56:21, pdr wrote: > > It looks like clippedOverflowRectForPaintInvalidation already has code for > > continuations. Does it not work? > > The continuation code in clippedOverflowRectForPaintInvalidation only does some additional calculation for outlineSize. Can you help me understand why we wouldn't want to include outlines here too? This patch is for hit testing and we can hit test outlines. Which caller of absoluteClippedOverflowRect() matters for your test? Having a better change description would make this much easier to review. https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderObject.h:864: virtual LayoutRect absoluteClippedOverflowRect() const; On 2014/12/02 at 22:16:31, c.shu wrote: > On 2014/12/02 21:56:21, pdr wrote: > > Can you just update RenderInline::clippedOverflowRectForPaintInvalidation > > instead of making this virtual? > > I put the code in absoluteClippedOverflowRect for performance consideration. > clippedOverflowRectForPaintInvalidation is called by many other places. Putting the code in here would have many duplications in calculation and cause performance regression in rendering, I believe. > Another way to avoid making absoluteClippedOverflowRect virtual is to check if isRenderInline in the base. What do you think? Remember that making a function virtual pretty much* adds an if statement to all calls to absoluteClippedOverflowRect for all render objects. I'm not yet sure where this is being called from (see previous comment) so I can't comment on which approach will be best.
https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1023: return clippedOverflowRectForPaintInvalidation(view()); > Can you help me understand why we wouldn't want to include outlines here too? > This patch is for hit testing and we can hit test outlines. It's not that we wouldn't want to include outlines. The continuation-related code in clippedOverflowRectForPaintInvalidation is for something else. When it calculates the rect, it doesn't take care the whole continuation flow. It is Ok for clippedOverflowRectForPaintInvalidation since it will traverse through the tree anyway. But absoluteClippedOverflowRect() has to do more. > Which caller of absoluteClippedOverflowRect() matters for your test? Having a > better change description would make this much easier to review. The caller is hasOffscreenRect() in page/SpatialNavigation.cpp. I will add details to the description. https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderObject.h (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderObject.h:864: virtual LayoutRect absoluteClippedOverflowRect() const; > Remember that making a function virtual pretty much* adds an if statement to all > calls to absoluteClippedOverflowRect for all render objects. I'm not yet sure > where this is being called from (see previous comment) so I can't comment on > which approach will be best. There are only a few callers, 1. hasOffscreenRect in SpatialNavigation.cpp 2. HTMLAreaElement::computePath 3. writePositionAndStyle in SVGRenderTreeAsText.cpp All of them are in uncommon code path. However, clippedOverflowRectForPaintInvalidation is a common function in the rendering code.
leviw: ptal, thanks.
https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1029: while (endContinuation->inlineElementContinuation()) { Nit: no braces needed. https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1034: LayoutRect rect = curr->clippedOverflowRectForPaintInvalidation(view()); This is N^2 since we'll walk up to the view for every renderer :( Could we at least get a comment about that fact and about possible optimizations? What comes to mind is potentially mapping all these rects into a common ancestor's address space then mapping the result into the view.
https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1029: while (endContinuation->inlineElementContinuation()) { On 2014/12/09 20:39:56, leviw wrote: > Nit: no braces needed. Acknowledged. https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderInline.cpp:1034: LayoutRect rect = curr->clippedOverflowRectForPaintInvalidation(view()); On 2014/12/09 20:39:56, leviw wrote: > This is N^2 since we'll walk up to the view for every renderer :( > > Could we at least get a comment about that fact and about possible > optimizations? What comes to mind is potentially mapping all these rects into a > common ancestor's address space then mapping the result into the view. thanks for pointing out this performance issue. I will add the comments. btw, I thought it walks up to the containinigBlock(), which is an anonymous block, and then goes through its siblings. Did my code do something wrong? Is it correct to pass view() for the clippedOverflowRectForPaintInvalidation() function?
On 2014/12/10 at 02:40:51, c.shu wrote: > https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... > File Source/core/rendering/RenderInline.cpp (right): > > https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... > Source/core/rendering/RenderInline.cpp:1029: while (endContinuation->inlineElementContinuation()) { > On 2014/12/09 20:39:56, leviw wrote: > > Nit: no braces needed. > > Acknowledged. > > https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... > Source/core/rendering/RenderInline.cpp:1034: LayoutRect rect = curr->clippedOverflowRectForPaintInvalidation(view()); > On 2014/12/09 20:39:56, leviw wrote: > > This is N^2 since we'll walk up to the view for every renderer :( > > > > Could we at least get a comment about that fact and about possible > > optimizations? What comes to mind is potentially mapping all these rects into a > > common ancestor's address space then mapping the result into the view. > > thanks for pointing out this performance issue. I will add the comments. btw, I thought it walks up to the containinigBlock(), which is an anonymous block, and then goes through its siblings. Did my code do something wrong? Is it correct to pass view() for the clippedOverflowRectForPaintInvalidation() function? It normally only walks up to its paint invalidation container (and there's also frequently a PaintInvalidationState on the stack to speed up that process). Passing it view() means it's going to walk all the way up to the view for every renderer it visits.
On 2014/12/10 at 03:43:35, leviw wrote: > On 2014/12/10 at 02:40:51, c.shu wrote: > > https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... > > File Source/core/rendering/RenderInline.cpp (right): > > > > https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... > > Source/core/rendering/RenderInline.cpp:1029: while (endContinuation->inlineElementContinuation()) { > > On 2014/12/09 20:39:56, leviw wrote: > > > Nit: no braces needed. > > > > Acknowledged. > > > > https://codereview.chromium.org/767283005/diff/1/Source/core/rendering/Render... > > Source/core/rendering/RenderInline.cpp:1034: LayoutRect rect = curr->clippedOverflowRectForPaintInvalidation(view()); > > On 2014/12/09 20:39:56, leviw wrote: > > > This is N^2 since we'll walk up to the view for every renderer :( > > > > > > Could we at least get a comment about that fact and about possible > > > optimizations? What comes to mind is potentially mapping all these rects into a > > > common ancestor's address space then mapping the result into the view. > > > > thanks for pointing out this performance issue. I will add the comments. btw, I thought it walks up to the containinigBlock(), which is an anonymous block, and then goes through its siblings. Did my code do something wrong? Is it correct to pass view() for the clippedOverflowRectForPaintInvalidation() function? > > It normally only walks up to its paint invalidation container (and there's also frequently a PaintInvalidationState on the stack to speed up that process). Passing it view() means it's going to walk all the way up to the view for every renderer it visits. So if the continuation is N renderers and the average depth of those renderers is M, we'll visit N*M nodes. My assumption that N is usually small.
Thanks for the review, leviw and pdr. I have uploaded a new CL.
On 2014/12/10 16:09:28, c.shu wrote: > Thanks for the review, leviw and pdr. I have uploaded a new CL. leviw, can you follow up the new CL? thanks.
leviw@chromium.org changed reviewers: + jchaffraix@chromium.org, wangxianzhu@chromium.org
https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderInline.cpp:1042: context(FloatRect(enclosingIntRect(rect))); If you're eventually taking the enclosingIntRect of the result (on L1044), this enclosingIntRect is extra work. https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderInline.h (right): https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderInline.h:100: virtual LayoutRect absoluteClippedOverflowRect() const override final; Nit: You only need final here. That implies override.
https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderInline.cpp (right): https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderInline.cpp:1042: context(FloatRect(enclosingIntRect(rect))); On 2015/01/20 20:19:24, leviw wrote: > If you're eventually taking the enclosingIntRect of the result (on L1044), this > enclosingIntRect is extra work. Done. https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... File Source/core/rendering/RenderInline.h (right): https://codereview.chromium.org/767283005/diff/20001/Source/core/rendering/Re... Source/core/rendering/RenderInline.h:100: virtual LayoutRect absoluteClippedOverflowRect() const override final; On 2015/01/20 20:19:24, leviw wrote: > Nit: You only need final here. That implies override. I don't have to change this line now after rebaseline.
LGTM.
The CQ bit was checked by c.shu@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/767283005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188780 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
