From 134ef338424759cf44bc9fcc62ea1d43f65784dd Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 24 Nov 2016 12:14:58 +1100 Subject: [PATCH 1/3] Move "insert a CSS rule" algorithm to style This also changes the algorithm to match w3c/csswg-drafts#754 instead of the current cssom draft, because that makes the code simpler and matches implementation of other browsers. --- components/script/dom/cssrulelist.rs | 76 +++++----------------------- components/style/stylesheets.rs | 56 ++++++++++++++++++++ 2 files changed, 70 insertions(+), 62 deletions(-) diff --git a/components/script/dom/cssrulelist.rs b/components/script/dom/cssrulelist.rs index deb6b93e8b1..c10b9196092 100644 --- a/components/script/dom/cssrulelist.rs +++ b/components/script/dom/cssrulelist.rs @@ -15,13 +15,23 @@ use dom::cssstylesheet::CSSStyleSheet; use dom::window::Window; use parking_lot::RwLock; use std::sync::Arc; -use style::parser::ParserContextExtraData; -use style::stylesheets::{CssRules, KeyframesRule, Origin}; +use style::stylesheets::{CssRules, KeyframesRule, RulesMutateError}; use style::stylesheets::CssRule as StyleCssRule; no_jsmanaged_fields!(RulesSource); no_jsmanaged_fields!(CssRules); +impl From for Error { + fn from(other: RulesMutateError) -> Self { + match other { + RulesMutateError::Syntax => Error::Syntax, + RulesMutateError::IndexSize => Error::IndexSize, + RulesMutateError::HierarchyRequest => Error::HierarchyRequest, + RulesMutateError::InvalidState => Error::InvalidState, + } + } +} + #[dom_struct] pub struct CSSRuleList { reflector_: Reflector, @@ -64,21 +74,9 @@ impl CSSRuleList { CSSRuleListBinding::Wrap) } - /// https://drafts.csswg.org/cssom/#insert-a-css-rule - /// /// Should only be called for CssRules-backed rules. Use append_lazy_rule /// for keyframes-backed rules. pub fn insert_rule(&self, rule: &str, idx: u32, nested: bool) -> Fallible { - use style::stylesheets::SingleRuleParseError; - /// Insert an item into a vector, appending if it is out of bounds - fn insert(vec: &mut Vec, index: usize, item: T) { - if index >= vec.len() { - vec.push(item); - } else { - vec.insert(index, item); - } - } - let css_rules = if let RulesSource::Rules(ref rules) = self.rules { rules } else { @@ -90,58 +88,12 @@ impl CSSRuleList { let doc = window.Document(); let index = idx as usize; + let new_rule = css_rules.insert_rule(rule, doc.url().clone(), index, nested)?; - let new_rule = { - let rules = css_rules.0.read(); - let state = if nested { - None - } else { - Some(CssRules::state_at_index(&rules, index)) - }; - - let rev_state = CssRules::state_at_index_rev(&rules, index); - - // Step 1, 2 - // XXXManishearth get url from correct location - // XXXManishearth should we also store the namespace map? - let parse_result = StyleCssRule::parse(&rule, Origin::Author, - doc.url().clone(), - ParserContextExtraData::default(), - state); - - if let Err(SingleRuleParseError::Syntax) = parse_result { - return Err(Error::Syntax) - } - - // Step 3, 4 - if index > rules.len() { - return Err(Error::IndexSize); - } - - let (new_rule, new_state) = try!(parse_result.map_err(|_| Error::HierarchyRequest)); - - if new_state > rev_state { - // We inserted a rule too early, e.g. inserting - // a regular style rule before @namespace rules - return Err((Error::HierarchyRequest)); - } - - // Step 6 - if let StyleCssRule::Namespace(..) = new_rule { - if !CssRules::only_ns_or_import(&rules) { - return Err(Error::InvalidState); - } - } - - new_rule - }; - - insert(&mut css_rules.0.write(), index, new_rule.clone()); let sheet = self.sheet.get(); let sheet = sheet.as_ref().map(|sheet| &**sheet); let dom_rule = CSSRule::new_specific(&window, sheet, new_rule); - insert(&mut self.dom_rules.borrow_mut(), - index, MutNullableHeap::new(Some(&*dom_rule))); + self.dom_rules.borrow_mut().insert(index, MutNullableHeap::new(Some(&*dom_rule))); Ok((idx)) } diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index 12ca0423cfa..cce7e40c223 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -60,6 +60,22 @@ impl From> for CssRules { } } +pub enum RulesMutateError { + Syntax, + IndexSize, + HierarchyRequest, + InvalidState, +} + +impl From for RulesMutateError { + fn from(other: SingleRuleParseError) -> Self { + match other { + SingleRuleParseError::Syntax => RulesMutateError::Syntax, + SingleRuleParseError::Hierarchy => RulesMutateError::HierarchyRequest, + } + } +} + impl CssRules { // used in CSSOM pub fn only_ns_or_import(rules: &[CssRule]) -> bool { @@ -102,6 +118,46 @@ impl CssRules { State::Body } } + + /// https://drafts.csswg.org/cssom/#insert-a-css-rule + pub fn insert_rule(&self, rule: &str, base_url: ServoUrl, index: usize, nested: bool) + -> Result { + let mut rules = self.0.write(); + + // Step 1, 2 + if index > rules.len() { + return Err(RulesMutateError::IndexSize); + } + + let state = if nested { + None + } else { + Some(CssRules::state_at_index(&rules, index)) + }; + + // Step 3, 4 + // XXXManishearth should we also store the namespace map? + let (new_rule, new_state) = try!(CssRule::parse(&rule, Origin::Author, base_url, + ParserContextExtraData::default(), state)); + + // Step 5 + let rev_state = CssRules::state_at_index_rev(&rules, index); + 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 !CssRules::only_ns_or_import(&rules) { + return Err(RulesMutateError::InvalidState); + } + } + + rules.insert(index, new_rule.clone()); + Ok(new_rule) + } } #[derive(Debug)] From 03bcb7a26a475a87e9a91c7faf8d917ec0534817 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 24 Nov 2016 12:30:08 +1100 Subject: [PATCH 2/3] Move "remove a CSS rule" algorithm to style --- components/script/dom/cssrulelist.rs | 26 +------------------------- components/style/stylesheets.rs | 28 +++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/components/script/dom/cssrulelist.rs b/components/script/dom/cssrulelist.rs index c10b9196092..0f2212319dc 100644 --- a/components/script/dom/cssrulelist.rs +++ b/components/script/dom/cssrulelist.rs @@ -16,7 +16,6 @@ use dom::window::Window; use parking_lot::RwLock; use std::sync::Arc; use style::stylesheets::{CssRules, KeyframesRule, RulesMutateError}; -use style::stylesheets::CssRule as StyleCssRule; no_jsmanaged_fields!(RulesSource); no_jsmanaged_fields!(CssRules); @@ -97,37 +96,14 @@ impl CSSRuleList { Ok((idx)) } - // https://drafts.csswg.org/cssom/#remove-a-css-rule - // https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-deleterule // In case of a keyframe rule, index must be valid. pub fn remove_rule(&self, index: u32) -> ErrorResult { let index = index as usize; match self.rules { RulesSource::Rules(ref css_rules) => { - // https://drafts.csswg.org/cssom/#remove-a-css-rule - { - let rules = css_rules.0.read(); - - // Step 1, 2 - if index >= rules.len() { - return Err(Error::IndexSize); - } - - // Step 3 - let ref rule = rules[index]; - - // Step 4 - if let StyleCssRule::Namespace(..) = *rule { - if !CssRules::only_ns_or_import(&rules) { - return Err(Error::InvalidState); - } - } - } - - // Step 5, 6 + css_rules.remove_rule(index)?; let mut dom_rules = self.dom_rules.borrow_mut(); - css_rules.0.write().remove(index); dom_rules[index].get().map(|r| r.detach()); dom_rules.remove(index); Ok(()) diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index cce7e40c223..f098e50dbbb 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -119,7 +119,7 @@ impl CssRules { } } - /// https://drafts.csswg.org/cssom/#insert-a-css-rule + // https://drafts.csswg.org/cssom/#insert-a-css-rule pub fn insert_rule(&self, rule: &str, base_url: ServoUrl, index: usize, nested: bool) -> Result { let mut rules = self.0.write(); @@ -158,6 +158,32 @@ impl CssRules { rules.insert(index, new_rule.clone()); Ok(new_rule) } + + // https://drafts.csswg.org/cssom/#remove-a-css-rule + pub fn remove_rule(&self, index: usize) -> Result<(), RulesMutateError> { + let mut rules = self.0.write(); + + // Step 1, 2 + if index >= rules.len() { + return Err(RulesMutateError::IndexSize); + } + + { + // Step 3 + let ref rule = rules[index]; + + // Step 4 + if let CssRule::Namespace(..) = *rule { + if !CssRules::only_ns_or_import(&rules) { + return Err(RulesMutateError::InvalidState); + } + } + } + + // Step 5, 6 + rules.remove(index); + Ok(()) + } } #[derive(Debug)] From 3e7818f1775db03e8adedf32f80aee43f5765166 Mon Sep 17 00:00:00 2001 From: Xidorn Quan Date: Thu, 24 Nov 2016 12:58:44 +1100 Subject: [PATCH 3/3] Refactor the state computation code --- components/style/stylesheets.rs | 49 ++++++++++----------------------- 1 file changed, 15 insertions(+), 34 deletions(-) diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs index f098e50dbbb..aeede52cee1 100644 --- a/components/style/stylesheets.rs +++ b/components/style/stylesheets.rs @@ -87,38 +87,6 @@ impl CssRules { }) } - // Provides the parser state at a given insertion index - pub fn state_at_index(rules: &[CssRule], at: usize) -> State { - let mut state = State::Start; - if at > 0 { - if let Some(rule) = rules.get(at - 1) { - state = match *rule { - // CssRule::Charset(..) => State::Start, - // CssRule::Import(..) => State::Imports, - CssRule::Namespace(..) => State::Namespaces, - _ => State::Body, - }; - } - } - state - } - - // Provides the maximum allowed parser state at a given index, - // searching in reverse. If inserting at this index, the parser - // must not be in a state greater than this post-insertion. - pub fn state_at_index_rev(rules: &[CssRule], at: usize) -> State { - if let Some(rule) = rules.get(at) { - match *rule { - // CssRule::Charset(..) => State::Start, - // CssRule::Import(..) => State::Imports, - CssRule::Namespace(..) => State::Namespaces, - _ => State::Body, - } - } else { - State::Body - } - } - // https://drafts.csswg.org/cssom/#insert-a-css-rule pub fn insert_rule(&self, rule: &str, base_url: ServoUrl, index: usize, nested: bool) -> Result { @@ -129,10 +97,13 @@ impl CssRules { 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 { - Some(CssRules::state_at_index(&rules, index)) + rules.get(index - 1).map(CssRule::rule_state) }; // Step 3, 4 @@ -141,7 +112,8 @@ impl CssRules { ParserContextExtraData::default(), state)); // Step 5 - let rev_state = CssRules::state_at_index_rev(&rules, index); + // Computes the maximum allowed parser state at a given index. + let rev_state = rules.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 @@ -274,6 +246,15 @@ impl CssRule { } } + fn rule_state(&self) -> State { + match *self { + // CssRule::Charset(..) => State::Start, + // CssRule::Import(..) => State::Imports, + CssRule::Namespace(..) => State::Namespaces, + _ => State::Body, + } + } + /// Call `f` with the slice of rules directly contained inside this rule. /// /// Note that only some types of rules can contain rules. An empty slice is used for others.