Recompute overflow after transform changes

Recompute overflow after transform changes

This CL adds a special code path that is very similar to simplifiedLayout.
When the only change on an element is the transform, we can do the minimal
work to recompute the layout overflow without much overhead from layout.

This is done by adding an overflow recalculation phase after style changes,
if a RenderObject doesn't need layout and has moved transformed children,
the special code path will kick in.

BUG=352460
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171923

[esprehn, 2014-03-26 02:06:19]

The overflow computation should probably be in a step at the end of recalcStyle,
otherwise you do it way too much if you transform many things at once.

You want to do containingBlock()->setOverflowDirty() in recalcStyle and then
traverse the tree and update them after.

https://codereview.chromium.org/203463007/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/203463007/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderObject.cpp:2117:
container->updateOverflowForTransform();
This needs to be async, otherwise if you transform a thousand things you
recompute this a thousand times.

[trchen, 2014-03-28 04:16:52]

Hello esprehn, this is the new version of the CL as we discussed offline last
time. I haven't run any numbers for it yet but PTAL when you have time. Thanks!

[trchen, 2014-03-28 23:16:18]

Hey the latest patchset looks much more normal (correctness and
performance-wise). Just need to update test expectations later. I think it is
ready for review. PTAL, thanks!

[Xianzhu, 2014-04-10 20:39:57]

It looks great to remove the SimplifiedLayout flags. You might also want to
remove the comments mentioning the flags (e.g. in RenderStyle.cpp). Would be
awesome if my StyleDifference change could be based on this one.

[esprehn, 2014-04-10 23:46:14]

I think you have a bug where you go up one level too many in the styleDidChange
case.

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:4926: if (!overflowChanged)
I don't think you need this local variable

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1801: diff = StyleDifferenceLayout;
Why the special case for SVG?

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2103:
container->markContainingBlocksForOverflowRecalc();
This doesn't look right, you want to call
markContainingBlocksForOverflowRecalc() on yourself like we do above, not go up
one level from there.

[trchen, 2014-04-11 22:04:55]

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:4926: if (!overflowChanged)
On 2014/04/10 23:46:14, esprehn wrote:
> I don't think you need this local variable

Done.

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1801: diff = StyleDifferenceLayout;
On 2014/04/10 23:46:14, esprehn wrote:
> Why the special case for SVG?

Because SVG is special. They do rely on layout to update transform correctly.
This behavior is the same as before. The logic was somehow obfuscated in the big
if-block we just removed.

https://codereview.chromium.org/203463007/diff/130001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:2103:
container->markContainingBlocksForOverflowRecalc();
On 2014/04/10 23:46:14, esprehn wrote:
> This doesn't look right, you want to call
> markContainingBlocksForOverflowRecalc() on yourself like we do above, not go
up
> one level from there.

Done.

[trchen, 2014-04-18 03:33:06]

Do you have other concern?

[esprehn, 2014-04-18 04:35:26]

Patch looks great, thanks for fixing this!

lgtm

[trchen, 2014-04-18 04:56:46]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2014-04-18 04:56:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/203463007/210001

[commit-bot: I haz the power, 2014-04-18 05:17:31]

Change committed as 171923

[ojan, 2014-04-19 02:19:30]

This change is great. Just a few things you could maybe do in a followup patch?

https://codereview.chromium.org/203463007/diff/210001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:619: if (!renderView ||
!renderView->needsOverflowRecalcAfterStyleChange())
Can renderView really ever be null here? We only ever call this as
view()->recalcOverflowAfterStyleChange(). So we should be able to
ASSERT(renderView) here.

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:4888: continue;
These 6 lines of code are identical to the ones in the loop above. Maybe move to
a static helper function?

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1953:
markContainingBlocksForOverflowRecalc();
Not sure if this matters, but you probably want to skip this if it already
needed overflow recalc. Otherwise, we'll call containingBlock, which can walk up
the ancestor chain.

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1242: void
setNeedsSimplifiedNormalFlowLayout(bool b) {
m_bitfields.setNeedsSimplifiedNormalFlowLayout(b); }
Can you delete this as well? And of course all the code that uses this dirty
bit.

[trchen, 2014-04-19 05:08:21]

