Generic color parser

[tiaanl]

I would like to abstract the color parsing to allow construction of any kind of color value. Currently color parsing is based around the color::Color struct:

let color = Color::parse(&component_parser, input);

This is good when you want the details from the parsed color, but the data contained in the Color type is not ideal for calculations like color space conversions, interpolation, etc.

Rather than tweak the fields in the color type to work better for any use, I propose to extract the parsing code into a more generic parser that can construct any kind of color value. Benefits:

• Parsing of color values does not dictate the format of data you get out.
• Avoids having the data parsed into one format and then converted into another.
• When adding more complex parsing, like none values, relative color syntax, etc. the Color type will get even more complex and further away from a calculation friendly format.
• Increases stability of the color parsing API for third parties.

Example interface:

pub trait ColorFactory {
    fn from_rgba(red: u8, green: u8, blue: u8, alpha: f32) -> Self;
    fn from_lab(lightness: f32, a: f32, b: f32, alpha: f32) -> Self;
    fn from_current_color() -> Self;
}

pub fn parse_color<'i, 't, ComponentParser, C>(
    component_parser: &ComponentParser,
    input: &mut Parser<'i, 't>,
) -> Result<Color, ParseError<'i, ComponentParser::Error>>
where
    ComponentParser: ColorComponentParser<'i>,
    C: ColorFactory,
{
    todo!()
}

and users of the library would have:

enum MyColor {
    CurrentColor,
    Rgba(u8, u8, u8, f32),
    Lab(f32, f32, f32, f32),
}

impl ColorFactory for MyColor {
    fn from_rgba(red: u8, green: u8, blue: u8, alpha: f32) -> Self {
        todo!()
    }
    fn from_lab(lightness: f32, a: f32, b: f32, alpha: f32) -> Self {
        todo!()
    }
    fn from_current_color() -> Self {
        todo!()
    }
}

let color: MyColor = parse_color(&component_parser, input);

Thoughts are super welcome.

Ping @emilio @GPHemsley

[emilio]

If we do this, rather than having two traits, can we just merge ColorComponentParser and ColorFactory into a single trait? So something like:

trait ColorParser<'i> {
    type Error: 'i;
    type Color;

    fn from_rgba(...) -> Self::Color;
    ...
    fn from_current_color() -> Self::Color;

    fn parse_angle_or_number<'t>(&self, input: ...) -> ...;

That seems like it'd be simpler.

[tiaanl]

I would prefer to keep these two separate as the one modifies the behavior of the parser and the other changes what the output of the parser is. e.g. if I want a different color type like (u8, u8, u8, u8) I only want to implement the "factory" functions and pass that to the parser and not worry about having to implement the parsing bits like parse_angle_or_number(..) and vice versa.

[emilio]

Well, we'd keep the default implementations that we have now, right? So you can implement just what you need, really. ColorComponentParser already changes the output of the parser via the Error type.

[GPHemsley]

I think I like the idea of abstraction and separating out functionality in principle, but I think I'm struggling to understand the usecase here.

What are you suggesting gets abstracted out of Color? The parser, or the construction of a color from constituent parts?

Can you give a before/after example of how dependent code would improve?

And does the rearranging I've done as part of #314 work towards this goal, or against it?

[tiaanl]

This biggest use case right now would be to avoid parsing the color into memory in one format and then having to convert it into another (more optimized) version after that. While the changes you are making in #314 lays out the data in a "correct" way according to the spec, it is not necessarily the most optimized for use in Firefox.

So this change will allow us to store the color data in any format we choose, but by default it will be stored into the color format that is specified in this library.

For normal use of the library:

let color = cssparser::Color::parse(&mut default_component_parser, &mut input)?;
match color {
    Color::Rgb(..),
    Color::Lab(..),
    Color::ColorFunction(..),
}

But this change will allow us to do the following in FF:

struct MyColor(u8, u8, u8, u8);
let color: MyColor = cssparser::parse_into(&mut default_component_parser, &mut input)?;
assert_eq!(color.0, ..);

And then the use case we are avoiding:

let color = Color::parse(...)?;
let better_color = match color {
    Color::Rgb(r, g, b, a) => MyColor(r, g, b, a),
    Color::Laba(l, a, b, al) => MyColor(...<convert to rgba>...),
}

So to answer your questions directly, I think this change would be better described as separating the color format from the parser so that they can be used independently, in stead of extracting something.

And yes it will allow your changes in #314 to be the "default" color format and the changes we need in FF will not interfere.

Hope this explains a bit better.

[emilio]

This was done, right?