1456320 - animation-type-longhand.js should be an automatically generated build artifact

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Mats Palmgren (inactive)]

I've just manually updated devtools/server/actors/animation-type-longhand.js
because it hardcodes stuff about CSS properties.  I feel like it's a waste of my time updating stuff like this that really should be automatically generated build artifacts.  Every property already has an "animation_value_type" in Stylo, can we automatically generate this file from those values please?

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

That used to be something embedded in nsCSSPropList.h, and it gets moved to devtools in the process of removing that old list.

See the reasoning in the commit[1] for why this is currently hardcoded.


[1] https://hg.mozilla.org/mozilla-central/rev/616bbf7a8815

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Also cc daisuke and hiro in case they have some thought about this.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Yeah, totally agree.  This is really annoying. It's exactly what I was concerned in bug 1448757.  

I did skim the devtools code, though I couldn't find the place where we use 'coord' animation type, it seems to me that we can detect whether the animation is 'color' type or not in devtool (the way what I have in my mind is to try parse the keyframe value), so we can make animation_value_type four types, i.e. 'none', 'discrete', 'computed' and 'special' (bug 1454834), then use it in devtools where we want to know that the animation is 'discrete'.  If we really need and are using 'coord' in devtools and we have ideas to use other animation types in future, we might have to tweak animation_value_type to fit the devtools purpose.

Anyway, I totally agree we should use animation_value_type or something (that we might not have as of today).

As a side note (mainly for daisuke), if we use the animation type only thorough KeyframeEffect.getProperties() (I mean, when we need the animation type, we call getProperties()?), we should add the type in AnimationPropertyDetails.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

We do use `coord`[1], and that's the trickiest one as I mentioned in that bug.

[1] https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/devtools/client/inspector/animation-old/graph-helper.js#141-143

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Oh yeah, Xidorn knows better than I do, even it's in animation area. :)  It seems to me that the difference between 'coord' and 'float' is just omitting 'calc' and using parseFloat and we can unify them.  That's said, if it needs just for distance calculation, we already have a function for that, I am wondering why we didn't use it there.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I guess one route forward is to have style system generate just none, discrete, and other, and for the type of other, animation inspector can try to parse the value with various function and see which works, maybe with the help of CSSLexer. I don't think, for example, coord and float have any common token as color.

I think other than those need to be done in discrete, numbers should be handled the same way given the code piece I found.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Yes, we are using the distance[1][2].
Also, though we are making new animation inspector, that needs to know only 'discrete' and 'color'[3].

[1] https://searchfox.org/mozilla-central/source/devtools/server/actors/animation.js#599
[2] https://searchfox.org/mozilla-central/source/devtools/server/actors/animation.js#544
[3] https://searchfox.org/mozilla-central/source/devtools/client/inspector/animation/components/keyframes-graph/KeyframesGraphPath.js#46

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thanks!  So the way Xidorn suggested in comment 6 sounds the best way we can do for me.