Skip to content

Commit

Permalink
style: Use a more similar representation in Rust and C++ for grid lines.
Browse files Browse the repository at this point in the history
Option<> is not FFI-safe, so if we want to use the same representation
everywhere we need to get rid of it. This also makes it take the same amount of
memory as the C++ representation, and it's not very complex, I'd think.

Differential Revision: https://phabricator.services.mozilla.com/D35195
  • Loading branch information
emilio committed Jul 8, 2019
1 parent 3232210 commit 9809124
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 60 deletions.
37 changes: 9 additions & 28 deletions components/style/properties/gecko.mako.rs
Expand Up @@ -1138,19 +1138,14 @@ fn static_assert() {

let line = &mut self.gecko.${value.gecko};
line.mLineName.set_move(unsafe {
RefPtr::from_addrefed(match v.ident {
Some(i) => i.0,
None => atom!(""),
}.into_addrefed())
RefPtr::from_addrefed(v.ident.into_addrefed())
});
line.mHasSpan = v.is_span;
if let Some(integer) = v.line_num {
// clamping the integer between a range
line.mInteger = cmp::max(
nsStyleGridLine_kMinLine,
cmp::min(integer, nsStyleGridLine_kMaxLine),
);
}
// clamping the integer between a range
line.mInteger = cmp::max(
nsStyleGridLine_kMinLine,
cmp::min(v.line_num, nsStyleGridLine_kMaxLine),
);
}

