1359786 - Reuse lengthPairType for positionType in web animations' web-platform-tests

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Jeremy Chen ([e-mail])]

We'll add a new type "lengthPairType" in Bug 1354437. Since the existing positionType is similar to this lengthPairType, we shall just reuse it.

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359786 - only check the mid-point while testing interpolation results in animations' web-platform-tests.

In case of interpolation, we shall just check at 500ms.
No need to check 0ms and 1000ms.

Review commit: https://reviewboard.mozilla.org/r/134938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134938/

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359786 - reuse existing lengthPairType for positionType in web animations' web-platform-tests.

Since lengthPairType and positionType share the same testing logics, we shall
just reuse lengthPairType::{testInterpolation, testAddition} for positionType.

Review commit: https://reviewboard.mozilla.org/r/134940/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134940/

Comment 3

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359786 - update MANIFEST for web-platform-tests.

Review commit: https://reviewboard.mozilla.org/r/134942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134942/

Comment 4

[Jeremy Chen ([e-mail])]

Comment on attachment 8863114 [details]
Bug 1359786 - update MANIFEST for web-platform-tests.

https://reviewboard.mozilla.org/r/134942/#review137886

TBH, I'm not expecting the MANIFEST would be changed in this bug. However, after I ran --manifest-update, some diffs indeed showed up. So, I just include the diffs in this separate patch, cause IIRC, we shall always keep the MANIFEST up-to-date.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863112 [details]
Bug 1359786 - only check the mid-point while testing interpolation results in animations' web-platform-tests.

https://reviewboard.mozilla.org/r/134938/#review137888

Thanks for doing this!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863113 [details]
Bug 1359786 - reuse existing lengthPairType for positionType in web animations' web-platform-tests.

https://reviewboard.mozilla.org/r/134940/#review137890

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863114 [details]
Bug 1359786 - update MANIFEST for web-platform-tests.

https://reviewboard.mozilla.org/r/134942/#review137892

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Btw, you can specify '-u web-platform-tests --artifact' for try to run only web-platform-tests without building for this kind of cases.

Comment 9

[Jeremy Chen ([e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Btw, you can specify '-u web-platform-tests --artifact' for try to run only
> web-platform-tests without building for this kind of cases.

Looks like we got 4 unexpected pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae1fc7b720239644bb269006a9f2e4a920ecd499&selectedJob=95430643

So, I'm pushing a separated patch to re-enable these tests. Once the patch is merged to m-c, maybe we can resolve Bug 1291187 as well.

Thank you for the pointer. Since I need to push another round for the additional 4th patch, I'll push with this specific syntax.

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Bug 1359786 - re-enable interpolation tests for word-spacing and flex-basis.

Review commit: https://reviewboard.mozilla.org/r/134962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134962/

Comment 11

[Jeremy Chen ([e-mail])]

Comment on attachment 8863112 [details]
Bug 1359786 - only check the mid-point while testing interpolation results in animations' web-platform-tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134938/diff/1-2/

Comment 12

[Jeremy Chen ([e-mail])]

Comment on attachment 8863113 [details]
Bug 1359786 - reuse existing lengthPairType for positionType in web animations' web-platform-tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134940/diff/1-2/

Comment 13

[Jeremy Chen ([e-mail])]

Comment on attachment 8863114 [details]
Bug 1359786 - update MANIFEST for web-platform-tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134942/diff/1-2/

Comment 14

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d033104c770

Comment 15

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #9)
> So, I'm pushing a separated patch to re-enable these tests. Once the patch
> is merged to m-c, maybe we can resolve Bug 1291187 as well.

Double checked that the issue of Bug 1291187 still remains... not sure why we got these tests unexpected-pass...
Hi hiro, any thoughts? Do you think re-enabling these tests is fine?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863130 [details]
Bug 1359786 - re-enable interpolation tests for word-spacing and flex-basis.

https://reviewboard.mozilla.org/r/134962/#review137908

This chage is fine to me.
I think the problem does not happen when the computed value is calculated through in StyleAnimationValue::AddWeighted() (i.e not at offset 0 or 1).  So it's probably just a serialization problem (I mean it's not an animation problem).
A bad sideeffect of this change what I can think of is that it's hard to notice if someone fix bug 1291187 unintentionally.  So it would be good to add an automation test for it. We have already such test cases somewhere in layout/style/test?

Ah, also it would be nice to check these test cases on stylo.

Comment 17

[Jeremy Chen ([e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Comment on attachment 8863130 [details]
> Bug 1359786 - re-enable interpolation tests for word-spacing and flex-basis.
> 
> https://reviewboard.mozilla.org/r/134962/#review137908
> 
> This chage is fine to me.
> I think the problem does not happen when the computed value is calculated
> through in StyleAnimationValue::AddWeighted() (i.e not at offset 0 or 1). 
> So it's probably just a serialization problem (I mean it's not an animation
> problem).
> A bad sideeffect of this change what I can think of is that it's hard to
> notice if someone fix bug 1291187 unintentionally.  So it would be good to
> add an automation test for it. We have already such test cases somewhere in
> layout/style/test?

Fair enough. Filed Bug 1360929 for this.

> Ah, also it would be nice to check these test cases on stylo.

Thanks for the notice.
I just checked that all these four tests could pass on stylo perfectly. \o/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9447fa866d
only check the mid-point while testing interpolation results in animations' web-platform-tests. r=hiro DONTBUILD (test-only)
https://hg.mozilla.org/integration/mozilla-inbound/rev/18b49cc74621
reuse existing lengthPairType for positionType in web animations' web-platform-tests. r=hiro DONTBUILD (test-only)
https://hg.mozilla.org/integration/mozilla-inbound/rev/076b7dd00036
update MANIFEST for web-platform-tests. r=hiro DONTBUILD (test-only)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2793e0dbd4f
re-enable interpolation tests for word-spacing and flex-basis. r=hiro DONTBUILD (test-only)

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #17)

> > Ah, also it would be nice to check these test cases on stylo.
> 
> Thanks for the notice.
> I just checked that all these four tests could pass on stylo perfectly. \o/

Yay! Thanks for the check!

Comment 20

[Jeremy Chen ([e-mail])]

(In reply to Pulsebot from comment #18)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9447fa866d
> only check the mid-point while testing interpolation results in animations'
> web-platform-tests. r=hiro DONTBUILD (test-only)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/18b49cc74621
> reuse existing lengthPairType for positionType in web animations'
> web-platform-tests. r=hiro DONTBUILD (test-only)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/076b7dd00036
> update MANIFEST for web-platform-tests. r=hiro DONTBUILD (test-only)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c2793e0dbd4f
> re-enable interpolation tests for word-spacing and flex-basis. r=hiro
> DONTBUILD (test-only)

BTW, they are not good uses of DONTBUILD, I should've avoid using this keyword...:(

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fc9447fa866d
https://hg.mozilla.org/mozilla-central/rev/18b49cc74621
https://hg.mozilla.org/mozilla-central/rev/076b7dd00036
https://hg.mozilla.org/mozilla-central/rev/c2793e0dbd4f