1454524 - Remove some unused comparator code in KeyframeUtils

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1454524 - Remove some unused comparator code in KeyframeUtils.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

(Came across when investigating usage of some tables in nsCSSProps. Unfortunately it isn't going to help removing any bits of code there at the end, though.)

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Nice catch!  I haven't noticed that it's not used any more.  Though it's not used, I am not convinced whether we can really remove it.  Especially sorting shorthand by sub property count.  IIRC, we don't have such automation test cases and probably it's not yet spec-ed.  Let me take time to check our code.

CCing Brian, he might know something about it.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

> probably it's not yet spec-ed.

Great!  It's already spec-ed.  From https://drafts.csswg.org/web-animations-1/#calculating-computed-keyframes

> 2. Shorthand properties with fewer longhand components override those with more longhand components (e.g. border-top overrides border-color).

So there must be some test cases in web platform tests.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yeah, I rewrote the same code in rust as part of Stylo.[1] I think we don't need this once dropping Gecko style system.

[1] https://github.com/servo/servo/pull/17831/files

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Thanks.  I had actually got involved in that bug (bug 1371493 comment 5), but don't recall. :)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8968371 [details]
Bug 1454524 - Remove some unused comparator code in KeyframeUtils.

https://reviewboard.mozilla.org/r/237056/#review242888

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70235b1de25d
Remove some unused comparator code in KeyframeUtils. r=hiro

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

I'm wondering whether it is possible to remove other uses of nsCSSProps::PropertyIDLName() and nsCSSProps::PropertyIDLNameSortPosition(). There seems to be some remaining in animation code.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

But that's not a blocker anyway.

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/70235b1de25d