Send scroll animation takeOver message in the same frame that it's created

Send scroll animation takeOver message in the same frame that it's created

We send a takeOver message from the MT to CC when a main thread scrolling
reason is added. Before this CL, that msg actually made its way to CC when
::updateCompositorAnimations was called, needlessly skipping a frame.

BUG=615970
Committed: https://crrev.com/73c83ce1b28eea72dc6b4c109adf1e6915533656
Cr-Commit-Position: refs/heads/master@{#401705}

[ymalik, 2016-06-21 21:14:27]

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

[ymalik, 2016-06-21 21:14:40]

Description was changed from

==========
Send scroll animation takeOver message in the same frame that it's created

We send a takeOver message from the MT to CC when a main thread scrolling
reason is added. Before this CL, that msg actually made its way to CC when
::updateCompositorAnimations was called, needlessly skipping a frame.

BUG=615970
==========

to

==========
Send scroll animation takeOver message in the same frame that it's created

We send a takeOver message from the MT to CC when a main thread scrolling
reason is added. Before this CL, that msg actually made its way to CC when
::updateCompositorAnimations was called, needlessly skipping a frame.

BUG=615970
==========

[ymalik, 2016-06-21 21:14:40]

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

[commit-bot: I haz the power, 2016-06-21 21:14:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2085873004/1

[commit-bot: I haz the power, 2016-06-21 21:40:08]

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

[commit-bot: I haz the power, 2016-06-21 21:40:10]

Dry run: Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[ajuma, 2016-06-21 23:27:04]

lgtm

[ymalik, 2016-06-21 23:37:08]

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

[ymalik, 2016-06-21 23:37:23]

+jbroman for Source/platform

[jbroman, 2016-06-22 15:25:34]

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:314:
ScrollAnimatorCompositorCoordinator::updateCompositorAnimations();
Is it intentional that this is explicitly called this way? It seems confusing to
call a virtual method this way. Is there a reason this shouldn't do a virtual
call?

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:160:
// DocumentLifecycle::LifecycleState::CompositingClean.
Can you DCHECK this?

[ymalik, 2016-06-22 19:46:00]

jbroman, PTAL :)

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
(right):

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:314:
ScrollAnimatorCompositorCoordinator::updateCompositorAnimations();
On 2016/06/22 15:25:33, jbroman wrote:
> Is it intentional that this is explicitly called this way? It seems confusing
to
> call a virtual method this way. Is there a reason this shouldn't do a virtual
> call?

We only need to do what the base class is doing. 

Refactored into separate function for clarity.

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/2085873004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:160:
// DocumentLifecycle::LifecycleState::CompositingClean.
On 2016/06/22 15:25:33, jbroman wrote:
> Can you DCHECK this?

The DocumentLifecycle is in core and not accessible from platform.  Added DCHECK
in the caller.

[jbroman, 2016-06-22 19:50:36]

lgtm

[ymalik, 2016-06-22 19:52:15]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-06-22 19:52:15]

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/2085873004/#ps40001
(title: "fix assert")

[commit-bot: I haz the power, 2016-06-22 19:52:43]

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

[commit-bot: I haz the power, 2016-06-22 21:03:41]

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

[commit-bot: I haz the power, 2016-06-22 21:03:42]

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-06-23 19:11:12]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-06-23 19:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2085873004/#ps60001
(title: "fix DCHECK")

[commit-bot: I haz the power, 2016-06-23 19:11:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2085873004/60001

[jbroman, 2016-06-23 19:12:27]

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

https://codereview.chromium.org/2085873004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:671:
DCHECK(m_page->deprecatedLocalMainFrame()->document()->lifecycle().state() >=
DocumentLifecycle::CompositingClean);
nit: suggest DCHECK_GE for nicer error messages

[commit-bot: I haz the power, 2016-06-23 20:33:42]

Description was changed from

==========
Send scroll animation takeOver message in the same frame that it's created

We send a takeOver message from the MT to CC when a main thread scrolling
reason is added. Before this CL, that msg actually made its way to CC when
::updateCompositorAnimations was called, needlessly skipping a frame.

BUG=615970
==========

to

==========
Send scroll animation takeOver message in the same frame that it's created

We send a takeOver message from the MT to CC when a main thread scrolling
reason is added. Before this CL, that msg actually made its way to CC when
::updateCompositorAnimations was called, needlessly skipping a frame.

BUG=615970
==========

[commit-bot: I haz the power, 2016-06-23 20:33:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-23 20:36:28]

Description was changed from

==========
Send scroll animation takeOver message in the same frame that it's created

We send a takeOver message from the MT to CC when a main thread scrolling
reason is added. Before this CL, that msg actually made its way to CC when
::updateCompositorAnimations was called, needlessly skipping a frame.

BUG=615970
==========

to

==========
Send scroll animation takeOver message in the same frame that it's created

We send a takeOver message from the MT to CC when a main thread scrolling
reason is added. Before this CL, that msg actually made its way to CC when
::updateCompositorAnimations was called, needlessly skipping a frame.

BUG=615970

Committed: https://crrev.com/73c83ce1b28eea72dc6b4c109adf1e6915533656
Cr-Commit-Position: refs/heads/master@{#401705}
==========

[commit-bot: I haz the power, 2016-06-23 20:36:29]

Patchset 4 (id:??) landed as
https://crrev.com/73c83ce1b28eea72dc6b4c109adf1e6915533656
Cr-Commit-Position: refs/heads/master@{#401705}