Recompute slow scroll regions after overflow recalc

Recompute slow scroll regions after overflow recalc

This ensures that we always get correct slow scroll regions 
even when full layout is skipped in favor of faster OverflowRecalc.

TEST=scrollingcoordinator/non-fast-scrollable-transform-changed.html
BUG=417345
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201483

[majidvp, 2015-08-05 13:26:06]

majidvp@chromium.org changed reviewers:
+ rbyers@chromium.org

[majidvp, 2015-08-05 13:26:07]

trchen@: Is there an animation perf benchmark that I should keep an eye on for
regressions that this may cause?

[Rick Byers, 2015-08-05 15:02:59]

Thanks for working on this Majid!

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html
(right):

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:27: <iframe
id='nonFastRegion' onload='runAfterLayoutAndPaint(window.runTests)'
src="data:text/html;charset=utf-8,<div style='height:1000px; width:1000px;
background: linear-gradient(to bottom, red, white);'>This should be
covered.</div>">
nit: any reason to use 'src=data:text/html;...' instead of the more modern
'srcdoc='?

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:28:
</iframe>
The original bug report relied only on a div, not an iframe.  Any reason you
choose the more complicated iframe case here?  I assume the FrameView method you
hooked into gets invoked for the div case too, so you still fixed the original
bug right?

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:56: // wait
for 1 frame plus animation duration before checking
I'm a little worried that this test is brittle to other things that trigger
layout (if layout got triggered accidentally the test would pass without testing
your fix).  Can you add some checks for window.internals layout APIs to verify
that a layout hasn't occurred?  I think the typical pattern is to dirty layout
and verify that needsLayoutCount is still non-zero at the end of your test (eg.
see my scrollingElement-noLayout-quirks.html test).

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:70: function
waitForAnimationWalltime(wait) {
Is this guaranteed to be non-flaky?  I thought it was possible for the animation
to take longer than this (eg. under heavy load).  Can you point to an example of
another animation-time-dependent test for how we normally handle cases like
this?

Alternately maybe we could use the transitionend event?  Or even just a single
transform instead of an animation (just need to be careful to ensure you don't
start the test until all loading/layouts are done)?

https://codereview.chromium.org/1256043007/diff/20001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/1256043007/diff/20001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.h:67: // Called when any frame
recalculates its overflows after style change
Nit: add period to be consistent with style above
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_...).

[Rick Byers, 2015-08-05 15:04:05]

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

https://codereview.chromium.org/1256043007/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:646: if (ScrollingCoordinator*
scrollingCoordinator = this->scrollingCoordinator())
Please add a comment saying why this is the right place to hook (i.e. that
anything that updates overflow without causing layout also needs to update
non-fast scroll rects) - it's a bit of an implementation assumption that could
possibly change in the future I think.

[majidvp, 2015-08-05 15:39:35]

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html
(right):

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:27: <iframe
id='nonFastRegion' onload='runAfterLayoutAndPaint(window.runTests)'
src="data:text/html;charset=utf-8,<div style='height:1000px; width:1000px;
background: linear-gradient(to bottom, red, white);'>This should be
covered.</div>">
On 2015/08/05 15:02:59, Rick Byers wrote:
> nit: any reason to use 'src=data:text/html;...' instead of the more modern
> 'srcdoc='?

Acknowledged.

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:28:
</iframe>
On 2015/08/05 15:02:59, Rick Byers wrote:
> The original bug report relied only on a div, not an iframe.  Any reason you
> choose the more complicated iframe case here?  I assume the FrameView method
you
> hooked into gets invoked for the div case too, so you still fixed the original
> bug right?

The original case is fixed because the same code path is exercised. The div's
are not non-fast region on HDPI displays but iframes are. This means that if you
load the test interactively it is more likely that it works as expected with
iframe which is why I used it. Note run-webkit-tests runs in LDPI mode by
default so it does not matter there.

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:56: // wait
for 1 frame plus animation duration before checking
On 2015/08/05 15:02:59, Rick Byers wrote:
> I'm a little worried that this test is brittle to other things that trigger
> layout (if layout got triggered accidentally the test would pass without
testing
> your fix).  Can you add some checks for window.internals layout APIs to verify
> that a layout hasn't occurred?  I think the typical pattern is to dirty layout
> and verify that needsLayoutCount is still non-zero at the end of your test
(eg.
> see my scrollingElement-noLayout-quirks.html test).

This makes sense. Thanks for pointing it out.

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:70: function
waitForAnimationWalltime(wait) {
On 2015/08/05 15:02:59, Rick Byers wrote:
> Is this guaranteed to be non-flaky?  I thought it was possible for the
animation
> to take longer than this (eg. under heavy load).  Can you point to an example
of
> another animation-time-dependent test for how we normally handle cases like
> this?
> 
> Alternately maybe we could use the transitionend event?  Or even just a single
> transform instead of an animation (just need to be careful to ensure you don't
> start the test until all loading/layouts are done)?

I was originally looking at using webkitanimationend event but that didn't fire
for transition animation. I was not aware of |transitionend|. Using that makes
sense.

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

https://codereview.chromium.org/1256043007/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:646: if (ScrollingCoordinator*
scrollingCoordinator = this->scrollingCoordinator())
On 2015/08/05 15:04:05, Rick Byers wrote:
> Please add a comment saying why this is the right place to hook (i.e. that
> anything that updates overflow without causing layout also needs to update
> non-fast scroll rects) - it's a bit of an implementation assumption that could
> possibly change in the future I think.

