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

stylo: Use atoms as the pseudo-element back-end. #12815

Merged
merged 12 commits into from Aug 16, 2016

stylo: We have an Atom back-end now.

  • Loading branch information
emilio committed Aug 16, 2016
commit 010bce128bdb12b5c39ed8dbfdd82485fa3861f7
@@ -26,6 +26,9 @@ pub struct GeckoSelectorImpl;
///
/// This should allow us to avoid random FFI overhead when cloning/dropping
/// pseudos.
///
/// Also, we can further optimize PartialEq and hash comparing/hashing only the
/// atoms.
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
pub struct PseudoElement(Atom, bool);

This comment has been minimized.

@SimonSapin

SimonSapin Aug 13, 2016

Member

Maybe use struct syntax with named fields? I particular to say what this bool is.

This comment has been minimized.

@emilio

emilio Aug 13, 2016

Author Member

It's documented above (though the diff makes it difficult to find), but
yeah, it's fair, I'll do it.

On 08/13/2016 08:04 AM, Simon Sapin wrote:

In components/style/gecko_selector_impl.rs
#12815 (comment):

  • MozNumberWrapper,
  • MozNumberText,
  • MozNumberSpinBox,
  • MozNumberSpinUp,
  • MozNumberSpinDown,
  • MozProgressBar,
  • MozRangeTrack,
  • MozRangeProgress,
  • MozRangeThumb,
  • MozMeterBar,
  • MozPlaceholder,

- MozColorSwatch,

  • AnonBox(AnonBoxPseudoElement),
    -}
    +pub struct PseudoElement(Atom, bool);

Maybe use struct syntax with named fields? I particular to say what this
|bool| is.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/servo/servo/pull/12815/files/8a92235c7c372b7d884a6928608a7477e2af7c70#r74687932,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQwull1-KtkBA4749IbfIn2nU6g_1AUks5qfd0PgaJpZM4Jh0-a.


@@ -41,8 +44,9 @@ impl PseudoElement {
}

#[inline]
fn from_atom_unchecked(atom: Atom, is_anon_box: bool) -> Self {
pub fn from_atom_unchecked(atom: Atom, is_anon_box: bool) -> Self {
if cfg!(debug_assertions) {
// Do the check on debug regardless.
match Self::from_atom(&*atom, true) {
Some(pseudo) => {
assert_eq!(pseudo.is_anon_box(), is_anon_box);
@@ -16,6 +16,7 @@ use gecko_bindings::ptr::{GeckoArcPrincipal, GeckoArcURI};
use gecko_bindings::structs::ServoElementSnapshot;
use gecko_bindings::structs::nsRestyleHint;
use gecko_bindings::structs::{SheetParsingMode, nsIAtom};
use gecko_string_cache::Atom;
use snapshot::GeckoElementSnapshot;
use std::mem::transmute;
use std::ptr;
@@ -39,29 +40,6 @@ use traversal::RecalcStyleOnly;
use url::Url;
use wrapper::{DUMMY_BASE_URL, GeckoDocument, GeckoElement, GeckoNode, NonOpaqueStyleData};

// TODO: This is ugly and should go away once we get an atom back-end.
pub fn pseudo_element_from_atom(pseudo: *mut nsIAtom,
in_ua_stylesheet: bool) -> Result<PseudoElement, String> {
use gecko_bindings::bindings::Gecko_GetAtomAsUTF16;
use selectors::parser::{ParserContext, SelectorImpl};

let pseudo_string = unsafe {
let mut length = 0;
let mut buff = Gecko_GetAtomAsUTF16(pseudo, &mut length);

// Handle the annoying preceding colon in front of everything in nsCSSAnonBoxList.h.
debug_assert!(length >= 2 && *buff == ':' as u16 && *buff.offset(1) != ':' as u16);
buff = buff.offset(1);
length -= 1;

String::from_utf16(slice::from_raw_parts(buff, length as usize)).unwrap()
};

let mut context = ParserContext::new();
context.in_user_agent_stylesheet = in_ua_stylesheet;
GeckoSelectorImpl::parse_pseudo_element(&context, &pseudo_string).map_err(|_| pseudo_string)
}

/*
* For Gecko->Servo function calls, we need to redeclare the same signature that was declared in
* the C header in Gecko. In order to catch accidental mismatches, we run rust-bindgen against
@@ -286,13 +264,8 @@ pub extern "C" fn Servo_GetComputedValuesForAnonymousBox(parent_style_or_null: *
let data = PerDocumentStyleData::borrow_mut_from_raw(raw_data);
data.flush_stylesheets();

let pseudo = match pseudo_element_from_atom(pseudo_tag, /* ua_stylesheet = */ true) {
Ok(pseudo) => pseudo,
Err(pseudo) => {
warn!("stylo: Unable to parse anonymous-box pseudo-element: {}", pseudo);
return ptr::null_mut();
}
};
let atom = unsafe { Atom::from_static(pseudo_tag) };

This comment has been minimized.

@bholley

bholley Aug 13, 2016

Contributor

This is a nice optimization, but a bit scary, since it would lead to an unpaired Release() if we ever got it wrong (this only works because ReleaseAtom() is a no-op for static atoms).

Can we make the gecko Atom struct carry an extra bit that indicates whether the atom was created with from_static, and skip the ReleaseAtom() call in that case? That would reduce the risk here, and also save an FFI call.

This comment has been minimized.

@emilio

emilio Aug 13, 2016

Author Member

It's probably better if we generate accessors for the atom bitfield, with the extra benefit that gives us the length (which means in some cases we don't need to make an ffi call to make some comparisons).

This comment has been minimized.

@emilio

emilio Aug 13, 2016

Author Member

Oh, also, note that from_atom_unchecked still checks the atom in debug builds.

let pseudo = PseudoElement::from_atom_unchecked(atom, /* anon_box = */ true);

type Helpers = ArcHelpers<ServoComputedValues, ComputedValues>;

@@ -320,14 +293,8 @@ pub extern "C" fn Servo_GetComputedValuesForPseudoElement(parent_style: *mut Ser
}
};

let pseudo = match pseudo_element_from_atom(pseudo_tag, /* ua_stylesheet = */ true) {
Ok(pseudo) => pseudo,
Err(pseudo) => {
warn!("stylo: Unable to parse anonymous-box pseudo-element: {}", pseudo);
return parent_or_null();
}
};

let atom = unsafe { Atom::from_static(pseudo_tag) };
let pseudo = PseudoElement::from_atom_unchecked(atom, /* anon_box = */ false);

// The stylist consumes stylesheets lazily.
let data = PerDocumentStyleData::borrow_mut_from_raw(raw_data);
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.