Fix error in determining the target position for smooth scroll animation curve

Fix error in determining the target position for the smooth scroll
animation curve.

This is for the user scroll events that have a
mouse wheel handler and get handled on the main thread.

This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it
is waiting to be sent to the the compositor.

BUG=552556
Committed: https://crrev.com/d0a670ec608697b975b7ca8e5bf9ef976afd4862
Cr-Commit-Position: refs/heads/master@{#366799}

[ymalik, 2015-12-23 18:48:57]

Description was changed from

==========
Fix bug in ScrollAnimator

BUG=
==========

to

==========
This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it 
is waiting to be sent to the the compositor.

BUG=552556
==========

[ymalik, 2015-12-23 18:48:57]

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

[Rick Byers, 2015-12-23 19:13:34]

skobes@ is obviously the expert here, but with most everyone out for the
holidays I'm happy to do my best to review.  But /ccing skobes@ so that he can
take a look when he comes back and give you any post-submit suggestions.

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll.html
(right):

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll.html:29:
shouldBecomeEqual("document.scrollingElement.scrollTop == 80 && " +
Should this perhaps use different expected scroll position values than the first
scroll?  That way if the test is broken and no scrolling happens at all for this
second test, it'll fail instead of just maintaining the value from the first
test?

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:64: return
m_animationCurve || m_runState == RunState::WaitingToSendToCompositor
nit: please add brackets around the condition expression so that the reader
doesn't need to remember that || binds more tightly that ?:

[ymalik, 2015-12-23 19:20:37]

ymalik@chromium.org changed reviewers:
+ jbroman@chromium.org

[ymalik, 2015-12-23 19:20:38]

+Jeremy for Source/platform/scroll

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll.html
(right):

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/smooth-scroll/mousewheel-scroll.html:29:
shouldBecomeEqual("document.scrollingElement.scrollTop == 80 && " +
On 2015/12/23 19:13:33, Rick Byers wrote:
> Should this perhaps use different expected scroll position values than the
first
> scroll?  That way if the test is broken and no scrolling happens at all for
this
> second test, it'll fail instead of just maintaining the value from the first
> test?

Done.

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1547003002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:64: return
m_animationCurve || m_runState == RunState::WaitingToSendToCompositor
On 2015/12/23 19:13:34, Rick Byers wrote:
> nit: please add brackets around the condition expression so that the reader
> doesn't need to remember that || binds more tightly that ?:

Done.

[Rick Byers, 2015-12-23 19:21:52]

LGTM

[Rick Byers, 2015-12-23 19:25:29]

Actually one little thing: please clarify in the CL summary that this is about
smooth scroll (there's not enough big-picture context in "fix error in
determining ...").

Also the first line of your CL description should repeat the CL summary (I think
it's only the description that gets included as the git CL description - the
title is just for rietveld).  You don't want git to see the summary as: "This
bug was introduced in crrev.com/1534813004, where when we"

[ymalik, 2015-12-23 19:30:34]

Description was changed from

==========
This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it 
is waiting to be sent to the the compositor.

BUG=552556
==========

to

==========
Fix error in determining the target position for the smooth scroll 
animation curve. 

This is for the user scroll events that have a
mouse wheel handler and get handled on the main thread.

This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it 
is waiting to be sent to the the compositor.

BUG=552556
==========

[ymalik, 2015-12-23 19:31:01]

On 2015/12/23 19:25:29, Rick Byers wrote:
> Actually one little thing: please clarify in the CL summary that this is about
> smooth scroll (there's not enough big-picture context in "fix error in
> determining ...").
> 
> Also the first line of your CL description should repeat the CL summary (I
think
> it's only the description that gets included as the git CL description - the
> title is just for rietveld).  You don't want git to see the summary as: "This
> bug was introduced in crrev.com/1534813004, where when we"

Done. Thanks.

[Rick Byers, 2015-12-23 19:31:55]

LGTM thanks

[jbroman, 2015-12-23 19:39:38]

rbyers is an owner of Source/platform/ (by virtue of being in API_OWNERS), so
you're welcome to land without a review from me.

[ymalik, 2015-12-23 19:42:22]

On 2015/12/23 19:39:38, jbroman wrote:
> rbyers is an owner of Source/platform/ (by virtue of being in API_OWNERS), so
> you're welcome to land without a review from me.

Ah I see. The extension I use to find owners suggests that ScrollAnimator still
needs
an lgtm. Will submit. Thanks.

[ymalik, 2015-12-23 19:42:44]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2015-12-23 19:43:34]

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

[commit-bot: I haz the power, 2015-12-23 21:28:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-23 21:28:05]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ymalik, 2015-12-24 02:36:54]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2015-12-24 02:37:11]

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

[commit-bot: I haz the power, 2015-12-24 04:04:36]

Description was changed from

==========
Fix error in determining the target position for the smooth scroll 
animation curve. 

This is for the user scroll events that have a
mouse wheel handler and get handled on the main thread.

This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it 
is waiting to be sent to the the compositor.

BUG=552556
==========

to

==========
Fix error in determining the target position for the smooth scroll 
animation curve. 

This is for the user scroll events that have a
mouse wheel handler and get handled on the main thread.

This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it 
is waiting to be sent to the the compositor.

BUG=552556
==========

[commit-bot: I haz the power, 2015-12-24 04:04:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-24 04:06:04]

Description was changed from

==========
Fix error in determining the target position for the smooth scroll 
animation curve. 

This is for the user scroll events that have a
mouse wheel handler and get handled on the main thread.

This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it 
is waiting to be sent to the the compositor.

BUG=552556
==========

to

==========
Fix error in determining the target position for the smooth scroll
animation curve.

This is for the user scroll events that have a
mouse wheel handler and get handled on the main thread.

This bug was introduced in crrev.com/1534813004, where when we
get consecutive scroll events, the first one overwritten if it
is waiting to be sent to the the compositor.

BUG=552556

Committed: https://crrev.com/d0a670ec608697b975b7ca8e5bf9ef976afd4862
Cr-Commit-Position: refs/heads/master@{#366799}
==========

[commit-bot: I haz the power, 2015-12-24 04:06:05]

Patchset 3 (id:??) landed as
https://crrev.com/d0a670ec608697b975b7ca8e5bf9ef976afd4862
Cr-Commit-Position: refs/heads/master@{#366799}