Acknowledged.

https://codereview.chromium.org/1256043007/diff/20001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/1256043007/diff/20001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.h:67: // Called when any frame
recalculates its overflows after style change
On 2015/08/05 15:02:59, Rick Byers wrote:
> Nit: add period to be consistent with style above
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Punctuation,_...).

Acknowledged.

[Rick Byers, 2015-08-06 03:15:19]

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html
(right):

https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:28:
</iframe>
On 2015/08/05 15:39:35, majidvp wrote:
> On 2015/08/05 15:02:59, Rick Byers wrote:
> > The original bug report relied only on a div, not an iframe.  Any reason you
> > choose the more complicated iframe case here?  I assume the FrameView method
> you
> > hooked into gets invoked for the div case too, so you still fixed the
original
> > bug right?
> 
> The original case is fixed because the same code path is exercised. The div's
> are not non-fast region on HDPI displays but iframes are.

Are you sure about this?  I believe iframes are fast-scrolling on Android and
ChromeOS high-dpi at least (although IIRC there was a separate set of flags
involved in enabling that - maybe there is some confusion there?).

> This means that if you
> load the test interactively it is more likely that it works as expected with
> iframe which is why I used it. Note run-webkit-tests runs in LDPI mode by
> default so it does not matter there.

[Rick Byers, 2015-08-06 03:19:47]

https://codereview.chromium.org/1256043007/diff/40001/LayoutTests/scrollingco...
File LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html
(right):

https://codereview.chromium.org/1256043007/diff/40001/LayoutTests/scrollingco...
LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:58:
shouldBe('internals.needsLayoutCount()', '0');
I don't think this verifies that no layout occurred (I'm not sure we have a way
to do that directly).  Instead it just means that there's no layout pending.

But I guess you can't dirty layout as I was originally thinking since you're
waiting for an animation to finish - we'll definitely do a layout.

I see FrameView::layoutCount() but it looks like that's used only by unit tests
(perhaps it's too unpredictable for a layout test).  Can you find any example of
other tests that verify we don't do a layout during an animation?  What do the
tests for trchen's feature that regressed this scenario look like in this
regard?

[majidvp, 2015-08-29 00:23:30]

On 2015/08/06 03:15:19, Rick Byers wrote:
>
https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
> File LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html
> (right):
> 
>
https://codereview.chromium.org/1256043007/diff/20001/LayoutTests/scrollingco...
> LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:28:
> </iframe>
> On 2015/08/05 15:39:35, majidvp wrote:
> > On 2015/08/05 15:02:59, Rick Byers wrote:
> > > The original bug report relied only on a div, not an iframe.  Any reason
you
> > > choose the more complicated iframe case here?  I assume the FrameView
method
> > you
> > > hooked into gets invoked for the div case too, so you still fixed the
> original
> > > bug right?
> > 
> > The original case is fixed because the same code path is exercised. The
div's
> > are not non-fast region on HDPI displays but iframes are.
> 
> Are you sure about this?  I believe iframes are fast-scrolling on Android and
> ChromeOS high-dpi at least (although IIRC there was a separate set of flags
> involved in enabling that - maybe there is some confusion there?).
> 
You are right about iframes being fast scrollable on some hdpi. I have now
replaced
iframe with a DIV to better match the original bug.

[majidvp, 2015-08-29 00:36:05]

On 2015/08/06 03:19:47, Rick Byers wrote:
>
https://codereview.chromium.org/1256043007/diff/40001/LayoutTests/scrollingco...
> File LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html
> (right):
> 
>
https://codereview.chromium.org/1256043007/diff/40001/LayoutTests/scrollingco...
> LayoutTests/scrollingcoordinator/non-fast-scrollable-animation.html:58:
> shouldBe('internals.needsLayoutCount()', '0');
> I don't think this verifies that no layout occurred (I'm not sure we have a
way
> to do that directly).  Instead it just means that there's no layout pending.
> 
> But I guess you can't dirty layout as I was originally thinking since you're
> waiting for an animation to finish - we'll definitely do a layout.
> 
> I see FrameView::layoutCount() but it looks like that's used only by unit
tests
> (perhaps it's too unpredictable for a layout test).  Can you find any example
of
> other tests that verify we don't do a layout during an animation?  What do the
> tests for trchen's feature that regressed this scenario look like in this
> regard?

I went back and checked trchen@'s test cases. It turned out that it is possible
to
 exercise the same optimized layout path without needing animations. It only
requires the style difference to be limited to only a transform change[1]. I
used
this to simplify the test. It also makes it possible to ensure there is no
layout
happening.

[1] The surprising thing was that if style goes from no transform to transform
it 
does not hit this path but if it goes from a no-op transform (translateX(0)) to
a
transform it does. I didn't realize this initially. duh!

[Rick Byers, 2015-08-29 20:35:22]

Nice, this is much simpler / more direct - thank you!
LGTM

[majidvp, 2015-08-31 12:34:23]

The CQ bit was checked by majidvp@chromium.org

[commit-bot: I haz the power, 2015-08-31 12:34:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1256043007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1256043007/80001

[commit-bot: I haz the power, 2015-08-31 14:12:22]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201483