Thanks for the suggestion! I will upload a followup CL soon.

https://codereview.chromium.org/203463007/diff/210001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:619: if (!renderView ||
!renderView->needsOverflowRecalcAfterStyleChange())
On 2014/04/19 02:19:30, ojan wrote:
> Can renderView really ever be null here? We only ever call this as
> view()->recalcOverflowAfterStyleChange(). So we should be able to
> ASSERT(renderView) here.

Sounds good. Will do it.

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:4888: continue;
On 2014/04/19 02:19:30, ojan wrote:
> These 6 lines of code are identical to the ones in the loop above. Maybe move
to
> a static helper function?

Sounds reasonable to me. Will do it.

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:1953:
markContainingBlocksForOverflowRecalc();
On 2014/04/19 02:19:30, ojan wrote:
> Not sure if this matters, but you probably want to skip this if it already
> needed overflow recalc. Otherwise, we'll call containingBlock, which can walk
up
> the ancestor chain.

Yes I think we should do the early exit. Will do it.

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
Source/core/rendering/RenderObject.h:1242: void
setNeedsSimplifiedNormalFlowLayout(bool b) {
m_bitfields.setNeedsSimplifiedNormalFlowLayout(b); }
On 2014/04/19 02:19:30, ojan wrote:
> Can you delete this as well? And of course all the code that uses this dirty
> bit.

It still serves a purpose to indicate that "some RenderObject in my subtree
needs positioned movement layout, and I'll have to recompute my overflow after
that.".

I have thought about whether we can also make a fast path to move positioned
elements without doing a layout. I haven't thought through it yet. If it is
doable then we can remove these dirty bits altogether.

[ojan, 2014-04-21 19:44:27]

> File Source/core/rendering/RenderObject.h (right):
> 
>
https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
> Source/core/rendering/RenderObject.h:1242: void
> setNeedsSimplifiedNormalFlowLayout(bool b) {
> m_bitfields.setNeedsSimplifiedNormalFlowLayout(b); }
> On 2014/04/19 02:19:30, ojan wrote:
> > Can you delete this as well? And of course all the code that uses this dirty
> > bit.
> 
> It still serves a purpose to indicate that "some RenderObject in my subtree
> needs positioned movement layout, and I'll have to recompute my overflow after
> that.".

Got it.

> I have thought about whether we can also make a fast path to move positioned
> elements without doing a layout. I haven't thought through it yet. If it is
> doable then we can remove these dirty bits altogether.

Yes. I think we can and should do this. See
https://code.google.com/p/chromium/issues/detail?id=313562#c11

[James Cook, 2014-04-22 17:27:36]

On 2014/04/21 19:44:27, ojan wrote:
> > File Source/core/rendering/RenderObject.h (right):
> > 
> >
>
https://codereview.chromium.org/203463007/diff/210001/Source/core/rendering/R...
> > Source/core/rendering/RenderObject.h:1242: void
> > setNeedsSimplifiedNormalFlowLayout(bool b) {
> > m_bitfields.setNeedsSimplifiedNormalFlowLayout(b); }
> > On 2014/04/19 02:19:30, ojan wrote:
> > > Can you delete this as well? And of course all the code that uses this
dirty
> > > bit.
> > 
> > It still serves a purpose to indicate that "some RenderObject in my subtree
> > needs positioned movement layout, and I'll have to recompute my overflow
after
> > that.".
> 
> Got it.
> 
> > I have thought about whether we can also make a fast path to move positioned
> > elements without doing a layout. I haven't thought through it yet. If it is
> > doable then we can remove these dirty bits altogether.
> 
> Yes. I think we can and should do this. See
> https://code.google.com/p/chromium/issues/detail?id=313562#c11

This change is apparently causing test failures at the Chrome OS login screen.
See crbug.com/365507 - can the patch be reverted?

[eseidel, 2014-04-22 18:13:07]

Is the CrOS login screen special? or does this issue repro in other
configurations of Chrome?

[aboxhall, 2014-04-22 18:13:48]

A revert of this CL has been created in
https://codereview.chromium.org/246783004/ by aboxhall@chromium.org.

The reason for reverting is: Seems very likely to have broken ChromeOS login
page: https://code.google.com/p/chromium/issues/detail?id=365507.