Use PropertyId instead of Atom for CSSStyleDeclaration::get_computed_style

components/atoms/static_atoms.txt

@@ -32,21 +32,6 @@ number
 
 dir
 
-bottom
-top
-left
-right
-width
-height
-margin-bottom
-margin-top
-margin-left
-margin-right
-padding-bottom
-padding-top
-padding-left
-padding-right
-
 DOMContentLoaded
 select
 input

components/layout/query.rs

@@ -24,7 +24,6 @@ use script_layout_interface::wrapper_traits::{LayoutNode, ThreadSafeLayoutElemen
 use script_traits::LayoutMsg as ConstellationMsg;
 use script_traits::UntrustedNodeAddress;
 use sequential;
-use servo_atoms::Atom;
 use std::cmp::{min, max};
 use std::ops::Deref;
 use std::sync::{Arc, Mutex};
@@ -32,8 +31,8 @@ use style::computed_values;
 use style::context::StyleContext;
 use style::dom::TElement;
 use style::logical_geometry::{WritingMode, BlockFlowDirection, InlineBaseDirection};
+use style::properties::{style_structs, PropertyId, PropertyDeclarationId, LonghandId};
 use style::properties::longhands::{display, position};
-use style::properties::style_structs;
 use style::selector_parser::PseudoElement;
 use style::stylist::Stylist;
 use style_traits::ToCss;
@@ -75,7 +74,7 @@ pub struct LayoutThreadData {
     pub scroll_area_response: Rect<i32>,
 
     /// A queued response for the resolved style property of an element.
-    pub resolved_style_response: Option<String>,
+    pub resolved_style_response: String,
 
     /// A queued response for the offset parent/rect of a node.
     pub offset_parent_response: OffsetParentResponse,
@@ -628,8 +627,8 @@ pub fn process_node_scroll_area_request< N: LayoutNode>(requested_node: N, layou
 pub fn process_resolved_style_request<'a, N, C>(requested_node: N,
                                                 style_context: &'a C,
                                                 pseudo: &Option<PseudoElement>,
-                                                property: &Atom,
+                                                property: &PropertyId,
-                                                layout_root: &mut Flow) -> Option<String>
+                                                layout_root: &mut Flow) -> String
     where N: LayoutNode,
           C: StyleContext<'a>
 {
@@ -667,13 +666,25 @@ pub fn process_resolved_style_request<'a, N, C>(requested_node: N,
             // The pseudo doesn't exist, return nothing.  Chrome seems to query
             // the element itself in this case, Firefox uses the resolved value.
             // https://www.w3.org/Bugs/Public/show_bug.cgi?id=29006
-            return None;
+            return String::new();
         }
         Some(layout_el) => layout_el
     };
 
     let style = &*layout_el.resolved_style();
 
+    let longhand_id = match *property {
+        PropertyId::Longhand(id) => id,
+
+        // Firefox returns blank strings for the computed value of shorthands,
+        // so this should be web-compatible.
+        PropertyId::Shorthand(_) => return String::new(),
+
+        PropertyId::Custom(ref name) => {
+            return style.computed_value_to_string(PropertyDeclarationId::Custom(name))
+        }
+    };
+
     // Clear any temporarily-resolved data to maintain our invariants. See the comment
     // at the top of this function.
     display_none_root.map(|r| clear_descendant_data(r, &|e| e.as_node().clear_data()));
@@ -697,7 +708,7 @@ pub fn process_resolved_style_request<'a, N, C>(requested_node: N,
             layout_el: <N::ConcreteThreadSafeLayoutNode as ThreadSafeLayoutNode>::ConcreteThreadSafeLayoutElement,
             layout_root: &mut Flow,
             requested_node: N,
