Skip to content

Avoid std::fmt in serialization code #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 30, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ encoding_rs = "0.7"
cssparser-macros = {path = "./macros", version = "0.3"}
dtoa-short = "0.3"
heapsize = {version = ">= 0.3, < 0.5", optional = true}
itoa = "0.3"
matches = "0.1"
phf = "0.7"
procedural-masquerade = {path = "./procedural-masquerade", version = "0.1"}
Expand Down
28 changes: 18 additions & 10 deletions src/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,18 +100,26 @@ impl ToCss for RGBA {
fn to_css<W>(&self, dest: &mut W) -> fmt::Result
where W: fmt::Write,
{
// Try first with two decimal places, then with three.
let mut rounded_alpha = (self.alpha_f32() * 100.).round() / 100.;
if clamp_unit_f32(rounded_alpha) != self.alpha {
rounded_alpha = (self.alpha_f32() * 1000.).round() / 1000.;
}
let serialize_alpha = self.alpha != 255;

dest.write_str(if serialize_alpha { "rgba(" } else { "rgb(" })?;
self.red.to_css(dest)?;
dest.write_str(", ")?;
self.green.to_css(dest)?;
dest.write_str(", ")?;
self.blue.to_css(dest)?;
if serialize_alpha {
dest.write_str(", ")?;

// Try first with two decimal places, then with three.
let mut rounded_alpha = (self.alpha_f32() * 100.).round() / 100.;
if clamp_unit_f32(rounded_alpha) != self.alpha {
rounded_alpha = (self.alpha_f32() * 1000.).round() / 1000.;
}

if self.alpha == 255 {
write!(dest, "rgb({}, {}, {})", self.red, self.green, self.blue)
} else {
write!(dest, "rgba({}, {}, {}, {})",
self.red, self.green, self.blue, rounded_alpha)
rounded_alpha.to_css(dest)?;
}
dest.write_char(')')
}
}

Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ fn parse_border_spacing(_context: &ParserContext, input: &mut Parser)
#![recursion_limit="200"] // For color::parse_color_keyword

extern crate dtoa_short;
extern crate itoa;
#[macro_use] extern crate cssparser_macros;
#[macro_use] extern crate matches;
#[macro_use] extern crate procedural_masquerade;
Expand Down
93 changes: 67 additions & 26 deletions src/serializer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use dtoa_short::{self, Notation};
use itoa;
use std::ascii::AsciiExt;
use std::fmt::{self, Write};
use std::io;
use std::str;

use super::Token;
Expand All @@ -24,23 +26,6 @@ pub trait ToCss {
self.to_css(&mut s).unwrap();
s
}

/// Serialize `self` in CSS syntax and return a result compatible with `std::fmt::Show`.
///
/// Typical usage is, for a `Foo` that implements `ToCss`:
///
/// ```{rust,ignore}
/// use std::fmt;
/// impl fmt::Show for Foo {
/// #[inline] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { self.fmt_to_css(f) }
/// }
/// ```
///
/// (This is a convenience wrapper for `to_css` and probably should not be overridden.)
#[inline]
fn fmt_to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
self.to_css(dest).map_err(|_| fmt::Error)
}
}

#[inline]
Expand Down Expand Up @@ -90,7 +75,7 @@ impl<'a> ToCss for Token<'a> {
serialize_unquoted_url(&**value, dest)?;
dest.write_str(")")?;
},
Token::Delim(value) => write!(dest, "{}", value)?,
Token::Delim(value) => dest.write_char(value)?,

Token::Number { value, int_value, has_sign } => {
write_numeric(value, int_value, has_sign, dest)?
Expand All @@ -112,7 +97,11 @@ impl<'a> ToCss for Token<'a> {
},

Token::WhiteSpace(content) => dest.write_str(content)?,
Token::Comment(content) => write!(dest, "/*{}*/", content)?,
Token::Comment(content) => {
dest.write_str("/*")?;
dest.write_str(content)?;
dest.write_str("*/")?
}
Token::Colon => dest.write_str(":")?,
Token::Semicolon => dest.write_str(";")?,
Token::Comma => dest.write_str(",")?,
Expand Down Expand Up @@ -143,6 +132,32 @@ impl<'a> ToCss for Token<'a> {
}
}

fn to_hex_byte(value: u8) -> u8 {
match value {
0...9 => value + b'0',
_ => value - 10 + b'a',
}
}

fn hex_escape<W>(ascii_byte: u8, dest: &mut W) -> fmt::Result where W:fmt::Write {
let high = ascii_byte >> 4;
let b3;
let b4;
let bytes = if high > 0 {
let low = ascii_byte & 0x0F;
b4 = [b'\\', to_hex_byte(high), to_hex_byte(low), b' '];
&b4[..]
} else {
b3 = [b'\\', to_hex_byte(ascii_byte), b' '];
&b3[..]
};
dest.write_str(unsafe { str::from_utf8_unchecked(&bytes) })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use write_char, I think, but not sure if worth it, this would be faster and it's trivially ascii so...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When writing more than one byte/char it might be better to minimize the number of calls, but I haven’t measured.

}

