Closed Bug 1408310 Opened 7 years ago Closed 7 years ago

make WebKitCSSMatrix use Servo for parsing transform lists

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: heycam, Assigned: boris)

References

(
URL
)

Details

Attachments

(6 files)

Bug 1408310 - Part 2: Use CSS parser for DOMMatrix::SetMatrixValue.
59 bytes, text/x-review-board-request
baku
: review+
heycam
: review+
Details
Bug 1408310 - Part 4: Store mIsServo into DOMMatrixReadOnly.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.
59 bytes, text/x-review-board-request
heycam
: review+
emilio
: review+
Details
Bug 1408310 - Part 1: Fix nsCSSParser if we fail to parse a transform property.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1408310 - Part 3: Replace DOMMatrixReadOnly::Identity with IsIdentity.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Servo PR, #19388
41 bytes, text/x-github-pull-request
Details | Review 
WebKitCSSMatrix::SetMatrixValue currently uses nsCSSParser to parse a <transform-list> value.
Assignee: nobody → boris.chiou
OK, looks like we can just add an FFI to parse the transform property by Servo parser [1].

[1] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/servo/components/style/properties/longhand/box.mako.rs#1197-1200
Oh, we have an FFI to parse a property already [1]. (Yeah, we added this FFI for Animations!)

[1] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/servo/ports/geckolib/glue.rs#2303-2309
According to the spec, https://drafts.fxtf.org/geometry/#webkitcssmatrix, WebKitCSSMatrix is an alias of DOMMatrix now, so I'm wondering should we use the same DOMMatirx::SetMatrixValue() function for both WebKitCSSMatrix and DOMMatrix?
Oh, DOMMatrix::SetMatrixValue() doesn't support matrix3d.... :(
Yeah, we should be parsing the value in the same way in both DOMMatrix::SetMatrixValue and WebKitCSSMatrix::SetMatrixValue.  I see we have bug 1364400 to make DOMMatrix::SetMatrixValue parse a CSS <transform-list> value instead of an SVG transform="" attribute value (which is what it does currently).  There's also bug 1366738 for implementing the Web IDL bindings functionality for the alias, which would also solve that.

Having a single function that both DOMMatrix::SetMatrixValue and WebKitCSSMatrix::SetMatrixValue can use to parse a <transform-list> with Servo sounds good.
Oh Thanks. In this bug, I'd like to update DOMMatrix::SetMatrixValue() to use Servo/Gecko css parser, and try to let both WebKitCSSMatrix::SetMatrixValue() and DOMMatrix::SetMatrixValue() use the same parser code. Therefore, maybe we can fix this bug and bug 1364400 together.
See Also: → 1364400
Status: NEW → ASSIGNED
Found a bug:
  var xxx = new WebKitCSSMatrix("translate(calc(100px + 50px))")
got an assertion.
There is an existing bug of nsCSSParser while calling ParseTransformProperty many times:
e.g.
      new WebKitCSSMatrix("none;");
      new WebKitCSSMatrix("translate(10px, 20px)");

The second constructor of WebKitCSSMatrix hit an assertion:

Assertion failure: PropertyAt(prop)->GetUnit() == eCSSUnit_Null (not initial state), at /layout/style/nsCSSDataBlock.cpp:811
#01: (anonymous namespace)::CSSParserImpl::ParseProperty(nsCSSPropertyID, nsTSubstring<char16_t> const&, nsIURI*, nsIURI*, nsIPrincipal*, mozilla::css::Declaration*, bool*, bool, bool)[/objdirs/obj-browser-stylo-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3093d4b]
#02: nsDOMCSSDeclaration::ParsePropertyValue(nsCSSPropertyID, nsTSubstring<char16_t> const&, bool)[/objdirs/obj-browser-stylo-debug/dist/NightlyDebug.app/Contents/MacOS/XUL +0x30d85bc]

I think the root cause is: we reuse nsCSSParserImpl object after destroying the previous nsCSSParser, and we clean the previous status _not enough_ after the first parser error, i.e. "none;". "none" is a keyword for transform, and we parse it successfully after seeing "none". However, scanner is not at EOF, so we change the parse status to error [1]. And then we just return false and didn't rollback/clean nsCSSParserImpl::mTempData after seeing this error. I will add one extra patch for this.

[1] https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/layout/style/nsCSSParser.cpp#1844
Comment on attachment 8930015 [details]
Bug 1408310 - Part 1: Fix nsCSSParser if we fail to parse a transform property.

https://reviewboard.mozilla.org/r/201216/#review206728

::: layout/style/nsCSSParser.cpp:1846
(Diff revision 1)
>                                   aDisallowRelativeValues);
>    // We should now be at EOF
>    if (parsedOK && GetToken(true)) {
>      parsedOK = false;
> +    mTempData.ClearProperty(eCSSProperty_transform);
> +    mTempData.AssertInitialState();

Can you move this AssertInitialState() call to the end of the function?  I think we want to assert that whether we had an invalid value or not.
Attachment #8930015 - Flags: review?(cam) → review+
Comment on attachment 8928909 [details]
Bug 1408310 - Part 2: Use CSS parser for DOMMatrix::SetMatrixValue.

https://reviewboard.mozilla.org/r/200222/#review206732

::: testing/web-platform/meta/css/geometry-1/DOMMatrix-001.html.ini:90
(Diff revision 3)
>    [new DOMMatrix("scale(2) translateX(5px) translateY(5px)")]
>      expected: FAIL

It seems like this sub-test (and many others) should be passing now.  Can you check why it's still failing?  (And does it still fail when using the Servo transform list parser?)
(In reply to Cameron McCormack (:heycam) from comment #23)
> Comment on attachment 8928909 [details]
> Bug 1408310 - Part 2: Use CSS parser for DOMMatrix::SetMatrixValue.
> 
> https://reviewboard.mozilla.org/r/200222/#review206732
> 
> ::: testing/web-platform/meta/css/geometry-1/DOMMatrix-001.html.ini:90
> (Diff revision 3)
> >    [new DOMMatrix("scale(2) translateX(5px) translateY(5px)")]
> >      expected: FAIL
> 
> It seems like this sub-test (and many others) should be passing now.  Can
> you check why it's still failing?  (And does it still fail when using the
> Servo transform list parser?)

Agree. I can check it. Thanks.
Comment on attachment 8928910 [details]
Bug 1408310 - Part 4: Store mIsServo into DOMMatrixReadOnly.

https://reviewboard.mozilla.org/r/200224/#review206742

::: dom/base/Element.cpp:3771
(Diff revision 3)
> -  DOMMatrixReadOnly* matrix = new DOMMatrix(this, transform);
> +  bool isServo = primaryFrame->StyleContext()->IsServo();
> +  DOMMatrixReadOnly* matrix = new DOMMatrix(this, transform, isServo);

You can just call IsStyledByServo() on the current Element, instead of going to the frame.  (Same for the other two changes in this file.)
Attachment #8928910 - Flags: review?(cam) → review+
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review206752

Sorry for the drive-by review... Is there any way we can duplicate a little less code? :/

::: servo/components/style/values/computed/length.rs:273
(Diff revision 4)
>      }
> +
> +    /// Compute the value into pixel length as CSSFloat without context,
> +    /// so it returns Err(()) if there is any value other than pixel length.
> +    pub fn to_computed_pixel_length_without_context(&self) -> Result<CSSFloat, ()> {
> +        if self.vw.is_some() || self.vh.is_some() || self.vmin.is_some() || self.vmax.is_some() ||

This is somewhat unfortunate, and somewhat footgunny (adding a new relative unit and forgetting to add it here and suddenly you ignore it).

How useful is `calc()` in `DOMMatrix`? How well-tested is it? Does Gecko support it? And Chrome? If it needs to be supported, this needs a comment on the definition of the type, reminding people of this.

::: servo/components/style/values/specified/mod.rs:219
(Diff revision 4)
>      pub fn get(&self) -> f32 {
>          self.calc_clamping_mode.map_or(self.value, |mode| mode.clamp(self.value))
>      }
>  
> +    /// Returns the numeric value as f64, clamped if needed.
> +    pub fn get64(&self) -> f64 {

Let's just use `get() as f64`, I'd say.

::: servo/components/style/values/specified/transform.rs:293
(Diff revision 4)
> +    /// Return the equivalent 3d matrix of this specified transform list.
> +    /// This is used by DOMMatrix, which doesn't have any layout info, and
> +    /// we only accept pixel length in this case. If there is any relative length
> +    /// or percentage, it returns Err(()).
> +    /// Normally, we return a pair: the first one is the transform matrix, and the second one
> +    /// indicates if there is any 3d transform function in this transform list.

It feels very unfortunate to just duplicate all this code with the `computed::Transform` variant :(

::: servo/components/style/values/specified/transform.rs:296
(Diff revision 4)
> +    /// or percentage, it returns Err(()).
> +    /// Normally, we return a pair: the first one is the transform matrix, and the second one
> +    /// indicates if there is any 3d transform function in this transform list.
> +    pub fn to_transform_3d_matrix(&self) -> Result<(Transform3D<CSSFloat>, bool), ()> {
> +        let list = &self.0;
> +        if list.len() == 0 {

nit: `is_empty()`?

But why the special case here? it seems like just removing this if condition would produce the same result wouldn't it?

::: servo/components/style/values/specified/transform.rs:411
(Diff revision 4)
> +                GenericTransformOperation::Matrix3D(m) => {
> +                    contain_3d = true;
> +                    m.into()
> +                },
> +                GenericTransformOperation::Matrix(m) => m.into(),
> +                _ => unreachable!(),

Please list the variants that are actually unreachable and why.

(I guess it's because we never parse them, like `InterpolateMatrix`).

::: servo/ports/geckolib/glue.rs:4646
(Diff revision 4)
> +    let result = unsafe { result.as_mut() }.expect("not a valid matrix");
> +    let contain_3d = unsafe { contain_3d.as_mut() }.expect("not a valid bool");
> +
> +    let id = PropertyId::Longhand(LonghandId::Transform);
> +    let mut declarations = SourcePropertyDeclaration::new();
> +    if let Ok(_) = parse_property_into(&mut declarations, id, value,

Why using all the `parse_property_into` stuff instead of just `input.parse_entirely(|i| Transform::parse(context, i))`?
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review206752

> Sorry for the drive-by review... Is there any way we can duplicate a little less code? :/

OK. I will try to find a better way to avoid the duplicated code. Thanks for this review.
Attachment #8928909 - Flags: review?(cam)
Attachment #8928911 - Flags: review?(cam)
Comment on attachment 8928909 [details]
Bug 1408310 - Part 2: Use CSS parser for DOMMatrix::SetMatrixValue.

https://reviewboard.mozilla.org/r/200222/#review206732

> It seems like this sub-test (and many others) should be passing now.  Can you check why it's still failing?  (And does it still fail when using the Servo transform list parser?)

Looks like ```isIdentity``` and ```is2D``` are not set correctly (which are inherited from ```DOMMatrixReadOnly```), so these tests failed. I should fix this.
Attachment #8928909 - Flags: review?(amarchesini)
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review206750

::: servo/components/style/values/computed/length.rs:271
(Diff revision 4)
>      pub fn to_computed_value_zoomed(&self, context: &Context, base_size: FontBaseSize) -> CalcLengthOrPercentage {
>          self.to_computed_value_with_zoom(context, |abs| context.maybe_zoom_text(abs.into()).0, base_size)
>      }
> +
> +    /// Compute the value into pixel length as CSSFloat without context,
> +    /// so it returns Err(()) if there is any value other than pixel length.

I guess you want to say if there is any non-absolute unit.  (Because we do accept "mm" units for example.)

::: servo/components/style/values/computed/length.rs:279
(Diff revision 4)
> +           self.em.is_some() || self.ex.is_some() || self.ch.is_some() || self.rem.is_some() ||
> +           self.percentage.is_some() {
> +            return Err(());
> +        }
> +
> +        Ok(self.absolute.map(|len| len.to_px()).unwrap_or(0.))

Can we expect("...") or unwrap() instead of unwrap_or(0.)?  Because any cases where we couldn't convert to pixels should be handled above.

::: servo/components/style/values/computed/transform.rs:339
(Diff revision 4)
>              LengthOrPercentage::Length(px) => px.px(),
>              LengthOrPercentage::Percentage(_) => 0.,
>              LengthOrPercentage::Calc(calc) => calc.length().px(),
>          };
>  
> +        const TWO_PI: f32 = 2.0f32 * ::std::f32::consts::PI;

Don't need the "::std::" in here.

::: servo/components/style/values/specified/transform.rs:52
(Diff revision 4)
> +#[cfg_attr(rustfmt, rustfmt_skip)]
> +impl From<Matrix> for Transform3D<f64> {
> +    #[inline]
> +    fn from(m: Matrix) -> Self {
> +        Transform3D::row_major(
> +            m.a.get64(), m.b.get64(), 0.0, 0.0,

I think I would rather just see "as f64" instead of adding a get64() function.

Looking at radians64(), I see that radians() does some special clamping.  Is that something we have to worry about when we cast the f64 matrix into an f32 matrix?  What would happen if we returned infinity values back to the caller?

::: servo/components/style/values/specified/transform.rs:290
(Diff revision 4)
>          })?))
>      }
>  
> +    /// Return the equivalent 3d matrix of this specified transform list.
> +    /// This is used by DOMMatrix, which doesn't have any layout info, and
> +    /// we only accept pixel length in this case. If there is any relative length

s/pixel length/absolute lengths/

::: servo/components/style/values/specified/transform.rs:294
(Diff revision 4)
> +    /// This is used by DOMMatrix, which doesn't have any layout info, and
> +    /// we only accept pixel length in this case. If there is any relative length
> +    /// or percentage, it returns Err(()).
> +    /// Normally, we return a pair: the first one is the transform matrix, and the second one
> +    /// indicates if there is any 3d transform function in this transform list.
> +    pub fn to_transform_3d_matrix(&self) -> Result<(Transform3D<CSSFloat>, bool), ()> {

It would be nice if we could have a generic function that both specified::Transform and computed::Transform could use, since they are a bit similar, but I guess that might be a bit hard...

::: servo/components/style/values/specified/transform.rs:311
(Diff revision 4)
> +        // We intentionally use Transform3D<f64> during computation to avoid error propagation
> +        // because using f32 to compute triangle functions (e.g. in create_rotation()) is not
> +        // accurate enough. In Gecko, we also use "double" to compute the triangle functions.
> +        // Therefore, let's use Transform3D<f64> during matrix computation and cast it into f32
> +        // in the end.

Should we be doing this in computed::Transform::to_transform_3d_matrix too?

::: servo/components/style/values/specified/transform.rs:319
(Diff revision 4)
> +        // Therefore, let's use Transform3D<f64> during matrix computation and cast it into f32
> +        // in the end.
> +        let mut transform = Transform3D::<f64>::identity();
> +        let mut contain_3d = false;
> +
> +        const TWO_PI: f64 = 2.0f64 * ::std::f64::consts::PI;

Don't need the "::std::" in here.

::: servo/components/style/values/specified/transform.rs:411
(Diff revision 4)
> +                GenericTransformOperation::Matrix3D(m) => {
> +                    contain_3d = true;
> +                    m.into()
> +                },
> +                GenericTransformOperation::Matrix(m) => m.into(),
> +                _ => unreachable!(),

Instead of this, can you explicit list the remaining transform operations?  Then if we add any new ones in the future we won't forget to update this function.

::: servo/ports/geckolib/glue.rs:4713
(Diff revision 4)
>  
>  #[no_mangle]
>  pub unsafe extern "C" fn Servo_SourceSizeList_Drop(list: RawServoSourceSizeListOwned) {
>      let _ = list.into_box::<SourceSizeList>();
>  }
> +

Nit: Drop this.
Attachment #8928911 - Flags: review+
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review206750

> It would be nice if we could have a generic function that both specified::Transform and computed::Transform could use, since they are a bit similar, but I guess that might be a bit hard...

It's a good suggestion. I will try to do this, just as Emilio's suggestion, to remove some duplicated code. Thanks.
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review206752

> This is somewhat unfortunate, and somewhat footgunny (adding a new relative unit and forgetting to add it here and suddenly you ignore it).
> 
> How useful is `calc()` in `DOMMatrix`? How well-tested is it? Does Gecko support it? And Chrome? If it needs to be supported, this needs a comment on the definition of the type, reminding people of this.

In ```DOMMatrix```, we can use ```calc()```. However, ```DOMMatrix``` doesn't accept relative lengths, so ```calc(1px + 2px)``` is acceptable, but ```calc(1px + 1em)``` is not acceptable, which returns a DOM exception (syntax error). Gecko doesn't support calc well, but Chrome and Webkit support this and I intentionally make the result of Stylo more simliar to Webkit/Chrome. I will add some comment on this type (i.e. specified::CalcLengthOrPercentage) to remind prople of this.
Comment on attachment 8928909 [details]
Bug 1408310 - Part 2: Use CSS parser for DOMMatrix::SetMatrixValue.

https://reviewboard.mozilla.org/r/200222/#review207742
Attachment #8928909 - Flags: review?(amarchesini) → review+
Comment on attachment 8928909 [details]
Bug 1408310 - Part 2: Use CSS parser for DOMMatrix::SetMatrixValue.

https://reviewboard.mozilla.org/r/200222/#review207738

Those test failures in DOMMatrix-001.html are due to:
1. We don't support IsIdentity(), which is fixed in the next patch.
2. If the input is a 3d matrix, but the 3d parts are all zero, Is2D() should return true. However, gfx::Matrix4x4 still return false in this case. We should fix this in other bugs, e.g. Bug 1364400.
3. Some matrix elements should be 0, but our computation returns an extremely small value, e.g. 9.992007221626409e-16. (Won't fix for now)
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review207760

I don't understand why the `expect` in the calc code is correct, care to explain?

Also, I don't understand why the existence of some of the traits.

::: servo/components/style/values/computed/angle.rs:55
(Diff revision 5)
> +        Ok(Angle::from_radians(self.radians().animate(&other.radians(), procedure)?))
> +    }
> +}
> +
> +/// A helper trait to convert an angle type into f64 radians.
> +pub trait Radians64 {

Ditto for this trait, there's no reason for it to live here... This is just to make transform code generic, let's keep it in the `transform` module. Or you can use something like `AsRef<computed::Angle>`, and make the `specified::Angle` implement it, I guess...

::: servo/components/style/values/computed/length.rs:279
(Diff revision 5)
> +           self.em.is_some() || self.ex.is_some() || self.ch.is_some() || self.rem.is_some() ||
> +           self.percentage.is_some() {
> +            return Err(());
> +        }
> +
> +        Ok(self.absolute.map(|len| len.to_px()).expect("Should unwrap pixel value successfully"))

What does guarantee that we have any absolute length? I don't understand this `expect()`. Doesn't it fail with `calc(3em)`?

::: servo/components/style/values/generics/transform.rs:349
(Diff revision 5)
>          }
>      }
>  }
>  
> +/// Support the conversion to a 3d matrix.
> +pub trait ToMatrix {

I don't understand why this trait is needed. Why can't we just `impl<..> TransformOperation<..> where...`?

::: servo/components/style/values/generics/transform.rs:359
(Diff revision 5)
> +    fn to_3d_matrix(&self, reference_box: Option<&Rect<Au>>) -> Result<Transform3D<f64>, ()>;
> +}
> +
> +#[cfg_attr(rustfmt, rustfmt_skip)]
> +impl<Angle, Number, Length, Integer, LoN, LoP, LoPoNumber> ToMatrix for
> +    TransformOperation<Angle, Number, Length, Integer, LoN, LoP, LoPoNumber>

nit: Indentation looks weird. Why `rustfmt_skip`? Can you run rustfmt on this signature at least?

::: servo/components/style/values/generics/transform.rs:393
(Diff revision 5)
> +        let reference_height = reference_box.map(|v| v.size.height);
> +        let matrix = match *self {
> +            Rotate3D(ax, ay, az, theta) => {
> +                let theta = TWO_PI - theta.radians64();
> +                let (ax, ay, az, theta) =
> +                    transform_helper::get_normalized_vector_and_angle(ax.into(),

nit: Use block indentation for arguments.

::: servo/components/style/values/generics/transform.rs:645
(Diff revision 5)
> +        Ok((cast_3d_transform(transform), contain_3d))
> +    }
> +}
> +
> +/// Helper functions for transform.
> +pub mod transform_helper {

Why this module? Not that it hurts, but we could just put the functions in the `transform` module.

::: servo/ports/geckolib/glue.rs:4643
(Diff revision 5)
> +    contain_3d: *mut bool,
> +    result: *mut RawGeckoGfxMatrix4x4
> +) -> bool {
> +    use style::properties::longhands::transform;
> +
> +    let context = ParserContext::new(Origin::Author,

nit: Block indentation for arguments.

::: servo/ports/geckolib/glue.rs:4650
(Diff revision 5)
> +                                     Some(CssRuleType::Style),
> +                                     ParsingMode::DEFAULT,
> +                                     QuirksMode::NoQuirks);
> +    let string = unsafe { (*value).to_string() };
> +    let mut input = ParserInput::new(&string);
> +    if let Ok(t) = Parser::new(&mut input).parse_entirely(|t| transform::parse(&context, t)) {

nit: This can look nicer if you early return and deindent the code below, but not a big deal:

```
let transform = match Parser::new(..).parse_entirely(..) {
    Ok(t) => t,
    Err(..) => return false,
};

let (m, is_3d) = match t.to_transform_3d_matrix(None) {
    Ok(result) => result,
    Err(..) => return false,
};

// ...

true
```

::: testing/web-platform/meta/css/geometry-1/DOMMatrix-001.html.ini:209
(Diff revision 5)
>    [new DOMMatrix("scale(2) translateX(calc(2 * 2.5px)) translateY(5px)")]
> -    expected: FAIL
> +    expected:
> +      if stylo: PASS
> +      FAIL
>  
>    [new DOMMatrix("scale(2) translateX(5px) translateY(5px) rotate(5deg) rotate(-5deg)")]

Why do these still fail?
Attachment #8928911 - Flags: review?(emilio)
Oh, I just read the "why do these fail" bit from comment 38, so nvm about that.
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review207770

::: servo/components/style/values/computed/length.rs:279
(Diff revision 5)
> +           self.em.is_some() || self.ex.is_some() || self.ch.is_some() || self.rem.is_some() ||
> +           self.percentage.is_some() {
> +            return Err(());
> +        }
> +
> +        Ok(self.absolute.map(|len| len.to_px()).expect("Should unwrap pixel value successfully"))

Oh, ok, I guess the point of this is that if we didn't forget anything above us, then we wouldn't parse successfully.

Mind making the panic message something like: `"Someone added another unit and forgot to handle it here"`?

Also, maybe it's not worth to panic here, and we could do something like:

```
match self.absolute {
    Some(abs) => Ok(abs.to_px()),
    None => {
        debug_assert!(false, "Someone forgot to handle an unit here: {:?}", self);
        Err(())
    }
}
```

::: servo/components/style/values/computed/length.rs:459
(Diff revision 5)
>          }
>      }
>  }
>  
> +/// Convert a length type into the absolute lengths.
> +pub trait ConvertToAbsoluteLength {

I think this trait should be defined where it's used, and maybe even implemented. Also, we don't prefix any trait with `Convert` anywhere else, so probably should be just called `ToAbsoluteLength`.
Comment on attachment 8931268 [details]
Bug 1408310 - Part 3: Replace DOMMatrixReadOnly::Identity with IsIdentity.

https://reviewboard.mozilla.org/r/202414/#review207798

::: dom/webidl/DOMMatrix.webidl:75
(Diff revision 1)
>      DOMMatrix flipY();
>      DOMMatrix inverse();
>  
>      // Helper methods
>      readonly attribute boolean is2D;
> -    readonly attribute boolean identity;
> +    readonly attribute boolean isIdentity;

Why we think removing identity is webcompatible?

But looks like some other browsers have isIdentity, so I guess we can try.
Attachment #8931268 - Flags: review?(bugs) → review+
Comment on attachment 8928909 [details]
Bug 1408310 - Part 2: Use CSS parser for DOMMatrix::SetMatrixValue.

https://reviewboard.mozilla.org/r/200222/#review207964

Thanks for the explanation of the test failures.
Attachment #8928909 - Flags: review?(cam) → review+
Comment on attachment 8931268 [details]
Bug 1408310 - Part 3: Replace DOMMatrixReadOnly::Identity with IsIdentity.

https://reviewboard.mozilla.org/r/202414/#review207798

> Why we think removing identity is webcompatible?
> 
> But looks like some other browsers have isIdentity, so I guess we can try.

Yes. I tried other browsers and they use isIdentity now, and the last published/draft version of spec also use isIdenetity. Thanks.
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review207770

> Oh, ok, I guess the point of this is that if we didn't forget anything above us, then we wouldn't parse successfully.
> 
> Mind making the panic message something like: `"Someone added another unit and forgot to handle it here"`?
> 
> Also, maybe it's not worth to panic here, and we could do something like:
> 
> ```
> match self.absolute {
>     Some(abs) => Ok(abs.to_px()),
>     None => {
>         debug_assert!(false, "Someone forgot to handle an unit here: {:?}", self);
>         Err(())
>     }
> }
> ```

Thanks for the suggestion. Yes, that is what I want.

> I think this trait should be defined where it's used, and maybe even implemented. Also, we don't prefix any trait with `Convert` anywhere else, so probably should be just called `ToAbsoluteLength`.

I see. I will update this. Thanks.
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review207760

> I don't understand why this trait is needed. Why can't we just `impl<..> TransformOperation<..> where...`?

The definition of Transform is ```pub struct Transform<T>(pub Vec<T>)```, which doesn't know if ```T``` is ```TransformOperation<...>```. Therefore, I have to define ```ToMatrix```, so I can use the trait bounding to implement it, e.g. ```impl<T: ToMatrix> Transform<T> {...}```. If we use ```impl<..> TransformOperation<..> where...```, we have to update the definition of ```Transform<T>```, right? Or do we have a better way to implement it? Thanks.
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review208138

Looks good, thanks a lot!

::: servo/components/style/values/computed/angle.rs:75
(Diff revision 6)
>      fn animate_fallback(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
>          Ok(Angle::from_radians(self.radians().animate(&other.radians(), procedure)?))
>      }
>  }
>  
> +impl AsRef<Angle> for Angle {

Huh, I would've expected this to be implemented already...

::: testing/web-platform/meta/css/geometry/DOMMatrix-001.html.ini:202
(Diff revision 6)
>  
>    [new DOMMatrixReadOnly(matrix)]
>      expected: FAIL
>  
>    [new DOMMatrixReadOnly("scale(2, 2), translateX(5px) translateY(5px)")]
>      expected: FAIL

nit: Maybe file and annotate the expectations with the bug number?
Attachment #8928911 - Flags: review?(emilio) → review+
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review208138

> Huh, I would've expected this to be implemented already...

Me, too. But I got a compilation error because of this. :(

> nit: Maybe file and annotate the expectations with the bug number?

OK, I think we chould fix the rest part in Bug 1364400. Thanks.
Comment on attachment 8928911 [details]
Bug 1408310 - Part 5: Use Servo CSS parser for DOMMatrix on Stylo.

https://reviewboard.mozilla.org/r/200226/#review208138

> OK, I think we chould fix the rest part in Bug 1364400. Thanks.

Besides, I notice that no one left comments in the wpt ini files (it causes some errors?). Anyway, I will leave a comment in Bug 136400 after landing this.
Attached file Servo PR, #19388 — 

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66904bbad713
Part 1: Fix nsCSSParser if we fail to parse a transform property. r=heycam
https://hg.mozilla.org/integration/autoland/rev/d5237111097f
Part 2: Use CSS parser for DOMMatrix::SetMatrixValue. r=baku,heycam
https://hg.mozilla.org/integration/autoland/rev/96876638fdc3
Part 3: Replace DOMMatrixReadOnly::Identity with IsIdentity. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d02482644cc1
Part 4: Store mIsServo into DOMMatrixReadOnly. r=heycam
https://hg.mozilla.org/integration/autoland/rev/be664d70c5dd
Part 5: Use Servo CSS parser for DOMMatrix on Stylo. r=emilio,heycam

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: