Page MenuHomePhabricator
Differential D14654

Bug 1512883 - Part 1: Clamp to non-negative value after doing interpolation for circle(), ellipse(), and inset().
ClosedPublic
Actions

Authored by boris on Dec 15 2018, 12:25 AM.

Details

Reviewers
emilio
birtles
Commits
Restricted Diffusion Commit
rMOZILLACENTRAL70912a8087d7: Bug 1512883 - Part 1: Clamp to non-negative value after doing interpolation for…
Bugzilla Bug ID
1512883
Summary

Replace LengthOrPercentage with NonNegativeLengthOrPercentage on
ShapeRadius, Circle, Ellipse. And derive ToAnimatedValue for ShapeSource and
its related types, so we clamp its interpolated results into non-negative
values. (i.e. The radius of circle()/ellipse() and the border-radius of
inset().)

Note: We may get negative values when using a negative easing function, so the
clamp is necessary to avoid the incorrect result or any undefined behavior.

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

boris created this revision.Dec 15 2018, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2018, 12:25 AM
boris added a child revision: D14655: Bug 1512883 - Part 2: Tests..Dec 15 2018, 12:25 AM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 15 2018, 12:25 AM
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: Restricted Project.
Harbormaster completed remote builds in B27412: Diff 44707.Dec 15 2018, 12:25 AM
boris requested review of this revision.Dec 15 2018, 12:25 AM
Herald added a subscriber: emilio. · View Herald TranscriptDec 15 2018, 12:25 AM
boris updated this revision to Diff 44715.Dec 15 2018, 12:32 AM
boris edited the summary of this revision. (Show Details)
boris added reviewers: emilio, birtles.Dec 15 2018, 12:35 AM
emilio added a comment.Dec 15 2018, 3:36 AM

So, in order to avoid all these manual implementations of ToAnimatedValue, we could / should instead use NonNegativeLengthOrPercentage and co on the types, and then this will work automatically and can be derived. Did you try that already and hit any roadblock with it? I know this is already not what the border-radius code is doing, but arguably that's not great.

The problem I have with this patch is that it's very easy to forget to clamp properly a type on one of the branches that right now just move the value, which is a problem you don't have with #[derive].

emilio added a comment.Dec 15 2018, 4:53 AM

Given I suggested it, and I saw it was a bit non-trivial when I gave it a shot, I opened D14673 with a bunch of border types (including BorderCornerRadius, which was the main offender here) migrated to it.

emilio requested changes to this revision.Dec 15 2018, 5:02 AM

Thanks for looking at this btw!

servo/components/style/values/computed/basic_shape.rs
78

I think all this code can go away if you write this on top of D14673.

134

Same, trivial_to_animated_value for now instead?

servo/components/style/values/specified/svg_path.rs
148 ↗(On Diff #44715)

Ideally, we'd be able to assume that if <T as ToAnimatedValue>::AnimatedValue == T we could just make it a no-op. But there are a few offenders. BorderCornerRadius was one, ComputedMaxLength / ComputedMozLength / FontSizeAdjust are others. I don't even know what FontSize is trying to do btw, should probably animate as a Length. Aaanyway, I didn't fix it for now.

In any case, instead of this you can just use the trivial_to_animated_value macro we have elsewhere.

This revision now requires changes to proceed.Dec 15 2018, 5:02 AM
boris marked 2 inline comments as done.Dec 17 2018, 10:05 PM
boris added inline comments.
servo/components/style/values/computed/basic_shape.rs
78

Yes. Just follow the same idea: using NonNegativeLengthOrPercentage for ShapeRadius to make this simpler.

134

Yes. I should use trivial_to_animated_value. :)

boris updated this revision to Diff 45668.Dec 18 2018, 9:33 PM
boris edited the summary of this revision. (Show Details)
boris marked an inline comment as done.
boris marked 2 inline comments as done.Dec 18 2018, 9:34 PM
boris marked an inline comment as done.Dec 18 2018, 10:04 PM
boris added inline comments.
servo/components/style/values/computed/basic_shape.rs
26–32

This was formatted by rustfmt. I ran it because this line is too long. :)

boris updated this revision to Diff 45691.Dec 18 2018, 10:07 PM
emilio accepted this revision.Dec 18 2018, 10:07 PM

Much better, and hopefulley easier to write as well! :)

servo/components/style/values/specified/svg_path.rs
26 ↗(On Diff #45668)

I suspect servo will undo a lot of the formatting changes here, can you make sure that the rustfmt config file in servo/rustfmt.toml is used?

boris marked 2 inline comments as done.Dec 18 2018, 10:27 PM
boris added inline comments.
servo/components/style/values/specified/svg_path.rs
26 ↗(On Diff #45668)

This is an accident. I updated this file during debugging and forgot to rollback this file.

I ran rustfmt only in specified/basic_shape.rs and computed/basic_shape.rs for adjusting the too-long-line. The rest files were changed manually.

BTW, my :RustFmt runs with servo/rustfmt.toml. (I just tested it by updating the TOML file to see the effect.)

boris updated this revision to Diff 45761.Dec 19 2018, 12:18 AM
boris marked an inline comment as done.
birtles accepted this revision.Dec 19 2018, 12:48 AM
birtles added inline comments.
servo/components/style/values/animated/mod.rs
345

Not sure, but maybe this should be s/use ToAnimatedValue derive/derive ToAnimatedValue/

346

s/ToAnimatiedValue/ToAnimatedValue/

347–349

Perhaps this sentence might be a little easier to follow as:

"However, the general version of "impl ToAnimatedValue for Box<[T]>" needs to clone |T| and convert it into |T::AnimatedValue|. However, for SVGPathData that is unnecessary--moving |T| is sufficient. So here,..."

This revision is now accepted and ready to land.Dec 19 2018, 12:48 AM
boris updated this revision to Diff 46024.Dec 19 2018, 7:08 PM
boris marked 3 inline comments as done.
Closed by commit rMOZILLACENTRAL70912a8087d7: Bug 1512883 - Part 1: Clamp to non-negative value after doing interpolation for… (authored by boris). · Explain WhyDec 19 2018, 8:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
servo/
components/
style/
gecko/
4 lines
properties/
longhands/
2 lines
2 lines
values/
animated/
15 lines
computed/
16 lines
generics/
78 lines
1 line
1 line
1 line
specified/
17 lines

Diff 46097

View Options

servo/components/style/gecko/values.rs

Loading...
View Options

servo/components/style/properties/longhands/box.mako.rs

Loading...
View Options

servo/components/style/properties/longhands/svg.mako.rs

Loading...
View Options

servo/components/style/values/animated/mod.rs

Loading...
View Options

servo/components/style/values/computed/basic_shape.rs

Loading...
View Options

servo/components/style/values/generics/basic_shape.rs

Loading...
View Options

servo/components/style/values/generics/border.rs

Loading...
View Options

servo/components/style/values/generics/position.rs

Loading...
View Options

servo/components/style/values/generics/rect.rs

Loading...
View Options

servo/components/style/values/specified/basic_shape.rs

Loading...
Privacy · Cookies · Legal