Closed Bug 1379921 Opened 7 years ago Closed 7 years ago

stylo: make font-variant-alternates animatable

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(2 files, 1 obsolete file)

Need to make font-variant-alternates animatable discretely.
Comment on attachment 8889781 [details]
Bug 1379921 - Part 3: remove test fail annotations from reftest.

https://reviewboard.mozilla.org/r/160866/#review166120

Yay! The last remaining failure for SMIL reftest passed!
Attachment #8889781 - Flags: review?(hikezoe) → review+
Comment on attachment 8889780 [details]
Bug 1379921 - Part 2: remove test fail annotations from meta in wpt.

https://reviewboard.mozilla.org/r/160864/#review166122
Attachment #8889780 - Flags: review?(hikezoe) → review+
Comment on attachment 8889779 [details]
Bug 1379921 - Part 1: make font-variant-alternates animatable.

https://reviewboard.mozilla.org/r/160862/#review166152

::: servo/components/style/properties/gecko.mako.rs:2132
(Diff revision 1)
> +        use properties::longhands::font_variant_alternates::VariantAlternates;
> +        use properties::longhands::font_variant_alternates::VariantAlternatesList;
> +        use values::CustomIdent;
> +
> +        if self.gecko.mFont.variantAlternates == NS_FONT_VARIANT_ALTERNATES_NORMAL as u16 {
> +            return VariantAlternatesList(Box::new([]));

I am not sure which way is preferable but in font-variant-alternates implematation we use vec![].into_boxed_slice() there.

::: servo/components/style/properties/gecko.mako.rs:2135
(Diff revision 1)
> +
> +        if self.gecko.mFont.variantAlternates == NS_FONT_VARIANT_ALTERNATES_NORMAL as u16 {
> +            return VariantAlternatesList(Box::new([]));
> +        }
> +
> +        let mut alternates = Vec::new();

Accoring to the setter implementation we can set the vector length here. I.e. Vec::with_capacity(self.gecko.mFont.alternateValues.Capacity())?

::: servo/components/style/properties/gecko.mako.rs:2144
(Diff revision 1)
> +
> +        <%
> +            property_need_ident_list = "styleset character_variant".split()
> +        %>
> +        % for value in property_need_ident_list:
> +            let mut ${value}_list = Vec::new();

Oh, I did not know that Vec::new() does not allocate memory.

::: servo/components/style/properties/gecko.mako.rs:2161
(Diff revision 1)
> +                    NS_FONT_VARIANT_ALTERNATES_${value.upper()} => {
> +                        ${value}_list.push(CustomIdent(ident));
> +                    },
> +                % endfor
> +                x => {
> +                    panic!("Found unexpected value for font-varoamt-alternates: {:?}", x);

Nit: font-variant-alternates
Attachment #8889779 - Flags: review?(hikezoe) → review+
Thank you very much, Hiro.

I updated the patch.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39959a9753b784503cbfeaaec6b910b557de28f7
After I look the result of try, will PR.

Thanks!
Comment on attachment 8889779 [details]
Bug 1379921 - Part 1: make font-variant-alternates animatable.

https://reviewboard.mozilla.org/r/160862/#review166518

::: servo/components/style/properties/gecko.mako.rs:2135
(Diff revision 2)
> +
> +        if self.gecko.mFont.variantAlternates == NS_FONT_VARIANT_ALTERNATES_NORMAL as u16 {
> +            return VariantAlternatesList(vec![].into_boxed_slice());
> +        }
> +
> +        let mut alternates = Vec::with_capacity(self.gecko.mFont.alternateValues.len());

Nice! Sorry, I thought capcity() has been exposed into Rust side.  len() will be less than the length we really need in the case where we have historical-forms, but I think it's OK.
Attachment #8889779 - Attachment is obsolete: true
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6649bd06cdb7
Part 2: remove test fail annotations from meta in wpt. r=hiro
https://hg.mozilla.org/integration/autoland/rev/f61a06a600af
Part 3: remove test fail annotations from reftest. r=hiro
https://hg.mozilla.org/mozilla-central/rev/6649bd06cdb7
https://hg.mozilla.org/mozilla-central/rev/f61a06a600af
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.