Closed
Bug 1390702
Opened 7 years ago
Closed 7 years ago
stylo: make font-variation-settings animatable
Categories
(Core :: CSS Parsing and Computation, enhancement, P4)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: daisuke, Assigned: daisuke)
References
()
Details
Attachments
(3 files, 3 obsolete files)
Should support animation for this property as well.
Comment 1•7 years ago
|
||
I think this is low priority since the feature is prefed off by default on nightly.
Priority: -- → P4
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25fbd331d57e5964454907e5ffd96995524553b1
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8898108 [details] Bug 1390702 - Part 1: Implement clone_font_variation_settings method. https://reviewboard.mozilla.org/r/169440/#review174762 ::: commit-message-139c4:1 (Diff revision 1) > +Bug 1390702 - Part 1: Implement clone_font_variation_settigs method. r?birtles s/settigs/settings/
Attachment #8898108 -
Flags: review?(bbirtles) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review174760 ::: commit-message-a1d22:1 (Diff revision 1) > +Bug 1390702 - Part 3: Implement Animatable for FontSettings. r?birtles Part 2 ::: servo/components/style/properties/longhand/font.mako.rs:2059 (Diff revision 1) > + fn get_animatable_pairs<'a>(self_settings: &'a T, other_settings: &'a T) > + -> Result<Vec<(&'a FontTag, &'a FontTag)>, ()> { > + if let (&FontSettings::Tag(ref self_tags), &FontSettings::Tag(ref other_tags)) = > + (self_settings, other_settings) { > + fn contains(tags: &Vec<FontTag>, target: &FontTag) -> bool { It might make sense to move all these animation defintions to animated_properties.mako.rs like we do with FontWeight. What do you think? ::: servo/components/style/properties/longhand/font.mako.rs:2064 (Diff revision 1) > + for t in tags.iter() { > + if t.tag == target.tag { > + return true; > + } > + } > + false I'm sure there is a most rust-like way of writing this. In this case, I think you want `tags.iter().any(|&t| t.tag == target.tag)` or something like that. ::: servo/components/style/properties/longhand/font.mako.rs:2072 (Diff revision 1) > + } > + } > + false > + } > + > + fn contains_for_pair(tags: &Vec<(&FontTag, &FontTag)>, target: &FontTag) -> bool { What do this mean? What is it supposed to do? This method is a desert of comments. We should explain what behavior we're aiming for. ::: servo/components/style/properties/longhand/font.mako.rs:2073 (Diff revision 1) > + for &(t, _) in tags.iter() { > + if t.tag == target.tag { > + return true; > + } > + } > + false Likewise, I think you can write this more cleanly. ::: servo/components/style/properties/longhand/font.mako.rs:2081 (Diff revision 1) > + for st in self_tags { > + if !contains(other_tags, st) { > + return Err(()); > + }; > + } > + for ot in other_tags { > + if !contains(self_tags, ot) { > + return Err(()); > + }; > + } We need to be careful about O(n) complexity here. If |self_tags| has length M, and |other_tags| has length N, then we will do M * N + N * M comparisons, which is roughly O(n^2) complexity. (Later in this method we have another loop that is M * N + M * ~M.) One way you could do this is: 1. Copy references to the lists into temporary vectors: O(n) 2. Sort the vectors using sort_by()[1] to sort by tag name: worst case O(n log n) which is still better than O(n^2) 3. Iterate through both lists backwards de-duplicating by skipping repeated elements as you go. e.g. Any time you find an item in list A that isn't at your current position in list B, you have an error That should be order O(n). So overall that approach will be worst-case O(n log n) which still isn't great but is much better than O(n^2). To improve this further, we'd probably want to introduce a separate AnimatedValue type that stores a sorted (and preferably de-duplicated) list. [1] https://doc.rust-lang.org/std/primitive.slice.html#method.sort ::: servo/components/style/properties/longhand/font.mako.rs:2119 (Diff revision 1) > + #[inline] > + fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) > + -> Result<Self, ()> { > + let pairs = get_animatable_pairs(self, other)?; > + let animated: Vec<FontTag> = pairs.iter().map(|&(st, ot)| { > + st.add_weighted(&ot, self_portion, other_portion).unwrap() Shouldn't this be ? instead of unwrap() ? ::: servo/components/style/properties/longhand/font.mako.rs:2129 (Diff revision 1) > + let mut distance = SquaredDistance::Value(0.); > + for &(st, ot) in pairs.iter() { > + distance = distance + st.compute_squared_distance(&ot)?; > + } I think you want fold() for this: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.fold ::: servo/components/style/properties/longhand/font.mako.rs:2137 (Diff revision 1) > + impl ToAnimatedZero for T { > + #[inline] > + fn to_animated_zero(&self) -> Result<Self, ()> { Err(()) } > + } > + Is this actually needed? ::: servo/components/style/properties/longhand/font.mako.rs:2148 (Diff revision 1) > + #[inline] > + fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) -> Result<Self, ()> { > + if self.tag != other.tag { > + return Err(()); > + } > + let tag = self.tag; I'm not sure you need this line? Could you just pass self.tag directly further down?
Attachment #8898109 -
Flags: review?(bbirtles)
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8898110 [details] Bug 1390702 - Part 3: Make font-variation-settings animatable. https://reviewboard.mozilla.org/r/169444/#review174766 (Unless of course we decide to introduce a separate AnimationValue type...) ::: commit-message-3bdc2:1 (Diff revision 1) > +Bug 1390702 - Part 3: Make font-variation-settigs animatable. r?birtles s/settigs/settings/
Attachment #8898110 -
Flags: review?(bbirtles) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8898111 [details] Bug 1390702 - Part 4: Add test into w-p-t. https://reviewboard.mozilla.org/r/169446/#review174768 Looks good to me although I wouldn't mind have a very quick look when I review part 2. ::: testing/web-platform/meta/web-animations/animation-model/animation-types/interpolation-per-property.html.ini:8 (Diff revision 1) > type: testharness > [filter: interpolate different length of filter-function-list with drop-shadow function] > expected: FAIL > bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1345709 > > + [font-variation-settings supports animation as float even if prural tags] s/prural/plural/ But actually I think: s/even if prural tags/with multiple tags/ would be better. ::: testing/web-platform/meta/web-animations/animation-model/animation-types/interpolation-per-property.html.ini:13 (Diff revision 1) > + [font-variation-settings supports animation as float even if prural tags] > + expected: > + if stylo: PASS > + FAIL > + > + [font-variation-settings supports animation as float even if prural tags and has duplicate] with multiple duplicate tags ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1914 (Diff revision 1) > + testAnimationSamples(animation, idlName, > + [{ time: 250, expected: '"wght" 1.2, "wdht" 2' }, > + { time: 750, expected: '"wght" 1.4, "wdht" 4' } ]); Is it worth checking time 0? (If the test fails due to the sort order differing, having a test at time 0 might make that more obvious?) Up to you. ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1917 (Diff revision 1) > + '"wght" 1.5, "wdht" 5'] }, > + { duration: 1000, fill: 'both' }); > + testAnimationSamples(animation, idlName, > + [{ time: 250, expected: '"wght" 1.2, "wdht" 2' }, > + { time: 750, expected: '"wght" 1.4, "wdht" 4' } ]); > + }, property + ' supports animation as float even if prural tags'); Obviously we'll need to make this description match the one in the meta .ini file. ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1930 (Diff revision 1) > + { duration: 1000, fill: 'both' }); > + testAnimationSamples(animation, idlName, > + [{ time: 250, expected: '"wght" 1.2, "wdht" 2' }, > + { time: 750, expected: '"wght" 1.4, "wdht" 4' } ]); > + }, property + > + ' supports animation as float even if prural tags and has duplicate'); Likewise, we'll need to make this description match the one in the meta .ini file. ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1932 (Diff revision 1) > + test(function(t) { > + var idlName = propertyToIDL(property); > + var target = createTestElement(t, setup); > + var animation = > + target.animate({ [idlName]: ['"wght" 1.1, "wdht" 1', > + '"wdht" 5'] }, > + { duration: 1000, fill: 'both' }); > + testAnimationSamples(animation, idlName, > + [{ time: 499, expected: '"wght" 1.1, "wdht" 1' }, > + { time: 500, expected: '"wdht" 5' } ]); > + }, property + ' supports animation as discrete if tags are not match'); > + }, Would it be better to just list these in property-list.js like we do for, say, 'flex-basis': types: [ 'lengthPercentageOrCalc', { type: 'discrete', options: [ [ 'auto', '10px' ] ] } ] e.g. types: [ 'fontVariationSettings', { type: 'discrete', options: [ [ '"wght" 1.1, "wdht" 1', '"wdht" 5' ], // etc. (are there other combinations we should test?) ] } ] What do you think? ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:1969 (Diff revision 1) > color: colorType, > discrete: discreteType, > filterList: filterListType, > integer: integerType, > positiveInteger: positiveIntegerType, > length: lengthType, > percentage: percentageType, > lengthPercentageOrCalc: lengthPercentageOrCalcType, > lengthPair: lengthPairType, > positiveNumber: positiveNumberType, > opacity: opacityType, > transformList: transformListType, > visibility: visibilityType, > boxShadowList: boxShadowListType, > textShadowList: textShadowListType, > rect: rectType, > position: positionType, > dasharray: dasharrayType, > fontStretch: fontStretchType, > + fontVariationSettings: fontVariationSettingsType, (We should sort this list rather than just adding to the end. And then we should sort the definition of the types to match, but I guess that will mess up the revision history blame.)
Attachment #8898111 -
Flags: review?(bbirtles)
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review174760 > It might make sense to move all these animation defintions to animated_properties.mako.rs like we do with FontWeight. What do you think? Okay. I thought since this Animatable is for font-variation-settings only, wrote in here. However, FontWeight, yes, it was same too. I'll move whole Animatable related codes. > We need to be careful about O(n) complexity here. > > If |self_tags| has length M, and |other_tags| has length N, then we will do M * N + N * M comparisons, which is roughly O(n^2) complexity. > > (Later in this method we have another loop that is M * N + M * ~M.) > > One way you could do this is: > > 1. Copy references to the lists into temporary vectors: O(n) > 2. Sort the vectors using sort_by()[1] to sort by tag name: worst case O(n log n) which is still better than O(n^2) > 3. Iterate through both lists backwards de-duplicating by skipping repeated elements as you go. > e.g. Any time you find an item in list A that isn't at your current position in list B, you have an error > That should be order O(n). > > So overall that approach will be worst-case O(n log n) which still isn't great but is much better than O(n^2). To improve this further, we'd probably want to introduce a separate AnimatedValue type that stores a sorted (and preferably de-duplicated) list. > > [1] https://doc.rust-lang.org/std/primitive.slice.html#method.sort Thank you for the advice. Yeah, my first approach was like yours. But I thought better to avoid to create new such the vector in every add_weighted called. However, I will re-write using smallvec.
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review174760 > Is this actually needed? Actually, I think no need to implement since svg does not support this property. However, need by here.. https://searchfox.org/mozilla-central/source/servo/components/style/properties/helpers/animated_properties.mako.rs#774
Assignee | ||
Comment 13•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb6be57cac839e231cef5ed1f3074f04367c27e0
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review176158 This seems good but I want to check the dedup_by_key part again. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1259 (Diff revision 2) > + if let Ok(animated_values) = > + self_valid_tags.iter().zip(other_valid_tags.iter()).map(|(st, ot)| { > + st.add_weighted(&ot, self_portion, other_portion) > + }).collect::<Result<Vec<FontSettingTag>, ()>>() { > + Ok(GenericFontSettings::Tag(animated_values)) > + } else { > + Err(()) > + } This seems a bit awkward. Would something like the following work: self_valid_tags.iter() .zip(other_valid_tags.iter()) .map(|(st, ot)| std.add_weighted(&ot, self_portion, other_portion)) .collect::<Result<Vec<FontSettingTag>, ()>>()? .map(GenericFontSettings::Tag); or even: self_valid_tags.iter() .zip(other_valid_tags.iter()) .map(|(st, ot)| std.add_weighted(&ot, self_portion, other_portion)?) .collect::<Vec<FontSettingTag>, ()>() .map(GenericFontSettings::Tag); ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1329 (Diff revision 2) > + > +type FontSettingTag = GenericFontSettingTag<FontSettingTagFloat>; > + > +fn get_valid_font_setting_tags<'a>(settings: &'a FontVariationSettings) > + -> Result<Vec<(&'a FontSettingTag)>, ()> { > + use std::cmp::Ordering; (See below, I think we don't need this.) ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1336 (Diff revision 2) > + valid_tags.sort_by(|a, b| { > + if a.tag >= b.tag { > + Ordering::Less > + } else { > + Ordering::Greater > + } > + }); I think this can just be: valid_tags.sort_by(|a, b| a.tag.cmp(b.tag)); ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1343 (Diff revision 2) > + Ordering::Less > + } else { > + Ordering::Greater > + } > + }); > + valid_tags.dedup_by_key(|t| t.tag); If we have: "wght" 1, "wght" 2 won't this produce: "wght" 1 when it should produce: "wght" 2 ?
Attachment #8898109 -
Flags: review?(bbirtles)
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8898111 [details] Bug 1390702 - Part 4: Add test into w-p-t. https://reviewboard.mozilla.org/r/169446/#review176160
Attachment #8898111 -
Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review176570 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1342 (Diff revision 3) > + valid_tags.sort_by(|a, b| { > + if a.tag > b.tag { > + Ordering::Greater > + } else { > + Ordering::Less > + } > + }); I'm not sure how the timsort will behave if we return 'less' for equal items. Can you either describe why this works, or else do something we know is correct?
Attachment #8898109 -
Flags: review?(bbirtles)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d5e371d88dd53edb85ee9e0fd500d0199004f70
Comment 30•7 years ago
|
||
As discussed, I'm a little bit concerned about partially duplicating the dedup code in this patch. Let's see if we can write an iterator that does this instead.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da34123a220a75b1978460a111e6b734e2abb358
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review177710 This looks really good. I want to have one more look though, and I think someone who knows rust better than I do like heycam should also check it. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1115 (Diff revision 5) > + let mut animated = Vec::new(); > + while let Some((st, ot)) = iter.next() { > + animated.insert(0, st.animate(&ot, procedure)?); > + } This feels like a map() operation. Any reason we can't do it as a map() Also, why do we need to insert at the beginning of animated? I'm assuming that's slower than appending to the end. If we really do need that ordering then I guess we should either: a) Make the iterator work in the opposite direction, or, if we can't do that, b) Just append and then reverse it. But I wonder if we really need a specific ordering here. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1132 (Diff revision 5) > + while let Some((st, ot)) = iter.next() { > + distance = distance + st.compute_squared_distance(&ot)?; > + } > + if iter.is_finished { Could you do something like: iter.map(|(a, b)| a.compute_squared_distance(&b)) .collect()? .sum() .map(SquaredDistance::Value); ? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1169 (Diff revision 5) > +impl Animate for FontSettingTagFloat { > + #[inline] > + fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> { > + self.0.animate(&other.0, procedure).map(FontSettingTagFloat) > + } > +} > + > +impl ComputeSquaredDistance for FontSettingTagFloat { > + #[inline] > + fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> { > + self.0.compute_squared_distance(&other.0) > + } > +} Can we derive these? i.e. use #[derive(Animate, ComputeSquaredDistance)] ? I guess that needs to go on values::generics::FontSettingTagFloat though. We custom derive definitions for these. e.g. see servo/components/style_derive/compute_squared_distance.rs In fact, it's worth checking if there are other definitions here we can derive. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1201 (Diff revision 5) > + self_state: FontSettingTagIterState<'a>, > + other_state: FontSettingTagIterState<'a>, I don't think 'self' and 'other' makes sense here. Should it just be 'a' and 'b' or something like that? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1207 (Diff revision 5) > + fn new(self_settings: &'a FontVariationSettings, other_settings: &'a FontVariationSettings) > + -> Result<FontSettingTagIter<'a>, ()> { Nit: might be good to run rustfmt over this to see how to this should be aligned. At the moment the alignment seems off. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1213 (Diff revision 5) > + let mut sorted_tags = Vec::with_capacity(tags.len()); > + for t in tags.iter() { > + sorted_tags.push(t); > + } Is there no simpler way of doing this? e.g. extend_from_slice? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1260 (Diff revision 5) > + (Some(st), Some(ot)) if st.tag == ot.tag => Some((st, ot)), > + (None, None) => { > + self.is_finished = true; > + None > + }, > + _ => None Probably add a comment about this last line saying: // Mismatched lists
Attachment #8898109 -
Flags: review?(bbirtles)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a846209e7b5cd7ee6e3fce25ef93f808a0b321f7
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review178408 Please request review from heycam after addressing the following comments. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1114 (Diff revision 6) > + FontSettingTagIter::new(self, other)? > + .map(|r| r.and_then(|(st, ot)| st.animate(&ot, procedure))) > + .collect::<Result<Vec<FontSettingTag>, ()>>() > + .map(GenericFontSettings::Tag) I think the lines beginning with a . here probably need another level of indentation? rustfmt can probably tell you though. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1124 (Diff revision 6) > + FontSettingTagIter::new(self, other)? > + .map(|r| r.and_then(|(st, ot)| st.compute_squared_distance(&ot))) > + .sum() (Assuming the lines with a . are supposed to be indented, these lines should be too.) ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1172 (Diff revision 6) > + index: tags.len(), tags, prev_tag: 0 > + } > + } > +} > + > +struct FontSettingTagIter<'a> { We need a comment somewhere here explaining what this does. Specifically, that it takes two lists of settings, does a stable sort on them, iterates through them backwards skipping duplicates (i.e. only returning the last of each such item). And we should also explain why we want that behavior. Perhaps something like: /// Iterator for font-variation-settings tag lists /// /// [CSS fonts level 4](https://drafts.csswg.org/css-fonts-4/#descdef-font-face-font-variation-settings) /// defines the animation of font-variation-settings as follows: /// /// Two declarations of font-feature-settings[sic] can be animated between if they are "like". /// "Like" declarations are ones where the same set of properties appear (in any order). /// Because succesive[sic] duplicate properties are applied instead of prior duplicate /// properties, two declarations can be "like" even if they have differing number of /// properties. If two declarations are "like" then animation occurs pairwise between /// corresponding values in the declarations. /// /// In other words if we have the following lists: /// /// "wght" 1.4, "wdth" 5, "wght" 2 /// "wdth" 8, "wght" 4, "wdth" 10 /// /// We should animate between: /// /// "wdth" 5, "wght" 2 /// "wght" 4, "wdth" 10 /// /// This iterator supports this by sorting the two lists, then iterating them in reverse, /// and skipping entries with repeated tag names. It will return Some(Err()) if it reaches the /// end of one list before the other, but it does *not* ensure the tags in the corresponding /// lists match. /// /// For the above example, this iterator would return: /// /// Some(Ok("wght" 2, "wght" 4)) /// Some(Ok("wdth" 5, "wdth" 10)) /// None See comments below, however. I wonder why the iterator does not ensure the tags in corresponding lists match. It seems like it easily could and it might make more sense as an API if it did. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1178 (Diff revision 6) > + fn new(self_settings: &'a FontVariationSettings, other_settings: &'a FontVariationSettings) > + -> Result<FontSettingTagIter<'a>, ()> { As per previous review, I don't think self and other make sense here. Also, is that really the alignment rustfmt suggested? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1221 (Diff revision 6) > + type Item = Result<(&'a FontSettingTag, &'a FontSettingTag), ()>; > + > + fn next(&mut self) -> Option<Result<(&'a FontSettingTag, &'a FontSettingTag), ()>> { > + match (FontSettingTagIter::next_tag(&mut self.a_state), > + FontSettingTagIter::next_tag(&mut self.b_state)) { > + (Some(at), Some(bt)) => Some(Ok((at, bt))), Would it make more sense to check if the tags match here and return an error in that case? From an interface point of view that might make more sense. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1223 (Diff revision 6) > + fn next(&mut self) -> Option<Result<(&'a FontSettingTag, &'a FontSettingTag), ()>> { > + match (FontSettingTagIter::next_tag(&mut self.a_state), > + FontSettingTagIter::next_tag(&mut self.b_state)) { > + (Some(at), Some(bt)) => Some(Ok((at, bt))), > + (None, None) => None, > + _ => Some(Err(())) // Length of both tags are difference. // Lists differ in number of unique tags
Attachment #8898109 -
Flags: review?(bbirtles) → review+
Comment 42•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #41) > Comment on attachment 8898109 [details] > Bug 1390702 - Part 2: Implement Animatable for FontSettings. > > https://reviewboard.mozilla.org/r/169442/#review178408 > > Please request review from heycam after addressing the following comments. I think Manish would be a better person since heycam is busy on memory stuff.
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8898111 [details] Bug 1390702 - Part 4: Add test into w-p-t. https://reviewboard.mozilla.org/r/169446/#review178426 ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1489 (Diff revisions 5 - 6) > 'The value should be ' + testSample.expected + > ' at ' + testSample.time + 'ms'); > }); > } > > +function toOrderedArray(string, delimiter) { Do we need to pass in the delimiter as a parameter. It's always ',' right? ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1490 (Diff revisions 5 - 6) > + const array = string.split(delimiter) > + .map(function(value) { > + return value.trim(); > + }); If we don't need to pass in the delimiter we could just do: return string.split(/\s*,\s/).sort(); Otherwise, we could at least do: return string.split(',') .map(str => str.trim()) .sort(); ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1498 (Diff revisions 5 - 6) > + }); > + array.sort(); > + return array; > +} > + > +function testAnimationSamplesWithAnyOrder(animation, idlName, Perhaps we should add a comment here: // Some list-based CSS properties such as font-variant-settings don't specify // an order for serializing computed values so this sorts the expected and // actual value lists first before comparing them. ? ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1501 (Diff revisions 5 - 6) > + var target = type > + ? animation.effect.target.parentElement > + : animation.effect.target; Would var target = animation.effect.target.constructor.name === 'CSSPseudoElement' ? animation.effect.target.parentElement ? animation.effect.target; be more clear? ::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1512 (Diff revisions 5 - 6) > + assert_array_equals(computedValues, expectedValues, > + 'The computed values should be ' + expectedValues + > + ' at ' + testSample.time + 'ms'); I guess this works because toString() for arrays does a join() on the contents. We could make this a little easier to read as: assert_array_equals(computedValues, expectedValues, `The computed values should be ${expectedValues}` + ` at ${testSample.time}ms`); If you prefer.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 49•7 years ago
|
||
mozreview-review |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review178832 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1022 (Diff revision 7) > +/// "wdth" 5, "wght" 2 > +/// "wght" 4, "wdth" 10 > +/// > +/// This iterator supports this by sorting the two lists, then iterating them in reverse, > +/// and skipping entries with repeated tag names. It will return Some(Err()) if it reaches the > +/// end of one list before the other, or in case of tag names which appeared at same timing differ. "... or if the tag names do not match." ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1085 (Diff revision 7) > + // Lists differ in number of unique tags. > + // Or in case of tag names of at is different from bt. It's not obvious which lines these comments correspond to. Either put it above or to the right of the error line. I think that now that this covers all error cases I'm not sure we need this comment. If we do, then I think we should just say: _ => Some(Err(())), // Mismatch number of unique tags or tag types You could perhaps even just say "Tag mismatch".
Comment 50•7 years ago
|
||
mozreview-review |
Comment on attachment 8902068 [details] Bug 1390702 - Part 5: Rewrite handling of CSSPseudoElements to improve readability. https://reviewboard.mozilla.org/r/173476/#review178834 ::: commit-message-996a1:1 (Diff revision 1) > +Bug 1390702 - Part 5: Change a way to select target element. r?birtles Bug 1390702 - Part 5: Rewrite handling of CSSPseudoElements to improve readability. r?birtles
Attachment #8902068 -
Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 55•7 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c437010ded934f90e9e02ca24d55254f54d9cb1
Assignee | ||
Comment 56•7 years ago
|
||
Hi Manish, I'm sorry to bother you, couldn't you review patch 2? https://reviewboard.mozilla.org/r/169442/diff/8#index_header Thanks.
Flags: needinfo?(manishearth)
Assignee | ||
Comment 57•7 years ago
|
||
I talked with Manish on IRC. As result, I'll PR this with Brian's r+.
Flags: needinfo?(manishearth)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 63•7 years ago
|
||
rebased and try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=db933c091d0c62501f60817788cdbe13f4ef8ef9
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8898108 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8898109 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8898110 -
Attachment is obsolete: true
Assignee | ||
Comment 66•7 years ago
|
||
Comment 67•7 years ago
|
||
Pushed by dakatsuka@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c939207a9436 Part 4: Add test into w-p-t. r=birtles https://hg.mozilla.org/integration/autoland/rev/82d9f4f8887e Part 5: Rewrite handling of CSSPseudoElements to improve readability. r=birtles
Comment 68•7 years ago
|
||
mozreview-review |
Comment on attachment 8898109 [details] Bug 1390702 - Part 2: Implement Animatable for FontSettings. https://reviewboard.mozilla.org/r/169442/#review180676 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:983 (Diff revision 8) > +} > + > +type FontSettingTag = GenericFontSettingTag<FontSettingTagFloat>; > + > +struct FontSettingTagIterState<'a> { > + tags: Vec<(&'a FontSettingTag)>, nit: the parentheses are unnecessary
Attachment #8898109 -
Flags: review?(manishearth)
Updated•7 years ago
|
Attachment #8898109 -
Flags: review?(manishearth) → review+
Comment 69•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c939207a9436 https://hg.mozilla.org/mozilla-central/rev/82d9f4f8887e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•