pub fn copy_${value.name}_from(&mut self, other: &Self) {
Expand All @@ -1166,26 +1161,12 @@ fn static_assert() {
}

pub fn clone_${value.name}(&self) -> longhands::${value.name}::computed_value::T {
use crate::gecko_bindings::structs::{nsStyleGridLine_kMinLine, nsStyleGridLine_kMaxLine};

longhands::${value.name}::computed_value::T {
is_span: self.gecko.${value.gecko}.mHasSpan,
ident: {
let name = unsafe { Atom::from_raw(self.gecko.${value.gecko}.mLineName.mRawPtr) };
if name == atom!("") {
None
} else {
Some(CustomIdent(name))
}
ident: unsafe {
Atom::from_raw(self.gecko.${value.gecko}.mLineName.mRawPtr)
},
line_num:
if self.gecko.${value.gecko}.mInteger == 0 {
None
} else {
debug_assert!(nsStyleGridLine_kMinLine <= self.gecko.${value.gecko}.mInteger);
debug_assert!(self.gecko.${value.gecko}.mInteger <= nsStyleGridLine_kMaxLine);
Some(self.gecko.${value.gecko}.mInteger)
},
line_num: self.gecko.${value.gecko}.mInteger,
}
}
% endfor
Expand Down
1 change: 0 additions & 1 deletion components/style/properties/longhands/position.mako.rs
Expand Up @@ -322,7 +322,6 @@ ${helpers.predefined_type(
animation_value_type="discrete",
spec="https://drafts.csswg.org/css-grid/#propdef-grid-%s-%s" % (kind, range),
products="gecko",
boxed=True,
)}
% endfor

Expand Down
8 changes: 5 additions & 3 deletions components/style/properties/shorthands/position.mako.rs
Expand Up @@ -144,6 +144,7 @@
products="gecko">
use crate::values::specified::GridLine;
use crate::parser::Parse;
use crate::Zero;

// NOTE: Since both the shorthands have the same code, we should (re-)use code from one to implement
// the other. This might not be a big deal for now, but we should consider looking into this in the future
Expand All @@ -157,8 +158,8 @@
GridLine::parse(context, input)?
} else {
let mut line = GridLine::auto();
if start.line_num.is_none() && !start.is_span {
line.ident = start.ident.clone(); // ident from start value should be taken
if start.line_num.is_zero() && !start.is_span {
line.ident = start.ident.clone(); // ident from start value should be taken
}

line
Expand Down Expand Up @@ -186,6 +187,7 @@
products="gecko">
use crate::values::specified::GridLine;
use crate::parser::Parse;
use crate::Zero;

// The code is the same as `grid-{row,column}` except that this can have four values at most.
pub fn parse_value<'i, 't>(
Expand All @@ -194,7 +196,7 @@
) -> Result<Longhands, ParseError<'i>> {
fn line_with_ident_from(other: &GridLine) -> GridLine {
let mut this = GridLine::auto();
if other.line_num.is_none() && !other.is_span {
if other.line_num.is_zero() && !other.is_span {
this.ident = other.ident.clone();
}

Expand Down
5 changes: 1 addition & 4 deletions components/style/values/animated/grid.rs
Expand Up @@ -62,10 +62,7 @@ impl Animate for TrackSize {
}
}

impl Animate for generics::TrackRepeat<LengthPercentage, Integer>
where
generics::RepeatCount<Integer>: PartialEq,
{
impl Animate for generics::TrackRepeat<LengthPercentage, Integer> {
fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
// If the keyword, auto-fit/fill, is the same it can result in different
// number of tracks. For both auto-fit/fill, the number of columns isn't
Expand Down
51 changes: 29 additions & 22 deletions components/style/values/generics/grid.rs
Expand Up @@ -5,6 +5,7 @@
//! Generic types for the handling of
//! [grids](https://drafts.csswg.org/css-grid/).

use crate::{Atom, Zero};
use crate::parser::{Parse, ParserContext};
use crate::values::computed::{Context, ToComputedValue};
use crate::values::specified;
Expand All @@ -29,36 +30,42 @@ use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};
ToResolvedValue,
ToShmem,
)]
pub struct GridLine<Integer> {
#[repr(C)]
pub struct GenericGridLine<Integer> {
/// Flag to check whether it's a `span` keyword.
pub is_span: bool,
/// A custom identifier for named lines.
/// A custom identifier for named lines, or the empty atom otherwise.
///
/// <https://drafts.csswg.org/css-grid/#grid-placement-slot>
pub ident: Option<CustomIdent>,
pub ident: Atom,
/// Denotes the nth grid line from grid item's placement.
pub line_num: Option<Integer>,
pub line_num: Integer,
}

impl<Integer> GridLine<Integer> {
pub use self::GenericGridLine as GridLine;

impl<Integer> GridLine<Integer>
where
Integer: Zero,
{
/// The `auto` value.
pub fn auto() -> Self {
Self {
is_span: false,
line_num: None,
ident: None,
line_num: Zero::zero(),
ident: atom!(""),
}
}

/// Check whether this `<grid-line>` represents an `auto` value.
pub fn is_auto(&self) -> bool {
self.ident.is_none() && self.line_num.is_none() && !self.is_span
self.ident == atom!("") && self.line_num.is_zero() && !self.is_span
}
}

impl<Integer> ToCss for GridLine<Integer>
where
Integer: ToCss,
Integer: ToCss + Zero,
{
fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
where
Expand All @@ -72,18 +79,18 @@ where
dest.write_str("span")?;
}

if let Some(ref i) = self.line_num {
if !self.line_num.is_zero() {
if self.is_span {
dest.write_str(" ")?;
}
i.to_css(dest)?;
self.line_num.to_css(dest)?;
}

if let Some(ref s) = self.ident {
if self.is_span || self.line_num.is_some() {
if self.ident != atom!("") {
if self.is_span || !self.line_num.is_zero() {
dest.write_str(" ")?;
}
s.to_css(dest)?;
CustomIdent(self.ident.clone()).to_css(dest)?;
}

Ok(())
Expand Down Expand Up @@ -114,25 +121,25 @@ impl Parse for GridLine<specified::Integer> {
return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
}

if grid_line.line_num.is_some() || grid_line.ident.is_some() {
if !grid_line.line_num.is_zero() || grid_line.ident != atom!("") {
val_before_span = true;
}

grid_line.is_span = true;
} else if let Ok(i) = input.try(|i| specified::Integer::parse(context, i)) {
// FIXME(emilio): Probably shouldn't reject if it's calc()...
if i.value() == 0 || val_before_span || grid_line.line_num.is_some() {
if i.value() == 0 || val_before_span || grid_line.line_num.value() != 0 {
return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
}

grid_line.line_num = Some(i);
grid_line.line_num = i;
} else if let Ok(name) = input.try(|i| i.expect_ident_cloned()) {
if val_before_span || grid_line.ident.is_some() {
if val_before_span || grid_line.ident != atom!("") {
return Err(location.new_custom_error(StyleParseErrorKind::UnspecifiedError));
}
// NOTE(emilio): `span` is consumed above, so we only need to
// reject `auto`.
grid_line.ident = Some(CustomIdent::from_ident(location, &name, &["auto"])?);
grid_line.ident = CustomIdent::from_ident(location, &name, &["auto"])?.0;
} else {
break;
}
Expand All @@ -143,12 +150,12 @@ impl Parse for GridLine<specified::Integer> {
}

if grid_line.is_span {
if let Some(i) = grid_line.line_num {
if i.value() <= 0 {
if !grid_line.line_num.is_zero() {
if grid_line.line_num.value() <= 0 {
// disallow negative integers for grid spans
return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
}
} else if grid_line.ident.is_none() {
} else if grid_line.ident == atom!("") {
// integer could be omitted
return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
}
Expand Down
16 changes: 14 additions & 2 deletions components/style/values/specified/mod.rs
Expand Up @@ -17,9 +17,9 @@ use crate::context::QuirksMode;
use crate::parser::{Parse, ParserContext};
use crate::values::serialize_atom_identifier;
use crate::values::specified::calc::CalcNode;
use crate::{Atom, Namespace, Prefix};
use crate::{Atom, Namespace, Prefix, Zero};
use cssparser::{Parser, Token};
use num_traits::{One, Zero};
use num_traits::One;
use std::f32;
use std::fmt::{self, Write};
use std::ops::Add;
Expand Down Expand Up @@ -449,6 +449,18 @@ pub struct Integer {
was_calc: bool,
}

impl Zero for Integer {
#[inline]
fn zero() -> Self {
Self::new(0)
}

#[inline]
fn is_zero(&self) -> bool {
self.value() == 0
}
}

impl One for Integer {
#[inline]
fn one() -> Self {
Expand Down

0 comments on commit 9809124

Please sign in to comment.