Auto merge of #6741 - servo:fix-setpropertypriority, r=pcwalton

Fix CSSStyleDeclaration::setPropertyPriority and some refactoring

r? @Ms2ger

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6741)
<!-- Reviewable:end -->

components/script/dom/cssstyledeclaration.rs

@@ -16,11 +16,12 @@ use dom::window::{Window, WindowHelpers};
 use util::str::DOMString;
 use selectors::parser::PseudoElement;
 use string_cache::Atom;
-use style::properties::{is_supported_property, longhands_from_shorthand, parse_style_attribute};
+use style::properties::{is_supported_property, longhands_from_shorthand, parse_one_declaration};
 use style::properties::PropertyDeclaration;
 
 use std::ascii::AsciiExt;
 use std::borrow::ToOwned;
+use std::cell::Ref;
 
 // http://dev.w3.org/csswg/cssom/#the-cssstyledeclaration-interface
 #[dom_struct]
@@ -50,7 +51,7 @@ macro_rules! css_properties(
     );
 );
 
-fn serialize_list(list: &Vec<PropertyDeclaration>) -> DOMString {
+fn serialize_list(list: &[Ref<PropertyDeclaration>]) -> DOMString {
     list.iter().fold(String::new(), |accum, ref declaration| {
         accum + &declaration.value() + " "
     })
@@ -78,25 +79,24 @@ impl CSSStyleDeclaration {
 }
 
 trait PrivateCSSStyleDeclarationHelpers {
-    fn get_declaration(self, property: &Atom) -> Option<PropertyDeclaration>;
+    fn get_declaration(&self, property: &Atom) -> Option<Ref<PropertyDeclaration>>;
-    fn get_important_declaration(self, property: &Atom) -> Option<PropertyDeclaration>;
+    fn get_important_declaration(&self, property: &Atom) -> Option<Ref<PropertyDeclaration>>;
-    fn get_computed_style(self, property: &Atom) -> Option<DOMString>;
 }
 
-impl<'a> PrivateCSSStyleDeclarationHelpers for &'a CSSStyleDeclaration {
+impl PrivateCSSStyleDeclarationHelpers for HTMLElement {
-    fn get_declaration(self, property: &Atom) -> Option<PropertyDeclaration> {
+    fn get_declaration(&self, property: &Atom) -> Option<Ref<PropertyDeclaration>> {
-        let owner = self.owner.root();
+        let element = ElementCast::from_ref(self);
-        let element = ElementCast::from_ref(owner.r());
+        element.get_inline_style_declaration(property)
-        element.get_inline_style_declaration(property).map(|decl| decl.clone())
     }
 
-    fn get_important_declaration(self, property: &Atom) -> Option<PropertyDeclaration> {
+    fn get_important_declaration(&self, property: &Atom) -> Option<Ref<PropertyDeclaration>> {
-        let owner = self.owner.root();
+        let element = ElementCast::from_ref(self);
-        let element = ElementCast::from_ref(owner.r());
+        element.get_important_inline_style_declaration(property)
-        element.get_important_inline_style_declaration(property).map(|decl| decl.clone())
     }
+}
 
-    fn get_computed_style(self, property: &Atom) -> Option<DOMString> {
+impl CSSStyleDeclaration {
+    fn get_computed_style(&self, property: &Atom) -> Option<DOMString> {
         let owner = self.owner.root();
         let node = NodeCast::from_ref(owner.r());
         if !node.is_in_doc() {
@@ -144,6 +144,8 @@ impl<'a> CSSStyleDeclarationMethods for &'a CSSStyleDeclaration {
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue
     fn GetPropertyValue(self, property: DOMString) -> DOMString {
+        let owner = self.owner.root();
+
         // Step 1
         let property = Atom::from_slice(&property.to_ascii_lowercase());
 
@@ -161,7 +163,7 @@ impl<'a> CSSStyleDeclarationMethods for &'a CSSStyleDeclaration {
             // Step 2.2
             for longhand in longhand_properties.iter() {
                 // Step 2.2.1
-                let declaration = self.get_declaration(&Atom::from_slice(&longhand));
+                let declaration = owner.get_declaration(&Atom::from_slice(&longhand));
 
                 // Step 2.2.2 & 2.2.3
                 match declaration {
@@ -175,11 +177,12 @@ impl<'a> CSSStyleDeclarationMethods for &'a CSSStyleDeclaration {
         }
 
         // Step 3 & 4
-        if let Some(ref declaration) = self.get_declaration(&property) {
+        // FIXME: redundant let binding https://github.com/rust-lang/rust/issues/22252
-            declaration.value()
+        let result = match owner.get_declaration(&property) {
-        } else {
+            Some(declaration) => declaration.value(),
-            "".to_owned()
+            None => "".to_owned(),
-        }
+        };
+        result
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertypriority
@@ -192,14 +195,18 @@ impl<'a> CSSStyleDeclarationMethods for &'a CSSStyleDeclaration {
         if let Some(longhand_properties) = longhand_properties {
             // Step 2.1 & 2.2 & 2.3
             if longhand_properties.iter()
-                                  .map(|longhand| self.GetPropertyPriority(longhand.clone()))
+                                  .map(|&longhand| self.GetPropertyPriority(longhand.to_owned()))
                                   .all(|priority| priority == "important") {
 
                 return "important".to_owned();
             }
         // Step 3
-        } else if self.get_important_declaration(&property).is_some() {
+        } else {
-            return "important".to_owned();
+            // FIXME: extra let binding https://github.com/rust-lang/rust/issues/22323
+            let owner = self.owner.root();
+            if owner.get_important_declaration(&property).is_some() {
+                return "important".to_owned();
+            }
         }
 
         // Step 4
@@ -228,37 +235,31 @@ impl<'a> CSSStyleDeclarationMethods for &'a CSSStyleDeclaration {
         }
 
         // Step 5
-        let priority = priority.to_ascii_lowercase();
+        let priority = match &*priority {
-        if priority != "!important" && !priority.is_empty() {
+            "" => StylePriority::Normal,
-            return Ok(());
+            p if p.eq_ignore_ascii_case("important") => StylePriority::Important,
-        }
+            _ => return Ok(()),
+        };
 
         // Step 6
-        let mut synthesized_declaration = property;
-        synthesized_declaration.push_str(": ");
-        synthesized_declaration.push_str(&value);
-
         let owner = self.owner.root();
         let window = window_from_node(owner.r());
-        let decl_block = parse_style_attribute(&synthesized_declaration, &window.r().get_url());
+        let declarations = parse_one_declaration(&property, &value, &window.r().get_url());
 
         // Step 7
-        if decl_block.normal.len() == 0 {
+        let declarations = if let Ok(declarations) = declarations {
+            declarations
+        } else {
             return Ok(());
-        }
+        };
 
         let owner = self.owner.root();
         let element = ElementCast::from_ref(owner.r());
 
         // Step 8
-        for decl in decl_block.normal.iter() {
+        for decl in declarations {
             // Step 9
-            let style_priority = if priority.is_empty() {
+            element.update_inline_style(decl, priority);
-                StylePriority::Normal
-            } else {
-                StylePriority::Important
-            };
-            element.update_inline_style(decl.clone(), style_priority);
         }
 
         let document = document_from_node(element);
@@ -274,34 +275,25 @@ impl<'a> CSSStyleDeclarationMethods for &'a CSSStyleDeclaration {
             return Err(Error::NoModificationAllowed);
         }
 
-        // Step 2
+        // Step 2 & 3
-        let property = property.to_ascii_lowercase();
-
-        // Step 3
         if !is_supported_property(&property) {
             return Ok(());
         }
 
         // Step 4
-        let priority = priority.to_ascii_lowercase();
+        let priority = match &*priority {
-        if priority != "important" && !priority.is_empty() {
+            "" => StylePriority::Normal,
-            return Ok(());
+            p if p.eq_ignore_ascii_case("important") => StylePriority::Important,
-        }
+            _ => return Ok(()),
+        };
 
         let owner = self.owner.root();
-        let window = window_from_node(owner.r());
-        let decl_block = parse_style_attribute(&property, &window.r().get_url());
         let element = ElementCast::from_ref(owner.r());
 
-        // Step 5
+        // Step 5 & 6
-        for decl in decl_block.normal.iter() {
+        match longhands_from_shorthand(&property) {
-            // Step 6
+            Some(properties) => element.set_inline_style_property_priority(properties, priority),
-            let style_priority = if priority.is_empty() {
+            None => element.set_inline_style_property_priority(&[&*property], priority)
-                StylePriority::Normal
-            } else {
-                StylePriority::Important
-            };
-            element.update_inline_style(decl.clone(), style_priority);
         }
 
         let document = document_from_node(element);
@@ -328,21 +320,18 @@ impl<'a> CSSStyleDeclarationMethods for &'a CSSStyleDeclaration {
         // Step 3
         let value = self.GetPropertyValue(property.clone());
 
-        let longhand_properties = longhands_from_shorthand(&property);
+        let owner = self.owner.root();
-        match longhand_properties {
+        let elem = ElementCast::from_ref(owner.r());
+
+        match longhands_from_shorthand(&property) {
+            // Step 4
             Some(longhands) => {
-                // Step 4
                 for longhand in longhands.iter() {
-                    try!(self.RemoveProperty(longhand.clone()));
+                    elem.remove_inline_style_property(longhand)
                 }
             }
-
+            // Step 5
-            None => {
+            None => elem.remove_inline_style_property(&property)
-                // Step 5
-                let owner = self.owner.root();
-                let elem = ElementCast::from_ref(owner.r());
-                elem.remove_inline_style_property(property)
-            }
         }
 
         // Step 6

components/script/dom/element.rs

@@ -564,7 +564,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
     }
 }
 
-#[derive(PartialEq)]
+#[derive(PartialEq, Eq, Copy, Clone)]
 pub enum StylePriority {
     Important,
     Normal,
@@ -581,10 +581,11 @@ pub trait ElementHelpers<'a> {
     fn style_attribute(self) -> &'a DOMRefCell<Option<PropertyDeclarationBlock>>;
     fn summarize(self) -> Vec<AttrInfo>;
     fn is_void(self) -> bool;
-    fn remove_inline_style_property(self, property: DOMString);
+    fn remove_inline_style_property(self, property: &str);
     fn update_inline_style(self, property_decl: PropertyDeclaration, style_priority: StylePriority);
-    fn get_inline_style_declaration(self, property: &Atom) -> Option<PropertyDeclaration>;
+    fn set_inline_style_property_priority(self, properties: &[&str], style_priority: StylePriority);
-    fn get_important_inline_style_declaration(self, property: &Atom) -> Option<PropertyDeclaration>;
+    fn get_inline_style_declaration(self, property: &Atom) -> Option<Ref<'a, PropertyDeclaration>>;
+    fn get_important_inline_style_declaration(self, property: &Atom) -> Option<Ref<'a, PropertyDeclaration>>;
     fn serialize(self, traversal_scope: TraversalScope) -> Fallible<DOMString>;
     fn get_root_element(self) -> Root<Element>;
     fn lookup_prefix(self, namespace: Namespace) -> Option<DOMString>;
@@ -652,14 +653,14 @@ impl<'a> ElementHelpers<'a> for &'a Element {
         }
     }
 
-    fn remove_inline_style_property(self, property: DOMString) {
+    fn remove_inline_style_property(self, property: &str) {
         let mut inline_declarations = self.style_attribute.borrow_mut();
         if let &mut Some(ref mut declarations) = &mut *inline_declarations {
             let index = declarations.normal
                                     .iter()
                                     .position(|decl| decl.name() == property);
             if let Some(index) = index {
-                Arc::make_unique(&mut declarations.normal).remove(index);
+                Arc::get_mut(&mut declarations.normal).unwrap().remove(index);
                 return;
             }
 
@@ -667,7 +668,7 @@ impl<'a> ElementHelpers<'a> for &'a Element {
                                     .iter()
                                     .position(|decl| decl.name() == property);
             if let Some(index) = index {
-                Arc::make_unique(&mut declarations.important).remove(index);
+                Arc::get_mut(&mut declarations.important).unwrap().remove(index);
                 return;
             }
         }
@@ -677,10 +678,14 @@ impl<'a> ElementHelpers<'a> for &'a Element {
         let mut inline_declarations = self.style_attribute().borrow_mut();
         if let &mut Some(ref mut declarations) = &mut *inline_declarations {
             let existing_declarations = if style_priority == StylePriority::Important {
-                Arc::make_unique(&mut declarations.important)
+                &mut declarations.important
             } else {
-                Arc::make_unique(&mut declarations.normal)
+                &mut declarations.normal
             };
+            // Element is the only owner of the Arc’s for its style attribute,
+            // except during selector matching.
+            // But selector matching does not run concurrently with script.
+            let existing_declarations = Arc::get_mut(existing_declarations).unwrap();
 
             for declaration in existing_declarations.iter_mut() {
                 if declaration.name() == property_decl.name() {
@@ -704,24 +709,49 @@ impl<'a> ElementHelpers<'a> for &'a Element {
         });
     }
 
-    fn get_inline_style_declaration(self, property: &Atom) -> Option<PropertyDeclaration> {
+    fn set_inline_style_property_priority(self, properties: &[&str], style_priority: StylePriority) {
-        let inline_declarations = self.style_attribute.borrow();
+        let mut inline_declarations = self.style_attribute().borrow_mut();
-        inline_declarations.as_ref().and_then(|declarations| {
+        if let &mut Some(ref mut declarations) = &mut *inline_declarations {
-            declarations.normal
+            let (from, to) = if style_priority == StylePriority::Important {
-                        .iter()
+                (&mut declarations.normal, &mut declarations.important)
-                        .chain(declarations.important.iter())
+            } else {
-                        .find(|decl| decl.matches(&property))
+                (&mut declarations.normal, &mut declarations.important)
-                        .map(|decl| decl.clone())
+            };
+            // Element is the only owner of the Arc’s for its style attribute,
+            // except during selector matching.
+            // But selector matching does not run concurrently with script.
+            let from = Arc::get_mut(from).unwrap();
+            let to = Arc::get_mut(to).unwrap();
+            let mut new_from = Vec::new();
+            for declaration in from.drain(..) {
+                if properties.contains(&declaration.name()) {
+                    to.push(declaration)
+                } else {
+                    new_from.push(declaration)
+                }
+            }
+            mem::replace(from, new_from);
+        }
+    }
+
+    fn get_inline_style_declaration(self, property: &Atom) -> Option<Ref<'a, PropertyDeclaration>> {
+        Ref::filter_map(self.style_attribute.borrow(), |inline_declarations| {
+            inline_declarations.as_ref().and_then(|declarations| {
+                declarations.normal
+                            .iter()
+                            .chain(declarations.important.iter())
+                            .find(|decl| decl.matches(&property))
+            })
         })
     }
 
-    fn get_important_inline_style_declaration(self, property: &Atom) -> Option<PropertyDeclaration> {
+    fn get_important_inline_style_declaration(self, property: &Atom) -> Option<Ref<'a, PropertyDeclaration>> {
-        let inline_declarations = self.style_attribute.borrow();
+        Ref::filter_map(self.style_attribute.borrow(), |inline_declarations| {
-        inline_declarations.as_ref().and_then(|declarations| {
+            inline_declarations.as_ref().and_then(|declarations| {
-            declarations.important
+                declarations.important
-                        .iter()
+                            .iter()
-                        .find(|decl| decl.matches(&property))
+                            .find(|decl| decl.matches(&property))
-                        .map(|decl| decl.clone())
+            })
         })
     }
 

components/script/lib.rs

@@ -10,6 +10,7 @@
 #![feature(borrow_state)]
 #![feature(box_raw)]
 #![feature(box_syntax)]
+#![feature(cell_extras)]
 #![feature(core)]
 #![feature(core_intrinsics)]
 #![feature(custom_attribute)]

components/style/build.rs

@@ -5,12 +5,12 @@
 use std::env;
 use std::fs::File;
 use std::io::Write;
-use std::process::{Command, Stdio};
+use std::process::{Command, Stdio, exit};
 use std::path::Path;
 
 
 fn main() {
-    let python = if Command::new("python2.7").arg("--version").status().unwrap().success() {
+    let python = if Command::new("python2.7").arg("--version").output().unwrap().status.success() {
         "python2.7"
     } else {
         "python"
@@ -22,14 +22,23 @@ fn main() {
         .env("PYTHONPATH", &mako)
         .env("TEMPLATE", &template)
         .arg("-c")
-        .arg("from os import environ; from mako.template import Template; \
+        .arg(r#"
-              from mako import exceptions; \n\
+import os
-              try:\n    print(Template(filename=environ['TEMPLATE']).render());\n\
+import sys
-              except:\n    print exceptions.html_error_template().render()")
+from mako.template import Template
+from mako import exceptions
+try:
+    print(Template(filename=os.environ['TEMPLATE'], input_encoding='utf8').render().encode('utf8'))
+except:
+    sys.stderr.write(exceptions.text_error_template().render().encode('utf8'))
+    sys.exit(1)
+"#)
         .stderr(Stdio::inherit())
         .output()
         .unwrap();
-    assert!(result.status.success());
+    if !result.status.success() {
+        exit(1)
+    }
     let out = env::var("OUT_DIR").unwrap();
     File::create(&Path::new(&out).join("properties.rs")).unwrap().write_all(&result.stdout).unwrap();
 }

components/style/properties.mako.rs

@@ -5534,6 +5534,15 @@ pub fn parse_style_attribute(input: &str, base_url: &Url) -> PropertyDeclaration
     parse_property_declaration_list(&context, &mut Parser::new(input))
 }
 
+pub fn parse_one_declaration(name: &str, input: &str, base_url: &Url)
+                             -> Result<Vec<PropertyDeclaration>, ()> {
+    let context = ParserContext::new(Origin::Author, base_url);
+    let mut results = vec![];
+    match PropertyDeclaration::parse(name, &context, &mut Parser::new(input), &mut results) {
+        PropertyDeclarationParseResult::ValidOrIgnoredDeclaration => Ok(results),
+        _ => Err(())
+    }
+}
 
 struct PropertyDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
@@ -5574,9 +5583,9 @@ pub fn parse_property_declaration_list(context: &ParserContext, input: &mut Pars
         match declaration {
             Ok((results, important)) => {
                 if important {
-                    important_declarations.push_all(&results);
+                    important_declarations.extend(results);
                 } else {
-                    normal_declarations.push_all(&results);
+                    normal_declarations.extend(results);
                 }
             }
             Err(range) => {
@@ -5660,7 +5669,7 @@ impl<T: ToCss> DeclaredValue<T> {
     }
 }
 
-#[derive(Clone, PartialEq)]
+#[derive(PartialEq)]
 pub enum PropertyDeclaration {
     % for property in LONGHANDS:
         ${property.camel_case}(DeclaredValue<longhands::${property.ident}::SpecifiedValue>),
@@ -6543,11 +6552,12 @@ pub fn modify_style_for_inline_sides(style: &mut Arc<ComputedValues>,
 }
 
 pub fn is_supported_property(property: &str) -> bool {
-    match property {
+    match_ignore_ascii_case! { property,
-        % for property in SHORTHANDS + LONGHANDS:
+        % for property in SHORTHANDS + LONGHANDS[:-1]:
             "${property.name}" => true,
         % endfor
-        _ => false,
+        "${LONGHANDS[-1].name}" => true
+        _ => false
     }
 }
 
@@ -6579,16 +6589,24 @@ macro_rules! longhand_properties_idents {
     }
 }
 
-pub fn longhands_from_shorthand(shorthand: &str) -> Option<Vec<String>> {
+// Extra space here because < seems to be removed by Mako when immediately followed by &.
-    match shorthand {
+//                                                         ↓
-        % for property in SHORTHANDS:
+pub fn longhands_from_shorthand(shorthand: &str) -> Option< &'static [&'static str]> {
-            "${property.name}" => Some(vec!(
+    % for property in SHORTHANDS:
+        static ${property.ident.upper()}: &'static [&'static str] = &[
             % for sub in property.sub_properties:
-                "${sub.name}".to_owned(),
+                "${sub.name}",
             % endfor
-            )),
+        ];
+    % endfor
+    match_ignore_ascii_case!{ shorthand,
+        % for property in SHORTHANDS[:-1]:
+            "${property.name}" => Some(${property.ident.upper()}),
         % endfor
-        _ => None,
+        % for property in SHORTHANDS[-1:]:
+            "${property.name}" => Some(${property.ident.upper()})
+        % endfor
+        _ => None
     }
 }
 

tests/wpt/mozilla/meta/MANIFEST.json

@@ -75,6 +75,18 @@
             "url": "/_mozilla/css/class-namespaces.html"
           }
         ],
+        "css/setpropertypriority.html": [
+          {
+            "path": "css/setpropertypriority.html",
+            "references": [
+              [
+                "/_mozilla/css/setpropertypriority_ref.html",
+                "=="
+              ]
+            ],
+            "url": "/_mozilla/css/setpropertypriority.html"
+          }
+        ],
         "mozilla/canvas/drawimage_html_image_1.html": [
           {
             "path": "mozilla/canvas/drawimage_html_image_1.html",
@@ -800,6 +812,18 @@
           "url": "/_mozilla/css/class-namespaces.html"
         }
       ],
+      "css/setpropertypriority.html": [
+        {
+          "path": "css/setpropertypriority.html",
+          "references": [
+            [
+              "/_mozilla/css/setpropertypriority_ref.html",
+              "=="
+            ]
+          ],
+          "url": "/_mozilla/css/setpropertypriority.html"
+        }
+      ],
       "mozilla/canvas/drawimage_html_image_1.html": [
         {
           "path": "mozilla/canvas/drawimage_html_image_1.html",

tests/wpt/mozilla/tests/css/setpropertypriority.html

@@ -0,0 +1,9 @@
+<link rel=match href=setpropertypriority_ref.html>
+<body>
+<p id="hello" style="color: green">This text should be green.</p>
+<style>
+  #hello { color: red !important }
+</style>
+<script>
+  document.getElementById("hello").style.setPropertyPriority("color", "important");
+</script>

tests/wpt/mozilla/tests/css/setpropertypriority_ref.html

@@ -0,0 +1 @@
+<p style="color: green">This text should be green.