Fix smooth scroll jump when switching scroll handling between MT and CC

Fix smooth scroll jump when switching scroll handling from MT to CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL finishes the
the animations on MT before switching to compositor scrolling.

ScrollingCoordinator proxys the call to clear main thread scrolling reasons to
ScrollAnimator which calls weblayer->clearMTSReasons when the scroll offset
animation is finished. ScrollAnimator adds a temporary main thread scrolling
reason until the animation is finished.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/002b8a254e34de53be24bea77e3e3ad3fd640f29
Cr-Commit-Position: refs/heads/master@{#377476}

[ymalik, 2016-01-29 16:57:51]

Description was changed from

==========
Fix smooth scroll judder when switching scroll handling between MT and CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL aborts the
animation when the thread that handles the scrolls changes.

BUG=581875
==========

to

==========
Fix smooth scroll judder when switching scroll handling between MT and CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL aborts the
animation when the thread that handles the scrolls changes.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-02-04 01:43:19]

Description was changed from

==========
Fix smooth scroll judder when switching scroll handling between MT and CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL aborts the
animation when the thread that handles the scrolls changes.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix smooth scroll jump when switching scroll handling from MT to CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL finishes the
the animations on MT before switching to compositor scrolling.

ScrollingCoordinator proxys the call to clear main thread scrolling reasons to
ScrollAnimator which calls weblayer->clearMTSReasons when the scroll offset 
animation is finished. ScrollAnimator adds a temporary main thread scrolling 
reason until the animation is finished.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-02-04 01:43:19]

ymalik@chromium.org changed reviewers:
+ ajuma@chromium.org, skobes@chromium.org

[ymalik, 2016-02-04 01:45:48]

Hey guys, 

Any tips on how I can test this?

@skobes
Can you verify that this patch fixes the jump issue in crbug.com/581875

[ajuma, 2016-02-04 15:05:48]

https://codereview.chromium.org/1648293003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/1648293003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:347:
scrollbarGraphicsLayer->setMainThreadScrollingReasons(MainThreadScrollingReason::kScrollbarScrolling);
Is changing this from adding a reason to replacing the existing reasons
intentional?

https://codereview.chromium.org/1648293003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/1648293003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:1192:
scrollLayer->addMainThreadScrollingReasons(reasons);
Would it simplify things if the main thread scroll animator always adds a main
thread scrolling reason when it starts an animation (or when a composited layer
is created during an animation;
ScrollAnimator::layerForCompositedScrollingDidChange gets called in that case)
and removes this reason when it finishes? Then other code is free to add/remove
other reasons independently, without having to tell the scrollable area.

[ymalik, 2016-02-04 18:45:59]

@Ali, PTAL :)

I removed the proxying clear main thread scrolling reasons call to scrollable
area by always setting a temporary MT scrolling reason from scroll animator.

[skobes, 2016-02-04 20:04:40]

Confirmed, this fixes the jump. :)

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:694:
uint32_t mainThreadScrollingReasonsToClear = ~0;
I had to change this to ~0u to compile on Windows.

[ajuma, 2016-02-04 20:08:40]

https://codereview.chromium.org/1648293003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1648293003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:966: DCHECK(main_thread_scrolling_reasons_to_clear);
Is it possible that we reach here and main_thread_scrolling_reasons_ and
main_thread_scrolling_reasons_to_clear have an empty intersection? (If so, we
should still check for that and early out to avoid an unnecessary commit.)

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:197:
addMainThreadScrollingReason();
Will this also affect input-driven smooth scroll (given that it's in the base
class rather than in ScrollAnimator)?

[ymalik, 2016-02-04 21:53:20]

@Ali, 

PTAL :)

https://codereview.chromium.org/1648293003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1648293003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:966: DCHECK(main_thread_scrolling_reasons_to_clear);
On 2016/02/04 20:08:40, ajuma wrote:
> Is it possible that we reach here and main_thread_scrolling_reasons_ and
> main_thread_scrolling_reasons_to_clear have an empty intersection? (If so, we
> should still check for that and early out to avoid an unnecessary commit.)

Yeah totally. Perhaps we should do the same for AddMainThreadScrollingReasons
above?

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:694:
uint32_t mainThreadScrollingReasonsToClear = ~0;
On 2016/02/04 20:04:40, skobes wrote:
> I had to change this to ~0u to compile on Windows.

