servo: Merge #15256 - script: Refactor CSSStyleDeclaration and fix some bugs in the way (from emilio:cssstyledeclaration); r=SimonSapin
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 28 Jan 2017 06:18:38 -0800
changeset 340742 fc8c73a1b77b009b1db058aef980e8e912527086
parent 340741 dbe3d34971cc73165f188f316f17dea0ec1908ac
child 340743 2e5cea5fbfc820d802080f70ecaacc2b149a0e48
push id86548
push userkwierso@gmail.com
push dateSat, 04 Feb 2017 01:35:21 +0000
treeherdermozilla-inbound@e7b96d015d03 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersSimonSapin
servo: Merge #15256 - script: Refactor CSSStyleDeclaration and fix some bugs in the way (from emilio:cssstyledeclaration); r=SimonSapin <!-- Please describe your changes on the following line: --> This is preliminar work for the style attribute restyle hint. Source-Repo: https://github.com/servo/servo Source-Revision: 7a40f472337f0a9a44e2146f4005ec99da38f96b
servo/components/script/dom/csskeyframerule.rs
servo/components/script/dom/cssstyledeclaration.rs
servo/components/script/dom/cssstylerule.rs
servo/components/style/properties/declaration_block.rs
--- a/servo/components/script/dom/csskeyframerule.rs
+++ b/servo/components/script/dom/csskeyframerule.rs
@@ -1,13 +1,14 @@
 /* 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 dom::bindings::codegen::Bindings::CSSKeyframeRuleBinding::{self, CSSKeyframeRuleMethods};
+use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{JS, MutNullableJS, Root};
 use dom::bindings::reflector::{DomObject, reflect_dom_object};
 use dom::bindings::str::DOMString;
 use dom::cssrule::{CSSRule, SpecificCSSRule};
 use dom::cssstyledeclaration::{CSSModificationAccess, CSSStyleDeclaration, CSSStyleOwner};
 use dom::cssstylesheet::CSSStyleSheet;
 use dom::window::Window;
 use parking_lot::RwLock;
@@ -42,18 +43,18 @@ impl CSSKeyframeRule {
     }
 }
 
 impl CSSKeyframeRuleMethods for CSSKeyframeRule {
     // https://drafts.csswg.org/css-animations/#dom-csskeyframerule-style
     fn Style(&self) -> Root<CSSStyleDeclaration> {
         self.style_decl.or_init(|| {
             CSSStyleDeclaration::new(self.global().as_window(),
-                                     CSSStyleOwner::CSSRule(JS::from_ref(self.global().as_window()),
-                                                                 self.keyframerule.read().block.clone()),
+                                     CSSStyleOwner::CSSRule(JS::from_ref(self.upcast()),
+                                                            self.keyframerule.read().block.clone()),
                                      None,
                                      CSSModificationAccess::ReadWrite)
         })
     }
 }
 
 impl SpecificCSSRule for CSSKeyframeRule {
     fn ty(&self) -> u16 {
--- a/servo/components/script/dom/cssstyledeclaration.rs
+++ b/servo/components/script/dom/cssstyledeclaration.rs
@@ -2,22 +2,24 @@
  * 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 dom::bindings::codegen::Bindings::CSSStyleDeclarationBinding::{self, CSSStyleDeclarationMethods};
 use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
 use dom::bindings::error::{Error, ErrorResult, Fallible};
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{JS, Root};
-use dom::bindings::reflector::{Reflector, reflect_dom_object};
+use dom::bindings::reflector::{DomObject, Reflector, reflect_dom_object};
 use dom::bindings::str::DOMString;
+use dom::cssrule::CSSRule;
 use dom::element::Element;
 use dom::node::{Node, NodeDamage, window_from_node};
 use dom::window::Window;
 use parking_lot::RwLock;
+use servo_url::ServoUrl;
 use std::ascii::AsciiExt;
 use std::sync::Arc;
 use style::parser::ParserContextExtraData;
 use style::properties::{Importance, PropertyDeclarationBlock, PropertyId, LonghandId, ShorthandId};
 use style::properties::{parse_one_declaration, parse_style_attribute};
 use style::selector_parser::PseudoElement;
 use style_traits::ToCss;
 
@@ -29,56 +31,111 @@ pub struct CSSStyleDeclaration {
     readonly: bool,
     pseudo: Option<PseudoElement>,
 }
 
 #[derive(HeapSizeOf, JSTraceable)]
 #[must_root]
 pub enum CSSStyleOwner {
     Element(JS<Element>),
-    CSSRule(JS<Window>,
+    CSSRule(JS<CSSRule>,
             #[ignore_heap_size_of = "Arc"]
             Arc<RwLock<PropertyDeclarationBlock>>),
 }
 
 impl CSSStyleOwner {
-    fn style_attribute(&self) -> Option<Arc<RwLock<PropertyDeclarationBlock>>> {
+    // Mutate the declaration block associated to this style owner, and
+    // optionally indicate if it has changed (assumed to be true).
+    fn mutate_associated_block<F, R>(&self, f: F) -> R
+        where F: FnOnce(&mut PropertyDeclarationBlock, &mut bool) -> R,
+    {
+        // TODO(emilio): This has some duplication just to avoid dummy clones.
+        //
+        // This is somewhat complex but the complexity is encapsulated.
+        let mut changed = true;
         match *self {
             CSSStyleOwner::Element(ref el) => {
-                if let Some(ref pdb) = *el.style_attribute().borrow() {
-                    Some(pdb.clone())
+                let mut attr = el.style_attribute().borrow_mut();
+                let (result, needs_clear) = if attr.is_some() {
+                    let lock = attr.as_ref().unwrap();
+                    let mut pdb = lock.write();
+                    let result = f(&mut pdb, &mut changed);
+                    if changed {
+                        el.set_style_attr(pdb.to_css_string());
+                        el.upcast::<Node>().dirty(NodeDamage::NodeStyleDamaged);
+                    }
+                    (result, pdb.declarations.is_empty())
                 } else {
-                    None
+                    let mut pdb = PropertyDeclarationBlock {
+                        important_count: 0,
+                        declarations: vec![],
+                    };
+                    let result = f(&mut pdb, &mut changed);
+
+                    // Here `changed` is somewhat silly, because we know the
+                    // exact conditions under it changes.
+                    if !pdb.declarations.is_empty() {
+                        el.set_style_attr(pdb.to_css_string());
+                        el.upcast::<Node>().dirty(NodeDamage::NodeStyleDamaged);
+                        *attr = Some(Arc::new(RwLock::new(pdb)));
+                    }
+
+                    (result, false)
+                };
+
+                if needs_clear {
+                    *attr = None;
+                }
+                result
+            }
+            CSSStyleOwner::CSSRule(ref rule, ref pdb) => {
+                let result = f(&mut *pdb.write(), &mut changed);
+                if changed {
+                    rule.global().as_window().Document().invalidate_stylesheets();
+                }
+                result
+            }
+        }
+    }
+
+    fn with_block<F, R>(&self, f: F) -> R
+        where F: FnOnce(&PropertyDeclarationBlock) -> R,
+    {
+        match *self {
+            CSSStyleOwner::Element(ref el) => {
+                match *el.style_attribute().borrow() {
+                    Some(ref pdb) => f(&pdb.read()),
+                    None => {
+                        let pdb = PropertyDeclarationBlock {
+                            important_count: 0,
+                            declarations: vec![],
+                        };
+                        f(&pdb)
+                    }
                 }
             }
             CSSStyleOwner::CSSRule(_, ref pdb) => {
-                Some(pdb.clone())
+                f(&pdb.read())
             }
         }
     }
 
     fn window(&self) -> Root<Window> {
         match *self {
             CSSStyleOwner::Element(ref el) => window_from_node(&**el),
-            CSSStyleOwner::CSSRule(ref window, _) => Root::from_ref(&**window),
+            CSSStyleOwner::CSSRule(ref rule, _) => Root::from_ref(rule.global().as_window()),
         }
     }
 
-    fn flush_style(&self, pdb: &PropertyDeclarationBlock) {
-        if let CSSStyleOwner::Element(ref el) = *self {
-            el.set_style_attr(pdb.to_css_string());
-        }
-    }
-
-    fn dirty(&self) {
+    fn base_url(&self) -> ServoUrl {
         match *self {
-            CSSStyleOwner::Element(ref el) =>
-                el.upcast::<Node>().dirty(NodeDamage::NodeStyleDamaged),
-            CSSStyleOwner::CSSRule(ref window, _) =>
-                window.Document().invalidate_stylesheets(),
+            CSSStyleOwner::Element(ref el) => window_from_node(&**el).Document().base_url(),
+            CSSStyleOwner::CSSRule(ref rule, _) => {
+                rule.parent_stylesheet().style_stylesheet().base_url.clone()
+            }
         }
     }
 }
 
 #[derive(PartialEq, HeapSizeOf)]
 pub enum CSSModificationAccess {
     ReadWrite,
     Readonly,
@@ -142,109 +199,84 @@ impl CSSStyleDeclaration {
     }
 
     fn get_property_value(&self, id: PropertyId) -> DOMString {
         if self.readonly {
             // Readonly style declarations are used for getComputedStyle.
             return self.get_computed_style(id);
         }
 
-        if let Some(ref lock) = self.owner.style_attribute() {
-            let mut string = String::new();
-            lock.read().property_value_to_css(&id, &mut string).unwrap();
-            DOMString::from(string)
-        } else {
-            // No style attribute is like an empty style attribute: no matching declaration.
-            DOMString::new()
-        }
+        let mut string = String::new();
+
+        self.owner.with_block(|ref pdb| {
+            pdb.property_value_to_css(&id, &mut string).unwrap();
+        });
+
+        DOMString::from(string)
     }
 
     fn set_property(&self, id: PropertyId, value: DOMString, priority: DOMString) -> ErrorResult {
         // Step 1
         if self.readonly {
             return Err(Error::NoModificationAllowed);
         }
 
-        if value.is_empty() {
-            // Step 4
-            let empty = {
-                if let Some(ref lock) = self.owner.style_attribute() {
-                    let mut style_attribute = lock.write();
-                    style_attribute.remove_property(&id);
-                    style_attribute.declarations.is_empty()
-                } else {
-                    // No style attribute is like an empty style attribute: nothing to remove.
-                    return Ok(())
-                }
-            };
-            if let (&CSSStyleOwner::Element(ref el), true) = (&self.owner, empty) {
-                *el.style_attribute().borrow_mut() = None;
+        self.owner.mutate_associated_block(|ref mut pdb, mut changed| {
+            if value.is_empty() {
+                // Step 4
+                *changed = pdb.remove_property(&id);
+                return Ok(());
             }
-        } else {
+
             // Step 5
             let importance = match &*priority {
                 "" => Importance::Normal,
                 p if p.eq_ignore_ascii_case("important") => Importance::Important,
-                _ => return Ok(()),
+                _ => {
+                    *changed = false;
+                    return Ok(());
+                }
             };
 
             // Step 6
             let window = self.owner.window();
             let declarations =
-                parse_one_declaration(id, &value, &window.get_url(), window.css_error_reporter(),
+                parse_one_declaration(id, &value, &self.owner.base_url(),
+                                      window.css_error_reporter(),
                                       ParserContextExtraData::default());
 
             // Step 7
-            let mut declarations = match declarations {
+            let declarations = match declarations {
                 Ok(declarations) => declarations,
-                Err(_) => return Ok(())
+                Err(_) => {
+                    *changed = false;
+                    return Ok(());
+                }
             };
 
             // Step 8
             // Step 9
-            match self.owner.style_attribute() {
-                Some(ref lock) => {
-                    let mut style_attribute = lock.write();
-                    for declaration in declarations {
-                        style_attribute.set_parsed_declaration(declaration.0, importance);
-                    }
-                    self.owner.flush_style(&style_attribute);
-                }
-                None => {
-                    let important_count = if importance.important() {
-                        declarations.len() as u32
-                    } else {
-                        0
-                    };
-                    for decl in &mut declarations {
-                        decl.1 = importance
-                    }
-                    let block = PropertyDeclarationBlock {
-                        declarations: declarations,
-                        important_count: important_count,
-                    };
-                    if let CSSStyleOwner::Element(ref el) = self.owner {
-                        el.set_style_attr(block.to_css_string());
-                        *el.style_attribute().borrow_mut() = Some(Arc::new(RwLock::new(block)));
-                    } else {
-                        panic!("set_property called on a CSSStyleDeclaration with a non-Element owner");
-                    }
-                }
+            // We could try to be better I guess?
+            *changed = !declarations.is_empty();
+            for declaration in declarations {
+                // TODO(emilio): We could check it changed
+                pdb.set_parsed_declaration(declaration.0, importance);
             }
-        }
 
-        self.owner.dirty();
-        Ok(())
+            Ok(())
+        })
     }
 }
 
 impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-length
     fn Length(&self) -> u32 {
-        self.owner.style_attribute().as_ref().map_or(0, |lock| lock.read().declarations.len() as u32)
+        self.owner.with_block(|ref pdb| {
+            pdb.declarations.len() as u32
+        })
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-item
     fn Item(&self, index: u32) -> DOMString {
         self.IndexedGetter(index).unwrap_or_default()
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue
@@ -262,27 +294,24 @@ impl CSSStyleDeclarationMethods for CSSS
     fn GetPropertyPriority(&self, property: DOMString) -> DOMString {
         let id = if let Ok(id) = PropertyId::parse(property.into()) {
             id
         } else {
             // Unkwown property
             return DOMString::new()
         };
 
-        if let Some(ref lock) = self.owner.style_attribute() {
-            if lock.read().property_priority(&id).important() {
+        self.owner.with_block(|ref pdb| {
+            if pdb.property_priority(&id).important() {
                 DOMString::from("important")
             } else {
                 // Step 4
                 DOMString::new()
             }
-        } else {
-            // No style attribute is like an empty style attribute: no matching declaration.
-            DOMString::new()
-        }
+        })
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-setproperty
     fn SetProperty(&self,
                    property: DOMString,
                    value: DOMString,
                    priority: DOMString)
                    -> ErrorResult {
@@ -299,39 +328,33 @@ impl CSSStyleDeclarationMethods for CSSS
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-setpropertypriority
     fn SetPropertyPriority(&self, property: DOMString, priority: DOMString) -> ErrorResult {
         // Step 1
         if self.readonly {
             return Err(Error::NoModificationAllowed);
         }
 
         // Step 2 & 3
-        let id = if let Ok(id) = PropertyId::parse(property.into()) {
-            id
-        } else {
-            // Unkwown property
-            return Ok(())
+        let id = match PropertyId::parse(property.into()) {
+            Ok(id) => id,
+            Err(..) => return Ok(()), // Unkwown property
         };
 
         // Step 4
         let importance = match &*priority {
             "" => Importance::Normal,
             p if p.eq_ignore_ascii_case("important") => Importance::Important,
             _ => return Ok(()),
         };
 
-        if let Some(ref lock) = self.owner.style_attribute() {
-            let mut style_attribute = lock.write();
-
+        self.owner.mutate_associated_block(|ref mut pdb, mut changed| {
             // Step 5 & 6
-            style_attribute.set_importance(&id, importance);
+            *changed = pdb.set_importance(&id, importance);
+        });
 
-            self.owner.flush_style(&style_attribute);
-            self.owner.dirty();
-        }
         Ok(())
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-setpropertyvalue
     fn SetPropertyValue(&self, property: DOMString, value: DOMString) -> ErrorResult {
         self.SetProperty(property, value, DOMString::new())
     }
 
@@ -345,35 +368,20 @@ impl CSSStyleDeclarationMethods for CSSS
         let id = if let Ok(id) = PropertyId::parse(property.into()) {
             id
         } else {
             // Unkwown property, cannot be there to remove.
             return Ok(DOMString::new())
         };
 
         let mut string = String::new();
-        let empty = {
-            if let Some(ref lock) = self.owner.style_attribute() {
-                let mut style_attribute = lock.write();
-                // Step 3
-                style_attribute.property_value_to_css(&id, &mut string).unwrap();
-
-                // Step 4 & 5
-                style_attribute.remove_property(&id);
-                self.owner.flush_style(&style_attribute);
-                style_attribute.declarations.is_empty()
-            } else {
-                // No style attribute is like an empty style attribute: nothing to remove.
-                return Ok(DOMString::new())
-            }
-        };
-        if let (&CSSStyleOwner::Element(ref el), true) = (&self.owner, empty) {
-            *el.style_attribute().borrow_mut() = None;
-        }
-        self.owner.dirty();
+        self.owner.mutate_associated_block(|mut pdb, mut changed| {
+            pdb.property_value_to_css(&id, &mut string).unwrap();
+            *changed = pdb.remove_property(&id);
+        });
 
         // Step 6
         Ok(DOMString::from(string))
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-cssfloat
     fn CssFloat(&self) -> DOMString {
         self.GetPropertyValue(DOMString::from("float"))
@@ -381,54 +389,50 @@ impl CSSStyleDeclarationMethods for CSSS
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-cssfloat
     fn SetCssFloat(&self, value: DOMString) -> ErrorResult {
         self.SetPropertyValue(DOMString::from("float"), value)
     }
 
     // https://dev.w3.org/csswg/cssom/#the-cssstyledeclaration-interface
     fn IndexedGetter(&self, index: u32) -> Option<DOMString> {
-        self.owner.style_attribute().as_ref().and_then(|lock| {
-            lock.read().declarations.get(index as usize).map(|entry| {
+        self.owner.with_block(|ref pdb| {
+            pdb.declarations.get(index as usize).map(|entry| {
                 let (ref declaration, importance) = *entry;
                 let mut css = declaration.to_css_string();
                 if importance.important() {
                     css += " !important";
                 }
                 DOMString::from(css)
             })
         })
     }
 
     // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-csstext
     fn CssText(&self) -> DOMString {
-        self.owner.style_attribute().as_ref().map_or(DOMString::new(), |lock|
-            DOMString::from(lock.read().to_css_string()))
+        self.owner.with_block(|ref pdb| {
+            DOMString::from(pdb.to_css_string())
+        })
     }
 
     // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-csstext
     fn SetCssText(&self, value: DOMString) -> ErrorResult {
         let window = self.owner.window();
 
         // Step 1
         if self.readonly {
             return Err(Error::NoModificationAllowed);
         }
 
-        // Step 3
-        let decl_block = parse_style_attribute(&value, &window.get_url(), window.css_error_reporter(),
-                                               ParserContextExtraData::default());
-        if let CSSStyleOwner::Element(ref el) = self.owner {
-            *el.style_attribute().borrow_mut() = if decl_block.declarations.is_empty() {
-                el.set_style_attr(String::new());
-                None // Step 2
-            } else {
-                el.set_style_attr(decl_block.to_css_string());
-                Some(Arc::new(RwLock::new(decl_block)))
-            };
-        }
-        self.owner.dirty();
+        self.owner.mutate_associated_block(|mut pdb, mut _changed| {
+            // Step 3
+            *pdb = parse_style_attribute(&value,
+                                         &self.owner.base_url(),
+                                         window.css_error_reporter(),
+                                         ParserContextExtraData::default());
+        });
+
         Ok(())
     }
 
     // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-_camel_cased_attribute
     css_properties_accessors!(css_properties);
 }
--- a/servo/components/script/dom/cssstylerule.rs
+++ b/servo/components/script/dom/cssstylerule.rs
@@ -1,13 +1,14 @@
 /* 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 dom::bindings::codegen::Bindings::CSSStyleRuleBinding::{self, CSSStyleRuleMethods};
+use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{JS, MutNullableJS, Root};
 use dom::bindings::reflector::{DomObject, reflect_dom_object};
 use dom::bindings::str::DOMString;
 use dom::cssrule::{CSSRule, SpecificCSSRule};
 use dom::cssstyledeclaration::{CSSModificationAccess, CSSStyleDeclaration, CSSStyleOwner};
 use dom::cssstylesheet::CSSStyleSheet;
 use dom::window::Window;
 use parking_lot::RwLock;
@@ -53,15 +54,15 @@ impl SpecificCSSRule for CSSStyleRule {
     }
 }
 
 impl CSSStyleRuleMethods for CSSStyleRule {
     // https://drafts.csswg.org/cssom/#dom-cssstylerule-style
     fn Style(&self) -> Root<CSSStyleDeclaration> {
         self.style_decl.or_init(|| {
             CSSStyleDeclaration::new(self.global().as_window(),
-                                     CSSStyleOwner::CSSRule(JS::from_ref(self.global().as_window()),
-                                                                 self.stylerule.read().block.clone()),
+                                     CSSStyleOwner::CSSRule(JS::from_ref(self.upcast()),
+                                                            self.stylerule.read().block.clone()),
                                      None,
                                      CSSModificationAccess::ReadWrite)
         })
     }
 }
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -187,43 +187,58 @@ impl PropertyDeclarationBlock {
 
         self.declarations.push((declaration, importance));
         if importance.important() {
             self.important_count += 1;
         }
     }
 
     /// Set the declaration importance for a given property, if found.
-    pub fn set_importance(&mut self, property: &PropertyId, new_importance: Importance) {
+    ///
+    /// Returns whether any declaration was updated.
+    pub fn set_importance(&mut self, property: &PropertyId, new_importance: Importance) -> bool {
+        let mut updated_at_least_one = false;
         for &mut (ref declaration, ref mut importance) in &mut self.declarations {
             if declaration.id().is_or_is_longhand_of(property) {
                 match (*importance, new_importance) {
                     (Importance::Normal, Importance::Important) => {
                         self.important_count += 1;
                     }
                     (Importance::Important, Importance::Normal) => {
                         self.important_count -= 1;
                     }
-                    _ => {}
+                    _ => {
+                        continue;
+                    }
                 }
+                updated_at_least_one = true;
                 *importance = new_importance;
             }
         }
+        updated_at_least_one
     }
 
     /// https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-removeproperty
-    pub fn remove_property(&mut self, property: &PropertyId) {
+    ///
+    /// Returns whether any declaration was actually removed.
+    pub fn remove_property(&mut self, property: &PropertyId) -> bool {
         let important_count = &mut self.important_count;
+        let mut removed_at_least_one = false;
         self.declarations.retain(|&(ref declaration, importance)| {
             let remove = declaration.id().is_or_is_longhand_of(property);
-            if remove && importance.important() {
-                *important_count -= 1
+            if remove {
+                removed_at_least_one = true;
+                if importance.important() {
+                    *important_count -= 1
+                }
             }
             !remove
-        })
+        });
+
+        removed_at_least_one
     }
 
     /// Take a declaration block known to contain a single property and serialize it.
     pub fn single_value_to_css<W>(&self, property: &PropertyId, dest: &mut W) -> fmt::Result
         where W: fmt::Write,
     {
         match property.as_shorthand() {
             Err(_longhand_or_custom) => {