-            property: &Atom) -> Option<String> {
+            longhand_id: LonghandId) -> String {
         let maybe_data = layout_el.borrow_layout_data();
         let position = maybe_data.map_or(Point2D::zero(), |data| {
             match (*data).flow_construction_result {
@@ -708,13 +719,13 @@ pub fn process_resolved_style_request<'a, N, C>(requested_node: N,
                 _ => Point2D::zero()
             }
         });
-        let property = match *property {
+        let property = match longhand_id {
-            atom!("bottom") => PositionProperty::Bottom,
+            LonghandId::Bottom => PositionProperty::Bottom,
-            atom!("top") => PositionProperty::Top,
+            LonghandId::Top => PositionProperty::Top,
-            atom!("left") => PositionProperty::Left,
+            LonghandId::Left => PositionProperty::Left,
-            atom!("right") => PositionProperty::Right,
+            LonghandId::Right => PositionProperty::Right,
-            atom!("width") => PositionProperty::Width,
+            LonghandId::Width => PositionProperty::Width,
-            atom!("height") => PositionProperty::Height,
+            LonghandId::Height => PositionProperty::Height,
             _ => unreachable!()
         };
         let mut iterator =
@@ -723,27 +734,25 @@ pub fn process_resolved_style_request<'a, N, C>(requested_node: N,
                                                              position);
         sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root,
                                                                     &mut iterator);
-        iterator.result.map(|r| r.to_css_string())
+        iterator.result.map(|r| r.to_css_string()).unwrap_or(String::new())
     }
 
     // TODO: we will return neither the computed nor used value for margin and padding.
