Bug 582228. Speed up getting style attributes. r=bzbarsky@mit.edu, a=dbaron@mozilla.com

content/base/src/nsAttrValue.cpp

@@ -80,10 +80,10 @@ nsAttrValue::nsAttrValue(const nsAString& aValue)
   SetTo(aValue);
 }
 
-nsAttrValue::nsAttrValue(nsICSSStyleRule* aValue)
+nsAttrValue::nsAttrValue(nsICSSStyleRule* aValue, const nsAString* aSerialized)
     : mBits(0)
 {
-  SetTo(aValue);
+  SetTo(aValue, aSerialized);
 }
 
 #ifdef MOZ_SVG
@@ -313,12 +313,13 @@ nsAttrValue::SetTo(PRInt16 aInt)
 }
 
 void
-nsAttrValue::SetTo(nsICSSStyleRule* aValue)
+nsAttrValue::SetTo(nsICSSStyleRule* aValue, const nsAString* aSerialized)
 {
   if (EnsureEmptyMiscContainer()) {
     MiscContainer* cont = GetMiscContainer();
     NS_ADDREF(cont->mCSSStyleRule = aValue);
     cont->mType = eCSSStyleRule;
+    SetMiscAtomOrString(aSerialized);
   }
 }
 
@@ -431,6 +432,7 @@ nsAttrValue::ToString(nsAString& aResult) const
       if (decl) {
         decl->ToString(aResult);
       }
+      const_cast<nsAttrValue*>(this)->SetMiscAtomOrString(&aResult);
 
       break;
     }
@@ -1260,7 +1262,10 @@ nsAttrValue::SetMiscAtomOrString(const nsAString* aValue)
                "Trying to re-set atom or string!");
   if (aValue) {
     PRUint32 len = aValue->Length();
-    NS_ASSERTION(len, "Empty string?");
+    // We're allowing eCSSStyleRule attributes to store empty strings as it
+    // can be beneficial to store an empty style attribute as a parsed rule.
+    // Add other types as needed.
+    NS_ASSERTION(len || Type() == eCSSStyleRule, "Empty string?");
     MiscContainer* cont = GetMiscContainer();
     if (len <= NS_ATTRVALUE_MAX_STRINGLENGTH_ATOM) {
       nsIAtom* atom = NS_NewAtom(*aValue);

content/base/src/nsAttrValue.h

@@ -98,7 +98,7 @@ public:
   nsAttrValue();
   nsAttrValue(const nsAttrValue& aOther);
   explicit nsAttrValue(const nsAString& aValue);
-  explicit nsAttrValue(nsICSSStyleRule* aValue);
+  nsAttrValue(nsICSSStyleRule* aValue, const nsAString* aSerialized);
 #ifdef MOZ_SVG
   explicit nsAttrValue(nsISVGValue* aValue);
 #endif
@@ -135,7 +135,7 @@ public:
   void SetTo(const nsAttrValue& aOther);
   void SetTo(const nsAString& aValue);
   void SetTo(PRInt16 aInt);
-  void SetTo(nsICSSStyleRule* aValue);
+  void SetTo(nsICSSStyleRule* aValue, const nsAString* aSerialized);
 #ifdef MOZ_SVG
   void SetTo(nsISVGValue* aValue);
 #endif

content/base/src/nsStyledElement.cpp

@@ -174,7 +174,7 @@ nsStyledElement::SetInlineStyleRule(nsICSSStyleRule* aStyleRule, PRBool aNotify)
     modification = !!mAttrsAndChildren.GetAttr(nsGkAtoms::style);
   }
 
-  nsAttrValue attrValue(aStyleRule);
+  nsAttrValue attrValue(aStyleRule, nsnull);
 
   // XXXbz do we ever end up with ADDITION here?  I doubt it.
   PRUint8 modType = modification ?
@@ -322,7 +322,7 @@ nsStyledElement::ParseStyleAttribute(const nsAString& aValue,
                                       NodePrincipal(),
                                       getter_AddRefs(rule));
         if (rule) {
-          aResult.SetTo(rule);
+          aResult.SetTo(rule, &aValue);
           return;
         }
       }

content/base/test/test_bug166235.html

