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

Add glue for calc values

  • Loading branch information
Manishearth committed Jul 19, 2016
commit 67bcb96ceab4914e6ea9d795a0dbad7e69980856
@@ -0,0 +1,36 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* 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/. */

//! This module contains conversion helpers between Servo and Gecko types
//! Ideally, it would be in geckolib itself, but coherence
//! forces us to keep the traits and implementations here

This comment has been minimized.

@heycam

heycam Jul 18, 2016

Member

Question: all uses of these two From impls are going to be in the geckolib crate, so why can't they live in ports/geckolib/values.rs?

This comment has been minimized.

@emilio

emilio Jul 18, 2016

Member

I think it is because you can only implement them either where CalcLengthOrPercentage is defined or where nsStyleCoord_CalcValue is defined, and doing it in gecko_bindings is impossible because is in the other way of the dependency chain.

Other solution would be to implement a custom trait as GeckoStyleCoordConvertible is. I'm fine with any of both options, either using the standard From and paying this price, or using a custom type :)

This comment has been minimized.

@bholley

bholley Jul 18, 2016

Contributor

I think I would prefer a custom trait if it means that we can keep this logic in geckolib.


use app_units::Au;
use gecko_bindings::structs::nsStyleCoord_CalcValue;
use values::computed::CalcLengthOrPercentage;

impl From<CalcLengthOrPercentage> for nsStyleCoord_CalcValue {
fn from(other: CalcLengthOrPercentage) -> nsStyleCoord_CalcValue {
let has_percentage = other.percentage.is_some();
nsStyleCoord_CalcValue {
mLength: other.length.map(|l| l.0).unwrap_or(0),
mPercent: other.percentage.unwrap_or(0.0),
mHasPercent: has_percentage,
}
}
}

impl From<nsStyleCoord_CalcValue> for CalcLengthOrPercentage {
fn from(other: nsStyleCoord_CalcValue) -> CalcLengthOrPercentage {
let percentage = if other.mHasPercent {
Some(other.mPercent)
} else {
None
};
CalcLengthOrPercentage {
length: Some(Au(other.mLength)),
percentage: percentage,
}
}
}
@@ -80,6 +80,8 @@ pub mod data;
pub mod dom;
pub mod element_state;
pub mod error_reporting;
#[cfg(feature = "gecko")]
pub mod gecko_conversions;
pub mod font_face;
pub mod keyframes;
pub mod logical_geometry;
@@ -3,4 +3,5 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

mod ns_style_auto_array;
mod ns_style_coord;
mod ns_t_array;
@@ -0,0 +1,34 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* 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};

// 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
// nsStyleCoord is one way to do it, but there are other structs using pairs
// of union and unit too

This comment has been minimized.

@bholley

bholley Jul 18, 2016

Contributor

Yeah, the main reason for the separation is that in Gecko, we sometimes have tuples of these things, and the unions and units are stored in separate arrays. Maybe we could have some sort of temporary struct that we synthesize (unsafely) and pass around (safely)? Would that impose a performance cost?

This comment has been minimized.

@Manishearth

Manishearth Jul 19, 2016

Author Member

Yes, this is the plan. Shouldn't have a perf cost (because we pass them in as arguments anyway); and I bet llvm will optimize it away even if it did.

A slight issue with this abstraction is that it's still technically unsafe because folks can always modify the values in nsStyleCoord directly. I could try to make those private via bindgen stuff, though.

This comment has been minimized.

@bholley

bholley Jul 19, 2016

Contributor

I was imagining that we'd create a some temporary tuple struct that isn't actually nsStyleCoord, but if we can make that nsStyleCoord itself with similar properties, that's even better.


impl nsStyleUnion {
/// Clean up any resources used by an nsStyleUnit
/// Currently, this only happens if the nsStyleUnit
/// is a Calc
pub unsafe fn reset(&mut self, unit: &mut nsStyleUnit) {
if *unit == nsStyleUnit::eStyleUnit_Calc {
Gecko_ResetStyleCoord(unit, self);
}
}

/// Set internal value to a calc() value
/// reset() the union before calling this
pub unsafe fn set_calc_value(&mut self, unit: &mut nsStyleUnit, v: nsStyleCoord_CalcValue) {
// Calc should have been cleaned up
assert!(*unit != nsStyleUnit::eStyleUnit_Calc);

This comment has been minimized.

@bholley

bholley Jul 18, 2016

Contributor

We should use debug_assert everywhere.

Gecko_SetStyleCoordCalcValue(unit, self, v);
}

pub unsafe fn get_calc(&self) -> nsStyleCoord_CalcValue {
(*(*self.mPointer.as_ref() as *const nsStyleCoord_Calc))._base
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.