Don't skip a frame when running smooth scroll animation on the main thread

Don't skip a frame when running smooth scroll animation on the main thread

This CL doesn't really fix the flickering in issue 588560, but its still
more correct to go into RunningOnMainThread rather than going into
WaitingToSendToCompositor needlessly.

BUG=588560
Committed: https://crrev.com/5bfc49a00f133a14bd75aec67add16e4ffc84054
Cr-Commit-Position: refs/heads/master@{#397442}

[ymalik, 2016-06-02 00:09:34]

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

[ymalik, 2016-06-02 00:09:34]

PTAL :)

[ymalik, 2016-06-02 00:11:03]

The CQ bit was checked by ymalik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 00:12:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029323003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2029323003/20001

[commit-bot: I haz the power, 2016-06-02 02:04:30]

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

[commit-bot: I haz the power, 2016-06-02 02:04:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ajuma, 2016-06-02 14:07:27]

lgtm

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

https://codereview.chromium.org/2029323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:390: return;
cc::Layer::AddMainThreadScrollingReasons already earlies-out when adding an
existing reason, so do we need this additional early-out here?

[ymalik, 2016-06-02 14:13:10]

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

https://codereview.chromium.org/2029323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:390: return;
On 2016/06/02 14:07:27, ajuma wrote:
> cc::Layer::AddMainThreadScrollingReasons already earlies-out when adding an
> existing reason, so do we need this additional early-out here?

We essentially call this function every time we're in RunningOnMainThread from
updateCompositorAnimation. I added this so that we aren't relying on
cc::Layer::AddMainThreadScrollingReasons to do so. I don't have a strong opinion
either way. WDYT?

[ajuma, 2016-06-02 14:20:27]

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

https://codereview.chromium.org/2029323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:390: return;
On 2016/06/02 14:13:10, ymalik wrote:
> On 2016/06/02 14:07:27, ajuma wrote:
> > cc::Layer::AddMainThreadScrollingReasons already earlies-out when adding an
> > existing reason, so do we need this additional early-out here?
> 
> We essentially call this function every time we're in RunningOnMainThread from
> updateCompositorAnimation. I added this so that we aren't relying on
> cc::Layer::AddMainThreadScrollingReasons to do so. I don't have a strong
opinion
> either way. WDYT?

The call to mainThreadScrollingReasons() winds up in
cc::Layer::main_thread_scrolling_reasons(), so I think in terms of efficiency it
basically amounts to the same thing. But I don't feel strongly about it either.

[ymalik, 2016-06-02 14:30:07]

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

[ymalik, 2016-06-02 14:30:08]

+jbroman for Source/platform

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

https://codereview.chromium.org/2029323003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:390: return;
On 2016/06/02 14:20:27, ajuma wrote:
> On 2016/06/02 14:13:10, ymalik wrote:
> > On 2016/06/02 14:07:27, ajuma wrote:
> > > cc::Layer::AddMainThreadScrollingReasons already earlies-out when adding
an
> > > existing reason, so do we need this additional early-out here?
> > 
> > We essentially call this function every time we're in RunningOnMainThread
from
> > updateCompositorAnimation. I added this so that we aren't relying on
> > cc::Layer::AddMainThreadScrollingReasons to do so. I don't have a strong
> opinion
> > either way. WDYT?
> 
> The call to mainThreadScrollingReasons() winds up in
> cc::Layer::main_thread_scrolling_reasons(), so I think in terms of efficiency
it
> basically amounts to the same thing. But I don't feel strongly about it
either.

Removed.

[ymalik, 2016-06-02 14:30:36]

The CQ bit was checked by ymalik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-02 14:30:52]

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

[jbroman, 2016-06-02 14:43:00]

rs lgtm given ajuma's expertise

[commit-bot: I haz the power, 2016-06-02 16:09:31]

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

[commit-bot: I haz the power, 2016-06-02 16:09:32]

Dry run: This issue passed the CQ dry run.

[ymalik, 2016-06-02 16:38:35]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-06-02 16:38:36]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2029323003/#ps40001
(title: "remove early out from addMainThreadScrollingReason")

[commit-bot: I haz the power, 2016-06-02 16:39:16]

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

[commit-bot: I haz the power, 2016-06-02 16:44:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-02 16:45:46]

Description was changed from

==========
Don't skip a frame when running smooth scroll animation on the main thread

This CL doesn't really fix the flickering in issue 588560, but its still
more correct to go into RunningOnMainThread rather than going into
WaitingToSendToCompositor needlessly.

BUG=588560
==========

to

==========
Don't skip a frame when running smooth scroll animation on the main thread

This CL doesn't really fix the flickering in issue 588560, but its still
more correct to go into RunningOnMainThread rather than going into
WaitingToSendToCompositor needlessly.

BUG=588560

Committed: https://crrev.com/5bfc49a00f133a14bd75aec67add16e4ffc84054
Cr-Commit-Position: refs/heads/master@{#397442}
==========

[commit-bot: I haz the power, 2016-06-02 16:45:47]

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