@@ -87,7 +87,7 @@ var originalStrings = [
 var clipboardHTML = [
   '<p id=\"test0\">This text should be copied.</p>',
   '<p id=\"test1\">This text should be copied.</p>',
-  '<p id=\"test2\">This<span style=\"-moz-user-select: text;\"> text should</span> be copied.</p>'
+  '<p id=\"test2\">This<span style=\"-moz-user-select: text\"> text should</span> be copied.</p>'
 ];
 
 // expected results for clipboard text/unicode
@@ -101,7 +101,7 @@ var clipboardUnicode = [
 var innerHTMLStrings = [
   'This text should be copied.',
   'This text should<span style=\"-moz-user-select: none;\"> NOT</span> be copied.',
-  'This<span style=\"-moz-user-select: -moz-none;\"><span style=\"-moz-user-select: text;\"> text should</span> NOT</span> be copied.'
+  'This<span style=\"-moz-user-select: -moz-none;\"><span style=\"-moz-user-select: text\"> text should</span> NOT</span> be copied.'
 ];
 
 // expected results for pasting into a TEXTAREA

content/base/test/test_bug418214.html

@@ -47,13 +47,22 @@ is(m2.getAttribute("style"), "border:: invalid",
 d2.style;
 s2.style;
 m2.style;
-   
-is(d2.getAttribute("style"), "",
-   "Getting .style should parse style on HTML");
-is(s2.getAttribute("style"), "",
-   "Getting .style should parse style on SVG");
+
+is(d2.getAttribute("style"), "border:: invalid",
+   "Getting .style shouldn't affect style attribute on HTML");
+is(s2.getAttribute("style"), "border:: invalid",
+   "Getting .style shouldn't affect style attribute on SVG");
 is(m2.getAttribute("style"), "border:: invalid",
-   "Getting .style shouldn't parse style on MathML");
+   "Getting .style shouldn't affect style attribute on MathML");
+
+d2.style.color = "green";
+s2.style.color = "green";
+is (m2.style, undefined, ".style shouldn't exist on MathML");
+
+is(d2.getAttribute("style"), "color: green;",
+   "Adjusting .style should parse style on HTML");
+is(s2.getAttribute("style"), "color: green;",
+   "Getting .style should parse style on SVG");
 
 d = document.adoptNode(d);
 s = document.adoptNode(s);
@@ -70,12 +79,21 @@ $("display").appendChild(d);
 $("display").appendChild(s);
 $("display").appendChild(m);
 
-is(d.getAttribute("style"), "",
-   "Insertion should parse style on HTML");
-is(s.getAttribute("style"), "",
-   "Insertion should parse style on SVG");
-is(m.getAttribute("style"), "",
-   "Insertion should parse style on MathML");
+is(d.getAttribute("style"), "border:: invalid",
+   "Adopting should not parse style on HTML");
+is(s.getAttribute("style"), "border:: invalid",
+   "Adopting should not parse style on SVG");
+is(m.getAttribute("style"), "border:: invalid",
+   "Adopting should not parse style on MathML");
+
+d.style.color = "green";
+s.style.color = "green";
+is (m.style, undefined, ".style shouldn't exist on MathML");
+
+is(d.getAttribute("style"), "color: green;",
+   "Adjusting .style should parse style on HTML");
+is(s.getAttribute("style"), "color: green;",
+   "Adjusting .style should parse style on SVG");
 
 </script>
 </pre>

content/base/test/test_htmlcopyencoder.html

@@ -49,14 +49,14 @@ function testHtmlCopyEncoder () {
   encoder.init(document, "text/html", de.OutputLFLineBreak | de.OutputSelectionOnly);
   encoder.setSelection(select);
   out = encoder.encodeToString();
-  expected = "<div style=\"display: none;\">\n\n<div id=\"draggable\" ondragstart=\"doDragStartSelection(event)\">This is a <em>draggable</em> bit of text.</div>\n\n</div>";
+  expected = "<div style=\"display: none\">\n\n<div id=\"draggable\" ondragstart=\"doDragStartSelection(event)\">This is a <em>draggable</em> bit of text.</div>\n\n</div>";
   is(out, expected, "test selection");
 
   encoder.init(document, "text/html", de.OutputLFLineBreak | de.OutputAbsoluteLinks | de.OutputEncodeHTMLEntities | de.OutputSelectionOnly | de.OutputRaw);
   encoder.setSelection(select);
   var outContext = {value:''}, outInfo = {value:''};
   out = encoder.encodeToStringWithContext(outContext, outInfo);
-  expected = "<div style=\"display: none;\">\n\n<div id=\"draggable\" ondragstart=\"doDragStartSelection(event)\">This is a <em>draggable</em> bit of text.</div>\n\n</div>";
+  expected = "<div style=\"display: none\">\n\n<div id=\"draggable\" ondragstart=\"doDragStartSelection(event)\">This is a <em>draggable</em> bit of text.</div>\n\n</div>";
   is(out, expected, "test encodeToStringWithContext with selection ");
 
   node.nextSibling.data="\nfoo bar\n";

content/xul/content/src/nsXULElement.cpp

@@ -1944,9 +1944,12 @@ nsXULElement::EnsureLocalStyle()
                 Clone(*getter_AddRefs(ruleClone));
             NS_ENSURE_SUCCESS(rv, rv);
 
+            nsString stringValue;
+            protoattr->mValue.ToString(stringValue);
+
             nsAttrValue value;
             nsCOMPtr<nsICSSStyleRule> styleRule = do_QueryInterface(ruleClone);
-            value.SetTo(styleRule);
+            value.SetTo(styleRule, &stringValue);
 
             rv = mAttrsAndChildren.SetAndTakeAttr(nsGkAtoms::style, value);
             NS_ENSURE_SUCCESS(rv, rv);
@@ -2308,19 +2311,25 @@ nsresult nsXULElement::MakeHeavyweight()
             continue;
         }
 
-        // XXX we might wanna have a SetAndTakeAttr that takes an nsAttrName
-        nsAttrValue attrValue(protoattr->mValue);
+        nsAttrValue attrValue;
         
         // Style rules need to be cloned.
-        if (attrValue.Type() == nsAttrValue::eCSSStyleRule) {
+        if (protoattr->mValue.Type() == nsAttrValue::eCSSStyleRule) {
             nsCOMPtr<nsICSSRule> ruleClone;
-            rv = attrValue.GetCSSStyleRuleValue()->Clone(*getter_AddRefs(ruleClone));
+            rv = protoattr->mValue.GetCSSStyleRuleValue()->Clone(*getter_AddRefs(ruleClone));
             NS_ENSURE_SUCCESS(rv, rv);
 
+            nsString stringValue;
+            protoattr->mValue.ToString(stringValue);
+
             nsCOMPtr<nsICSSStyleRule> styleRule = do_QueryInterface(ruleClone);
-            attrValue.SetTo(styleRule);
+            attrValue.SetTo(styleRule, &stringValue);
+        }
+        else {
+            attrValue.SetTo(protoattr->mValue);
         }
 
+        // XXX we might wanna have a SetAndTakeAttr that takes an nsAttrName
         if (protoattr->mName.IsAtom()) {
             rv = mAttrsAndChildren.SetAndTakeAttr(protoattr->mName.Atom(), attrValue);
         }
@@ -2837,7 +2846,7 @@ nsXULPrototypeElement::SetAttrAt(PRUint32 aPos, const nsAString& aValue,
                                      DocumentPrincipal(),
                                    getter_AddRefs(rule));
         if (rule) {
-            mAttributes[aPos].mValue.SetTo(rule);
+            mAttributes[aPos].mValue.SetTo(rule, &aValue);
 
             return NS_OK;
         }

content/xul/templates/tests/chrome/test_tmpl_storage_multiqueries.xul

@@ -31,11 +31,11 @@ var notWorkingYetDynamic = false;
 var expectedOutput =
 <output>
     <listitem anyid="true" label="Mammal: African Elephant"/>
-    <listitem anyid="true" label="Mammal: Gorilla" style="font-weight: bold;"/>
+    <listitem anyid="true" label="Mammal: Gorilla" style="font-weight:bold"/>
     <listitem anyid="true" label="Mammal: HIPPOPOTAMUS"/>
     <listitem anyid="true" label="Mammal: LAMA"/>
     <listitem anyid="true" label="Mammal: Lion"/>
-    <listitem anyid="true" label="Mammal: Nine-banded Armadillo" style="font-weight: bold;"/>
+    <listitem anyid="true" label="Mammal: Nine-banded Armadillo" style="font-weight:bold"/>
     <listitem anyid="true" label="Mammal: Polar Bear"/>
     <listitem anyid="true" label="Mammal: aardvark"/>
     <listitem anyid="true" label="Bird: Barn Owl" style="font-style: italic;"/>