fn char_escape<W>(ascii_byte: u8, dest: &mut W) -> fmt::Result where W:fmt::Write {
let bytes = [b'\\', ascii_byte];
dest.write_str(unsafe { str::from_utf8_unchecked(&bytes) })
}

/// Write a CSS identifier, escaping characters as necessary.
pub fn serialize_identifier<W>(mut value: &str, dest: &mut W) -> fmt::Result where W:fmt::Write {
Expand All @@ -161,7 +176,7 @@ pub fn serialize_identifier<W>(mut value: &str, dest: &mut W) -> fmt::Result whe
value = &value[1..];
}
if let digit @ b'0'...b'9' = value.as_bytes()[0] {
write!(dest, "\\3{} ", digit as char)?;
hex_escape(digit, dest)?;
value = &value[1..];
}
serialize_name(value, dest)
Expand All @@ -182,9 +197,9 @@ fn serialize_name<W>(value: &str, dest: &mut W) -> fmt::Result where W:fmt::Writ
if let Some(escaped) = escaped {
dest.write_str(escaped)?;
} else if (b >= b'\x01' && b <= b'\x1F') || b == b'\x7F' {
write!(dest, "\\{:x} ", b)?;
hex_escape(b, dest)?;
} else {
write!(dest, "\\{}", b as char)?;
char_escape(b, dest)?;
}
chunk_start = i + 1;
}
Expand All @@ -202,9 +217,9 @@ fn serialize_unquoted_url<W>(value: &str, dest: &mut W) -> fmt::Result where W:f
};
dest.write_str(&value[chunk_start..i])?;
if hex {
write!(dest, "\\{:X} ", b)?;
hex_escape(b, dest)?;
} else {
write!(dest, "\\{}", b as char)?;
char_escape(b, dest)?;
}
chunk_start = i + 1;
}
Expand Down Expand Up @@ -262,7 +277,7 @@ impl<'a, W> fmt::Write for CssStringWriter<'a, W> where W: fmt::Write {
self.inner.write_str(&s[chunk_start..i])?;
match escaped {
Some(x) => self.inner.write_str(x)?,
None => write!(self.inner, "\\{:x} ", b)?,
None => hex_escape(b, self.inner)?,
};
chunk_start = i + 1;
}
Expand All @@ -275,7 +290,33 @@ macro_rules! impl_tocss_for_int {
($T: ty) => {
impl<'a> ToCss for $T {
fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
write!(dest, "{}", *self)
struct AssumeUtf8<W: fmt::Write>(W);

impl<W: fmt::Write> io::Write for AssumeUtf8<W> {
#[inline]
fn write_all(&mut self, buf: &[u8]) -> io::Result<()> {
// Safety: itoa only emits ASCII, which is also well-formed UTF-8.
debug_assert!(buf.is_ascii());
self.0.write_str(unsafe { str::from_utf8_unchecked(buf) })
.map_err(|_| io::ErrorKind::Other.into())
}

#[inline]
fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
self.write_all(buf)?;
Ok(buf.len())
}

#[inline]
fn flush(&mut self) -> io::Result<()> {
Ok(())
}
}

match itoa::write(AssumeUtf8(dest), *self) {
Ok(_) => Ok(()),
Err(_) => Err(fmt::Error)
}
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,10 +300,10 @@ fn unquoted_url_escaping() {
let serialized = token.to_css_string();
assert_eq!(serialized, "\
url(\
\\1 \\2 \\3 \\4 \\5 \\6 \\7 \\8 \\9 \\A \\B \\C \\D \\E \\F \\10 \
\\11 \\12 \\13 \\14 \\15 \\16 \\17 \\18 \\19 \\1A \\1B \\1C \\1D \\1E \\1F \\20 \
\\1 \\2 \\3 \\4 \\5 \\6 \\7 \\8 \\9 \\a \\b \\c \\d \\e \\f \\10 \
\\11 \\12 \\13 \\14 \\15 \\16 \\17 \\18 \\19 \\1a \\1b \\1c \\1d \\1e \\1f \\20 \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe run this through try to ensure this doesn't break anything? (seems unlikely)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll do a Gecko Try run before landing in Servo.

!\\\"#$%&\\'\\(\\)*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]\
^_`abcdefghijklmnopqrstuvwxyz{|}~\\7F é\
^_`abcdefghijklmnopqrstuvwxyz{|}~\\7f é\
)\
");
let mut input = ParserInput::new(&serialized);
Expand Down