-    // Firefox returns blank strings for the computed value of shorthands,
+    match longhand_id {
-    // so this should be web-compatible.
+        LonghandId::MarginBottom | LonghandId::MarginTop |
-    match *property {
+        LonghandId::MarginLeft | LonghandId::MarginRight |
-        atom!("margin-bottom") | atom!("margin-top") |
+        LonghandId::PaddingBottom | LonghandId::PaddingTop |
-        atom!("margin-left") | atom!("margin-right") |
+        LonghandId::PaddingLeft | LonghandId::PaddingRight
-        atom!("padding-bottom") | atom!("padding-top") |
-        atom!("padding-left") | atom!("padding-right")
         if applies && style.get_box().display != display::computed_value::T::none => {
-            let (margin_padding, side) = match *property {
+            let (margin_padding, side) = match longhand_id {
-                atom!("margin-bottom") => (MarginPadding::Margin, Side::Bottom),
+                LonghandId::MarginBottom => (MarginPadding::Margin, Side::Bottom),
-                atom!("margin-top") => (MarginPadding::Margin, Side::Top),
+                LonghandId::MarginTop => (MarginPadding::Margin, Side::Top),
-                atom!("margin-left") => (MarginPadding::Margin, Side::Left),
+                LonghandId::MarginLeft => (MarginPadding::Margin, Side::Left),
-                atom!("margin-right") => (MarginPadding::Margin, Side::Right),
+                LonghandId::MarginRight => (MarginPadding::Margin, Side::Right),
-                atom!("padding-bottom") => (MarginPadding::Padding, Side::Bottom),
+                LonghandId::PaddingBottom => (MarginPadding::Padding, Side::Bottom),
-                atom!("padding-top") => (MarginPadding::Padding, Side::Top),
+                LonghandId::PaddingTop => (MarginPadding::Padding, Side::Top),
-                atom!("padding-left") => (MarginPadding::Padding, Side::Left),
+                LonghandId::PaddingLeft => (MarginPadding::Padding, Side::Left),
-                atom!("padding-right") => (MarginPadding::Padding, Side::Right),
+                LonghandId::PaddingRight => (MarginPadding::Padding, Side::Right),
                 _ => unreachable!()
             };
             let mut iterator =
@@ -753,23 +762,22 @@ pub fn process_resolved_style_request<'a, N, C>(requested_node: N,
                                                                style.writing_mode);
             sequential::iterate_through_flow_tree_fragment_border_boxes(layout_root,
                                                                         &mut iterator);
-            iterator.result.map(|r| r.to_css_string())
+            iterator.result.map(|r| r.to_css_string()).unwrap_or(String::new())
         },
 
-        atom!("bottom") | atom!("top") | atom!("right") |
+        LonghandId::Bottom | LonghandId::Top | LonghandId::Right | LonghandId::Left
-        atom!("left")
         if applies && positioned && style.get_box().display !=
                 display::computed_value::T::none => {
-            used_value_for_position_property(layout_el, layout_root, requested_node, property)
+            used_value_for_position_property(layout_el, layout_root, requested_node, longhand_id)
         }
-        atom!("width") | atom!("height")
+        LonghandId::Width | LonghandId::Height
         if applies && style.get_box().display !=
                 display::computed_value::T::none => {
-            used_value_for_position_property(layout_el, layout_root, requested_node, property)
+            used_value_for_position_property(layout_el, layout_root, requested_node, longhand_id)
         }
         // FIXME: implement used value computation for line-height
-        ref property => {
+        _ => {
-            style.computed_value_to_string(&*property).ok()
+            style.computed_value_to_string(PropertyDeclarationId::Longhand(longhand_id))
         }
     }
 }

components/layout_thread/lib.rs

@@ -465,7 +465,7 @@ impl LayoutThread {
                     scroll_root_id_response: None,
                     scroll_area_response: Rect::zero(),
                     overflow_response: NodeOverflowResponse(None),
-                    resolved_style_response: None,
+                    resolved_style_response: String::new(),
                     offset_parent_response: OffsetParentResponse::empty(),
                     margin_style_response: MarginStyleResponse::empty(),
                     stacking_context_scroll_offsets: HashMap::new(),
@@ -1017,7 +1017,7 @@ impl LayoutThread {
                         rw_data.scroll_root_id_response = None;
                     },
                     ReflowQueryType::ResolvedStyleQuery(_, _, _) => {
-                        rw_data.resolved_style_response = None;
+                        rw_data.resolved_style_response = String::new();
                     },
                     ReflowQueryType::OffsetParentQuery(_) => {
                         rw_data.offset_parent_response = OffsetParentResponse::empty();

components/script/dom/cssstyledeclaration.rs

@@ -12,7 +12,6 @@ use dom::element::Element;
 use dom::node::{Node, NodeDamage, window_from_node};
 use dom::window::Window;
 use parking_lot::RwLock;
-use servo_atoms::Atom;
 use std::ascii::AsciiExt;
 use std::sync::Arc;
 use style::parser::ParserContextExtraData;
@@ -74,12 +73,12 @@ impl CSSStyleDeclaration {
                            CSSStyleDeclarationBinding::Wrap)
     }
 
-    fn get_computed_style(&self, property: &Atom) -> Option<DOMString> {
+    fn get_computed_style(&self, property: PropertyId) -> DOMString {
         let node = self.owner.upcast::<Node>();
         if !node.is_in_doc() {
             // TODO: Node should be matched against the style rules of this window.
             // Firefox is currently the only browser to implement this.
-            return None;
+            return DOMString::new();
         }
         let addr = node.to_trusted_node_address();
         window_from_node(&*self.owner).resolved_style_query(addr, self.pseudo.clone(), property)
@@ -103,14 +102,7 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue
-    fn GetPropertyValue(&self, mut property: DOMString) -> DOMString {
+    fn GetPropertyValue(&self, property: DOMString) -> DOMString {
-        if self.readonly {
-            // Readonly style declarations are used for getComputedStyle.
-            property.make_ascii_lowercase();
-            let property = Atom::from(property);
-            return self.get_computed_style(&property).unwrap_or(DOMString::new());
-        }
-
         let id = if let Ok(id) = PropertyId::parse(property.into()) {
             id
         } else {
@@ -118,6 +110,11 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
             return DOMString::new()
         };
 
+        if self.readonly {
+            // Readonly style declarations are used for getComputedStyle.
+            return self.get_computed_style(id);
+        }
+
         let style_attribute = self.owner.style_attribute().borrow();
         let style_attribute = if let Some(ref lock) = *style_attribute {
             lock.read()

components/script/dom/window.rs

@@ -92,6 +92,7 @@ use std::sync::mpsc::TryRecvError::{Disconnected, Empty};
 use style::context::ReflowGoal;
 use style::error_reporting::ParseErrorReporter;
 use style::media_queries;
+use style::properties::PropertyId;
 use style::properties::longhands::overflow_x;
 use style::selector_parser::PseudoElement;
 use style::str::HTML_SPACE_CHARACTERS;
@@ -1295,16 +1296,16 @@ impl Window {
     }
 
     pub fn resolved_style_query(&self,
-                            element: TrustedNodeAddress,
+                                element: TrustedNodeAddress,
-                            pseudo: Option<PseudoElement>,
+                                pseudo: Option<PseudoElement>,
-                            property: &Atom) -> Option<DOMString> {
+                                property: PropertyId) -> DOMString {
         if !self.reflow(ReflowGoal::ForScriptQuery,
-                        ReflowQueryType::ResolvedStyleQuery(element, pseudo, property.clone()),
+                        ReflowQueryType::ResolvedStyleQuery(element, pseudo, property),
                         ReflowReason::Query) {
-            return None;
+            return DOMString::new();
         }
         let ResolvedStyleResponse(resolved) = self.layout_rpc.resolved_style();
-        resolved.map(DOMString::from)
+        DOMString::from(resolved)
     }
 
     pub fn offset_parent_query(&self, node: TrustedNodeAddress) -> (Option<Root<Element>>, Rect<Au>) {

components/script_layout_interface/message.rs

@@ -14,11 +14,11 @@ use profile_traits::mem::ReportsChan;
 use rpc::LayoutRPC;
 use script_traits::{ConstellationControlMsg, LayoutControlMsg};
 use script_traits::{LayoutMsg as ConstellationMsg, StackingContextScrollState, WindowSizeData};
-use servo_atoms::Atom;
 use servo_url::ServoUrl;
 use std::sync::Arc;
 use std::sync::mpsc::{Receiver, Sender};
 use style::context::ReflowGoal;
+use style::properties::PropertyId;
 use style::selector_parser::PseudoElement;
 use style::stylesheets::Stylesheet;
 
@@ -97,7 +97,7 @@ pub enum ReflowQueryType {
     NodeScrollRootIdQuery(TrustedNodeAddress),
     NodeGeometryQuery(TrustedNodeAddress),
     NodeScrollGeometryQuery(TrustedNodeAddress),
-    ResolvedStyleQuery(TrustedNodeAddress, Option<PseudoElement>, Atom),
+    ResolvedStyleQuery(TrustedNodeAddress, Option<PseudoElement>, PropertyId),
     OffsetParentQuery(TrustedNodeAddress),
     MarginStyleQuery(TrustedNodeAddress),
 }

components/script_layout_interface/rpc.rs

@@ -57,7 +57,7 @@ pub struct HitTestResponse {
     pub node_address: Option<UntrustedNodeAddress>,
 }
 
-pub struct ResolvedStyleResponse(pub Option<String>);
+pub struct ResolvedStyleResponse(pub String);
 
 #[derive(Clone)]
 pub struct OffsetParentResponse {

components/style/properties/properties.mako.rs

@@ -599,6 +599,12 @@ pub enum PropertyId {
     Custom(::custom_properties::Name),
 }
 
+impl fmt::Debug for PropertyId {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        self.to_css(formatter)
+    }
+}
+
 impl ToCss for PropertyId {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         match *self {
@@ -1347,18 +1353,21 @@ impl ComputedValues {
         false
     }
 
-    pub fn computed_value_to_string(&self, name: &str) -> Result<String, ()> {
+    pub fn computed_value_to_string(&self, property: PropertyDeclarationId) -> String {
-        match name {
+        match property {
             % for style_struct in data.active_style_structs():
                 % for longhand in style_struct.longhands:
-                "${longhand.name}" => Ok(self.${style_struct.ident}.${longhand.ident}.to_css_string()),
+                    PropertyDeclarationId::Longhand(LonghandId::${longhand.camel_case}) => {
+                        self.${style_struct.ident}.${longhand.ident}.to_css_string()
+                    }
                 % endfor
             % endfor
-            _ => {
+            PropertyDeclarationId::Custom(name) => {
-                let name = try!(::custom_properties::parse_name(name));
+                self.custom_properties
-                let map = try!(self.custom_properties.as_ref().ok_or(()));
+                    .as_ref()
-                let value = try!(map.get(&Atom::from(name)).ok_or(()));
+                    .and_then(|map| map.get(name))
-                Ok(value.to_css_string())
+                    .map(|value| value.to_css_string())
+                    .unwrap_or(String::new())
             }
         }
     }

tests/unit/style/stylist.rs

@@ -101,7 +101,7 @@ fn test_insert() {
     let rules_list = get_mock_rules(&[".intro.foo", "#top"]);
     let mut selector_map = SelectorMap::new();
     selector_map.insert(rules_list[1][0].clone());
-    assert_eq!(1, selector_map.id_hash.get(&atom!("top")).unwrap()[0].source_order);
+    assert_eq!(1, selector_map.id_hash.get(&Atom::from("top")).unwrap()[0].source_order);
     selector_map.insert(rules_list[0][0].clone());
     assert_eq!(0, selector_map.class_hash.get(&Atom::from("intro")).unwrap()[0].source_order);
     assert!(selector_map.class_hash.get(&Atom::from("foo")).is_none());