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)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

(
URL
)

Details

Attachments

(3 files, 3 obsolete files)

Bug 1390702 - Part 4: Add test into w-p-t.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1390702 - Part 5: Rewrite handling of CSSPseudoElements to improve readability.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Servo PR 18380
41 bytes, text/x-github-pull-request
Details | Review 
Should support animation for this property as well.
I think this is low priority since the feature is prefed off by default on nightly.
Priority: -- → P4
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 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 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 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)
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.
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
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 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 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)
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 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 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+
(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 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 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 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+
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)
I talked with Manish on IRC.
As result, I'll PR this with Brian's r+.
Flags: needinfo?(manishearth)
Attachment #8898108 - Attachment is obsolete: true
Attachment #8898109 - Attachment is obsolete: true
Attachment #8898110 - Attachment is obsolete: true
Attached file Servo PR 18380 — 

    
    

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

        Attachment #8898109 -
        Flags: review?(manishearth) → review+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: