Closed Bug 1371115 Opened 7 years ago Closed 7 years ago

stylo: implement style struct type 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:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(4 files, 23 obsolete files)

Bug 1371115 - Part 7: implements nsStyleBorder type properties animatable.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1371115 - Part 12: implements nsStyleImage type properties animatable.
59 bytes, text/x-review-board-request
hiro
: review+
birtles
: review+
Details
Bug 1371115 - Part 14: add tests for moz prefixed properties.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Bug 1371115 - Part 15: Remove test fail annotations from meta in wpt.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
Also, we need to implement discrete animatable properties that consist by style struct in Gecko. (e.g. nsStyleImage, nsStyleCoord)
Priority: -- → P2
Also, I'll append 3 or 4 more patches that implement style struct (e.g. nsStyleImage) which uses URLValueData.
Comment on attachment 8877910 [details]
Bug 1371115 - Part 1: implements nsStyleCoord type properties animatable.

https://reviewboard.mozilla.org/r/149018/#review153832

::: servo/components/style/properties/gecko.mako.rs:1240
(Diff revision 1)
> +        use values::generics::grid::{TrackSize, TrackBreadth};
> +        use values::computed::length::LengthOrPercentage;

Nit: Sort by alphabetical order.

::: servo/components/style/values/generics/gecko.rs:35
(Diff revision 1)
> +use gecko::values::GeckoStyleCoordConvertible;
> +use gecko_bindings::sugar::ns_style_coord::{CoordData, CoordDataMut, CoordDataValue};
> +impl<L> GeckoStyleCoordConvertible for ScrollSnapPoint<L> where L: GeckoStyleCoordConvertible {
> +    fn to_gecko_style_coord<T: CoordDataMut>(&self, coord: &mut T) {
> +        match self.repeated() {
> +            None => coord.set_value(CoordDataValue::None),
> +            Some(l) => l.to_gecko_style_coord(coord),
> +        };
> +    }
> +
> +    fn from_gecko_style_coord<T: CoordData>(coord: &T) -> Option<Self> {
> +        use gecko_bindings::structs::root::nsStyleUnit::eStyleUnit_None;
> +        Some(
> +            match coord.unit() {
> +                eStyleUnit_None => ScrollSnapPoint::None,
> +                _ => ScrollSnapPoint::Repeat(L::from_gecko_style_coord(coord)
> +                                             .expect("coord has valid data for GeckoStyleCoordConvertible"))
> +            }
> +        )
> +    }
> +}

Should these functons be inside servo/components/style/gecko/values.rs?

