Closed Bug 1368610 Opened 7 years ago Closed 7 years ago

stylo: implement primitive (bit, uXX) type of discrete animatable properties

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(2 files, 6 obsolete files)

Bug 1368610 - Part 6: Add tests for valid inherit value during animation.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR 17253
41 bytes, text/x-github-pull-request
Details | Review 
Implements position related properties such 'align-content', 'justify-self' and so on.
Would you mind categorizing remaining properties?  If we did not implement properties that have similar structures in a single bug, we can easily write redundant codes.
Thanks Hiro,
Please let me think about how can it be categorized.
Hiro, 
It looks, properties might be able to categorize as:

* bit
* u8
* atom
* nsStyleCoord
* nsStyleImage
* URLDataValue
* and so on (with complex one).

I want to start to fix from "bit" one.
Nice! It's easy to understand!  I guess we can handle u8 and atom also in the same bug for 'bit'. (Also URL data is pretty easy?)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Nice! It's easy to understand!  I guess we can handle u8 and atom also in
> the same bug for 'bit'. (Also URL data is pretty easy?)

Okay, Thanks!
I'll rename this bug for bit, u8 and atom ( also url? ).
Summary: stylo: implement position related discrete type animation → stylo: implement bit, u8, atom and URLValueData type of discrete animatable properties
Comment on attachment 8872887 [details]
Bug 1368610 - Part 2: Implements gecko keyword bit type properties animatable.

https://reviewboard.mozilla.org/r/144406/#review148232

::: servo/components/style/properties/gecko.mako.rs
(Diff revision 1)
> -    pub fn copy_font_variant_alternates_from(&mut self, other: &Self) {
> -        self.gecko.mFont.variantAlternates = other.gecko.mFont.variantAlternates;
> -        // FIXME: Copy alternateValues as well.
> +    // FIXME: Copy alternateValues as well.
> -        // self.gecko.mFont.alternateValues = other.gecko.mFont.alternateValues;
> +    // self.gecko.mFont.alternateValues = other.gecko.mFont.alternateValues;

Shoule we leave functions for font-variant-alternates? We will end up writing these functions when we copy alternateValues?  Have you checked that we will not re-implement these functions respectively?
Attachment #8872887 - Flags: review?(hikezoe) → review+
Comment on attachment 8872888 [details]
Bug 1368610 - Part 3: Implements other bit type properties animatable.

https://reviewboard.mozilla.org/r/144408/#review148234

::: servo/components/style/properties/gecko.mako.rs:1859
(Diff revision 1)
> +            weight: self.gecko.mFont.synthesis & NS_FONT_SYNTHESIS_WEIGHT as u8 != 0 as u8,
> +            style: self.gecko.mFont.synthesis & NS_FONT_SYNTHESIS_STYLE as u8 != 0 as u8

weight and style are defined as bool why casting as u8?

::: servo/components/style/properties/gecko.mako.rs:3746
(Diff revision 1)
> +    pub fn clone_text_emphasis_position(&self) -> longhands::text_emphasis_position::computed_value::T {
> +        longhands::text_emphasis_position::computed_value::T::from_gecko_keyword(self.gecko.mTextEmphasisPosition as u32)
> +    }

This looks pretty similar to what the second patch did except for the cast.
Can we unify them somehow in future?

::: servo/components/style/properties/gecko.mako.rs:3840
(Diff revision 1)
> +    pub fn clone_text_decoration_line(&self) -> longhands::text_decoration_line::computed_value::T {
> +        use properties::longhands::text_decoration_line::computed_value::T;
> +
> +        let mut result = T::empty();
> +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE as u8 != 0 {
> +            result.insert(longhands::text_decoration_line::UNDERLINE);
> +        }
> +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_OVERLINE as u8 != 0 {
> +            result.insert(longhands::text_decoration_line::OVERLINE);
> +        }
> +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH as u8 != 0 {
> +            result.insert(longhands::text_decoration_line::LINE_THROUGH);
> +        }
> +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_BLINK as u8 != 0 {
> +            result.insert(longhands::text_decoration_line::BLINK);
> +        }
> +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_OVERRIDE_ALL as u8 != 0 {
> +            result.insert(longhands::text_decoration_line::COLOR_OVERRIDE);
> +        }
> +        result
> +    }

It seems to me that all gecko's bits equal to servo's bits.
Why we can't use impl_simple_bit?

::: servo/components/style/properties/longhand/text.mako.rs:159
(Diff revision 1)
>  ${helpers.single_keyword("unicode-bidi",
>                           "normal embed isolate bidi-override isolate-override plaintext",
>                           animation_value_type="discrete",
>                           spec="https://drafts.csswg.org/css-writing-modes/#propdef-unicode-bidi")}
>  
>  // FIXME: This prop should be animatable.

We can drop this comment now.
Attachment #8872888 - Flags: review?(hikezoe) → review+
Comment on attachment 8872886 [details]
Bug 1368610 - Part 1: Implements bitflags properties animatable.

https://reviewboard.mozilla.org/r/144404/#review148228

I think we should implement bits() function for AlignJustifySelf and others.

::: servo/components/style/properties/gecko.mako.rs:659
(Diff revision 1)
>      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.gecko.fmt(f) }
>  }
>  %endif
>  </%def>
>  
> +<%def name="impl_simple_bit(ident, gecko_ffi_name, bitflags_field=None)">

I think gecko_ffi_name can be also generated in this macro so that we don't need to generate it respectively in caller site.

::: servo/components/style/properties/gecko.mako.rs:659
(Diff revision 1)
>      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.gecko.fmt(f) }
>  }
>  %endif
>  </%def>
>  
> +<%def name="impl_simple_bit(ident, gecko_ffi_name, bitflags_field=None)">

This function needs 'simple' word? I guess it's just copied from impl_simple_copy() right?
Attachment #8872886 - Flags: review?(hikezoe)
Comment on attachment 8872886 [details]
Bug 1368610 - Part 1: Implements bitflags properties animatable.

https://reviewboard.mozilla.org/r/144404/#review148228

Thanks, Hiro.
Also, I'll add from_bits() function into those.

> I think gecko_ffi_name can be also generated in this macro so that we don't need to generate it respectively in caller site.

Okay.

> This function needs 'simple' word? I guess it's just copied from impl_simple_copy() right?

This function is for the properties that can convert really simple.
If we change this name, we might be better to rename "impl_simple_image_array_property" which we added in bug 1367283?

I wonder, we can define something like "bit_type" which is similer to "single_keyword" in helpers.mako.rs?
(In reply to Daisuke Akatsuka (:daisuke) from comment #12)

> This function is for the properties that can convert really simple.

I am really wondering the properties in the second patch are not simple enough?
It looks to me that difference is from_gecko_keyword() or from_bits() (and to_gecko_keyword() or bits())?
To be clear, honestly most of properties here look simple to me.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> 
> > This function is for the properties that can convert really simple.
> 
> I am really wondering the properties in the second patch are not simple
> enough?
> It looks to me that difference is from_gecko_keyword() or from_bits() (and
> to_gecko_keyword() or bits())?

Yeah, the difference is only those.
I thought, although we could combine those, it is easy to read.
But okay, will combine those.(maybe add "is_gecko_keyword" flag?)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Comment on attachment 8872888 [details]
> Bug 1368610 - Part 3: Implements other bit properties.
> 
> https://reviewboard.mozilla.org/r/144408/#review148234
> 
> ::: servo/components/style/properties/gecko.mako.rs:1859
> (Diff revision 1)
> > +            weight: self.gecko.mFont.synthesis & NS_FONT_SYNTHESIS_WEIGHT as u8 != 0 as u8,
> > +            style: self.gecko.mFont.synthesis & NS_FONT_SYNTHESIS_STYLE as u8 != 0 as u8
> 
> weight and style are defined as bool why casting as u8?
> 
> ::: servo/components/style/properties/gecko.mako.rs:3746
> (Diff revision 1)
> > +    pub fn clone_text_emphasis_position(&self) -> longhands::text_emphasis_position::computed_value::T {
> > +        longhands::text_emphasis_position::computed_value::T::from_gecko_keyword(self.gecko.mTextEmphasisPosition as u32)
> > +    }
> 
> This looks pretty similar to what the second patch did except for the cast.
> Can we unify them somehow in future?
> 
> ::: servo/components/style/properties/gecko.mako.rs:3840
> (Diff revision 1)
> > +    pub fn clone_text_decoration_line(&self) -> longhands::text_decoration_line::computed_value::T {
> > +        use properties::longhands::text_decoration_line::computed_value::T;
> > +
> > +        let mut result = T::empty();
> > +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE as u8 != 0 {
> > +            result.insert(longhands::text_decoration_line::UNDERLINE);
> > +        }
> > +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_OVERLINE as u8 != 0 {
> > +            result.insert(longhands::text_decoration_line::OVERLINE);
> > +        }
> > +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH as u8 != 0 {
> > +            result.insert(longhands::text_decoration_line::LINE_THROUGH);
> > +        }
> > +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_BLINK as u8 != 0 {
> > +            result.insert(longhands::text_decoration_line::BLINK);
> > +        }
> > +        if self.gecko.mTextDecorationLine & structs::NS_STYLE_TEXT_DECORATION_LINE_OVERRIDE_ALL as u8 != 0 {
> > +            result.insert(longhands::text_decoration_line::COLOR_OVERRIDE);
> > +        }
> > +        result
> > +    }
> 
> It seems to me that all gecko's bits equal to servo's bits.
> Why we can't use impl_simple_bit?
> 
> ::: servo/components/style/properties/longhand/text.mako.rs:159
> (Diff revision 1)
> >  ${helpers.single_keyword("unicode-bidi",
> >                           "normal embed isolate bidi-override isolate-override plaintext",
> >                           animation_value_type="discrete",
> >                           spec="https://drafts.csswg.org/css-writing-modes/#propdef-unicode-bidi")}
> >  
> >  // FIXME: This prop should be animatable.
> 
> We can drop this comment now.

Okay, I'll fix them.
(In reply to Daisuke Akatsuka (:daisuke) from comment #15)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> > (In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> > 
> > > This function is for the properties that can convert really simple.
> > 
> > I am really wondering the properties in the second patch are not simple
> > enough?
> > It looks to me that difference is from_gecko_keyword() or from_bits() (and
> > to_gecko_keyword() or bits())?
> 
> Yeah, the difference is only those.
> I thought, although we could combine those, it is easy to read.
> But okay, will combine those.(maybe add "is_gecko_keyword" flag?)

I think it's OK with separated for them. I am just wondering what the 'simple' means.
(In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> Comment on attachment 8872886 [details]
> Bug 1368610 - Part 1: Implements simple bit properties.
> 
> https://reviewboard.mozilla.org/r/144404/#review148228
> 
> Thanks, Hiro.
> Also, I'll add from_bits() function into those.
> 

Should we define a trait something like "BitFlags"?
Maybe the trait define two method bits() and from_bits().
And if yes, may I implement the trait for align/justify-content as well?

Or we should just implement for AlignJustifySelf, AlignItems and JustifyItems?
(In reply to Daisuke Akatsuka (:daisuke) from comment #18)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> > Comment on attachment 8872886 [details]
> > Bug 1368610 - Part 1: Implements simple bit properties.
> > 
> > https://reviewboard.mozilla.org/r/144404/#review148228
> > 
> > Thanks, Hiro.
> > Also, I'll add from_bits() function into those.
> > 
> 
> Should we define a trait something like "BitFlags"?
> Maybe the trait define two method bits() and from_bits().
> And if yes, may I implement the trait for align/justify-content as well?

Sounds really reasonable to me.  I guess if all of bit type properties had the trait, all of them can be implemented with a single mako template for bit types properties?

> Or we should just implement for AlignJustifySelf, AlignItems and
> JustifyItems?

Yes, I am OK with implementing the functions respectively for now.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #18)
> > (In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> > > Comment on attachment 8872886 [details]
> > > Bug 1368610 - Part 1: Implements simple bit properties.
> > > 
> > > https://reviewboard.mozilla.org/r/144404/#review148228
> > > 
> > > Thanks, Hiro.
> > > Also, I'll add from_bits() function into those.
> > > 
> > 
> > Should we define a trait something like "BitFlags"?
> > Maybe the trait define two method bits() and from_bits().
> > And if yes, may I implement the trait for align/justify-content as well?
> 
> Sounds really reasonable to me.  I guess if all of bit type properties had
> the trait, all of them can be implemented with a single mako template for
> bit types properties?

I think, most of properties can implement.
But, how about properties such font-variant-numeric(in patch 2) that convert via to_gecko_keyword(), from_gecko_keyword()?
The trait's implementation should not handle Gecko depended code?
(In reply to Daisuke Akatsuka (:daisuke) from comment #20)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> > (In reply to Daisuke Akatsuka (:daisuke) from comment #18)
> > > (In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> > > > Comment on attachment 8872886 [details]
> > > > Bug 1368610 - Part 1: Implements simple bit properties.
> > > > 
> > > > https://reviewboard.mozilla.org/r/144404/#review148228
> > > > 
> > > > Thanks, Hiro.
> > > > Also, I'll add from_bits() function into those.
> > > > 
> > > 
> > > Should we define a trait something like "BitFlags"?
> > > Maybe the trait define two method bits() and from_bits().
> > > And if yes, may I implement the trait for align/justify-content as well?
> > 
> > Sounds really reasonable to me.  I guess if all of bit type properties had
> > the trait, all of them can be implemented with a single mako template for
> > bit types properties?
> 
> I think, most of properties can implement.
> But, how about properties such font-variant-numeric(in patch 2) that convert
> via to_gecko_keyword(), from_gecko_keyword()?
> The trait's implementation should not handle Gecko depended code?

Oh, we can't use #[cfg(feature = "gecko")] for trait?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #20)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> > > (In reply to Daisuke Akatsuka (:daisuke) from comment #18)
> > > > (In reply to Daisuke Akatsuka (:daisuke) from comment #12)
> > > > > Comment on attachment 8872886 [details]
> > > > > Bug 1368610 - Part 1: Implements simple bit properties.
> > > > > 
> > > > > https://reviewboard.mozilla.org/r/144404/#review148228
> > > > > 
> > > > > Thanks, Hiro.
> > > > > Also, I'll add from_bits() function into those.
> > > > > 
> > > > 
> > > > Should we define a trait something like "BitFlags"?
> > > > Maybe the trait define two method bits() and from_bits().
> > > > And if yes, may I implement the trait for align/justify-content as well?
> > > 
> > > Sounds really reasonable to me.  I guess if all of bit type properties had
> > > the trait, all of them can be implemented with a single mako template for
> > > bit types properties?
> > 
> > I think, most of properties can implement.
> > But, how about properties such font-variant-numeric(in patch 2) that convert
> > via to_gecko_keyword(), from_gecko_keyword()?
> > The trait's implementation should not handle Gecko depended code?
> 
> Oh, we can't use #[cfg(feature = "gecko")] for trait?

Oh, I see!
I'd like to try to implement the trait.
Thanks!
Comment on attachment 8872886 [details]
Bug 1368610 - Part 1: Implements bitflags properties animatable.

https://reviewboard.mozilla.org/r/144404/#review149076

This looks really great!

::: servo/components/style/properties/longhand/position.mako.rs:112
(Diff revision 2)
> +    #[cfg(feature = "gecko")]
> +    impl_align_bitsflags!(::values::specified::align::AlignItems);

Should we put this right after predefined_type for align-items?

::: servo/components/style/properties/longhand/text.mako.rs:174
(Diff revision 2)
> -            const OVERLINE = 0x01,
> -            const UNDERLINE = 0x02,
> +            const UNDERLINE = 1,
> +            const OVERLINE = 2,

Oh I am sorry, I wasn't aware of the fact that these two flags did not match to gecko's ones.  I don't think it's a good idea to change them without asking servo developers. I asked it on IRC but no one is available now on this time zone..

Ah, also I wonder we need to change the values to decimal numbers?

::: servo/components/style/properties/longhand/text.mako.rs:273
(Diff revision 2)
>                  longhands::_servo_text_decorations_in_effect::derive_from_text_decoration(context);
>          }
>      % endif
> +
> +    #[cfg(feature = "gecko")]
> +    impl ::style_traits::BitsFlags<SpecifiedValue, u8> for SpecifiedValue {

Do we really need this trait implementation for text-decolarion-line?

::: servo/components/style/values/specified/align.rs:347
(Diff revision 2)
> +    fn from_bits(bits: u16) -> Option<AlignJustifyContent> {
> +        use gecko_bindings::structs::{NS_STYLE_ALIGN_ALL_BITS, NS_STYLE_ALIGN_ALL_SHIFT};
> +
> +        let primary =
> +            AlignFlags::from_bits((bits & NS_STYLE_ALIGN_ALL_BITS as u16) as u8);
> +        let fallback =
> +            AlignFlags::from_bits((bits >> NS_STYLE_ALIGN_ALL_SHIFT as u16) as u8);
> +        if primary.is_some() && fallback.is_some() {
> +            Some(AlignJustifyContent::with_fallback(primary.unwrap(), fallback.unwrap()))
> +        } else {
> +            None
> +        }
> +    }
> +}

As we discussed on IRC, the fallback condition looks something wrong. I guess we can just copy the bits as it is.

::: servo/components/style_traits/values.rs:267
(Diff revision 2)
>      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
>          self.0.to_css(f)
>      }
>  }
> +
> +/// This trait behaves as bitframes.

What is the bitframes?  You mean BitFlags?

::: servo/components/style_traits/values.rs:269
(Diff revision 2)
>      }
>  }
> +
> +/// This trait behaves as bitframes.
> +#[cfg(feature = "gecko")]
> +pub trait BitsFlags<T, U> {

Let's rename this to GeckoBitFlags. BitsFlag is really confusing BitFlags.
Attachment #8872886 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> ::: servo/components/style/properties/longhand/text.mako.rs:174
> (Diff revision 2)
> > -            const OVERLINE = 0x01,
> > -            const UNDERLINE = 0x02,
> > +            const UNDERLINE = 1,
> > +            const OVERLINE = 2,
> 
> Oh I am sorry, I wasn't aware of the fact that these two flags did not match
> to gecko's ones.  I don't think it's a good idea to change them without
> asking servo developers. I asked it on IRC but no one is available now on
> this time zone..

Please ask servo developers that we can change the order.
Priority: -- → P2
Once bug 1369277 lands this will also need to address the fixme in gecko.mako.rs in that patch for doing cloning of SpecifiedURL (or ping me to do a followup)
(In reply to Manish Goregaokar [:manishearth] from comment #31)
> Once bug 1369277 lands this will also need to address the fixme in
> gecko.mako.rs in that patch for doing cloning of SpecifiedURL (or ping me to
> do a followup)

Thanks for the message.
Although this bug will not contain cloning SpecifiedURL, I'll fix that one in another related bug.
Comment on attachment 8874737 [details]
Bug 1368610 - Part 6: Add tests for valid inherit value during animation.

https://reviewboard.mozilla.org/r/146118/#review150038

The test cases in wpt are really good, but we should carefully consider adding another variant of files that include various property and its values in wpt.  I think we should figure out an easy way to maintain those property list.  Please open a new bug for it.

r=me with dropping the part of wpt.
Attachment #8874737 - Flags: review?(hikezoe) → review+
Comment on attachment 8874736 [details]
Bug 1368610 - Part 5: Implements properties that need to change only the animation type animatable.

https://reviewboard.mozilla.org/r/146116/#review150046

::: servo/components/style/properties/gecko.mako.rs:2538
(Diff revision 1)
>  
>      ${impl_animation_timing_function()}
>  
>      <% scroll_snap_type_keyword = Keyword("scroll-snap-type", "none mandatory proximity") %>
>  
> -    ${impl_keyword('scroll_snap_type_y', 'mScrollSnapTypeY', scroll_snap_type_keyword, need_clone=False)}
> +    ${impl_keyword('scroll_snap_type_y', 'mScrollSnapTypeY', scroll_snap_type_keyword, need_clone=True)}

I guess we can now drop need_clone argument here.

::: servo/components/style/properties/gecko.mako.rs:3711
(Diff revision 1)
>  
>      <%call expr="impl_coord_copy('_moz_tab_size', 'mTabSize')"></%call>
>  
>      <% text_size_adjust_keyword = Keyword("text-size-adjust", "auto none") %>
>  
> -    ${impl_keyword('_moz_text_size_adjust', 'mTextSizeAdjust', text_size_adjust_keyword, need_clone=False)}
> +    ${impl_keyword('_moz_text_size_adjust', 'mTextSizeAdjust', text_size_adjust_keyword, need_clone=True)}

Likewise here.
Attachment #8874736 - Flags: review?(hikezoe) → review+
Comment on attachment 8874736 [details]
Bug 1368610 - Part 5: Implements properties that need to change only the animation type animatable.

https://reviewboard.mozilla.org/r/146116/#review150046

> I guess we can now drop need_clone argument here.

Yeah, I thought same thing.
But we need this since impl_keyword has no default value for need_clone.
https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/gecko.mako.rs#360

Or, should we define the default value?
(In reply to Daisuke Akatsuka (:daisuke) from comment #47)
> Comment on attachment 8874736 [details]
> Bug 1368610 - Part 5: Implements discrete animatable properties that need to
> change only the animation type.
> 
> https://reviewboard.mozilla.org/r/146116/#review150046
> 
> > I guess we can now drop need_clone argument here.
> 
> Yeah, I thought same thing.
> But we need this since impl_keyword has no default value for need_clone.
> https://dxr.mozilla.org/mozilla-central/source/servo/components/style/
> properties/gecko.mako.rs#360
> 
> Or, should we define the default value?

No, if we make the property animatable but need_clone is still false, I think it's a bug because need_clone is set to true if animatable flag is true at the end of Longhand::__init__.
Comment on attachment 8874736 [details]
Bug 1368610 - Part 5: Implements properties that need to change only the animation type animatable.

https://reviewboard.mozilla.org/r/146116/#review150046

> Likewise here.

We could drop this line since we could make -moz-text-size-adjust be a normal single_keyword.
Comment on attachment 8874736 [details]
Bug 1368610 - Part 5: Implements properties that need to change only the animation type animatable.

https://reviewboard.mozilla.org/r/146116/#review150046

> Yeah, I thought same thing.
> But we need this since impl_keyword has no default value for need_clone.
> https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/gecko.mako.rs#360
> 
> Or, should we define the default value?

But scroll-snap-type-y is using helpers.longhand since that property uses scroll-snap-type-x structure as it is to compare and so on in shorthand/scroll-snap-type.
https://dxr.mozilla.org/mozilla-central/source/servo/components/style/properties/shorthand/box.mako.rs#301
Comment on attachment 8872886 [details]
Bug 1368610 - Part 1: Implements bitflags properties animatable.

https://reviewboard.mozilla.org/r/144404/#review151612

::: servo/components/style/properties/gecko.mako.rs:659
(Diff revision 6)
>      fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.gecko.fmt(f) }
>  }
>  %endif
>  </%def>
>  
> +<%def name="impl_primitive_fields(ident)">

I think now it's the time to use 'simple' word, because this macro calls impl_simple_copy() inside it.
I guess impl_simple_type_with_conversion() would be a better name?

::: servo/components/style/properties/longhand/position.mako.rs:28
(Diff revision 6)
>                                spec="https://drafts.csswg.org/css-logical-props/#propdef-offset-%s" % side,
>                                animation_value_type="ComputedValue", logical=True)}
>  % endfor
>  
> +#[cfg(feature = "gecko")]
> +macro_rules! impl_align_from_trait {

I think it would be nice to include 'conversion' in the macro name. impl_align_conversions?

::: servo/components/style/properties/longhand/text.mako.rs:174
(Diff revision 6)
> -            const OVERLINE = 0x01,
> -            const UNDERLINE = 0x02,
> -            const LINE_THROUGH = 0x04,
> -            const BLINK = 0x08,
> +            const UNDERLINE = 1,
> +            const OVERLINE = 2,
> +            const LINE_THROUGH = 4,
> +            const BLINK = 8,

Though I did ask before, why did you change the literal value from hex?

::: servo/components/style/properties/properties.mako.rs:56
(Diff revision 6)
>  macro_rules! property_name {
>      ($s: tt) => { atom!($s) }
>  }
>  
> +#[cfg(feature = "gecko")]
> +macro_rules! impl_bitflags_from_trait {

Likewise, impl_bitflags_conversions()?
Comment on attachment 8872886 [details]
Bug 1368610 - Part 1: Implements bitflags properties animatable.

https://reviewboard.mozilla.org/r/144404/#review151614

::: commit-message-fbe0e:14
(Diff revision 6)
> +To implement this, we introduce new trait "BitsFlags" for Gecko only.
> +Also, we change the bits value for text-decoration-line to same to Gecko.

This commit message should be revised too. We no longer implement the BitsFlags trait.
Comment on attachment 8872886 [details]
Bug 1368610 - Part 1: Implements bitflags properties animatable.

https://reviewboard.mozilla.org/r/144404/#review151616

::: commit-message-fbe0e:1
(Diff revision 6)
> +Bug 1368610 - Part 1: Implements bitsflags properties. r?hiro

Also, this patch does not implement properties, jut making those properties animatable.
Comment on attachment 8872887 [details]
Bug 1368610 - Part 2: Implements gecko keyword bit type properties animatable.

https://reviewboard.mozilla.org/r/144406/#review151620

::: commit-message-ef95e:1
(Diff revision 6)
> +Bug 1368610 - Part 2: Implements bit as gecko keyword properties. r?hiro

Likewise the commit message in part 1, should be revised.
Comment on attachment 8872888 [details]
Bug 1368610 - Part 3: Implements other bit type properties animatable.

https://reviewboard.mozilla.org/r/144408/#review151622

::: commit-message-7fa21:1
(Diff revision 6)
> +Bug 1368610 - Part 3: Implements other bit properties. r?hiro

Likewise.

::: servo/components/style/properties/longhand/position.mako.rs:402
(Diff revision 6)
> +        fn from(v: SpecifiedValue) -> u8 {
> +            use gecko_bindings::structs;
> +            use self::computed_value::AutoFlow;
> +
> +            let mut result: u8 = 0;
> +            result |= match v.autoflow {

Nit: result =
Thank you, Hiro!
I'll fix all things.

Also, may I change to rename this bug title to maybe "implement primitive (bit, uXX) type of discrete animatable properties"?
Comment on attachment 8874735 [details]
Bug 1368610 - Part 4: Implements numeric field type properties animatable.

https://reviewboard.mozilla.org/r/146114/#review150060

This looks good. But still, there seems an issue around the initial-letter handling.

::: commit-message-128ad:1
(Diff revision 3)
> +Bug 1368610 - Part 4: Implements numeric fields discrete animatable properties. r?hiro

This commit message should be revised too.

::: commit-message-128ad:13
(Diff revision 3)
> +* font-language-override
> +* -moz-force-broken-image-icon
> +* initial-letter
> +* -webkit-text-stroke-width
> +
> +To implement, we introduce a trait "GeckoNumericFields".

Drop this.

::: servo/components/style/properties/gecko.mako.rs:665
(Diff revision 4)
>      <%
>      if gecko_ffi_name is None:
>          gecko_ffi_name = "m" + to_camel_case(ident)
>      %>
>  
> +    #[allow(non_snake_case)]

This should have done in part 1, right?

::: servo/components/style/properties/gecko.mako.rs:672
(Diff revision 4)
>          self.gecko.${gecko_ffi_name} = From::from(v)
>      }
>  
>      <% impl_simple_copy(ident, gecko_ffi_name) %>
>  
> +    #[allow(non_snake_case)]

Likewise.

::: servo/components/style/properties/gecko.mako.rs:678
(Diff revision 4)
> +<%def name="impl_plural_simple_type_copy(ident, gecko_ffi_names)">
> +    pub fn copy_${ident}_from(&mut self, other: &Self) {
> +        % for gecko_ffi_name in gecko_ffi_names:
> +        self.gecko.${gecko_ffi_name} = other.gecko.${gecko_ffi_name};
> +        % endfor
> +    }
> +</%def>

I am not sure it's worth creating this macro. Currently we use this macro only properties which have just two struct, it requeres just two lines.

::: servo/components/style/properties/gecko.mako.rs:3812
(Diff revision 4)
> +            0 => T::Specified(Number::new(self.gecko.mInitialLetterSize), None),
> +            sink => T::Specified(Number::new(self.gecko.mInitialLetterSize), Some(Integer::new(sink))),
> +        }

It seems to me that this code is different from what the setter does. In the setter, if the sink is None, we set size.floor() to the mInitialLetterSink.  Is the setter something wrong?
Attachment #8874735 - Flags: review?(hikezoe)
Comment on attachment 8874735 [details]
Bug 1368610 - Part 4: Implements numeric field type properties animatable.

https://reviewboard.mozilla.org/r/146114/#review151684

::: servo/components/style/properties/gecko.mako.rs:1024
(Diff revision 5)
> +        % for i, side in enumerate(["H", "V"]):
> +        let servo${i} = match self.gecko.mBorderImageRepeat${side} as u32 {

Can we use ${side} instead of ${i}?

::: servo/components/style/properties/gecko.mako.rs:3807
(Diff revision 5)
> +        use properties::longhands::initial_letter::computed_value::T;
> +        use values::specified::{Number, Integer};
> +
> +        if self.gecko.mInitialLetterSize == 0. && self.gecko.mInitialLetterSink == 0 {
> +            T::Normal
> +        } else if self.gecko.mInitialLetterSize as i32 == self.gecko.mInitialLetterSink {

I don't know rust handles i32 cast as floor().  Really? Should we use floor() instead?
Attachment #8874735 - Flags: review?(hikezoe) → review+
Summary: stylo: implement bit, u8, atom and URLValueData type of discrete animatable properties → stylo: implement primitive (bit, uXX) type of discrete animatable properties
Attachment #8876021 - Attachment is obsolete: true
Attachment #8876021 - Flags: review?(hikezoe)
Attached file Servo PR 17253 — 

    
    

    
      
  

    
  

        Attachment #8872886 -
        Attachment is obsolete: true

    
  

        Attachment #8872887 -
        Attachment is obsolete: true

    
  

        Attachment #8872888 -
        Attachment is obsolete: true

    
  

        Attachment #8874735 -
        Attachment is obsolete: true

    
  

        Attachment #8874736 -
        Attachment is obsolete: true

    
    

    
  
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db8fa24af581
Part 6: Add tests for valid inherit value during animation. r=hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/db8fa24af581
Status: NEW → RESOLVED
Closed: 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: