Skip to content
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

Add bindings for calc() #12465

Merged
merged 6 commits into from Jul 20, 2016
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Handle Calc refcounting

  • Loading branch information
Manishearth committed Jul 19, 2016
commit 704d7a01c957574ed7acdc793ee29cbb75b8bc23
@@ -33,4 +33,4 @@ impl From<nsStyleCoord_CalcValue> for CalcLengthOrPercentage {
percentage: percentage,
}
}
}
}
@@ -80,9 +80,9 @@ pub mod data;
pub mod dom;
pub mod element_state;
pub mod error_reporting;
pub mod font_face;
#[cfg(feature = "gecko")]
pub mod gecko_conversions;
pub mod font_face;
pub mod keyframes;
pub mod logical_geometry;
pub mod matching;
@@ -132,6 +132,7 @@ unsafe impl Send for nsStyleUnion {}
unsafe impl Sync for nsStyleUnion {}
impl HeapSizeOf for nsStyleUnion { fn heap_size_of_children(&self) -> usize { 0 } }
use structs::nsStyleCoord_CalcValue as CalcValue;
use structs::nsStyleCoord_Calc as Calc;
use structs::SheetParsingMode;
use structs::nsMainThreadPtrHandle;
use structs::nsMainThreadPtrHolder;
@@ -269,6 +270,8 @@ extern "C" {
pub fn Gecko_SetStyleCoordCalcValue(unit: *mut nsStyleUnit,
value: *mut nsStyleUnion,
calc: CalcValue);
pub fn Gecko_AddRefCalcArbitraryThread(aPtr: *mut Calc);
pub fn Gecko_ReleaseCalcArbitraryThread(aPtr: *mut Calc);
pub fn Servo_StylesheetFromUTF8Bytes(bytes: *const u8, length: u32,
parsing_mode: SheetParsingMode,
base: *mut ThreadSafeURIHolder,
@@ -4885,12 +4885,11 @@ fn bindgen_test_layout_nsStyleCoord_CalcValue() {
#[derive(Debug)]
pub struct nsStyleCoord_Calc {
pub _base: nsStyleCoord_CalcValue,
pub mRefCnt: nsAutoRefCnt,
pub _mOwningThread: nsAutoOwningThread,
pub mRefCnt: ThreadSafeAutoRefCnt,
}
#[test]
fn bindgen_test_layout_nsStyleCoord_Calc() {
assert_eq!(::std::mem::size_of::<nsStyleCoord_Calc>() , 32usize);
assert_eq!(::std::mem::size_of::<nsStyleCoord_Calc>() , 24usize);
assert_eq!(::std::mem::align_of::<nsStyleCoord_Calc>() , 8usize);
}
#[repr(u32)]
@@ -4864,12 +4864,11 @@ fn bindgen_test_layout_nsStyleCoord_CalcValue() {
#[derive(Debug)]
pub struct nsStyleCoord_Calc {
pub _base: nsStyleCoord_CalcValue,
pub mRefCnt: nsAutoRefCnt,
pub _mOwningThread: nsAutoOwningThread,
pub mRefCnt: ThreadSafeAutoRefCnt,
}
#[test]
fn bindgen_test_layout_nsStyleCoord_Calc() {
assert_eq!(::std::mem::size_of::<nsStyleCoord_Calc>() , 32usize);
assert_eq!(::std::mem::size_of::<nsStyleCoord_Calc>() , 24usize);
assert_eq!(::std::mem::align_of::<nsStyleCoord_Calc>() , 8usize);
}
#[repr(u32)]
@@ -2,8 +2,9 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use structs::{nsStyleCoord_CalcValue, nsStyleCoord_Calc, nsStyleUnit, nsStyleUnion};
use bindings::{Gecko_ResetStyleCoord, Gecko_SetStyleCoordCalcValue};
use bindings::{Gecko_ResetStyleCoord, Gecko_SetStyleCoordCalcValue, Gecko_AddRefCalcArbitraryThread};
use std::mem::transmute;
use structs::{nsStyleCoord_CalcValue, nsStyleCoord_Calc, nsStyleUnit, nsStyleUnion, nsStyleCoord};

// Functions here are unsafe because it is possible to use the wrong nsStyleUnit

This comment has been minimized.

@heycam

heycam Jul 18, 2016

Member

I'd like to understand the difference between unsafe on the function interface vs wrapping the body of the function in unsafe here. Is it because we have a requirement that is not expressible in the type system, which must be adhered to otherwise we will have a memory safety issue, namely that you must pass in a reference to the nsStyleUnit associated with the nsStyleUnion and not just any one you have access to? And to signal to users of nsStyleUnion that we have other requirements the compiler won't catch, we use unsafe here?

This comment has been minimized.

@Manishearth

Manishearth Jul 18, 2016

Author Member

Basically yes. I will refactor this in a later PR to fix it.

// FIXME we should be pairing up nsStyleUnion and nsStyleUnit somehow
@@ -29,6 +30,25 @@ impl nsStyleUnion {
}

pub unsafe fn get_calc(&self) -> nsStyleCoord_CalcValue {
(*(*self.mPointer.as_ref() as *const nsStyleCoord_Calc))._base
(*self.as_calc())._base
}

pub unsafe fn addref_opt(&mut self, unit: &nsStyleUnit) {

This comment has been minimized.

@bholley

bholley Jul 18, 2016

Contributor

What does the _opt mean here? Optional? Seems kinda ambiguous.

But also, maybe this is the wrong level of abstraction? Shouldn't we just have a set() for nsStyleUnion that does the refcount munging as appropriate, and possibly a set_no_calc that asserts against calc in the cases we know it can't happen?

This comment has been minimized.

@bholley

bholley Jul 18, 2016

Contributor

Though I guess the issue here is that this needs the tuple abstraction layer we talked about above.

This comment has been minimized.

@Manishearth

Manishearth Jul 19, 2016

Author Member

Yeah, this is all temporary. I will be doing that tuple layer in a different PR, and it will have set_ methods and stuff. I'm trying to keep my PRs which span repos to be minimal in size.

if *unit == nsStyleUnit::eStyleUnit_Calc {
Gecko_AddRefCalcArbitraryThread(self.as_calc_mut());
}
}

pub unsafe fn as_calc_mut(&mut self) -> &mut nsStyleCoord_Calc {

This comment has been minimized.

@heycam

heycam Jul 19, 2016

Member

Do as_calc_mut and as_calc need to be pub? Looks like they're not called from elsewhere.

transmute(*self.mPointer.as_mut() as *mut nsStyleCoord_Calc)
}
pub unsafe fn as_calc(&self) -> &nsStyleCoord_Calc {
transmute(*self.mPointer.as_ref() as *const nsStyleCoord_Calc)
}
}

impl nsStyleCoord {
pub unsafe fn addref_opt(&mut self) {
self.mValue.addref_opt(&self.mUnit);
}
}
@@ -121,6 +121,7 @@
"nsStyleCoord", "nsStyleGradientStop", "nsStyleImageLayers",
"nsStyleImageLayers::Layer", "nsStyleImageLayers::LayerType",
"nsStyleUnit", "nsStyleUnion", "nsStyleCoord::CalcValue",
"nsStyleCoord::Calc",

"SheetParsingMode", "nsMainThreadPtrHandle",
"nsMainThreadPtrHolder", "nscolor", "nsFont", "FontFamilyList",
@@ -35,7 +35,7 @@ use style::properties::{CascadePropertyFn, ServoComputedValues, ComputedValues};
use style::properties::longhands;
use style::properties::style_struct_traits::*;
use values::{StyleCoordHelpers, GeckoStyleCoordConvertible, convert_nscolor_to_rgba};
use values::{convert_rgba_to_nscolor, debug_assert_unit_is_safe_to_copy};
use values::convert_rgba_to_nscolor;
use values::round_border_to_device_pixels;

#[derive(Clone, Debug)]
@@ -317,9 +317,10 @@ def set_gecko_property(ffi_name, expr):
&mut self.gecko.${union_ffi_name});
}
fn copy_${ident}_from(&mut self, other: &Self) {
debug_assert_unit_is_safe_to_copy(self.gecko.${unit_ffi_name});
unsafe { self.gecko.${union_ffi_name}.reset(&mut self.gecko.${unit_ffi_name}) };
self.gecko.${unit_ffi_name} = other.gecko.${unit_ffi_name};
self.gecko.${union_ffi_name} = other.gecko.${union_ffi_name};
unsafe { self.gecko.${union_ffi_name}.addref_opt(&self.gecko.${unit_ffi_name}) };

This comment has been minimized.

@heycam

heycam Jul 19, 2016

Member

Rather than assigning directly to the unit and union, and then calling addref_opt (or addref_if_calc if renamed), we could just have a single call to a set function on nsStyleUnion that takes a reference to the unit as an argument, and does the unit check and addref if necessary. Currently it's kind of splitting the responsibility of dealing with different units correctly across nsStyleUnion functions and code here poking directly into the unit/union.

}
% if need_clone:
fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
@@ -347,12 +348,14 @@ ${impl_split_style_coord(ident,
&mut self.gecko.${y_union_ffi_name});
}
fn copy_${ident}_from(&mut self, other: &Self) {
debug_assert_unit_is_safe_to_copy(self.gecko.${x_unit_ffi_name});
debug_assert_unit_is_safe_to_copy(self.gecko.${y_unit_ffi_name});
unsafe { self.gecko.${x_union_ffi_name}.reset(&mut self.gecko.${x_unit_ffi_name}) };
unsafe { self.gecko.${y_union_ffi_name}.reset(&mut self.gecko.${y_unit_ffi_name}) };
self.gecko.${x_unit_ffi_name} = other.gecko.${x_unit_ffi_name};
self.gecko.${x_union_ffi_name} = other.gecko.${x_union_ffi_name};
self.gecko.${y_unit_ffi_name} = other.gecko.${y_unit_ffi_name};
self.gecko.${y_union_ffi_name} = other.gecko.${y_union_ffi_name};
unsafe { self.gecko.${x_union_ffi_name}.addref_opt(&self.gecko.${x_unit_ffi_name}) };
unsafe { self.gecko.${y_union_ffi_name}.addref_opt(&self.gecko.${y_unit_ffi_name}) };

This comment has been minimized.

@heycam

heycam Jul 19, 2016

Member

Similarly here I think it would encapsulate the behaviour better to have a copy_from function on nsStyleUnion that deals with addrefing if needed.

}
% if need_clone:
fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
@@ -628,9 +631,10 @@ fn static_assert() {
}

fn copy_z_index_from(&mut self, other: &Self) {
debug_assert_unit_is_safe_to_copy(self.gecko.mZIndex.mUnit);
unsafe { self.gecko.mZIndex.mValue.reset(&mut self.gecko.mZIndex.mUnit) };
self.gecko.mZIndex.mUnit = other.gecko.mZIndex.mUnit;
self.gecko.mZIndex.mValue = other.gecko.mZIndex.mValue;
unsafe { self.gecko.mZIndex.mValue.addref_opt(&self.gecko.mZIndex.mUnit) };

This comment has been minimized.

@heycam

heycam Jul 19, 2016

Member

z-index doesn't take calc() values, so instead you could assert that the new unit value isn't calc.

}

fn clone_z_index(&self) -> longhands::z_index::computed_value::T {
@@ -43,10 +43,10 @@ pub trait StyleCoordHelpers {
impl StyleCoordHelpers for nsStyleCoord {
#[inline]
fn copy_from(&mut self, other: &Self) {
debug_assert_unit_is_safe_to_copy(self.mUnit);
debug_assert_unit_is_safe_to_copy(other.mUnit);
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = other.mUnit;
self.mValue = other.mValue;
unsafe { self.addref_opt(); }
}

#[inline]
@@ -56,7 +56,7 @@ impl StyleCoordHelpers for nsStyleCoord {

#[inline]
fn set_auto(&mut self) {
unsafe { self.mValue.reset(&mut self.mUnit)};
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = nsStyleUnit::eStyleUnit_Auto;
unsafe { *self.mValue.mInt.as_mut() = 0; }
}
@@ -67,7 +67,7 @@ impl StyleCoordHelpers for nsStyleCoord {

#[inline]
fn set_normal(&mut self) {
unsafe { self.mValue.reset(&mut self.mUnit)};
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = nsStyleUnit::eStyleUnit_Normal;
unsafe { *self.mValue.mInt.as_mut() = 0; }
}
@@ -78,7 +78,7 @@ impl StyleCoordHelpers for nsStyleCoord {

#[inline]
fn set_coord(&mut self, val: Au) {
unsafe { self.mValue.reset(&mut self.mUnit)};
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = nsStyleUnit::eStyleUnit_Coord;
unsafe { *self.mValue.mInt.as_mut() = val.0; }
}
@@ -94,7 +94,7 @@ impl StyleCoordHelpers for nsStyleCoord {

#[inline]
fn set_int(&mut self, val: i32) {
unsafe { self.mValue.reset(&mut self.mUnit)};
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = nsStyleUnit::eStyleUnit_Integer;
unsafe { *self.mValue.mInt.as_mut() = val; }
}
@@ -110,7 +110,7 @@ impl StyleCoordHelpers for nsStyleCoord {

#[inline]
fn set_enum(&mut self, val: i32) {
unsafe { self.mValue.reset(&mut self.mUnit)};
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = nsStyleUnit::eStyleUnit_Enumerated;
unsafe { *self.mValue.mInt.as_mut() = val; }
}
@@ -126,7 +126,7 @@ impl StyleCoordHelpers for nsStyleCoord {

#[inline]
fn set_percent(&mut self, val: f32) {
unsafe { self.mValue.reset(&mut self.mUnit)};
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = nsStyleUnit::eStyleUnit_Percent;
unsafe { *self.mValue.mFloat.as_mut() = val; }
}
@@ -142,7 +142,7 @@ impl StyleCoordHelpers for nsStyleCoord {

#[inline]
fn set_factor(&mut self, val: f32) {
unsafe { self.mValue.reset(&mut self.mUnit)};
unsafe { self.mValue.reset(&mut self.mUnit) };
self.mUnit = nsStyleUnit::eStyleUnit_Factor;
unsafe { *self.mValue.mFloat.as_mut() = val; }
}
@@ -174,7 +174,7 @@ impl GeckoStyleCoordConvertible for LengthOrPercentage {
*unit = nsStyleUnit::eStyleUnit_Percent;
unsafe { *union.mFloat.as_mut() = p; }
},
LengthOrPercentage::Calc(calc) => unsafe {union.set_calc_value(unit, calc.into()) },
LengthOrPercentage::Calc(calc) => unsafe { union.set_calc_value(unit, calc.into()) },
};
}

@@ -185,7 +185,7 @@ impl GeckoStyleCoordConvertible for LengthOrPercentage {
nsStyleUnit::eStyleUnit_Percent
=> Some(LengthOrPercentage::Percentage(unsafe { *union.mFloat.as_ref() })),
nsStyleUnit::eStyleUnit_Calc
=> Some(LengthOrPercentage::Calc(unsafe { union.get_calc().into()})),
=> Some(LengthOrPercentage::Calc(unsafe { union.get_calc().into() })),
_ => None,
}
}
@@ -207,7 +207,7 @@ impl GeckoStyleCoordConvertible for LengthOrPercentageOrAuto {
*unit = nsStyleUnit::eStyleUnit_Auto;
unsafe { *union.mInt.as_mut() = 0; }
},
LengthOrPercentageOrAuto::Calc(calc) => unsafe {union.set_calc_value(unit, calc.into()) },
LengthOrPercentageOrAuto::Calc(calc) => unsafe { union.set_calc_value(unit, calc.into()) },
};
}

@@ -220,7 +220,7 @@ impl GeckoStyleCoordConvertible for LengthOrPercentageOrAuto {
nsStyleUnit::eStyleUnit_Percent
=> Some(LengthOrPercentageOrAuto::Percentage(unsafe { *union.mFloat.as_ref() })),
nsStyleUnit::eStyleUnit_Calc
=> Some(LengthOrPercentageOrAuto::Calc(unsafe { union.get_calc().into()})),
=> Some(LengthOrPercentageOrAuto::Calc(unsafe { union.get_calc().into() })),
_ => None,
}
}
@@ -255,14 +255,15 @@ impl GeckoStyleCoordConvertible for LengthOrPercentageOrNone {
nsStyleUnit::eStyleUnit_Percent
=> Some(LengthOrPercentageOrNone::Percentage(unsafe { *union.mFloat.as_ref() })),
nsStyleUnit::eStyleUnit_Calc
=> Some(LengthOrPercentageOrNone::Calc(unsafe { union.get_calc().into()})),
=> Some(LengthOrPercentageOrNone::Calc(unsafe { union.get_calc().into() })),
_ => None,
}
}
}

impl<T: GeckoStyleCoordConvertible> GeckoStyleCoordConvertible for Option<T> {
fn to_gecko_style_coord(&self, unit: &mut nsStyleUnit, union: &mut nsStyleUnion) {
unsafe { union.reset(unit) };
if let Some(ref me) = *self {
me.to_gecko_style_coord(unit, union);
} else {
@@ -280,6 +281,7 @@ impl GeckoStyleCoordConvertible for Angle {
fn to_gecko_style_coord(&self,
unit: &mut nsStyleUnit,
union: &mut nsStyleUnion) {
unsafe { union.reset(unit) };
*unit = nsStyleUnit::eStyleUnit_Radian;
unsafe { *union.mFloat.as_mut() = self.radians() };
}
@@ -320,7 +322,3 @@ pub fn round_border_to_device_pixels(width: Au, au_per_device_px: Au) -> Au {
max(au_per_device_px, Au(width.0 / au_per_device_px.0 * au_per_device_px.0))
}
}

pub fn debug_assert_unit_is_safe_to_copy(unit: nsStyleUnit) {
debug_assert!(unit != nsStyleUnit::eStyleUnit_Calc, "stylo: Can't yet handle refcounted Calc");
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.