::: servo/components/style/values/generics/gecko.rs:50
(Diff revision 1)
> +    fn from_gecko_style_coord<T: CoordData>(coord: &T) -> Option<Self> {
> +        use gecko_bindings::structs::root::nsStyleUnit::eStyleUnit_None;
> +        Some(
> +            match coord.unit() {
> +                eStyleUnit_None => ScrollSnapPoint::None,
> +                _ => ScrollSnapPoint::Repeat(L::from_gecko_style_coord(coord)

I am concerned about the case that coord.unit() is a different type other that 'L'. I mean, what happens if the unit is Calc in the case where L is LengthOrPercentage?
Attachment #8877910 - Flags: review?(hikezoe) → review+
Comment on attachment 8877911 [details]
Bug 1371115 - Part 2: implements nsStyleGridLine type properties animatable.

https://reviewboard.mozilla.org/r/149020/#review153876

::: servo/components/style/properties/gecko.mako.rs:1217
(Diff revision 1)
> +            line_num:
> +                if self.gecko.${value.gecko}.mInteger == 0 { None }
> +                else { Some(Integer::new(self.gecko.${value.gecko}.mInteger)) },

This indentation is odd. Should be;

line_nume: {
    if self.gecko.${value.gecko}.mInteger == 0 {
        None
    } else {
        Some(...)
    }
},

Also should we add an assertion that checks mInteger is inside the range of [nsStyleGridLine_kMinLine, nsStyleGridLine_kMaxLine]?

::: servo/components/style/values/computed/mod.rs:572
(Diff revision 1)
>  /// The computed value of a grid `<track-list>`
>  /// (could also be `<auto-track-list>` or `<explicit-track-list>`)
>  pub type TrackList = GenericTrackList<TrackSize>;
>  
> +#[cfg(feature = "gecko")]
> +impl TrackSize {

This impl also should be in servo/components/style/gecko/conversions.rs or servo/components/style/gecko/values.rs.

::: servo/components/style/values/computed/mod.rs:573
(Diff revision 1)
>  /// (could also be `<auto-track-list>` or `<explicit-track-list>`)
>  pub type TrackList = GenericTrackList<TrackSize>;
>  
> +#[cfg(feature = "gecko")]
> +impl TrackSize {
> +    /// Return TrackSize from given two LengthOrPercentage

Nit: two nsStyleCorrd?
Attachment #8877911 - Flags: review?(hikezoe) → review+
Comment on attachment 8877913 [details]
Bug 1371115 - Part 4: implements nsStyleCounterData type properties animatable.

https://reviewboard.mozilla.org/r/149024/#review153898

::: servo/components/style/properties/gecko.mako.rs:4377
(Diff revision 1)
> +            use values::CustomIdent;
> +            use gecko_string_cache::Atom;
> +
> +            longhands::counter_increment::computed_value::T(
> +                self.gecko.m${counter_property}s.iter().map(|ref gecko_counter| {
> +                    (CustomIdent(Atom::from(gecko_counter.mCounter.to_string())), gecko_counter.mValue)

mCounter should also be an nsIAtom instead nsString just like animation name (bug 1329169). Please file a bug for it.
Attachment #8877913 - Flags: review?(hikezoe) → review+
Comment on attachment 8877916 [details]
Bug 1371115 - Part 7: implements nsStyleBorder type properties animatable.

https://reviewboard.mozilla.org/r/149030/#review153906
Attachment #8877916 - Flags: review?(hikezoe) → review+
Comment on attachment 8877917 [details]
Bug 1371115 - Part 8: implements nsFont type properties animatable.

https://reviewboard.mozilla.org/r/149032/#review153910

::: servo/components/style/properties/gecko.mako.rs:1628
(Diff revision 1)
> +                    FontFamilyType::eFamily_serif => FontFamily::Generic(atom!("serif")),
> +                    FontFamilyType::eFamily_sans_serif => FontFamily::Generic(atom!("sans-serif")),
> +                    FontFamilyType::eFamily_monospace => FontFamily::Generic(atom!("monospace")),
> +                    FontFamilyType::eFamily_cursive => FontFamily::Generic(atom!("cursive")),
> +                    FontFamilyType::eFamily_fantasy => FontFamily::Generic(atom!("fantasy")),
> +                    FontFamilyType::eFamily_moz_fixed => FontFamily::Generic(Atom::from("-moz_fixed")),

Nit: -moz-fixed

::: servo/components/style/properties/gecko.mako.rs:1630
(Diff revision 1)
> +                    FontFamilyType::eFamily_monospace => FontFamily::Generic(atom!("monospace")),
> +                    FontFamilyType::eFamily_cursive => FontFamily::Generic(atom!("cursive")),
> +                    FontFamilyType::eFamily_fantasy => FontFamily::Generic(atom!("fantasy")),
> +                    FontFamilyType::eFamily_moz_fixed => FontFamily::Generic(Atom::from("-moz_fixed")),
> +                    FontFamilyType::eFamily_named => FontFamily::FamilyName(FamilyName {
> +                        name: Atom::from(gecko_font_family_name.mName.to_string()),

Can't we call into() directly? We have "From<&'a nsAString> for Atom".

I.e.
 (&*gecko_font_family_name.mName).into()
?

::: servo/components/style/properties/gecko.mako.rs:1634
(Diff revision 1)
> +                    FontFamilyType::eFamily_named => FontFamily::FamilyName(FamilyName {
> +                        name: Atom::from(gecko_font_family_name.mName.to_string()),
> +                        quoted: false
> +                    }),
> +                    FontFamilyType::eFamily_named_quoted => FontFamily::FamilyName(FamilyName {
> +                        name: Atom::from(gecko_font_family_name.mName.to_string()),

Likewise here.
Attachment #8877917 - Flags: review?(hikezoe) → review+
Comment on attachment 8877918 [details]
Bug 1371115 - Part 9: rename macro from impl_gecko_keyword_from_trait to impl_gecko_keyword_conversions.

https://reviewboard.mozilla.org/r/149034/#review153920
Attachment #8877918 - Flags: review?(hikezoe) → review+
As for part 5 and 6, I am not confident to_string() is the best way we can do (I think it's not efficient). I will ask someone in servo channel, but if you have time, please feel free to ask someone there.
Comment on attachment 8877919 [details]
Bug 1371115 - Part 10: implements u8 and array type properties animatable.

I'm sorry, please let me cancel this review since I found one bug.
Attachment #8877919 - Flags: review?(hikezoe)
Also you will need to rebase them on this https://github.com/servo/servo/pull/17337
Comment on attachment 8877914 [details]
Bug 1371115 - Part 5: implements nsStyleTextOverflow type properties animatable.

https://reviewboard.mozilla.org/r/149026/#review154162
Attachment #8877914 - Flags: review?(hikezoe) → review+
Comment on attachment 8877915 [details]
Bug 1371115 - Part 6: implements nsStyleQuoteValues type properties animatable.

https://reviewboard.mozilla.org/r/149028/#review154164
Attachment #8877915 - Flags: review?(hikezoe) → review+
Comment on attachment 8877912 [details]
Bug 1371115 - Part 3: implements nsStyleSides type properties animatable.

https://reviewboard.mozilla.org/r/149022/#review153884

::: servo/components/style/properties/gecko.mako.rs:569
(Diff revision 1)
>                  .expect("clone for ${ident} failed")
>          }
>      % endif
>  </%def>
>  
> +<%def name="impl_sides_style_coord(ident)">

Can we simply call this impl_style_sides?

::: servo/components/style/properties/longhand/border.mako.rs:298
(Diff revision 1)
> +#[cfg(feature = "gecko")]
> +macro_rules! impl_rect_conversions {

Should this macro be in servo/components/style/gecko/conversions.rs?
Attachment #8877912 - Flags: review?(hikezoe) → review+
Comment on attachment 8877910 [details]
Bug 1371115 - Part 1: implements nsStyleCoord type properties animatable.

https://reviewboard.mozilla.org/r/149018/#review153832

> I am concerned about the case that coord.unit() is a different type other that 'L'. I mean, what happens if the unit is Calc in the case where L is LengthOrPercentage?

Ah, I see.
Okay, I will change this impl to:
```
impl GeckoStyleCoordConvertible for ScrollSnapPoint<LengthOrPercentage> {
}
```
then, handle only eStyleUnit_Coord and eStyleUnit_Percent. ( panic! for other type? )

Also, I move this impl to servo/components/style/gecko/values.rs

Thanks!
Comment on attachment 8877912 [details]
Bug 1371115 - Part 3: implements nsStyleSides type properties animatable.

https://reviewboard.mozilla.org/r/149022/#review153884

> Should this macro be in servo/components/style/gecko/conversions.rs?

If we move to conversions.rs, need to add #[macro_use] to "mod gecko" in style/lib.rs as well. However in case of this, we publish macros in style/gecko/non_ts_pseudo_class_list.rs as extra.

Brian gave an advice, why don't we create gecko_macros.rs similar to macros.rs.
For me, we can gather macros that depend on gecko to this file, and it is easy to understand.

I want to try this way.
Comment on attachment 8877913 [details]
Bug 1371115 - Part 4: implements nsStyleCounterData type properties animatable.

https://reviewboard.mozilla.org/r/149024/#review153898

> mCounter should also be an nsIAtom instead nsString just like animation name (bug 1329169). Please file a bug for it.

Filed bug 1373566 - Store mCounter in nsStyleCounterData as nsIAtom
Comment on attachment 8877919 [details]
Bug 1371115 - Part 10: implements u8 and array type properties animatable.

https://reviewboard.mozilla.org/r/149036/#review154810
Attachment #8877919 - Flags: review?(hikezoe) → review+
Comment on attachment 8880604 [details]
Bug 1371115 - Part 11: implements URLValue type properties animatable.

https://reviewboard.mozilla.org/r/151934/#review156944

::: servo/components/style/properties/gecko.mako.rs:669
(Diff revision 1)
> +                    unsafe {
> +                        let ref gecko_url_value = *self.gecko.${gecko_ffi_name}.mRawPtr;
> +                        Either::First(SpecifiedUrl::from_url_value_data(&gecko_url_value._base)
> +                                      .expect("${gecko_ffi_name} has valid URL value"))
> +                    }
> +                }

Nit: },
Attachment #8880604 - Flags: review?(hikezoe) → review+
Comment on attachment 8880605 [details]
Bug 1371115 - Part 12: implements nsStyleImage type properties animatable.

https://reviewboard.mozilla.org/r/151936/#review156950

::: layout/style/ServoBindings.cpp:1492
(Diff revision 1)
>  }
>  
> +const mozilla::css::URLValueData*
> +Gecko_GetURLValue(const nsStyleImage* aImage)
> +{
> +  MOZ_ASSERT(aImage);

Should we add an assertion to check aImage->mType == eStyleImageTypeURL?
Also in GetImageElement and GetGradientImageValue.

::: servo/components/style/gecko/conversions.rs:345
(Diff revision 1)
>              Gecko_SetGradientImageValue(self, gecko_gradient);
>          }
>      }
>  }
>  
> +impl Image {

It seems more intuitice to me that these functions are in impl nsStyleImage.

::: servo/components/style/gecko/conversions.rs:382
(Diff revision 1)
> +                Some(GenericImage::Gradient(Self::from_gecko_gradient(gecko_image)))
> +            },
> +            nsStyleImageType::eStyleImageType_Element => {
> +                use gecko_string_cache::Atom;
> +                use gecko_bindings::structs::nsIAtom;
> +                let atom = Gecko_GetImageElement(gecko_image) as *mut nsIAtom;

This looks fishy way, the casting *mut against const *.
I wonder why Gecko_GetImageElement() did not return non-const pointer in the first place?

::: servo/components/style/gecko/conversions.rs:391
(Diff revision 1)
> +        let ref url_value = *Gecko_GetURLValue(gecko_image);
> +        let mut url = SpecifiedUrl::from_url_value_data(url_value).expect("gecko_image has valid URL value");

If Gecko_GetURLValue() never returns nullptr, can we write just like below;

let url_value = Gecko_GetURLValue(gecko_image);
let mut url = ...::from_url_value_data(url_value.as_ref().unwrap())..

?
It smees to me that we are using as_ref().unwrap() patterns for such cases.

::: servo/components/style/gecko/conversions.rs:412
(Diff revision 1)
> +        use values::computed::image::LineDirection;
> +        use values::generics::image::{ColorStop, CompatMode, Circle, Ellipse, EndingShape, GradientKind, ShapeExtent};
> +        use values::specified::length::Percentage;
> +        use values::specified::position::{X, Y};
> +
> +        let ref gecko_gradient = *Gecko_GetGradientImageValue(gecko_image);

Same as above comment.

::: servo/components/style/gecko/conversions.rs:437
(Diff revision 1)
> +                            x => panic!("Found unexpected mBgPosY in nsStyleImage: {:?}", x),
> +                        };
> +                        LineDirection::Corner(horizontal_direction, vertical_direction)
> +                    },
> +                    _ => {
> +                        LineDirection::Angle(Angle::from_gecko_style_coord(&gecko_gradient.mAngle).unwrap())

Nit: Should we use expect()?

::: servo/components/style/gecko/conversions.rs:449
(Diff revision 1)
> +                        // We can't choose ShapeExtent::Contain and ShapeExtent::Cover
> +                        // since the gecko dose not have such the variables.

This comment is slightly misleading, actually contain and cover are(were) synonyms for closet-side and farthese-corner. We should just say they are synonyms.

::: servo/components/style/gecko/conversions.rs:459
(Diff revision 1)
> +                                Circle::Radius(
> +                                    Length::from_gecko_style_coord(&gecko_gradient.mRadiusX)
> +                                        .expect("mRadiusX has valid data")

Should we add an assertion to check X and Y values are the same?

::: servo/components/style/properties/gecko.mako.rs:3264
(Diff revision 1)
> +                        None => Either::First(None_),
> +                        Some(image) => Either::Second(image),
> +                    }

Should we put these the same order as clone_border_image_source?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:415
(Diff revision 1)
>                          % if not prop.is_animatable_with_computed_value:
>                              let value: longhands::${prop.ident}::computed_value::T = value.into();
>                          % endif
> -                        style.mutate_${prop.style_struct.ident.strip("_")}().set_${prop.ident}(value);
> +
> +                        <% method = "style.mutate_" + prop.style_struct.ident.strip("_") + "().set_" + prop.ident %>
> +                        % if prop.ident == "background_image" or prop.ident == "mask_image":

We should check has_uncacheable_values and use the value instead.
Attachment #8880605 - Flags: review?(hikezoe) → review+
Comment on attachment 8880607 [details]
Bug 1371115 - Part 14: implements StyleShapeSource type properties animatable.

https://reviewboard.mozilla.org/r/151940/#review156980

::: servo/components/style/properties/gecko.mako.rs:2833
(Diff revision 1)
>          }
>      }
>  
>      <% impl_shape_source("shape_outside", "mShapeOutside") %>
>  
> +    pub fn clone_shape_outside(&self) -> longhands::shape_outside::computed_value::T {

As discussed on IRC, we should move this into impl_shape_source.

::: servo/components/style/properties/gecko.mako.rs:2845
(Diff revision 1)
> +        let ref gecko_value = self.gecko.mShapeOutside;
> +        match gecko_value.mType {
> +            StyleShapeSourceType::None => ShapeSource::None,
> +            StyleShapeSourceType::URL => {
> +                unsafe {
> +                    let ref gecko_url = *Gecko_StyleShapeSource_GetURLValue(gecko_value);

We should add an assertion to check the returned value of Gecko_StyleShapeSource_GetURLValue() is not nullptr.

::: servo/components/style/properties/gecko.mako.rs:2859
(Diff revision 1)
> +                let box_option = match gecko_value.mReferenceBox {
> +                    StyleGeometryBox::NoBox => None,
> +                    _ => Some(ShapeBox::from(gecko_value.mReferenceBox))
> +                };
> +                unsafe {
> +                    let ref gecko_shape = *Gecko_StyleShapeSource_GetBasicShape(gecko_value);

Likewise here, we should add an assertion.
Attachment #8880607 - Flags: review?(hikezoe) → review+
Comment on attachment 8880605 [details]
Bug 1371115 - Part 12: implements nsStyleImage type properties animatable.

https://reviewboard.mozilla.org/r/151936/#review156950

> Should we add an assertion to check aImage->mType == eStyleImageTypeURL?
> Also in GetImageElement and GetGradientImageValue.

Yap, will add an assertion here.
But I think, we don't need the assertion for GetImageElement and GetGradientImageValue since those methods have the assertion.
https://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#462
Comment on attachment 8880606 [details]
Bug 1371115 - Part 13: implements nsStyleImageRequest type properties animatable.

https://reviewboard.mozilla.org/r/151938/#review156978

::: servo/components/style/gecko/url.rs:68
(Diff revision 1)
>          })
>      }
>  
> +    /// Convert from nsStyleImageRequest to SpecifiedUrl.
> +    pub unsafe fn from_image_request(image_request: &nsStyleImageRequest) -> Result<SpecifiedUrl, ()> {
> +        let ref image_value = *image_request.mImageValue.mRawPtr;

We should return Err() in case the mImageValue is nullptr.

::: servo/components/style/gecko/url.rs:69
(Diff revision 1)
> +        let ref url_value_data = image_value._base;
> +        let result = Self::from_url_value_data(url_value_data);
> +        if result.is_err() {
> +            return result;
> +        }
> +        let mut url = result.unwrap();
> +        url.build_image_value();
> +        Ok(url)

This can be written as

 let mut result = try!(Self::from_url_value_data(url_value_darta));
 result.build_image_value();
 Ok(result)

There might be smarter way but I don't know.
Attachment #8880606 - Flags: review?(hikezoe) → review+
(In reply to Daisuke Akatsuka (:daisuke) from comment #56)
> Comment on attachment 8880605 [details]
> Bug 1371115 - Part 12: implements nsStyleImage type properties animatable.
> 
> https://reviewboard.mozilla.org/r/151936/#review156950
> 
> > Should we add an assertion to check aImage->mType == eStyleImageTypeURL?
> > Also in GetImageElement and GetGradientImageValue.
> 
> Yap, will add an assertion here.
> But I think, we don't need the assertion for GetImageElement and
> GetGradientImageValue since those methods have the assertion.
> https://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.h#462

That is NS_ASSERTION though.
Comment on attachment 8880680 [details]
Bug 1371115 - Part 14: add tests for moz prefixed properties.

https://reviewboard.mozilla.org/r/152014/#review156990

::: dom/animation/test/mozilla/test_moz-prefixed-properties.html:19
(Diff revision 1)
> +      "red green": "rgb(255, 0, 0) rgb(0, 128, 0)",
> +      "#fc3": "rgb(255, 204, 51)",
> +      "currentColor": "rgb(255, 0, 0)",
> +      "red #fc3": "rgb(255, 0, 0) rgb(255, 204, 51)",
> +      "#ff00cc": "rgb(255, 0, 204)",
> +      "blue currentColor orange currentColor":
> +      "rgb(0, 0, 255) rgb(255, 0, 0) rgb(255, 165, 0) rgb(255, 0, 0)",

Do we really need all of test cases here?
It seems to me that some of them are the same for animation. I mean some of them for testing parser?
Attachment #8880680 - Flags: review?(hikezoe)
I guess we should also modify meta files for wpt.
Comment on attachment 8880605 [details]
Bug 1371115 - Part 12: implements nsStyleImage type properties animatable.

https://reviewboard.mozilla.org/r/151936/#review156950

> It seems more intuitice to me that these functions are in impl nsStyleImage.

Okay. And I will rename the function to something like 'into_image()'
Comment on attachment 8880680 [details]
Bug 1371115 - Part 14: add tests for moz prefixed properties.

https://reviewboard.mozilla.org/r/152014/#review156990

> Do we really need all of test cases here?
> It seems to me that some of them are the same for animation. I mean some of them for testing parser?

Yeah, actually there are some cases that have same meaning in case of using property_database.js.
But I'd like to use property_databasejs as it is since we need to cover all paths that build Servo object from Gecko data in clone_XX. Or should we re-define the test values?

Also, I will drop expectedValuesMap since I wanted to test either the computed value of animated element is same to parent style. So, we can test using computed style of parent as expected value.
Comment on attachment 8880680 [details]
Bug 1371115 - Part 14: add tests for moz prefixed properties.

https://reviewboard.mozilla.org/r/152014/#review156990

> Yeah, actually there are some cases that have same meaning in case of using property_database.js.
> But I'd like to use property_databasejs as it is since we need to cover all paths that build Servo object from Gecko data in clone_XX. Or should we re-define the test values?
> 
> Also, I will drop expectedValuesMap since I wanted to test either the computed value of animated element is same to parent style. So, we can test using computed style of parent as expected value.

Or we define a test case list which can ignore?
(In reply to Daisuke Akatsuka (:daisuke) from comment #62)
> Comment on attachment 8880680 [details]
> Bug 1371115 - Part 15: add tests for moz prefixed properties.
> 
> https://reviewboard.mozilla.org/r/152014/#review156990
> 
> > Do we really need all of test cases here?
> > It seems to me that some of them are the same for animation. I mean some of them for testing parser?
> 
> Yeah, actually there are some cases that have same meaning in case of using
> property_database.js.
> But I'd like to use property_databasejs as it is since we need to cover all
> paths that build Servo object from Gecko data in clone_XX. Or should we
> re-define the test values?

Then the expected values should be auto-genarated somehow?

> Also, I will drop expectedValuesMap since I wanted to test either the
> computed value of animated element is same to parent style. So, we can test
> using computed style of parent as expected value.

If each expected values are not exposed in the test, that's fine.
Comment on attachment 8880680 [details]
Bug 1371115 - Part 14: add tests for moz prefixed properties.

https://reviewboard.mozilla.org/r/152014/#review157554
Attachment #8880680 - Flags: review?(hikezoe) → review+
Attachment #8880607 - Attachment is obsolete: true
Comment on attachment 8881634 [details]
Bug 1371115 - Part 15: Remove test fail annotations from meta in wpt.

https://reviewboard.mozilla.org/r/152798/#review157942
Attachment #8881634 - Flags: review?(hikezoe) → review+
Attachment #8877910 - Attachment is obsolete: true
Attachment #8877911 - Attachment is obsolete: true
Attachment #8877912 - Attachment is obsolete: true
Attachment #8877913 - Attachment is obsolete: true
Attachment #8877914 - Attachment is obsolete: true
Attachment #8877915 - Attachment is obsolete: true
Attachment #8877917 - Attachment is obsolete: true
Attachment #8877918 - Attachment is obsolete: true
Attachment #8877919 - Attachment is obsolete: true
Attachment #8880604 - Attachment is obsolete: true
Attachment #8880606 - Attachment is obsolete: true
As discussed, I think we can write the corner-matching code more simply using match to avoid a number of unwrap()/expect() calls.
So, which one should I review?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #140)
> So, which one should I review?

Oh, sorry, Hiro.
You don't need to review.

I wanted to get review in patch 12 only since conflicted and structure of gradient changed.
Then I asked this to Brian since you were day off.

Thanks!
Comment on attachment 8880605 [details]
Bug 1371115 - Part 12: implements nsStyleImage type properties animatable.

https://reviewboard.mozilla.org/r/151936/#review159254

I've only looked over the parts that I'm told have changed. A number of the expect() and panic!() calls concern me but I don't really understand what guarantees we have regarding the inputs so maybe they're ok.

::: servo/components/style/gecko/conversions.rs:452
(Diff revision 7)
> +                };
> +
> +                let line_direction = {
> +                    if angle.is_some() && horizontal.is_none() && vertical.is_none() {
> +                        LineDirection::Angle(angle.expect("mAngle cound not convert to Angle"))
> +                    } else if angle.is_none() && has_corner_expression(horizontal) && has_corner_expression(vertical) {



::: servo/components/style/gecko/conversions.rs:385
(Diff revision 8)
> +                    Some(GenericImage::Rect(ImageRect {
> +                        url: url,
> +                        top: NumberOrPercentage::from_gecko_style_coord(&rect.data_at(0))
> +                            .expect("mCropRect[0] could not convert to NumberOrPercentage"),
> +                        right: NumberOrPercentage::from_gecko_style_coord(&rect.data_at(1))
> +                            .expect("mCropRect[1] could not convert to NumberOrPercentage"),
> +                        bottom: NumberOrPercentage::from_gecko_style_coord(&rect.data_at(2))
> +                            .expect("mCropRect[2] could not convert to NumberOrPercentage"),
> +                        left: NumberOrPercentage::from_gecko_style_coord(&rect.data_at(3))
> +                            .expect("mCropRect[3] could not convert to NumberOrPercentage")
> +                    }))

See comments below about panicking. I'm not sure how likely from_gecko_style_coord is to fail, but if it's at all possible then I'd suggest we write this as something like:

  match (NumberOrPercentage::from_gecko_style_coord(&rect.data_at(0)),
         NumberOrPercentage::from_gecko_style_coord(&rect.data_at(1)),
         NumberOrPercentage::from_gecko_style_coord(&rect.data_at(2)),
         NumberOrPercentage::from_gecko_style_coord(&rect.data_at(3))) {
    Some(top), Some(right), Some(bottom), Some(left)) => {
      Some(GenericImage::Rect(ImageRect { url, top, right, bottom, left }))
    }
    _ => {
      debug_assert!(false, "Failed to convert mCropRect to NumberOrPercentage");
      None
    }
  }

I guess that applies a number of places in this file however. It really just depends on if we ever expect from_gecko_style_coord to return None in any of these cases.

::: servo/components/style/gecko/conversions.rs:441
(Diff revision 8)
> +                    (Some(a), None, None) => LineDirection::Angle(a),
> +                    (None, Some(h), Some(v)) => {

This looks much better. Thanks!

Would it make sense to put the (Some, Some, Some) case first? Rather than last?

::: servo/components/style/gecko/conversions.rs:476
(Diff revision 8)
> +                    (None, None, None) => LineDirection::MozPosition(None, None),
> +                    (Some(_), Some(h), Some(v)) =>
> +                        LineDirection::MozPosition(Some(Position {
> +                            horizontal: h,
> +                            vertical: v,
> +                        }), angle),
> +                    x => panic!("Unexpected Linear gradient direction {:?}", x)

I wonder if we want this panic? As I understand it, we have configured rust with panic = "abort" on both dev and release channels, so this is going to crash the process if we ever encounter, for example, (None, Some, None). That seems somewhat likely so I'd probably just combine the (None, None, None) case and the "_" case here.

Or, alternatively, keep them separate, make them return the same thing, but add a debug_assert!() to the _ case.

Either way, I'd probably but these cases last.

::: servo/components/style/gecko/conversions.rs:510
(Diff revision 8)
> +                                    .expect("mRadiusX could not convert to Length");
> +                                debug_assert_eq!(radius,
> +                                                 Length::from_gecko_style_coord(&gecko_gradient.mRadiusY).unwrap());
> +                                Circle::Radius(radius)
> +                            },
> +                            x => Circle::Extent(gecko_size_to_keyword(x))

s/x/size/ ?

::: servo/components/style/gecko/conversions.rs:518
(Diff revision 8)
> +                                    LengthOrPercentage::from_gecko_style_coord(&gecko_gradient.mRadiusX)
> +                                        .expect("mRadiusX could not convert to LengthOrPercentage"),
> +                                    LengthOrPercentage::from_gecko_style_coord(&gecko_gradient.mRadiusY)
> +                                        .expect("mRadiusY could not convert to LengthOrPercentage")

(As before, I hope we're sure from_gecko_style_coord will never fail here.)

::: servo/components/style/gecko/conversions.rs:524
(Diff revision 8)
> +                                        .expect("mRadiusX could not convert to LengthOrPercentage"),
> +                                    LengthOrPercentage::from_gecko_style_coord(&gecko_gradient.mRadiusY)
> +                                        .expect("mRadiusY could not convert to LengthOrPercentage")
> +                                )
> +                            },
> +                            x => Ellipse::Extent(gecko_size_to_keyword(x))

Likewise here, s/x/size/

::: servo/components/style/gecko/conversions.rs:566
(Diff revision 8)
> +            } else {
> +                CompatMode::Modern
> +            };
> +
> +        Gradient {
> +            items: items,

(I believe you can actually just write 'items' once here now that Rust 1.17 is required.)

::: servo/components/style/gecko/conversions.rs:569
(Diff revision 8)
> +
> +        Gradient {
> +            items: items,
> +            repeating: gecko_gradient.mRepeating,
> +            kind: gradient_kind,
> +            compat_mode: compat_mode

Likewise for compat_mode
Comment on attachment 8880605 [details]
Bug 1371115 - Part 12: implements nsStyleImage type properties animatable.

https://reviewboard.mozilla.org/r/151936/#review159448
Attachment #8880605 - Flags: review?(bbirtles) → review+
Please update patch series to clear all review request for me!
Attachment #8883193 - Attachment is obsolete: true
Attachment #8883193 - Flags: review?(hikezoe)
Attachment #8883194 - Attachment is obsolete: true
Attachment #8883194 - Flags: review?(hikezoe)
Attachment #8883195 - Attachment is obsolete: true
Attachment #8883195 - Flags: review?(hikezoe)
Attachment #8883196 - Attachment is obsolete: true
Attachment #8883196 - Flags: review?(hikezoe)
Attachment #8883197 - Attachment is obsolete: true
Attachment #8883197 - Flags: review?(hikezoe)
Attachment #8883198 - Attachment is obsolete: true
Attachment #8883198 - Flags: review?(hikezoe)
Attachment #8883199 - Attachment is obsolete: true
Attachment #8883199 - Flags: review?(hikezoe)
Attachment #8883200 - Attachment is obsolete: true
Attachment #8883200 - Flags: review?(hikezoe)
Attachment #8883201 - Attachment is obsolete: true
Attachment #8883201 - Flags: review?(hikezoe)
Attachment #8883202 - Attachment is obsolete: true
Attachment #8883202 - Flags: review?(hikezoe)
Attachment #8883203 - Attachment is obsolete: true
Attachment #8883203 - Flags: review?(hikezoe)
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/154083993cbd
Part 7: implements nsStyleBorder type properties animatable. r=hiro
https://hg.mozilla.org/integration/autoland/rev/aeee80caf202
Part 12: implements nsStyleImage type properties animatable. r=birtles,hiro
https://hg.mozilla.org/integration/autoland/rev/549d48787587
Part 14: add tests for moz prefixed properties. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3d32ddb70fc2
Part 15: Remove test fail annotations from meta in wpt. r=hiro
Depends on: 1401809
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: