Web Animations: Don't participate in grouping when start time is set

Web Animations: Don't participate in grouping when start time is set

After a recent change[1] cc started sending back animation specified
start times when present. This was inadvertently applied to animations
in the same compositor group (started in the same frame), which could
cause animations to receive an inappropriate start time.

To avoid this regression, animations with a specified start time
are labeled with a distinct compositor group as they do not participate
in grouping.

[1] https://chromium.googlesource.com/chromium/src/+/33c2aaa27b1ab502cfa08b7d271b58b8b485b5cc

BUG=528291
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202617

[dstockwell, 2015-09-14 03:24:11]

dstockwell@chromium.org changed reviewers:
+ alancutter@chromium.org

[dstockwell, 2015-09-14 03:24:18]

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

[commit-bot: I haz the power, 2015-09-14 03:25:02]

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

[commit-bot: I haz the power, 2015-09-14 03:26:45]

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

[commit-bot: I haz the power, 2015-09-14 03:26:46]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dstockwell, 2015-09-14 06:05:13]

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

[commit-bot: I haz the power, 2015-09-14 06:05:17]

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

[commit-bot: I haz the power, 2015-09-14 06:42:23]

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

[commit-bot: I haz the power, 2015-09-14 06:42:23]

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

[ctserng, 2015-09-14 14:30:32]

tserng@amazon.com changed reviewers:
+ tserng@amazon.com

[ctserng, 2015-09-14 14:30:33]

https://codereview.chromium.org/1341673002/diff/20001/Source/core/animation/C...
File Source/core/animation/CompositorPendingAnimations.cpp (right):

https://codereview.chromium.org/1341673002/diff/20001/Source/core/animation/C...
Source/core/animation/CompositorPendingAnimations.cpp:76: if
(animation->preCommit(animation->hasStartTime() ? 0 : compositorGroup,
startOnCompositor)) {
Will we run the risk of hitting the assert here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[dstockwell, 2015-09-14 23:53:24]

On 2015/09/14 at 14:30:33, tserng wrote:
>
https://codereview.chromium.org/1341673002/diff/20001/Source/core/animation/C...
> File Source/core/animation/CompositorPendingAnimations.cpp (right):
> 
>
https://codereview.chromium.org/1341673002/diff/20001/Source/core/animation/C...
> Source/core/animation/CompositorPendingAnimations.cpp:76: if
(animation->preCommit(animation->hasStartTime() ? 0 : compositorGroup,
startOnCompositor)) {
> Will we run the risk of hitting the assert here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Yep, good point. Changed to a different approach.

[dstockwell, 2015-09-14 23:53:30]

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

[commit-bot: I haz the power, 2015-09-14 23:53:54]

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

[commit-bot: I haz the power, 2015-09-15 01:23:13]

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

[commit-bot: I haz the power, 2015-09-15 01:23:14]

Dry run: This issue passed the CQ dry run.

[ctserng, 2015-09-15 18:54:42]

I had created a ManualTest from the test case in the bug, do you think it would
be useful to include that or do you think the provided Layout test is
sufficient.

[dstockwell, 2015-09-16 00:51:14]

On 2015/09/15 at 18:54:42, tserng wrote:
> I had created a ManualTest from the test case in the bug, do you think it
would be useful to include that or do you think the provided Layout test is
sufficient.

Automated tests are preferable when possible. @loyso is actively working on
removing the remaining manual tests.

[dstockwell, 2015-09-16 00:51:23]

@alancutter, ping

[alancutter (OOO until 2018), 2015-09-16 05:35:30]

lgtm

https://codereview.chromium.org/1341673002/diff/40001/Source/core/animation/C...
File Source/core/animation/CompositorPendingAnimations.cpp (right):

https://codereview.chromium.org/1341673002/diff/40001/Source/core/animation/C...
Source/core/animation/CompositorPendingAnimations.cpp:68: while (compositorGroup
== 0 || compositorGroup == 1) {
Nit: We should use an enum instead of magic numbers.

[dstockwell, 2015-09-21 12:50:46]

On 2015/09/16 at 05:35:30, alancutter wrote:
> lgtm
> 
>
https://codereview.chromium.org/1341673002/diff/40001/Source/core/animation/C...
> File Source/core/animation/CompositorPendingAnimations.cpp (right):
> 
>
https://codereview.chromium.org/1341673002/diff/40001/Source/core/animation/C...
> Source/core/animation/CompositorPendingAnimations.cpp:68: while
(compositorGroup == 0 || compositorGroup == 1) {
> Nit: We should use an enum instead of magic numbers.

Ack. I'll update the bug after landing this, we should update cc to not need
either of these.

[dstockwell, 2015-09-21 12:50:50]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2015-09-21 12:51:01]

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

[commit-bot: I haz the power, 2015-09-21 15:07:40]

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

[commit-bot: I haz the power, 2015-09-21 15:07:41]

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_...)

[dstockwell, 2015-09-22 00:00:53]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2015-09-22 00:01:22]

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

[commit-bot: I haz the power, 2015-09-22 00:54:14]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202617