Bug 86013 – [chromium] Ensure animations get ticked at least once when added.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Without the threaded compositor, it is possible to add an animation and never have it committed or ticked (DRT for example). This patch ensures that these animations do get started.

Created attachment 141015 [details]
Patch

[James Robinson]

Comment on attachment 141015 [details]
Patch

Nice! This looks much cleaner. R=me, but if you don't mind upload a version that applies to ToT and seeing if it EWS's clean before landing.

Created attachment 141056 [details]
Patch

James, I've put this up for review again because something came up during the
merge I need to run by you.

A couple of the unit tests in CCLayerTreeHostTest had been marked flaky, but I
think I have addressed the flakiness. The background ticking was taking 1000
seconds, rather than milliseconds. I've retested these test using
--gtest_repeat=1000 --gtest_break_on_failure and they succeeded so I've removed
the FLAKY_ prefix.

[James Robinson]

Comment on attachment 141056 [details]
Patch

Oh wow.... yeah seconds and milliseconds are different things, as it turns out.  Nice catch!

[WebKit Review Bot]

Comment on attachment 141056 [details]
Patch

Rejecting attachment 141056 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
form/graphics/chromium/cc/CCThreadProxy.h
patching file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
Hunk #1 succeeded at 1135 (offset 56 lines).
Hunk #2 succeeded at 1154 (offset 56 lines).
Hunk #3 succeeded at 1189 (offset 56 lines).
Hunk #4 succeeded at 1266 (offset 56 lines).
Hunk #5 succeeded at 1292 (offset 56 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12666015

[Dana Jansens]

I mighta totally just broke this applying, while it was in the CQ. Sorry!

Created attachment 141067 [details]
Patch

freshening patch

[WebKit Review Bot]

Comment on attachment 141067 [details]
Patch

Clearing flags on attachment: 141067

Committed r116594: <http://trac.webkit.org/changeset/116594>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #8)
> (From update of attachment 141067 [details])
> Clearing flags on attachment: 141067
> 
> Committed r116594: <http://trac.webkit.org/changeset/116594>

I'm afraid this change made a test flaky.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=fast%2Flayers%2Fscroll-with-transform-composited-layer.html


Is it possible?

[Kent Tamura]

(In reply to comment #10)
> 
> I'm afraid this change made a test flaky.
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=fast%2Flayers%2Fscroll-with-transform-composited-layer.html
> 
> 
> Is it possible?

Not only scroll-with-transform-composited-layer.html.  Some composition-related tests are flaky since around this change.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLargeExpectations=true&tests=composit

[Kent Tamura]

Let me try rolling out the patch to see whether it resolves the flakiness.

[James Robinson]

Go for it unless Ian has any other ideas.

[Kent Tamura]

Reverted r116594 for reason:

r116594 might have made some composition tests flaky.

Committed r116714: <http://trac.webkit.org/changeset/116714>

[James Robinson]

I'm gonna guess the answer ends up being we need the timer to only start animations and not produce frames.

Created attachment 141323 [details]
Patch

Attempting to fix flake introduced by this test.

This patch used to bail before drawing if we were not visible, but may not be valid and I have removed this condition.

The timer should only be used if we add animation, which is not the case for the flaky compositor tests.

[Kent Tamura]

(In reply to comment #14)
> Reverted r116594 for reason:
> 
> r116594 might have made some composition tests flaky.
> 
> Committed r116714: <http://trac.webkit.org/changeset/116714>

I confirmed that the flakiness was resolved by the rollout.

[James Robinson]

Can you try to reproduce the flake locally first, Ian, and confirm whether this new patch actually resolves it or not?  I'd rather not debug flake via the landing + using the real bots if possible.

[James Robinson]

(In reply to comment #18)
> Can you try to reproduce the flake locally first, Ian, and confirm whether this new patch actually resolves it or not?  I'd rather not debug flake via the landing + using the real bots if possible.

Any progress here?  If not then it might be worth landing this anyway and seeing what happens.

(In reply to comment #19)
> (In reply to comment #18)
> > Can you try to reproduce the flake locally first, Ian, and confirm whether this new patch actually resolves it or not?  I'd rather not debug flake via the landing + using the real bots if possible.
> 
> Any progress here?  If not then it might be worth landing this anyway and seeing what happens.

No real progress. Tests do not seem flaky for me when I run locally. Running on mac_layout with the extra condition on drawing, I had a flaky compositing-related test that I did not have without.

With:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/357
Without:
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/361

I don't think that this is nearly conclusive proof that the latest patch fixes the flake, but I agree that it might be worth relanding to see.

[James Robinson]

Comment on attachment 141323 [details]
Patch

OK, thanks for checking. Let's give this a try then - please keep an eye on the bots after this lands to see if the flake re-appears.

[WebKit Review Bot]

Comment on attachment 141323 [details]
Patch

Rejecting attachment 141323 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 (offset -18 lines).
Hunk #4 succeeded at 284 (offset -16 lines).
Hunk #5 succeeded at 360 (offset -14 lines).
patching file Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h
patching file Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h
patching file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12685864

Created attachment 142023 [details]
Patch

Freshening patch.

Created attachment 142024 [details]
Patch

Freshening patch again. Sorry for the spam.

[WebKit Review Bot]

Comment on attachment 142024 [details]
Patch

Clearing flags on attachment: 142024

Committed r117171: <http://trac.webkit.org/changeset/117171>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.