From 2483541fb97e58f5d6123b7ebb4b67d868e36fb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 10 Apr 2017 08:01:38 +0800 Subject: [PATCH 1/7] Bug 1325878: Allow creating empty Servo MediaList. r=xidorn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: H7owjPB8dRi Signed-off-by: Emilio Cobos Álvarez --- ports/geckolib/glue.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index c0d40c87a83..52ef5c95615 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -1192,6 +1192,14 @@ pub extern "C" fn Servo_DeclarationBlock_RemovePropertyById(declarations: RawSer remove_property(declarations, get_property_id_from_nscsspropertyid!(property, ())) } +#[no_mangle] +pub extern "C" fn Servo_MediaList_Create() -> RawServoMediaListStrong { + + let global_style_data = &*GLOBAL_STYLE_DATA; + Arc::new(global_style_data.shared_lock.wrap(MediaList::default())).into_strong() + +} + #[no_mangle] pub extern "C" fn Servo_MediaList_GetText(list: RawServoMediaListBorrowed, result: *mut nsAString) { read_locked_arc(list, |list: &MediaList| { From fbd4049e466eb51e6c830927daa7539386f7811b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 10 Apr 2017 10:25:33 +0800 Subject: [PATCH 2/7] Bug 1325878: Create less hardcoded nsMediaList instances. r=xidorn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: K6T3MM1ZrFb Signed-off-by: Emilio Cobos Álvarez --- ports/geckolib/glue.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 52ef5c95615..4ef2f0ac43e 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -1197,7 +1197,16 @@ pub extern "C" fn Servo_MediaList_Create() -> RawServoMediaListStrong { let global_style_data = &*GLOBAL_STYLE_DATA; Arc::new(global_style_data.shared_lock.wrap(MediaList::default())).into_strong() +} +#[no_mangle] +pub extern "C" fn Servo_MediaList_Matches(list: RawServoMediaListBorrowed, + raw_data: RawServoStyleSetBorrowed) + -> bool { + let per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow(); + read_locked_arc(list, |list: &MediaList| { + list.evaluate(&per_doc_data.stylist.device) + }) } #[no_mangle] From 299922243617dc5e15a092b2ea6fd7d6b090dcef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 10 Apr 2017 11:17:51 +0800 Subject: [PATCH 3/7] Bug 1325878: Don't use nsMediaList for loading imports. r=xidorn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: HR23bqZcmcA Signed-off-by: Emilio Cobos Álvarez --- components/script/stylesheet_loader.rs | 8 ++++---- components/style/stylesheets.rs | 11 ++++++----- ports/geckolib/stylesheet_loader.rs | 22 +++++----------------- 3 files changed, 15 insertions(+), 26 deletions(-) diff --git a/components/script/stylesheet_loader.rs b/components/script/stylesheet_loader.rs index cdafde510c9..961d487a330 100644 --- a/components/script/stylesheet_loader.rs +++ b/components/script/stylesheet_loader.rs @@ -271,15 +271,15 @@ impl<'a> StylesheetLoader<'a> { impl<'a> StyleStylesheetLoader for StylesheetLoader<'a> { fn request_stylesheet( &self, - media: MediaList, - make_import: &mut FnMut(MediaList) -> ImportRule, + media: Arc>, + make_import: &mut FnMut(Arc>) -> ImportRule, make_arc: &mut FnMut(ImportRule) -> Arc>, ) -> Arc> { let import = make_import(media); let url = import.url.url().expect("Invalid urls shouldn't enter the loader").clone(); - //TODO (mrnayak) : Whether we should use the original loader's CORS setting? - //Fix this when spec has more details. + // TODO (mrnayak) : Whether we should use the original loader's CORS + // setting? Fix this when spec has more details. let source = StylesheetContextSource::Import(import.stylesheet.clone()); self.load(source, url, None, "".to_owned()); diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index 3aa214df9e2..407fe7ee401 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -825,8 +825,8 @@ pub trait StylesheetLoader { /// before they’re locked, while keeping the trait object-safe. fn request_stylesheet( &self, - media: MediaList, - make_import: &mut FnMut(MediaList) -> ImportRule, + media: Arc>, + make_import: &mut FnMut(Arc>) -> ImportRule, make_arc: &mut FnMut(ImportRule) -> Arc>, ) -> Arc>; } @@ -836,8 +836,8 @@ struct NoOpLoader; impl StylesheetLoader for NoOpLoader { fn request_stylesheet( &self, - media: MediaList, - make_import: &mut FnMut(MediaList) -> ImportRule, + media: Arc>, + make_import: &mut FnMut(Arc>) -> ImportRule, make_arc: &mut FnMut(ImportRule) -> Arc>, ) -> Arc> { make_arc(make_import(media)) @@ -906,6 +906,7 @@ impl<'a> AtRuleParser for TopLevelRuleParser<'a> { let specified_url = SpecifiedUrl::parse_from_string(url_string, &self.context)?; let media = parse_media_query_list(&self.context, input); + let media = Arc::new(self.shared_lock.wrap(media)); let noop_loader = NoOpLoader; let loader = if !specified_url.is_invalid() { @@ -920,7 +921,7 @@ impl<'a> AtRuleParser for TopLevelRuleParser<'a> { url: specified_url.take().unwrap(), stylesheet: Arc::new(Stylesheet { rules: CssRules::new(Vec::new(), self.shared_lock), - media: Arc::new(self.shared_lock.wrap(media)), + media: media, shared_lock: self.shared_lock.clone(), origin: self.context.stylesheet_origin, url_data: self.context.url_data.clone(), diff --git a/ports/geckolib/stylesheet_loader.rs b/ports/geckolib/stylesheet_loader.rs index 598d778cbdb..a79aa6e192f 100644 --- a/ports/geckolib/stylesheet_loader.rs +++ b/ports/geckolib/stylesheet_loader.rs @@ -5,11 +5,10 @@ use std::sync::Arc; use style::gecko_bindings::bindings::Gecko_LoadStyleSheet; use style::gecko_bindings::structs::{Loader, ServoStyleSheet}; -use style::gecko_bindings::sugar::ownership::HasArcFFI; +use style::gecko_bindings::sugar::ownership::{HasArcFFI, FFIArcHelpers}; use style::media_queries::MediaList; use style::shared_lock::Locked; use style::stylesheets::{ImportRule, Stylesheet, StylesheetLoader as StyleStylesheetLoader}; -use style_traits::ToCss; pub struct StylesheetLoader(*mut Loader, *mut ServoStyleSheet); @@ -22,21 +21,11 @@ impl StylesheetLoader { impl StyleStylesheetLoader for StylesheetLoader { fn request_stylesheet( &self, - media: MediaList, - make_import: &mut FnMut(MediaList) -> ImportRule, + media: Arc>, + make_import: &mut FnMut(Arc>) -> ImportRule, make_arc: &mut FnMut(ImportRule) -> Arc>, ) -> Arc> { - // TODO(emilio): We probably want to share media representation with - // Gecko in Stylo. - // - // This also allows us to get rid of a bunch of extra work to evaluate - // and ensure parity, and shouldn't be much Gecko work given we always - // evaluate them on the main thread. - // - // Meanwhile, this works. - let media_string = media.to_css_string(); - - let import = make_import(media); + let import = make_import(media.clone()); // After we get this raw pointer ImportRule will be moved into a lock and Arc // and so the Arc pointer inside will also move, @@ -52,8 +41,7 @@ impl StyleStylesheetLoader for StylesheetLoader { base_url_data, spec_bytes, spec_len as u32, - media_string.as_bytes().as_ptr(), - media_string.len() as u32); + media.into_strong()) } make_arc(import) } From 27ca9e0a467a2247efc5b86d6d47c44ef3c56310 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 10 Apr 2017 14:36:45 +0800 Subject: [PATCH 4/7] Bug 1325878: Support deep-cloning of ServoMediaLists. r=xidorn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MozReview-Commit-ID: K7NFe1tKrAZ Signed-off-by: Emilio Cobos Álvarez --- components/style/media_queries.rs | 2 +- ports/geckolib/glue.rs | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/components/style/media_queries.rs b/components/style/media_queries.rs index d92dd324f99..9409b464f15 100644 --- a/components/style/media_queries.rs +++ b/components/style/media_queries.rs @@ -20,7 +20,7 @@ pub use servo::media_queries::{Device, Expression}; pub use gecko::media_queries::{Device, Expression}; /// A type that encapsulates a media query list. -#[derive(Debug)] +#[derive(Debug, Clone)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub struct MediaList { /// The list of media queries. diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 4ef2f0ac43e..97f519cf0f7 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -1194,11 +1194,19 @@ pub extern "C" fn Servo_DeclarationBlock_RemovePropertyById(declarations: RawSer #[no_mangle] pub extern "C" fn Servo_MediaList_Create() -> RawServoMediaListStrong { - let global_style_data = &*GLOBAL_STYLE_DATA; Arc::new(global_style_data.shared_lock.wrap(MediaList::default())).into_strong() } +#[no_mangle] +pub extern "C" fn Servo_MediaList_DeepClone(list: RawServoMediaListBorrowed) -> RawServoMediaListStrong { + let global_style_data = &*GLOBAL_STYLE_DATA; + read_locked_arc(list, |list: &MediaList| { + Arc::new(global_style_data.shared_lock.wrap(list.clone())) + .into_strong() + }) +} + #[no_mangle] pub extern "C" fn Servo_MediaList_Matches(list: RawServoMediaListBorrowed, raw_data: RawServoStyleSetBorrowed) From 482740bb11405bbd9a7e7dda75e35dd1ffb87865 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 11 Apr 2017 13:36:20 +0800 Subject: [PATCH 5/7] Bug 1325878: Rewrite insert_rule to avoid lock re-entrancy problems. r=xidorn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In particular, you can insertRule("@import url(\"already-loaded.css\");") and we would try to clone the stylesheet, which without this change would borrow the lock for reading. MozReview-Commit-ID: 4jdNL5rngh7 Signed-off-by: Emilio Cobos Álvarez --- components/style/stylesheets.rs | 127 ++++++++++++++++++++------------ ports/geckolib/glue.rs | 26 ++++--- 2 files changed, 96 insertions(+), 57 deletions(-) diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index 407fe7ee401..8bdcc9bce8c 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -135,53 +135,6 @@ impl CssRules { }) } - /// https://drafts.csswg.org/cssom/#insert-a-css-rule - pub fn insert_rule(&mut self, - rule: &str, - parent_stylesheet: &Stylesheet, - index: usize, - nested: bool, - loader: Option<&StylesheetLoader>) - -> Result { - // Step 1, 2 - if index > self.0.len() { - return Err(RulesMutateError::IndexSize); - } - - // Computes the parser state at the given index - let state = if nested { - None - } else if index == 0 { - Some(State::Start) - } else { - self.0.get(index - 1).map(CssRule::rule_state) - }; - - // Step 3, 4 - // XXXManishearth should we also store the namespace map? - let (new_rule, new_state) = - try!(CssRule::parse(&rule, parent_stylesheet, state, loader)); - - // Step 5 - // Computes the maximum allowed parser state at a given index. - let rev_state = self.0.get(index).map_or(State::Body, CssRule::rule_state); - if new_state > rev_state { - // We inserted a rule too early, e.g. inserting - // a regular style rule before @namespace rules - return Err(RulesMutateError::HierarchyRequest); - } - - // Step 6 - if let CssRule::Namespace(..) = new_rule { - if !self.only_ns_or_import() { - return Err(RulesMutateError::InvalidState); - } - } - - self.0.insert(index, new_rule.clone()); - Ok(new_rule) - } - /// https://drafts.csswg.org/cssom/#remove-a-css-rule pub fn remove_rule(&mut self, index: usize) -> Result<(), RulesMutateError> { // Step 1, 2 @@ -207,6 +160,86 @@ impl CssRules { } } +/// A trait to implement helpers for `Arc>`. +pub trait CssRulesHelpers { + /// https://drafts.csswg.org/cssom/#insert-a-css-rule + /// + /// Written in this funky way because parsing an @import rule may cause us + /// to clone a stylesheet from the same document due to caching in the CSS + /// loader. + /// + /// TODO(emilio): We could also pass the write guard down into the loader + /// instead, but that seems overkill. + fn insert_rule(&self, + lock: &SharedRwLock, + rule: &str, + parent_stylesheet: &Stylesheet, + index: usize, + nested: bool, + loader: Option<&StylesheetLoader>) + -> Result; +} + +impl CssRulesHelpers for Arc> { + fn insert_rule(&self, + lock: &SharedRwLock, + rule: &str, + parent_stylesheet: &Stylesheet, + index: usize, + nested: bool, + loader: Option<&StylesheetLoader>) + -> Result { + let state = { + let read_guard = lock.read(); + let rules = self.read_with(&read_guard); + + // Step 1, 2 + if index > rules.0.len() { + return Err(RulesMutateError::IndexSize); + } + + // Computes the parser state at the given index + if nested { + None + } else if index == 0 { + Some(State::Start) + } else { + rules.0.get(index - 1).map(CssRule::rule_state) + } + }; + + // Step 3, 4 + // XXXManishearth should we also store the namespace map? + let (new_rule, new_state) = + try!(CssRule::parse(&rule, parent_stylesheet, state, loader)); + + { + let mut write_guard = lock.write(); + let mut rules = self.write_with(&mut write_guard); + // Step 5 + // Computes the maximum allowed parser state at a given index. + let rev_state = rules.0.get(index).map_or(State::Body, CssRule::rule_state); + if new_state > rev_state { + // We inserted a rule too early, e.g. inserting + // a regular style rule before @namespace rules + return Err(RulesMutateError::HierarchyRequest); + } + + // Step 6 + if let CssRule::Namespace(..) = new_rule { + if !rules.only_ns_or_import() { + return Err(RulesMutateError::InvalidState); + } + } + + rules.0.insert(index, new_rule.clone()); + } + + Ok(new_rule) + } + +} + /// The structure servo uses to represent a stylesheet. #[derive(Debug)] pub struct Stylesheet { diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 97f519cf0f7..e3a5185fd4f 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -83,8 +83,9 @@ use style::selector_parser::PseudoElementCascadeType; use style::sequential; use style::shared_lock::{SharedRwLock, SharedRwLockReadGuard, StylesheetGuards, ToCssWithGuard, Locked}; use style::string_cache::Atom; -use style::stylesheets::{CssRule, CssRules, CssRuleType, ImportRule, MediaRule, NamespaceRule, PageRule}; -use style::stylesheets::{Origin, Stylesheet, StyleRule}; +use style::stylesheets::{CssRule, CssRules, CssRuleType, CssRulesHelpers}; +use style::stylesheets::{ImportRule, MediaRule, NamespaceRule, Origin}; +use style::stylesheets::{PageRule, Stylesheet, StyleRule}; use style::stylesheets::StylesheetLoader as StyleStylesheetLoader; use style::supports::parse_condition_or_declaration; use style::thread_state; @@ -698,15 +699,20 @@ pub extern "C" fn Servo_CssRules_InsertRule(rules: ServoCssRulesBorrowed, }; let loader = loader.as_ref().map(|loader| loader as &StyleStylesheetLoader); let rule = unsafe { rule.as_ref().unwrap().as_str_unchecked() }; - write_locked_arc(rules, |rules: &mut CssRules| { - match rules.insert_rule(rule, sheet, index as usize, nested, loader) { - Ok(new_rule) => { - *unsafe { rule_type.as_mut().unwrap() } = new_rule.rule_type() as u16; - nsresult::NS_OK - } - Err(err) => err.into() + + let global_style_data = &*GLOBAL_STYLE_DATA; + match Locked::::as_arc(&rules).insert_rule(&global_style_data.shared_lock, + rule, + sheet, + index as usize, + nested, + loader) { + Ok(new_rule) => { + *unsafe { rule_type.as_mut().unwrap() } = new_rule.rule_type() as u16; + nsresult::NS_OK } - }) + Err(err) => err.into(), + } } #[no_mangle] From ac7bc414d9c6fb087aefc1e79c8db715051ef0d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 11 Apr 2017 22:13:16 +0800 Subject: [PATCH 6/7] Bug 1325878: Pass the MediaList down to Servo, making