stylo: make font-variant-alternates animatable

RESOLVED FIXED in Firefox 56

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: daisuke, Assigned: daisuke)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Need to make font-variant-alternates animatable discretely.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

11 months ago
mozreview-review
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 6

11 months ago
mozreview-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 7

11 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

11 months ago
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 12

11 months ago
mozreview-review
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8889779 - Attachment is obsolete: true

Comment 16

11 months ago
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

Comment 17

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6649bd06cdb7
https://hg.mozilla.org/mozilla-central/rev/f61a06a600af
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.