Making scheduler run ANIMATE after a COMMIT (instead of LayerTreeHostImpl).

Making scheduler run ANIMATE after a COMMIT (instead of LayerTreeHostImpl).

This moves the logic from the LayerTreeHostImpl into the scheduler which means
only ScheduledActionAnimate now updates animation state.

The further extends the work in http://crrev.com/206793003

BUG=346230
Committed: https://crrev.com/7fa5729cf6ac490cc3b257b7eb8093dcd1285e3a
Cr-Commit-Position: refs/heads/master@{#298396}

Committed: https://crrev.com/0bda234b69c475323123b89eba4a5e831f988821
Cr-Commit-Position: refs/heads/master@{#301358}

[mithro-old, 2014-10-01 15:26:34]

mithro@mithis.com changed reviewers:
+ ajuma@chromium.org, brianderson@chromium.org, danakj@chromium.org,
skyostil@google.com

[mithro-old, 2014-10-01 15:26:35]

Hello everyone,

This change is a continue of Sami's original work to make Animate a step that
the scheduler understands.

It is step (3) in the plan discussed at
https://codereview.chromium.org/347483002/. Noted that step (1) or step (2) have
not occurred yet as https://codereview.chromium.org/597623002/ shows, but I'm
unsure if we want to block this patch on them anymore...

This patch also blocking the moving of background ticking from LayerTreeHostImpl
to the Scheduler CL at https://codereview.chromium.org/595973002.

As there are likely to be interactions here that are not well tested or
understood, could you please take a careful look and make sure it is actually a
nop?

I haven't had the chance to check all the tests pass, but wanted to get this CL
out before Brian's vacation incase he had some thoughts.

Tim 'mithro' Ansell

[mithro-old, 2014-10-03 05:27:52]

Hi, 

Can people take a look? All the tests seem to pass and using a browser with this
seemed to work okay.

Tim

[ajuma, 2014-10-03 14:22:16]

On 2014/10/03 05:27:52, mithro wrote:
> Hi, 
> 
> Can people take a look? All the tests seem to pass and using a browser with
this
> seemed to work okay.
> 
> Tim

lgtm

[mithro-old, 2014-10-03 14:23:49]

I'll wait for Sami's LGTM before landing.

Tim 'mithro' Ansell
On 4 Oct 2014 00:22, <ajuma@chromium.org> wrote:

> On 2014/10/03 05:27:52, mithro wrote:
>
>> Hi,
>>
>
>  Can people take a look? All the tests seem to pass and using a browser
>> with
>>
> this
>
>> seemed to work okay.
>>
>
>  Tim
>>
>
> lgtm
>
> https://codereview.chromium.org/621823003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Sami, 2014-10-03 16:27:12]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2014-10-03 16:27:12]

Thanks for moving this where it belongs -- lgtm!

As a final sanity check before landing, please check that the boxes on this site
stay mostly aligned when you fling:
http://tdresser.github.io/sync-scroll-offset-test/ (disable rAF with the button
first).

[mithro-old, 2014-10-07 04:39:30]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-10-07 04:40:36]

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

[commit-bot: I haz the power, 2014-10-07 05:59:18]

Committed patchset #4 (id:50001) as b377b9cc65b7a999c1f9a9bcafc98a39ebdbf548

[commit-bot: I haz the power, 2014-10-07 06:00:25]

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

[loislo, 2014-10-14 07:06:24]

A revert of this CL (patchset #4 id:50001) has been created in
https://codereview.chromium.org/653013002/ by loislo@chromium.org.

The reason for reverting is: Some sites become "unresponsive" i.e. they work but
have no ui changes. and the user need to switch between tabs in order of getting
the actual page..

[mithro-old, 2014-10-14 07:08:04]

On 2014/10/14 07:06:24, loislo wrote:
> A revert of this CL (patchset #4 id:50001) has been created in
> https://codereview.chromium.org/653013002/ by mailto:loislo@chromium.org.
> 
> The reason for reverting is: Some sites become "unresponsive" i.e. they work
but
> have no ui changes. and the user need to switch between tabs in order of
getting
> the actual page..

Do you have a link to a bug, test case or something else that shows this?

[loislo, 2014-10-14 07:11:06]

On 2014/10/14 07:08:04, mithro wrote:
> On 2014/10/14 07:06:24, loislo wrote:
> > A revert of this CL (patchset #4 id:50001) has been created in
> > https://codereview.chromium.org/653013002/ by mailto:loislo@chromium.org.
> > 
> > The reason for reverting is: Some sites become "unresponsive" i.e. they work
> but
> > have no ui changes. and the user need to switch between tabs in order of
> getting
> > the actual page..
> 
> Do you have a link to a bug, test case or something else that shows this?

crbug.com/420254

[mithro-old, 2014-10-14 07:14:01]

I'm not seeing anything in that bug that indicates this change is the
culprit? Do you have some research not shared on that bug?

This patch also only landed on the 3rd of October, the Chrome version shown
there is stable, there is no way this patch has reached stable yet.

On 14 October 2014 18:11, <loislo@chromium.org> wrote:

> On 2014/10/14 07:08:04, mithro wrote:
>
>> On 2014/10/14 07:06:24, loislo wrote:
>> > A revert of this CL (patchset #4 id:50001) has been created in
>> > https://codereview.chromium.org/653013002/ by mailto:
>> loislo@chromium.org.
>> >
>> > The reason for reverting is: Some sites become "unresponsive" i.e. they
>> work
>> but
>> > have no ui changes. and the user need to switch between tabs in order of
>> getting
>> > the actual page..
>>
>
>  Do you have a link to a bug, test case or something else that shows this?
>>
>
> crbug.com/420254
>
> https://codereview.chromium.org/621823003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[loislo, 2014-10-14 07:23:40]

On 2014/10/14 07:11:06, loislo wrote:
> On 2014/10/14 07:08:04, mithro wrote:
> > On 2014/10/14 07:06:24, loislo wrote:
> > > A revert of this CL (patchset #4 id:50001) has been created in
> > > https://codereview.chromium.org/653013002/ by mailto:loislo@chromium.org.
> > > 
> > > The reason for reverting is: Some sites become "unresponsive" i.e. they
work
> > but
> > > have no ui changes. and the user need to switch between tabs in order of
> > getting
> > > the actual page..
> > 
> > Do you have a link to a bug, test case or something else that shows this?
> 
> crbug.com/420254

there are two other bugs that more specific but I can't share them with
non-googler. Sorry.

I bisected linux builds and found that the problem appeared between builds
298397 and 298385.
Further bisecting pointed me to this patch. I reverted it locally and it helped.

I hope danakj@ skyostil@ or ajuma@ will help you to solve the problem.

[loislo, 2014-10-14 07:26:35]

On 2014/10/14 07:23:40, loislo wrote:
> On 2014/10/14 07:11:06, loislo wrote:
> > On 2014/10/14 07:08:04, mithro wrote:
> > > On 2014/10/14 07:06:24, loislo wrote:
> > > > A revert of this CL (patchset #4 id:50001) has been created in
> > > > https://codereview.chromium.org/653013002/ by
mailto:loislo@chromium.org.
> > > > 
> > > > The reason for reverting is: Some sites become "unresponsive" i.e. they
> work
> > > but
> > > > have no ui changes. and the user need to switch between tabs in order of
> > > getting
> > > > the actual page..
> > > 
> > > Do you have a link to a bug, test case or something else that shows this?
> > 
> > crbug.com/420254
> 
> there are two other bugs that more specific but I can't share them with
> non-googler. Sorry.
> 
> I bisected linux builds and found that the problem appeared between builds
> 298397 and 298385.
> Further bisecting pointed me to this patch. I reverted it locally and it
helped.
> 
> I hope danakj@ skyostil@ or ajuma@ will help you to solve the problem.

It reached dev-channel.

[loislo, 2014-10-14 07:30:09]

On 2014/10/14 07:26:35, loislo wrote:
> On 2014/10/14 07:23:40, loislo wrote:
> > On 2014/10/14 07:11:06, loislo wrote:
> > > On 2014/10/14 07:08:04, mithro wrote:
> > > > On 2014/10/14 07:06:24, loislo wrote:
> > > > > A revert of this CL (patchset #4 id:50001) has been created in
> > > > > https://codereview.chromium.org/653013002/ by
> mailto:loislo@chromium.org.
> > > > > 
> > > > > The reason for reverting is: Some sites become "unresponsive" i.e.
they
> > work
> > > > but
> > > > > have no ui changes. and the user need to switch between tabs in order
of
> > > > getting
> > > > > the actual page..
> > > > 
> > > > Do you have a link to a bug, test case or something else that shows
this?
> > > 
> > > crbug.com/420254
> > 
> > there are two other bugs that more specific but I can't share them with
> > non-googler. Sorry.
> > 
> > I bisected linux builds and found that the problem appeared between builds
> > 298397 and 298385.
> > Further bisecting pointed me to this patch. I reverted it locally and it
> helped.
> > 
> > I hope danakj@ skyostil@ or ajuma@ will help you to solve the problem.
> 
> It reached dev-channel.

Looks like you are right. I was wrong with the bug.

[loislo, 2014-10-14 07:35:33]

On 2014/10/14 07:30:09, loislo wrote:
> On 2014/10/14 07:26:35, loislo wrote:
> > On 2014/10/14 07:23:40, loislo wrote:
> > > On 2014/10/14 07:11:06, loislo wrote:
> > > > On 2014/10/14 07:08:04, mithro wrote:
> > > > > On 2014/10/14 07:06:24, loislo wrote:
> > > > > > A revert of this CL (patchset #4 id:50001) has been created in
> > > > > > https://codereview.chromium.org/653013002/ by
> > mailto:loislo@chromium.org.
> > > > > > 
> > > > > > The reason for reverting is: Some sites become "unresponsive" i.e.
> they
> > > work
> > > > > but
> > > > > > have no ui changes. and the user need to switch between tabs in
order
> of
> > > > > getting
> > > > > > the actual page..
> > > > > 
> > > > > Do you have a link to a bug, test case or something else that shows
> this?
> > > > 
> > > > crbug.com/420254
> > > 
> > > there are two other bugs that more specific but I can't share them with
> > > non-googler. Sorry.
> > > 
> > > I bisected linux builds and found that the problem appeared between builds
> > > 298397 and 298385.
> > > Further bisecting pointed me to this patch. I reverted it locally and it
> > helped.
> > > 
> > > I hope danakj@ skyostil@ or ajuma@ will help you to solve the problem.
> > 
> > It reached dev-channel.
> 
> Looks like you are right. I was wrong with the bug.

I mean the behavior looks similar. I was misguided bu the latest comments in the
bug.
The tab does some animation and becomes visually unresponsive in the middle of
it but
I see blinking reload button which indicates that the tab does something.

If I switch to another tab and back I see the updated ui.

[mithro-old, 2014-10-14 07:38:37]

I'm a Googler and if you link the bugs I'll check them on my
tansell@chromium.org / tansell@google.com account.

Tim 'mithro' Ansell
On 14 Oct 2014 18:35, <loislo@chromium.org> wrote:

> On 2014/10/14 07:30:09, loislo wrote:
>
>> On 2014/10/14 07:26:35, loislo wrote:
>> > On 2014/10/14 07:23:40, loislo wrote:
>> > > On 2014/10/14 07:11:06, loislo wrote:
>> > > > On 2014/10/14 07:08:04, mithro wrote:
>> > > > > On 2014/10/14 07:06:24, loislo wrote:
>> > > > > > A revert of this CL (patchset #4 id:50001) has been created in
>> > > > > > https://codereview.chromium.org/653013002/ by
>> > mailto:loislo@chromium.org.
>> > > > > >
>> > > > > > The reason for reverting is: Some sites become "unresponsive"
>> i.e.
>> they
>> > > work
>> > > > > but
>> > > > > > have no ui changes. and the user need to switch between tabs in
>>
> order
>
>> of
>> > > > > getting
>> > > > > > the actual page..
>> > > > >
>> > > > > Do you have a link to a bug, test case or something else that
>> shows
>> this?
>> > > >
>> > > > crbug.com/420254
>> > >
>> > > there are two other bugs that more specific but I can't share them
>> with
>> > > non-googler. Sorry.
>> > >
>> > > I bisected linux builds and found that the problem appeared between
>> builds
>> > > 298397 and 298385.
>> > > Further bisecting pointed me to this patch. I reverted it locally and
>> it
>> > helped.
>> > >
>> > > I hope danakj@ skyostil@ or ajuma@ will help you to solve the
>> problem.
>> >
>> > It reached dev-channel.
>>
>
>  Looks like you are right. I was wrong with the bug.
>>
>
> I mean the behavior looks similar. I was misguided bu the latest comments
> in the
> bug.
> The tab does some animation and becomes visually unresponsive in the
> middle of
> it but
> I see blinking reload button which indicates that the tab does something.
>
> If I switch to another tab and back I see the updated ui.
>
>
> https://codereview.chromium.org/621823003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-10-14 07:42:17]

Can you please also share the reproduction method (or even better a test
case), otherwise I can't prevent it from occurring in the future.

Tim 'mithro' Ansell
On 14 Oct 2014 18:38, "Tim Ansell" <mithro@mithis.com> wrote:

> I'm a Googler and if you link the bugs I'll check them on my
> tansell@chromium.org / tansell@google.com account.
>
> Tim 'mithro' Ansell
> On 14 Oct 2014 18:35, <loislo@chromium.org> wrote:
>
>> On 2014/10/14 07:30:09, loislo wrote:
>>
>>> On 2014/10/14 07:26:35, loislo wrote:
>>> > On 2014/10/14 07:23:40, loislo wrote:
>>> > > On 2014/10/14 07:11:06, loislo wrote:
>>> > > > On 2014/10/14 07:08:04, mithro wrote:
>>> > > > > On 2014/10/14 07:06:24, loislo wrote:
>>> > > > > > A revert of this CL (patchset #4 id:50001) has been created in
>>> > > > > > https://codereview.chromium.org/653013002/ by
>>> > mailto:loislo@chromium.org.
>>> > > > > >
>>> > > > > > The reason for reverting is: Some sites become "unresponsive"
>>> i.e.
>>> they
>>> > > work
>>> > > > > but
>>> > > > > > have no ui changes. and the user need to switch between tabs in
>>>
>> order
>>
>>> of
>>> > > > > getting
>>> > > > > > the actual page..
>>> > > > >
>>> > > > > Do you have a link to a bug, test case or something else that
>>> shows
>>> this?
>>> > > >
>>> > > > crbug.com/420254
>>> > >
>>> > > there are two other bugs that more specific but I can't share them
>>> with
>>> > > non-googler. Sorry.
>>> > >
>>> > > I bisected linux builds and found that the problem appeared between
>>> builds
>>> > > 298397 and 298385.
>>> > > Further bisecting pointed me to this patch. I reverted it locally
>>> and it
>>> > helped.
>>> > >
>>> > > I hope danakj@ skyostil@ or ajuma@ will help you to solve the
>>> problem.
>>> >
>>> > It reached dev-channel.
>>>
>>
>>  Looks like you are right. I was wrong with the bug.
>>>
>>
>> I mean the behavior looks similar. I was misguided bu the latest comments
>> in the
>> bug.
>> The tab does some animation and becomes visually unresponsive in the
>> middle of
>> it but
>> I see blinking reload button which indicates that the tab does something.
>>
>> If I switch to another tab and back I see the updated ui.
>>
>>
>> https://codereview.chromium.org/621823003/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-10-14 07:58:04]

Wow, that was a Gmail fail; forwarding to loislo too.

I'm a Googler and if you link the bugs I'll check them on my
tansell@chromium.org / tansell@google.com account.

Can you please also share the full reproduction method (or even better a
test case), otherwise I can't prevent it from occurring again in the future.


On 14 October 2014 18:35, <loislo@chromium.org> wrote:

> On 2014/10/14 07:30:09, loislo wrote:
>
>> On 2014/10/14 07:26:35, loislo wrote:
>> > On 2014/10/14 07:23:40, loislo wrote:
>> > > On 2014/10/14 07:11:06, loislo wrote:
>> > > > On 2014/10/14 07:08:04, mithro wrote:
>> > > > > On 2014/10/14 07:06:24, loislo wrote:
>> > > > > > A revert of this CL (patchset #4 id:50001) has been created in
>> > > > > > https://codereview.chromium.org/653013002/ by
>> > mailto:loislo@chromium.org.
>> > > > > >
>> > > > > > The reason for reverting is: Some sites become "unresponsive"
>> i.e.
>> they
>> > > work
>> > > > > but
>> > > > > > have no ui changes. and the user need to switch between tabs in
>>
> order
>
>> of
>> > > > > getting
>> > > > > > the actual page..
>> > > > >
>> > > > > Do you have a link to a bug, test case or something else that
>> shows
>> this?
>> > > >
>> > > > crbug.com/420254
>> > >
>> > > there are two other bugs that more specific but I can't share them
>> with
>> > > non-googler. Sorry.
>> > >
>> > > I bisected linux builds and found that the problem appeared between
>> builds
>> > > 298397 and 298385.
>> > > Further bisecting pointed me to this patch. I reverted it locally and
>> it
>> > helped.
>> > >
>> > > I hope danakj@ skyostil@ or ajuma@ will help you to solve the
>> problem.
>> >
>> > It reached dev-channel.
>>
>
>  Looks like you are right. I was wrong with the bug.
>>
>
> I mean the behavior looks similar. I was misguided bu the latest comments
> in the
> bug.
> The tab does some animation and becomes visually unresponsive in the
> middle of
> it but
> I see blinking reload button which indicates that the tab does something.
>
> If I switch to another tab and back I see the updated ui.
>
>
> https://codereview.chromium.org/621823003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-10-15 07:18:18]

mithro@mithis.com changed reviewers:
+ loislo@chromium.org

[mithro-old, 2014-10-15 07:18:19]

https://codereview.chromium.org/621823003/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/621823003/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:339: // Ask to be animated if there are
animations
These lines appear to be the cause of the hang issue in
https://code.google.com/p/chromium/issues/detail?id=421931. I'm still trying to
figure out the exact cause though.

[Sami, 2014-10-20 09:44:04]

https://codereview.chromium.org/621823003/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/621823003/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:339: // Ask to be animated if there are
animations
On 2014/10/15 07:18:19, mithro wrote:
> These lines appear to be the cause of the hang issue in
> https://code.google.com/p/chromium/issues/detail?id=421931. I'm still trying
to
> figure out the exact cause though.

Sorry for missing this in the review :( This is weird since I'd think the *lack*
of calls to SetNeedsAnimate would cause hangs. I suspect the reason is related
to how we signal the beginning and end of animations to JS somehow. I ran into a
similar but opposite issue when originally splitting animating and drawing:
https://codereview.chromium.org/206793003/

[brianderson, 2014-10-20 18:05:05]

https://codereview.chromium.org/621823003/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/621823003/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:339: // Ask to be animated if there are
animations
On 2014/10/20 09:44:04, Sami wrote:
> On 2014/10/15 07:18:19, mithro wrote:
> > These lines appear to be the cause of the hang issue in
> > https://code.google.com/p/chromium/issues/detail?id=421931. I'm still trying
> to
> > figure out the exact cause though.
> 
> Sorry for missing this in the review :( This is weird since I'd think the
*lack*
> of calls to SetNeedsAnimate would cause hangs. I suspect the reason is related
> to how we signal the beginning and end of animations to JS somehow. I ran into
a
> similar but opposite issue when originally splitting animating and drawing:
> https://codereview.chromium.org/206793003/

These lines really shouldn't have caused us to hang. It is likely that it's only
causing us to hit an existing bug more often.

We should try to fix ToT so it can work with these lines in and then see if it
fixes the other hangs we are seeing in earlier versions of Chrome.

[brianderson, 2014-10-21 23:18:17]

This should be able to re-land at ToT now that Vlad fixed the root cause in
https://codereview.chromium.org/667053003

[mithro-old, 2014-10-27 13:21:05]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-10-27 13:22:16]

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

[commit-bot: I haz the power, 2014-10-27 14:08:55]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2014-10-27 14:09:39]

Patchset 6 (id:??) landed as
https://crrev.com/0bda234b69c475323123b89eba4a5e831f988821
Cr-Commit-Position: refs/heads/master@{#301358}