Skip to content

Commit

Permalink
style: Refactor grid types to preserve repeat() at computed value tim…
Browse files Browse the repository at this point in the history 
…e and use cbindgen.

I'm _really_ sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.

This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:

 * Make us preserve repeat() at computed-value time. This is per spec since
   interpolation needs to know about repeat(). Except for subgrid, which did the
   repeat expansion at parse-time and was a bit more annoying (plus it doesn't
   really animate yet so we don't need it to comply with the spec).

 * Tweaks the WPT tests for interpolation to adopt the resolution at:
   w3c/csswg-drafts#3503.

Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.

Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.

This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.

In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.

This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.

Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.

Differential Revision: https://phabricator.services.mozilla.com/D36598
  • Loading branch information
@emilio
emilio committed Aug 15, 2019
1 parent 0e8b185 commit 3e39998
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 505 deletions.
213 changes: 1 addition & 212 deletions components/style/properties/gecko.mako.rs
View file
Expand Up @@ -976,8 +976,7 @@ fn static_assert() {
skip_longhands="${skip_position_longhands} order
align-content justify-content align-self
justify-self align-items justify-items
grid-auto-flow grid-template-rows
grid-template-columns">
grid-auto-flow">
% for side in SIDES:
<% impl_split_style_coord(side.ident, "mOffset", side.index) %>
% endfor
Expand Down Expand Up @@ -1025,216 +1024,6 @@ fn static_assert() {

${impl_simple_copy('order', 'mOrder')}

% for kind in ["rows", "columns"]:
pub fn set_grid_template_${kind}(&mut self, v: longhands::grid_template_${kind}::computed_value::T) {
<% self_grid = "self.gecko.mGridTemplate%s" % kind.title() %>
use crate::gecko_bindings::structs::nsTArray;
use std::usize;
use crate::values::CustomIdent;
use crate::values::generics::grid::TrackListType::Auto;
use crate::values::generics::grid::{GridTemplateComponent, RepeatCount, TrackListValue, MAX_GRID_LINE};

#[inline]
fn set_line_names(servo_names: &[CustomIdent], gecko_names: &mut nsTArray<structs::RefPtr<structs::nsAtom>>) {
unsafe {
bindings::Gecko_ResizeAtomArray(gecko_names, servo_names.len() as u32);
}

for (servo_name, gecko_name) in servo_names.iter().zip(gecko_names.iter_mut()) {
gecko_name.set_move(unsafe {
RefPtr::from_addrefed(servo_name.0.clone().into_addrefed())
});
}
}

let max_lines = MAX_GRID_LINE as usize - 1; // for accounting the final <line-names>

let result = match v {
GridTemplateComponent::None => ptr::null_mut(),
GridTemplateComponent::TrackList(track) => {
let mut num_values = track.values.len();
if let Auto(_) = track.list_type {
num_values += 1;
}

num_values = cmp::min(num_values, max_lines);
let value = unsafe {
bindings::Gecko_CreateStyleGridTemplate(num_values as u32,
(num_values + 1) as u32).as_mut().unwrap()
};

let mut auto_idx = usize::MAX;
let mut auto_track_size = None;
if let Auto(idx) = track.list_type {
auto_idx = idx as usize;
let auto_repeat = track.auto_repeat.as_ref().expect("expected <auto-track-repeat> value");

if auto_repeat.count == RepeatCount::AutoFill {
value.set_mIsAutoFill(true);
}

value.mRepeatAutoIndex = idx as i16;
// NOTE: Gecko supports only one set of values in <auto-repeat>
// i.e., it can only take repeat(auto-fill, [a] 10px [b]), and no more.
set_line_names(&auto_repeat.line_names[0], &mut value.mRepeatAutoLineNameListBefore);
set_line_names(&auto_repeat.line_names[1], &mut value.mRepeatAutoLineNameListAfter);
auto_track_size = Some(auto_repeat.track_sizes.get(0).unwrap().clone());
} else {
unsafe {
bindings::Gecko_ResizeAtomArray(
&mut value.mRepeatAutoLineNameListBefore, 0);
bindings::Gecko_ResizeAtomArray(
&mut value.mRepeatAutoLineNameListAfter, 0);
}
}

let mut line_names = track.line_names.into_iter();
let mut values_iter = track.values.into_iter();
{
for (i, track_size) in value.mTrackSizingFunctions.iter_mut().enumerate().take(max_lines) {
let name_list = line_names.next().expect("expected line-names");
set_line_names(&name_list, &mut value.mLineNameLists[i]);
*track_size = if i == auto_idx {
auto_track_size.take().expect("expected <track-size> for <auto-track-repeat>")
} else {
match values_iter.next().expect("expected <track-size> value") {
TrackListValue::TrackSize(size) => size,
// FIXME(emilio): This shouldn't be
// representable in the first place.
TrackListValue::TrackRepeat(..) => {
unreachable!("Shouldn't have track-repeats in computed track lists")
}
}
};
}
}

let final_names = line_names.next().unwrap();
set_line_names(&final_names, value.mLineNameLists.last_mut().unwrap());

value
},
GridTemplateComponent::Subgrid(list) => {
let names_length = match list.fill_idx {
Some(_) => list.names.len() - 1,
None => list.names.len(),
};
let num_values = cmp::min(names_length, max_lines + 1);
let value = unsafe {
bindings::Gecko_CreateStyleGridTemplate(0, num_values as u32).as_mut().unwrap()
};
value.set_mIsSubgrid(true);

let mut names = list.names.into_vec();
if let Some(idx) = list.fill_idx {
value.set_mIsAutoFill(true);
value.mRepeatAutoIndex = idx as i16;
set_line_names(&names.swap_remove(idx as usize),
&mut value.mRepeatAutoLineNameListBefore);
}

for (servo_names, gecko_names) in names.iter().zip(value.mLineNameLists.iter_mut()) {
set_line_names(servo_names, gecko_names);
}

value
},
};

unsafe { bindings::Gecko_SetStyleGridTemplate(&mut ${self_grid}, result); }
}

pub fn copy_grid_template_${kind}_from(&mut self, other: &Self) {
unsafe {
bindings::Gecko_CopyStyleGridTemplateValues(&mut ${self_grid},
other.gecko.mGridTemplate${kind.title()}.mPtr);
}
}

pub fn reset_grid_template_${kind}(&mut self, other: &Self) {
self.copy_grid_template_${kind}_from(other)
}

pub fn clone_grid_template_${kind}(&self) -> longhands::grid_template_${kind}::computed_value::T {
<% self_grid = "self.gecko.mGridTemplate%s" % kind.title() %>
use crate::gecko_bindings::structs::nsTArray;
use crate::values::CustomIdent;
use crate::values::generics::grid::{GridTemplateComponent, LineNameList, RepeatCount};
use crate::values::generics::grid::{TrackList, TrackListType, TrackListValue, TrackRepeat};

let value = match unsafe { ${self_grid}.mPtr.as_ref() } {
None => return GridTemplateComponent::None,
Some(value) => value,
};

#[inline]
fn to_boxed_customident_slice(gecko_names: &nsTArray<structs::RefPtr<structs::nsAtom>>) -> Box<[CustomIdent]> {
let idents: Vec<CustomIdent> = gecko_names.iter().map(|gecko_name| {
CustomIdent(unsafe { Atom::from_raw(gecko_name.mRawPtr) })
}).collect();
idents.into_boxed_slice()
}

#[inline]
fn to_line_names_vec(
gecko_line_names: &nsTArray<nsTArray<structs::RefPtr<structs::nsAtom>>>,
) -> Vec<Box<[CustomIdent]>> {
gecko_line_names.iter().map(|gecko_names| {
to_boxed_customident_slice(gecko_names)
}).collect()
}

let repeat_auto_index = value.mRepeatAutoIndex as usize;
if value.mIsSubgrid() {
let mut names_vec = to_line_names_vec(&value.mLineNameLists);
let fill_idx = if value.mIsAutoFill() {
names_vec.insert(
repeat_auto_index,
to_boxed_customident_slice(&value.mRepeatAutoLineNameListBefore));
Some(repeat_auto_index as u32)
} else {
None
};
let names = names_vec.into_boxed_slice();

GridTemplateComponent::Subgrid(LineNameList{names, fill_idx})
} else {
let mut auto_repeat = None;
let mut list_type = TrackListType::Normal;
let line_names = to_line_names_vec(&value.mLineNameLists).into_boxed_slice();
let mut values = Vec::with_capacity(value.mTrackSizingFunctions.len());
for (i, track_size) in value.mTrackSizingFunctions.iter().enumerate() {
if i == repeat_auto_index {
list_type = TrackListType::Auto(repeat_auto_index as u16);

let count = if value.mIsAutoFill() {
RepeatCount::AutoFill
} else {
RepeatCount::AutoFit
};

let line_names = {
let mut vec: Vec<Box<[CustomIdent]>> = Vec::with_capacity(2);
vec.push(to_boxed_customident_slice(
&value.mRepeatAutoLineNameListBefore));
vec.push(to_boxed_customident_slice(
&value.mRepeatAutoLineNameListAfter));
vec.into_boxed_slice()
};

let track_sizes = vec!(track_size.clone());

auto_repeat = Some(TrackRepeat{count, line_names, track_sizes});
} else {
values.push(TrackListValue::TrackSize(track_size.clone()));
}
}

GridTemplateComponent::TrackList(TrackList{list_type, values, line_names, auto_repeat})
}
}
% endfor

${impl_simple_type_with_conversion("grid_auto_flow")}
</%self:impl_trait>

Expand Down
50 changes: 22 additions & 28 deletions components/style/properties/shorthands/position.mako.rs
View file
Expand Up @@ -268,7 +268,7 @@
>
use crate::parser::Parse;
use servo_arc::Arc;
use crate::values::generics::grid::{TrackSize, TrackList, TrackListType};
use crate::values::generics::grid::{TrackSize, TrackList};
use crate::values::generics::grid::{TrackListValue, concat_serialize_idents};
use crate::values::specified::{GridTemplateComponent, GenericGridTemplateComponent};
use crate::values::specified::grid::parse_line_names;
Expand Down Expand Up @@ -300,49 +300,50 @@
}
% endfor

let first_line_names = input.try(parse_line_names).unwrap_or(vec![].into_boxed_slice());
let first_line_names = input.try(parse_line_names).unwrap_or_default();
if let Ok(mut string) = input.try(|i| i.expect_string().map(|s| s.as_ref().to_owned().into())) {
let mut strings = vec![];
let mut values = vec![];
let mut line_names = vec![];
let mut names = first_line_names.into_vec();
let mut names = first_line_names;
loop {
line_names.push(names.into_boxed_slice());
line_names.push(names);
strings.push(string);
let size = input.try(|i| TrackSize::parse(context, i)).unwrap_or_default();
values.push(TrackListValue::TrackSize(size));
names = input.try(parse_line_names).unwrap_or(vec![].into_boxed_slice()).into_vec();
names = input.try(parse_line_names).unwrap_or_default();
if let Ok(v) = input.try(parse_line_names) {
names.extend(v.into_vec());
let mut names_vec = names.into_vec();
names_vec.extend(v.into_iter());
names = names_vec.into();
}

string = match input.try(|i| i.expect_string().map(|s| s.as_ref().to_owned().into())) {
Ok(s) => s,
_ => { // only the named area determines whether we should bail out
line_names.push(names.into_boxed_slice());
line_names.push(names.into());
break
},
};
}

if line_names.len() == values.len() {
// should be one longer than track sizes
line_names.push(vec![].into_boxed_slice());
line_names.push(Default::default());
}

let template_areas = TemplateAreas::from_vec(strings)
.map_err(|()| input.new_custom_error(StyleParseErrorKind::UnspecifiedError))?;
let template_rows = TrackList {
list_type: TrackListType::Normal,
values,
line_names: line_names.into_boxed_slice(),
auto_repeat: None,
values: values.into(),
line_names: line_names.into(),
auto_repeat_index: std::usize::MAX,
};

let template_cols = if input.try(|i| i.expect_delim('/')).is_ok() {
let value = GridTemplateComponent::parse_without_none(context, input)?;
if let GenericGridTemplateComponent::TrackList(ref list) = value {
if list.list_type != TrackListType::Explicit {
if !list.is_explicit() {
return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
}
}
Expand Down Expand Up @@ -407,13 +408,10 @@

let track_list = match *template_rows {
GenericGridTemplateComponent::TrackList(ref list) => {
// We should fail if there is a `repeat` function. `grid` and
// `grid-template` shorthands doesn't accept that. Only longhand accepts.
if list.auto_repeat.is_some() ||
list.values.iter().any(|v| match *v {
TrackListValue::TrackRepeat(_) => true,
_ => false,
}) {
// We should fail if there is a `repeat` function.
// `grid` and `grid-template` shorthands doesn't accept
// that. Only longhand accepts.
if !list.is_explicit() {
return Ok(());
}
list
Expand All @@ -429,11 +427,7 @@
// We should fail if there is a `repeat` function. `grid` and
// `grid-template` shorthands doesn't accept that. Only longhand accepts that.
GenericGridTemplateComponent::TrackList(ref list) => {
if list.auto_repeat.is_some() ||
list.values.iter().any(|v| match *v {
TrackListValue::TrackRepeat(_) => true,
_ => false,
}) {
if !list.is_explicit() {
return Ok(());
}
},
Expand Down Expand Up @@ -498,7 +492,7 @@
>
use crate::parser::Parse;
use crate::properties::longhands::{grid_auto_columns, grid_auto_rows, grid_auto_flow};
use crate::values::generics::grid::{GridTemplateComponent, TrackListType};
use crate::values::generics::grid::GridTemplateComponent;
use crate::values::specified::{GenericGridTemplateComponent, TrackSize};
use crate::values::specified::position::{AutoFlow, GridAutoFlow, GridTemplateAreas};

Expand Down Expand Up @@ -597,7 +591,7 @@

// It should fail to serialize if template-rows value is not Explicit.
if let GenericGridTemplateComponent::TrackList(ref list) = *self.grid_template_rows {
if list.list_type != TrackListType::Explicit {
if !list.is_explicit() {
return Ok(());
}
}
Expand All @@ -621,7 +615,7 @@

// It should fail to serialize if template-column value is not Explicit.
if let GenericGridTemplateComponent::TrackList(ref list) = *self.grid_template_columns {
if list.list_type != TrackListType::Explicit {
if !list.is_explicit() {
return Ok(());
}
}
Expand Down

0 comments on commit 3e39998

Please sign in to comment.