Thanks! Fixed.

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/1648293003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:197:
addMainThreadScrollingReason();
On 2016/02/04 20:08:40, ajuma wrote:
> Will this also affect input-driven smooth scroll (given that it's in the base
> class rather than in ScrollAnimator)?

You're right, this should not affect non*-input driven scroll. Moved the call
the clearing and adding to ScrollAnimator :).

[ajuma, 2016-02-04 22:45:29]

lgtm

https://codereview.chromium.org/1648293003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1648293003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:966: DCHECK(main_thread_scrolling_reasons_to_clear);
On 2016/02/04 21:53:20, ymalik1 wrote:
> On 2016/02/04 20:08:40, ajuma wrote:
> > Is it possible that we reach here and main_thread_scrolling_reasons_ and
> > main_thread_scrolling_reasons_to_clear have an empty intersection? (If so,
we
> > should still check for that and early out to avoid an unnecessary commit.)
> 
> Yeah totally. Perhaps we should do the same for AddMainThreadScrollingReasons
> above?

Yes, please do.

[skobes, 2016-02-04 22:56:20]

lgtm2

[ymalik, 2016-02-04 23:49:41]

Hey Guys,

Any tips on how to test this? More specifically, I'd be nice to test that an
animation started on MT actually finishes on MT while other main thread
scrolling reasons are cleared.

[skobes, 2016-02-04 23:59:14]

On 2016/02/04 23:49:41, ymalik1 wrote:
> Any tips on how to test this? More specifically, I'd be nice to test that an
> animation started on MT actually finishes on MT while other main thread
> scrolling reasons are cleared.

You may be able to write a layout test that checks
internals.getScrollAnimationState, similar to
virtual/threaded/fast/scroll-behavior/smooth-scroll/fixed-background-in-iframe.html.

[ymalik, 2016-02-05 17:56:35]

PTAL :)

I added a layout test that checks whether or not the main thread scrolling
reason gets added and removed. Ideally I would also like to check the animation
run state as Steve suggested, but that's somewhat harder because the helper
method returns RootFrameViewport and we are scrolling the the LayoutViewport
(RunState is always idle). 

Although I think this should suffice for now.

[skobes, 2016-02-05 19:29:51]

> Ideally I would also like to check the animation
> run state as Steve suggested, but that's somewhat harder because the helper
> method returns RootFrameViewport and we are scrolling the the LayoutViewport
> (RunState is always idle).

Oops.. this is a trivial fix though, just call
frameView->layoutViewportScrollableArea() in Internals.cpp's
scrollableAreaForNode().

[skobes, 2016-02-05 19:31:43]

https://codereview.chromium.org/1648293003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html
(right):

https://codereview.chromium.org/1648293003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html:23:
".localeCompare('')", "0");
Curious, why do you need localeCompare instead of == ?

[ymalik, 2016-02-06 21:46:46]

On 2016/02/05 19:29:51, skobes wrote:
> > Ideally I would also like to check the animation
> > run state as Steve suggested, but that's somewhat harder because the helper
> > method returns RootFrameViewport and we are scrolling the the LayoutViewport
> > (RunState is always idle).
> 
> Oops.. this is a trivial fix though, just call
> frameView->layoutViewportScrollableArea() in Internals.cpp's
> scrollableAreaForNode().

Yes. But the reasons I left it out was that the layout viewport scrollable are
is not 
technically the frame view's scrollable area. So new layout tests that do a
pinch zoom 
and call scrollableAreaForNode will get the layout viewport (which may not be
what 
actually scrolled). But I guess in that case, they will currently get the RFV
scrollable area, 
which is not what scrolled anyway, so it's probably okay to return
layoutViewportSA.

[ymalik, 2016-02-06 21:49:06]

PTAL :)

https://codereview.chromium.org/1648293003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html
(right):

https://codereview.chromium.org/1648293003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html:23:
".localeCompare('')", "0");
On 2016/02/05 19:31:43, skobes wrote:
> Curious, why do you need localeCompare instead of == ?

No good reason. Just remembered comparing strings that way. Changed to == since
its clearer :)

https://codereview.chromium.org/1648293003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html
(right):

https://codereview.chromium.org/1648293003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/scroll-behavior/smooth-scroll/main-thread-scrolling-reason-added.html:54:
runTest();
I realized that doing the RUNNING_ON_COMPOSITOR checks in RAF was flaky because
sometimes it would run this while the state was still
WAITING_TO_SENT_TO_COMPOSITOR. So waiting for scrollTop to not be 0 above..

