Paths

Table of Contentst

Differential D14673

Bug 1514449 - Use NonNegative more in the border code. r=boris,#style
ClosedPublic
Actions

Authored by emilio on Dec 15 2018, 4:50 AM.

Details

Reviewers

boris

Group Reviewers

Restricted Project

Commits

Restricted Diffusion Commit
rMOZILLACENTRALc3fe9b19eb35: Bug 1514449 - Use NonNegative more in the border code. r=boris,#style

Bugzilla Bug ID

1514449

Summary

This ended up not being so small of a patch as I'd have thought, since it
propagated a bit. But most of it is mechanical. Interesting part is
NonNegativeNumberOrPercentage and the actual uses of the NonNegative stuff and
during parsing.

This looks like it'd fix a few correctness issues during interpolation for all
the types except for BorderRadius and co (which handled it manually).

I should write tests for those in a different patch.

Depends on D14672

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.

Build Status

Buildable 27954
Build 37780: arc lint + arc unit

Event Timeline

emilio created this revision.Dec 15 2018, 4:50 AM

Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2018, 4:50 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 15 2018, 4:50 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 B27437: Diff 44745.Dec 15 2018, 4:50 AM

emilio requested review of this revision.Dec 15 2018, 4:50 AM

emilio mentioned this in D14654: Bug 1512883 - Part 1: Clamp to non-negative value after doing interpolation for circle(), ellipse(), and inset()..Dec 15 2018, 4:53 AM

xidorn added a subscriber: xidorn.Dec 15 2018, 5:08 AM

xidorn added inline comments.

servo/components/style/properties/longhands/border.mako.rs
164	Because it uses mako?
servo/components/style/values/generics/border.rs
53	Why not just make this `NonNegative` rather than propagating the prefixed type everywhere? Is it ever possible to have a negative border corner radius?

xidorn added inline comments.Dec 15 2018, 5:16 AM

servo/components/style/values/generics/border.rs
18	Similarly, maybe just apply `NonNegative` here and below, rather than propagating the type around, given that negative values are not allowed.
32	Same here.

emilio marked 4 inline comments as done.Dec 15 2018, 5:18 AM

emilio added inline comments.

servo/components/style/properties/longhands/border.mako.rs
164	Yeah... though it uses it to make a loop from 0 to 4, so it probably shouldn't. Anyway, can remove that comment if you want, just let me know.
servo/components/style/values/generics/border.rs
53	Yes, the animated corner radius can have negative lengths, so I cannot do this.

emilio marked 6 inline comments as done.Dec 15 2018, 5:19 AM

emilio added inline comments.

servo/components/style/values/generics/border.rs
18	The key is that negative types are allowed for the animated value. So the animated value of `BorderCornerRadius<NonNegativeLength>` is `BorderCornerRadius<Length>`, and then we clamp automatically when going from animated to computed instead of manually.
32	Same answer everywhere.

emilio marked 2 inline comments as done.Dec 15 2018, 5:20 AM

xidorn added inline comments.Dec 15 2018, 5:22 AM

servo/components/style/values/generics/border.rs
53	I see... That makes me wonder whether we should just make `NonNegative` negative-able in animation somehow rather than special-casing it elsewhere... since it is just animation wants it, and propagating the `NonNegative` prefix can be more error-prone.

emilio marked 2 inline comments as done.Dec 15 2018, 5:28 AM

emilio added inline comments.

servo/components/style/values/generics/border.rs
53	I'm not quite sure what you mean with "negative-able in animation", but yeah, I'm all in for a less error-prone mechanism. This is not so easy to mess-up once you get the specified type right, fwiw, you effectively force the computed value to be right as well and the only way to mess up is to write your own `ToAnimatedValue` implementation that doesn't use `#[derive]`. I'm all-in for better suggestions though... IMO we should just try to limit the usage of the `parse_non_negative` / `parse_with_clamping_mode` functions, and make them all go through newtypes. I think that's the best ideal situation I could come up right now to avoid messing this up. Would that be good enough in your opinion? In any case this is clearly an improvement over manual implementations or no implementation at all IMO.

emilio marked an inline comment as done.Dec 15 2018, 5:29 AM

emilio marked an inline comment as done.Dec 15 2018, 5:36 AM

emilio added inline comments.

servo/components/style/values/generics/border.rs
53	(Disclaimer: It's pretty early / late here, and as you could guess I didn't end up having much sleep, so sorry if I miss stuff in my comments or seem rude, I promise it's not intentional :/). /me goes try getting some more sleep.

Thanks for doing this for Bug 1512883. :)

servo/components/style/values/generics/border.rs
18	Or maybe just rename the generic types/parameters as `BorderImageSideWidth<NonNegativeLengthOrPercentage, NonNegativeNumber>`, just like what we do for `generics::basic_shape::BasicShape<..>` and `InsetRect<..>`, so the caller could know this should be an non-negative computed/specified type? (Either way is OK to me, so it's fine to keep the current version.)

This revision is now accepted and ready to land.Dec 17 2018, 9:25 PM

emilio marked an inline comment as done.Dec 17 2018, 9:35 PM

emilio added inline comments.

servo/components/style/values/generics/border.rs
18	That sounds fine, though I don't love spelling long type parameters more often. In practice as I said if you do it in the specified code you can't get it wrong. I think it'd be nicer to just make the parse_non_negative stuff private. Though there are a few similar cases we'd need to fix. I'm happy to stamp a patch changing the type parameter names if you think it's better :) Thanks for the review @boris!