Use a more compact RGBA color representation.

[emilio]

r? @SimonSapin

---

This change is 

[SimonSapin]

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

Cargo.toml, line 4 at r1 (raw file):

name = "cssparser"
version = "0.8.0"

@nox would like this to go after the serde upgrade. So… please find an agreement between you :)

---

src/color.rs, line 18 at r1 (raw file):

pub struct RGBA {
    /// The red component.
    pub r: u8,

Please avoid one-letter names in public APIs

---

src/color.rs, line 33 at r1 (raw file):

    #[inline]
    pub fn from_floats(red: f32, green: f32, blue: f32, alpha: f32) -> Self {
        let r = (red.max(0.).min(1.) * 255.).round() as u8;

The range of unclamped floats that map to 0_u8 or 255_u8 is half of that for other u8 values. I think we can make this more uniform by replacing 255 with 256 and round with floor, but I don’t know if that’s the right thing to do.

---

src/color.rs, line 54 at r1 (raw file):

    /// Returns the red channel in a floating point number form, from 0 to 1.
    #[inline]
    pub fn red(&self) -> f32 {

Rename this red_f32 to disambiguate? (An similarly for the other three channels)

---

src/color.rs, line 361 at r1 (raw file):

            (try!(from_hex(value[0])) * 17),
            (try!(from_hex(value[1])) * 17),
            (try!(from_hex(value[2])) * 17),

Remove extra parens here

---

src/color.rs, line 383 at r1 (raw file):

    fn clamp_percentage(val: f32) -> u8 {
        (val.max(0.).min(1.) * 255.) as u8

Use .round() (or .floor(), depending on discussion on RGBA::from_floats), rename this function to clamp_f32, and make RGBA::from_floats it.

---

src/color.rs, line 394 at r1 (raw file):

        // The spec says to clamp to the device gamut which may be wider than 0%
        // ... 100%, but moz2d doesn’t seem to have any support for this, so
        // let’s not bother.

Note that only CSS Color Level 3 says this, it was removed in Level 4 which says to clamp. So this comment can be removed.

---

src/tests.rs, line 325 at r1 (raw file):

        match c {
            Ok(Color::RGBA(ref rgba))
            => [rgba.red() * 255., rgba.green() * 255., rgba.blue() * 255., rgba.alpha()].to_json(),

Use the u8 values here, no need to scale them to 0...1 only to then scale them back to 0...255.

---

src/tests.rs, line 400 at r1 (raw file):

#[test]
fn serialize_rgb_full_alpha() {
    let c = Color::RGBA(RGBA::from_floats(1.0, 0.9, 0.8, 1.0));

Use RGBA::new here so that there’s not rounding going on.

---

src/tests.rs, line 407 at r1 (raw file):

#[test]
fn serialize_rgba() {
    let c = Color::RGBA(RGBA::from_floats(0.1, 0.2, 0.3, 0.6));

Use RGBA::new here, as above.

---

src/tests.rs, line 65 at r2 (raw file):

        (_, &Json::U64(b)) => almost_equals(a, &Json::F64(b as f64)),

        (&Json::F64(a), &Json::F64(b)) => (a - b).abs() < 1. / 255.,

I don’t really like this… I’ll look into changing the test data to use 0...255 integers so that almost_equals can hopefully be removed entirely. (Or if you want to, note that color3_hsl.json and color3_keywords.json are generated by python scripts in the same directory.)

---

Comments from Reviewable

[emilio]

I can't address the review comments right now (will do tomorrow probably). I wanted to leave a few replies though.

---

Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

Cargo.toml, line 4 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

@nox would like this to go after the serde upgrade. So… please find an agreement between you :)

We discussed and I'll do the CSSParser rebase if needed :)

---

src/color.rs, line 18 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Please avoid one-letter names in public APIs

I tend to agree with this, but I think in this case is justified. I think it'd be nice to keep these for the byte access, and the accessors for the float access.

See the Servo PR, I thought replacing red(), etc for red_f32() was pretty uggly, that's why I kept the public fields instead of adding two different accessors.

---

src/color.rs, line 33 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

The range of unclamped floats that map to 0_u8 or 255_u8 is half of that for other u8 values. I think we can make this more uniform by replacing 255 with 256 and round with floor, but I don’t know if that’s the right thing to do.

We were using round() for printing, so I kept that. Let me know if you want to change it, I'm happy to do so.

---

src/color.rs, line 54 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Rename this red_f32 to disambiguate? (An similarly for the other three channels)

See above, happy to concede here, but I'd like you to look at the Servo PR before.

---

src/color.rs, line 361 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Remove extra parens here

Sure

---

src/color.rs, line 383 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Use .round() (or .floor(), depending on discussion on RGBA::from_floats), rename this function to clamp_f32, and make RGBA::from_floats it.

Will update to use round for now per my comment above.

---

src/color.rs, line 394 at r1 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Note that only CSS Color Level 3 says this, it was removed in Level 4 which says to clamp. So this comment can be removed.

Will do :)

---

Comments from Reviewable

[emilio]

Regarding the adjustment of the test error, I can also look at it :)

---

Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

Comments from Reviewable

[SimonSapin]

Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

src/tests.rs, line 65 at r2 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

I don’t really like this… I’ll look into changing the test data to use 0...255 integers so that almost_equals can hopefully be removed entirely. (Or if you want to, note that color3_hsl.json and color3_keywords.json are generated by python scripts in the same directory.)

After same IEEE 754 wrangling, I managed to make the tests pass: emilio/rust-cssparser@color...servo:u8-color

---

Comments from Reviewable

[SimonSapin]

Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

src/color.rs, line 18 at r1 (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

I tend to agree with this, but I think in this case is justified. I think it'd be nice to keep these for the byte access, and the accessors for the float access.

See the Servo PR, I thought replacing red(), etc for red_f32() was pretty uggly, that's why I kept the public fields instead of adding two different accessors.

I agree with keeping fields public rather than adding more accessors. It’s the single-letter names that I don’t like. I counted a single occurrence of .r in servo/servo#15518, so I don’t think longer names are much of a burden.

For the accessors that convert to float, I think the name should show that a conversion is going on. red_f32 is not pretty, but I counted 4 uses, so again it doesn’t seem too much.

---

src/color.rs, line 33 at r1 (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

We were using round() for printing, so I kept that. Let me know if you want to change it, I'm happy to do so.

I spent some time thinking about this, see the code comment in emilio/rust-cssparser@color...servo:u8-color#diff-7b45540eaa2c29585a26a36f03a754adR369

---

Comments from Reviewable

[SimonSapin]

Last few changes (including some I forgot in the previous commit):
emilio/rust-cssparser@color...servo:u8-color_2

r=me with that applied and a when there’s a green servo try build (to avoid blocking nox too long :))

---

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[SimonSapin]

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[SimonSapin]

Reviewed 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[SimonSapin]

@bors-servo r+

[bors-servo]

📌 Commit 173ef5f has been approved by SimonSapin

[bors-servo]

⌛ Testing commit 173ef5f with merge 570d83f...

[bors-servo]

☀️ Test successful - status-travis
Approved by: SimonSapin
Pushing 570d83f to master...