Refactor how we handle trait bounds in style_derive #18239

nox · 2017-08-25T18:29:38Z

This change is

emilio

Overall looks good, got a few comments though.

emilio · 2017-08-26T19:17:52Z

components/style/values/computed/length.rs

 pub enum MozLength {
    LengthOrPercentageOrAuto(LengthOrPercentageOrAuto),
+    #[animation(error)]


Keep the FIXME, maybe?

emilio · 2017-08-26T19:17:52Z

components/style/values/generics/basic_shape.rs

-    Shape(BasicShape, Option<ReferenceBox>),
+    Shape(
+        BasicShape,
+        #[animation(equal)]


What about naming the attribute only_if_equal? Or error_if_unequal? Seems like it'd be clearer. And the last would be more consistent with animation(error).

emilio · 2017-08-26T19:17:52Z

components/style/values/computed/length.rs

 pub struct CalcLengthOrPercentage {
+    #[animation(equal)]


The semantics of this on ToAnimatedZero are quite different to Animate... I'm not sure how I feel about this.

emilio · 2017-08-26T19:17:52Z

components/style/values/generics/grid.rs

-    Keyword(TrackKeyword),
+    Keyword(
+        #[compute(clone)]
+        TrackKeyword,


I think it'd be easier to just impl ComputedValueAsSpecified for TrackKeyword in this case, any reason that cannot be done?

emilio · 2017-08-26T19:17:52Z

components/style/properties/longhand/font.mako.rs

        pub enum T {
            None,
+            #[animation(error)]


Hmm... shouldn't this be in the None variant?


        Remove some garbage code from style_derive


        Refactor how we handle trait bounds in style_derive

For the traits we derive which methods don't depend on associated types (i.e. all of them but ToAnimatedValue and ToComputedValue), we now add trait bounds for the actual field types directly, instead of bounding the type parameters.


        Fix the name of the #[animation] attribute, oops


        Support #[animation(error)] in more cases

The trait ToAnimatedZero now supports it, and it now applies to things with generics, avoiding the trait bounds for field types of the variant on which it applies.


        Derive Animate for ShapeRadius<L>


        Introduce #[animation(constant)] for the Animate trait

This allows us to handle fields that should be the same during animations.


        Derive a bunch of animation traits for some SVG types


        Support #[animation(constant)] when deriving ToAnimatedZero


        Improve handling of trait bounds when deriving fmap-like traits


        Implement #[compute(clone)] for #[derive(ToComputedValue)]

nox · 2017-08-28T09:05:58Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e8f1a86c067586de1a03d025562ef5ead853a11


        Derive ToComputedValue for TrackBreadth<L>


        Implement #[animate(fallback)] for #[derive(Animate)]

This allows us to derive the Animate trait, providing a fallback function for when the 2 values aren't similar.


        Derive Animate and ComputeSquaredDistance for computed font-size-adjust


        Derive Animate for BackgroundSize<L>


        Derive Animate for computed::Angle


        Implement #[css(derive_debug)]

This makes #[derive(ToCss)] derive Debug with a simple call to the to_css method.


        Implement #[distance(fallback)] for #[derive(ComputeSquaredDistance)]


        Document how style traits can be derived

nox · 2017-08-28T10:30:18Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46ef23b95d92a02108ca9cd47067a0d171bf00a6

Geckolib build error fixed.

emilio

Thanks for the documentation! Maybe an example of the generated code would be nice, but that can also be a followup if somebody is confused about it.

r=me

nox · 2017-08-28T12:25:17Z

@bors-servo r=emilio

bors-servo · 2017-08-28T12:25:18Z

📌 Commit ba4136b has been approved by emilio

bors-servo · 2017-08-28T15:13:15Z

⌛ Testing commit ba4136b with merge a266e96...


        Auto merge of #18239 - servo:derive-all-the-things, r=emilio

Refactor how we handle trait bounds in style_derive  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18239)

bors-servo · 2017-08-28T16:18:48Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing a266e96 to master...


        Auto merge of #18364 - mantaroh:svgpaintkind-interpolation, r=birtles

Don't allow interpolating SVGPaintKind::None. This is a PR for https://bugzilla.mozilla.org/show_bug.cgi?id=1396483 PR #18103 disallowed interpolation between fill:none and fill:none, but that change was regressed by the refactoring in PR #18239. This patch restores the intended behavior, disabling animation of SVGPaintKind::None.  ---  - [ x ] `./mach build -d` does not report any errors - [ x ] `./mach test-tidy` does not report any errors  There are already these tests in dom/smil/tests of gecko, this PR will enable these tests. For detail, see https://bugzilla.mozilla.org/show_bug.cgi?id=1396483 .    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18364)


        Don't allow interpolating SVGPaintKind::None.

PR servo#18103 disallowed interpolation between fill:none and fill:none, but that change was regressed by the refactoring in PR servo#18239. This patch restores the intended behavior, disabling animation of SVGPaintKind::None.

highfive assigned cbrewster Aug 25, 2017

highfive added the S-awaiting-review label Aug 25, 2017

nox force-pushed the derive-all-the-things branch 2 times, most recently from 07f470b to 2aa8f3a Aug 26, 2017

emilio reviewed Aug 26, 2017

View changes

nox added 5 commits Aug 24, 2017

Remove some garbage code from style_derive

13d47ba

Refactor how we handle trait bounds in style_derive

04ad28b

For the traits we derive which methods don't depend on associated types (i.e. all of them but ToAnimatedValue and ToComputedValue), we now add trait bounds for the actual field types directly, instead of bounding the type parameters.

Fix the name of the #[animation] attribute, oops

dc827c1

Support #[animation(error)] in more cases

af560a8

The trait ToAnimatedZero now supports it, and it now applies to things with generics, avoiding the trait bounds for field types of the variant on which it applies.

Derive Animate for ShapeRadius<L>

e49dbc4

nox force-pushed the derive-all-the-things branch from 2aa8f3a to 63b1f71 Aug 28, 2017

nox added 5 commits Aug 25, 2017

Introduce #[animation(constant)] for the Animate trait

eaf2f1e

This allows us to handle fields that should be the same during animations.

Derive a bunch of animation traits for some SVG types

405e34a

Support #[animation(constant)] when deriving ToAnimatedZero

8101887

Improve handling of trait bounds when deriving fmap-like traits

efc852f

Implement #[compute(clone)] for #[derive(ToComputedValue)]

735e093

nox force-pushed the derive-all-the-things branch 2 times, most recently from 0820314 to ccae9ca Aug 28, 2017

nox added 8 commits Aug 28, 2017

Derive ToComputedValue for TrackBreadth<L>

3751fe9

Implement #[animate(fallback)] for #[derive(Animate)]

4a4bf89

This allows us to derive the Animate trait, providing a fallback function for when the 2 values aren't similar.

Derive Animate and ComputeSquaredDistance for computed font-size-adjust

318d869

Derive Animate for BackgroundSize<L>

b543b17

Derive Animate for computed::Angle

1ace6d4

Implement #[css(derive_debug)]

4faadb4

This makes #[derive(ToCss)] derive Debug with a simple call to the to_css method.

Implement #[distance(fallback)] for #[derive(ComputeSquaredDistance)]

41c3be5

Document how style traits can be derived

Loading status checks…

ba4136b

nox force-pushed the derive-all-the-things branch from ccae9ca to ba4136b Aug 28, 2017

emilio approved these changes Aug 28, 2017

View changes

highfive assigned emilio and unassigned cbrewster Aug 28, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 28, 2017

highfive removed the S-awaiting-merge label Aug 28, 2017

bors-servo mentioned this pull request Aug 28, 2017

stylo: store specified value of grid layout repeat() function #18206

Merged

3 of 3 tasks complete

emilio deleted the derive-all-the-things branch Aug 28, 2017

mantaroh mentioned this pull request Sep 4, 2017

Don't allow interpolating SVGPaintKind::None. #18364

Merged

servo / servo

Refactor how we handle trait bounds in style_derive #18239

Refactor how we handle trait bounds in style_derive #18239

nox commented Aug 25, 2017 •

edited by SimonSapin

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Aug 28, 2017

nox commented Aug 28, 2017

emilio left a comment

nox commented Aug 28, 2017

bors-servo commented Aug 28, 2017

bors-servo commented Aug 28, 2017

bors-servo commented Aug 28, 2017

servo / servo

Join GitHub today

Refactor how we handle trait bounds in style_derive #18239

Refactor how we handle trait bounds in style_derive #18239

Conversation

nox commented Aug 25, 2017 • edited by SimonSapin

emilio left a comment

This comment has been minimized.

emilio Aug 26, 2017

This comment has been minimized.

emilio Aug 26, 2017

This comment has been minimized.

emilio Aug 26, 2017

This comment has been minimized.

emilio Aug 26, 2017

This comment has been minimized.

emilio Aug 26, 2017

nox commented Aug 28, 2017

nox commented Aug 28, 2017

emilio left a comment

nox commented Aug 28, 2017

bors-servo commented Aug 28, 2017

bors-servo commented Aug 28, 2017

bors-servo commented Aug 28, 2017

nox commented Aug 25, 2017 •

edited by SimonSapin