[skobes, 2016-02-09 00:52:56]

lgtm

[ymalik, 2016-02-09 01:14:37]

On 2016/02/09 00:52:56, skobes wrote:
> lgtm

Hey Guys,

I found some issues with this patch. Sorry should have mentioned earlier.

1) Because we add a temporary main thread scrolling reason, we always scroll on
the MT
because m_scrollableArea->shouldScrollOnMainThread() returns true. So instead of
calling
m_scrollableArea->shouldScrollOnMainThread(), we need to get the main thread
scrolling 
reasons, and see if its only because of kScrollingOnMainThread that was set
earlier. 
If so, we can still schedule the actual animation on the compositor. Without
this, for
MainThreadScrollingReasons like mousewheel listener, we will always scroll on
MT, which
is bad. 

2) We can't remove the temporary animation from resetAnimationState because
we're not in the
CompositingClean state. As per Ali's suggestion, I will add a new state
(something 
like PostAnimationCleanup) that we will be in after finishing an animation and
then remove the
temporary animation from ::updateCompositorAnimations. I will add the new state
logic in a 
separate CL to keep this CL focused.

Sorry for this long standing patch. Hopefully it's almost there :)

[skobes, 2016-02-09 01:26:22]

On 2016/02/09 01:14:37, ymalik1 wrote:
> 1) Because we add a temporary main thread scrolling reason, we always scroll
on
> the MT
> because m_scrollableArea->shouldScrollOnMainThread() returns true. So instead
of
> calling
> m_scrollableArea->shouldScrollOnMainThread(), we need to get the main thread
> scrolling 
> reasons, and see if its only because of kScrollingOnMainThread that was set
> earlier. 
> If so, we can still schedule the actual animation on the compositor. Without
> this, for
> MainThreadScrollingReasons like mousewheel listener, we will always scroll on
> MT, which
> is bad. 

The conceptual distinction here is "should the scroll position be updated on the
main thread" versus "should a scroll input be handled on the main thread". 
Maybe shouldScrollOnMainThread should return an enum {Yes, No,
InputHandlingOnly}, or maybe a separate method like
shouldHandleScrollInputOnMainThread.

[ymalik, 2016-02-10 16:33:50]

On 2016/02/09 01:26:22, skobes wrote:
> On 2016/02/09 01:14:37, ymalik1 wrote:
> > 1) Because we add a temporary main thread scrolling reason, we always scroll
> on
> > the MT
> > because m_scrollableArea->shouldScrollOnMainThread() returns true. So
instead
> of
> > calling
> > m_scrollableArea->shouldScrollOnMainThread(), we need to get the main thread
> > scrolling 
> > reasons, and see if its only because of kScrollingOnMainThread that was set
> > earlier. 
> > If so, we can still schedule the actual animation on the compositor. Without
> > this, for
> > MainThreadScrollingReasons like mousewheel listener, we will always scroll
on
> > MT, which
> > is bad. 
> 
> The conceptual distinction here is "should the scroll position be updated on
the
> main thread" versus "should a scroll input be handled on the main thread". 
> Maybe shouldScrollOnMainThread should return an enum {Yes, No,
> InputHandlingOnly}, or maybe a separate method like
> shouldHandleScrollInputOnMainThread.

I find that a little confusing because when you say "should the scroll input be
handled on the main thread", we are already on the MT. I find it easier to keep
shouldScrollOnMainThread, which strictly means should the animation be services
by the main thread. I added a conditional to
ScrollableArea::shouldScrollOnMainThread that checks if the only main thread
scrolling reason is the temporary one that is set by the scroll animator. If so,
then return false. PTAL :)

[ymalik, 2016-02-10 16:35:37]

@Ali, Steve 

I made a bunch of changes after your last lgtm. Can you PTAL :)

This changes include adding tests and fixing he bugs I described in the
comments.

[ajuma, 2016-02-10 18:43:32]

On 2016/02/10 16:35:37, ymalik1 wrote:
> @Ali, Steve 
> 
> I made a bunch of changes after your last lgtm. Can you PTAL :)
> 
> This changes include adding tests and fixing he bugs I described in the
> comments.

Still lgtm.

[skobes, 2016-02-10 18:53:34]

lgtm

On 2016/02/10 16:33:50, ymalik1 wrote:
> I find that a little confusing because when you say "should the scroll input
be
> handled on the main thread", we are already on the MT. I find it easier to
keep
> shouldScrollOnMainThread, which strictly means should the animation be
services
> by the main thread. I added a conditional to
> ScrollableArea::shouldScrollOnMainThread that checks if the only main thread
> scrolling reason is the temporary one that is set by the scroll animator. If
so,
> then return false. PTAL :)

The confusing part is that we have a non-transient "main thread scrolling
reason" that doesn't actually cause shouldScrollOnMainThread() to be true.  This
is because the cc::Layer's main_thread_scrolling_reasons_ is being used at two
different decision points:

(1) compositor thread deciding whether to forward input event to main
(2) main thread ScrollAnimator deciding whether to schedule animation on the
compositor

and these decisions no longer use the same criteria.  I'd prefer if this were
made more explicit in the API but I won't block this change on it.

[ymalik, 2016-02-10 20:11:21]

On 2016/02/10 18:53:34, skobes wrote:
> lgtm
> 
> On 2016/02/10 16:33:50, ymalik1 wrote:
> > I find that a little confusing because when you say "should the scroll input
> be
> > handled on the main thread", we are already on the MT. I find it easier to
> keep
> > shouldScrollOnMainThread, which strictly means should the animation be
> services
> > by the main thread. I added a conditional to
> > ScrollableArea::shouldScrollOnMainThread that checks if the only main thread
> > scrolling reason is the temporary one that is set by the scroll animator. If
> so,
> > then return false. PTAL :)
> 
> The confusing part is that we have a non-transient "main thread scrolling
> reason" that doesn't actually cause shouldScrollOnMainThread() to be true. 
This
> is because the cc::Layer's main_thread_scrolling_reasons_ is being used at two
> different decision points:
> 
> (1) compositor thread deciding whether to forward input event to main
> (2) main thread ScrollAnimator deciding whether to schedule animation on the
> compositor
> 
> and these decisions no longer use the same criteria.  I'd prefer if this were
> made more explicit in the API but I won't block this change on it.

Added a TODO to clean this up in a future CL. Thanks :)

[ymalik, 2016-02-10 20:12:05]

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

[ymalik, 2016-02-10 20:12:06]

+Jeremy for Source/platform

[jbroman, 2016-02-10 20:36:38]

The scrolling logic is a bit over my head, but rs lgtm (with nits).

https://codereview.chromium.org/1648293003/diff/220001/cc/input/main_thread_s...
File cc/input/main_thread_scrolling_reason.h (right):

https://codereview.chromium.org/1648293003/diff/220001/cc/input/main_thread_s...
cc/input/main_thread_scrolling_reason.h:21: enum : uint32_t {
kAnimatingScollOnMainThread = 1 << 13 };
nit: Scroll, not Scoll

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:45: WebLayer*
toWebLayer(blink::GraphicsLayer* layer)
Please make this a static function or enclose it in an anonymous namespace. It's
not advertised in the header, and it could conceivably collide with another
function sharing this name.

Also, this is in the blink namespace, so please remove the blink:: namespace
qualification.

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:53:
bool reattachCompositorPlayerIfNeeded(WebCompositorAnimationTimeline*);
nit: consider a short comment explaining what the bool means (it's not too hard
to find from the implementation, but still...)

[ymalik, 2016-02-10 21:57:57]

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

[ymalik, 2016-02-10 21:57:58]

+aelias 

Need your stamp for public/platform :)

https://codereview.chromium.org/1648293003/diff/220001/cc/input/main_thread_s...
File cc/input/main_thread_scrolling_reason.h (right):

https://codereview.chromium.org/1648293003/diff/220001/cc/input/main_thread_s...
cc/input/main_thread_scrolling_reason.h:21: enum : uint32_t {
kAnimatingScollOnMainThread = 1 << 13 };
On 2016/02/10 20:36:37, jbroman wrote:
> nit: Scroll, not Scoll

Wow good catch. nts: don't copy paste long variable names :)

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:45: WebLayer*
toWebLayer(blink::GraphicsLayer* layer)
On 2016/02/10 20:36:37, jbroman wrote:
> Please make this a static function or enclose it in an anonymous namespace.
It's
> not advertised in the header, and it could conceivably collide with another
> function sharing this name.
> 
> Also, this is in the blink namespace, so please remove the blink:: namespace
> qualification.

Done. Thanks!

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1648293003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:53:
bool reattachCompositorPlayerIfNeeded(WebCompositorAnimationTimeline*);
On 2016/02/10 20:36:38, jbroman wrote:
> nit: consider a short comment explaining what the bool means (it's not too
hard
> to find from the implementation, but still...)

Done.

[jbroman, 2016-02-10 22:42:55]

https://codereview.chromium.org/1648293003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1648293003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:45:
blink::WebLayer* toWebLayer(blink::GraphicsLayer* layer)
nit: you can put the anonymous namespace inside the blink namespace and remove
the blink:: qualifiers

[ymalik, 2016-02-11 16:05:44]

ymalik@chromium.org changed reviewers:
+ pdr@chromium.org
- aelias@chromium.org

[ymalik, 2016-02-11 16:05:45]

+pdr for public/Platform
- aelias I just realized he's on vacation

[pdr., 2016-02-12 00:13:34]

On 2016/02/11 at 16:05:45, ymalik wrote:
> +pdr for public/Platform
> - aelias I just realized he's on vacation

LGTM

[ymalik, 2016-02-12 17:55:52]

https://codereview.chromium.org/1648293003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1648293003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:45:
blink::WebLayer* toWebLayer(blink::GraphicsLayer* layer)
On 2016/02/10 22:42:55, jbroman wrote:
> nit: you can put the anonymous namespace inside the blink namespace and remove
> the blink:: qualifiers

Done.

[ymalik, 2016-02-24 21:00:28]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-02-24 21:00:29]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
skobes@chromium.org, jbroman@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1648293003/#ps280001
(title: "rebase master")

[commit-bot: I haz the power, 2016-02-24 21:06:03]

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

[commit-bot: I haz the power, 2016-02-24 22:45:54]

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

[commit-bot: I haz the power, 2016-02-24 22:45:55]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ymalik, 2016-02-25 00:58:54]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-02-25 00:58:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
skobes@chromium.org, pdr@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1648293003/#ps300001
(title: "skip irrelevant test on mac")

[commit-bot: I haz the power, 2016-02-25 01:02:53]

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

[commit-bot: I haz the power, 2016-02-25 02:44:33]

Description was changed from

==========
Fix smooth scroll jump when switching scroll handling from MT to CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL finishes the
the animations on MT before switching to compositor scrolling.

ScrollingCoordinator proxys the call to clear main thread scrolling reasons to
ScrollAnimator which calls weblayer->clearMTSReasons when the scroll offset 
animation is finished. ScrollAnimator adds a temporary main thread scrolling 
reason until the animation is finished.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix smooth scroll jump when switching scroll handling from MT to CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL finishes the
the animations on MT before switching to compositor scrolling.

ScrollingCoordinator proxys the call to clear main thread scrolling reasons to
ScrollAnimator which calls weblayer->clearMTSReasons when the scroll offset 
animation is finished. ScrollAnimator adds a temporary main thread scrolling 
reason until the animation is finished.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-02-25 02:44:35]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-02-25 02:45:25]

Description was changed from

==========
Fix smooth scroll jump when switching scroll handling from MT to CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL finishes the
the animations on MT before switching to compositor scrolling.

ScrollingCoordinator proxys the call to clear main thread scrolling reasons to
ScrollAnimator which calls weblayer->clearMTSReasons when the scroll offset 
animation is finished. ScrollAnimator adds a temporary main thread scrolling 
reason until the animation is finished.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix smooth scroll jump when switching scroll handling from MT to CC

To simulate position:sticky, some websites have JS code that conditionally
attaches position:fixed depending on the scroll offset. This CL finishes the
the animations on MT before switching to compositor scrolling.

ScrollingCoordinator proxys the call to clear main thread scrolling reasons to
ScrollAnimator which calls weblayer->clearMTSReasons when the scroll offset
animation is finished. ScrollAnimator adds a temporary main thread scrolling
reason until the animation is finished.

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/002b8a254e34de53be24bea77e3e3ad3fd640f29
Cr-Commit-Position: refs/heads/master@{#377476}
==========

[commit-bot: I haz the power, 2016-02-25 02:45:27]

Patchset 16 (id:??) landed as
https://crrev.com/002b8a254e34de53be24bea77e3e3ad3fd640f29
Cr-Commit-Position: refs/heads/master@{#377476}