Bug 582228. Speed up getting style attributes. r=bzbarsky@mit.edu, a=dbaron@mozilla.com
authorJonas Sicking <jonas@sicking.cc>
Thu, 12 Aug 2010 23:03:23 -0700
changeset 50380 c0a3fe7e99a96baeeedd2863457120d28475c87e
parent 50379 b799524f7810ae3d003da8a1e547a72f39cc3ead
child 50381 19367a75c1b655040187a62908a802151ce05949
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky, dbaron
bugs582228
milestone2.0b4pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 582228. Speed up getting style attributes. r=bzbarsky@mit.edu, a=dbaron@mozilla.com
content/base/src/nsAttrValue.cpp
content/base/src/nsAttrValue.h
content/base/src/nsStyledElement.cpp
content/base/test/test_bug166235.html
content/base/test/test_bug418214.html
content/base/test/test_htmlcopyencoder.html
content/xul/content/src/nsXULElement.cpp
content/xul/templates/tests/chrome/test_tmpl_storage_multiqueries.xul
--- a/content/base/src/nsAttrValue.cpp
+++ b/content/base/src/nsAttrValue.cpp
@@ -75,20 +75,20 @@ nsAttrValue::nsAttrValue(const nsAttrVal
 }
 
 nsAttrValue::nsAttrValue(const nsAString& aValue)
     : mBits(0)
 {
   SetTo(aValue);
 }
 
-nsAttrValue::nsAttrValue(nsICSSStyleRule* aValue)
+nsAttrValue::nsAttrValue(nsICSSStyleRule* aValue, const nsAString* aSerialized)
     : mBits(0)
 {
-  SetTo(aValue);
+  SetTo(aValue, aSerialized);
 }
 
 #ifdef MOZ_SVG
 nsAttrValue::nsAttrValue(nsISVGValue* aValue)
     : mBits(0)
 {
   SetTo(aValue);
 }
@@ -308,22 +308,23 @@ nsAttrValue::SetTo(const nsAString& aVal
 void
 nsAttrValue::SetTo(PRInt16 aInt)
 {
   ResetIfSet();
   SetIntValueAndType(aInt, eInteger, nsnull);
 }
 
 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);
   }
 }
 
 #ifdef MOZ_SVG
 void
 nsAttrValue::SetTo(nsISVGValue* aValue)
 {
   if (EnsureEmptyMiscContainer()) {
@@ -426,16 +427,17 @@ nsAttrValue::ToString(nsAString& aResult
     case eCSSStyleRule:
     {
       aResult.Truncate();
       MiscContainer *container = GetMiscContainer();
       css::Declaration *decl = container->mCSSStyleRule->GetDeclaration();
       if (decl) {
         decl->ToString(aResult);
       }
+      const_cast<nsAttrValue*>(this)->SetMiscAtomOrString(&aResult);
 
       break;
     }
 #ifdef MOZ_SVG
     case eSVGValue:
     {
       GetMiscContainer()->mSVGValue->GetValueString(aResult);
       break;
@@ -1255,17 +1257,20 @@ nsAttrValue::ParseIntMarginValue(const n
 void
 nsAttrValue::SetMiscAtomOrString(const nsAString* aValue)
 {
   NS_ASSERTION(GetMiscContainer(), "Must have MiscContainer!");
   NS_ASSERTION(!GetMiscContainer()->mStringBits,
                "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);
       if (atom) {
         cont->mStringBits = reinterpret_cast<PtrBits>(atom) | eAtomBase;
       }
     } else {
       nsStringBuffer* buf = GetStringBuffer(*aValue);
--- a/content/base/src/nsAttrValue.h
+++ b/content/base/src/nsAttrValue.h
@@ -93,17 +93,17 @@ public:
 
 class nsAttrValue {
 public:
   typedef nsTArray< nsCOMPtr<nsIAtom> > AtomArray;
 
   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
   explicit nsAttrValue(const nsIntMargin& aValue);
   ~nsAttrValue();
 
   static nsresult Init();
   static void Shutdown();
@@ -130,17 +130,17 @@ public:
 
   ValueType Type() const;
 
   void Reset();
 
   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
   void SetTo(const nsIntMargin& aValue);
 
   void SwapValueWith(nsAttrValue& aOther);
 
   void ToString(nsAString& aResult) const;
--- a/content/base/src/nsStyledElement.cpp
+++ b/content/base/src/nsStyledElement.cpp
@@ -169,17 +169,17 @@ nsStyledElement::SetInlineStyleRule(nsIC
     // this is getting the new attr value, not the old one....
     modification = GetAttr(kNameSpaceID_None, nsGkAtoms::style,
                            oldValueStr);
   }
   else if (aNotify && IsInDoc()) {
     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 ?
     static_cast<PRUint8>(nsIDOMMutationEvent::MODIFICATION) :
     static_cast<PRUint8>(nsIDOMMutationEvent::ADDITION);
 
   return SetAttrAndNotify(kNameSpaceID_None, nsGkAtoms::style, nsnull,
                           oldValueStr, attrValue, modType, hasListeners,
@@ -317,17 +317,17 @@ nsStyledElement::ParseStyleAttribute(con
         nsCOMPtr<nsIURI> baseURI = GetBaseURI();
 
         nsCOMPtr<nsICSSStyleRule> rule;
         cssParser.ParseStyleAttribute(aValue, doc->GetDocumentURI(),
                                       baseURI,
                                       NodePrincipal(),
                                       getter_AddRefs(rule));
         if (rule) {
-          aResult.SetTo(rule);
+          aResult.SetTo(rule, &aValue);
           return;
         }
       }
     }
   }
 
   aResult.SetTo(aValue);
 }
--- a/content/base/test/test_bug166235.html
+++ b/content/base/test/test_bug166235.html
@@ -82,31 +82,31 @@ var originalStrings = [
   'This text should be copied.',
   'This text should be copied.',
 ];
 
 // expected results for clipboard text/html
 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
 var clipboardUnicode = [
   'This text should be copied.',
   'This text should be copied.',
   'This text should be copied.'
 ];
 
 // expected results for .innerHTML
 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
 var textareaStrings = [
   'This text should be copied.',
   'This text should be copied.',
   'This text should be copied.'
 ];
--- a/content/base/test/test_bug418214.html
+++ b/content/base/test/test_bug418214.html
@@ -42,43 +42,61 @@ is(d2.getAttribute("style"), "border:: i
 is(s2.getAttribute("style"), "border:: invalid",
    "Shouldn't be parsing style on SVG on clone");
 is(m2.getAttribute("style"), "border:: invalid",
    "Shouldn't be parsing style on MathML on clone");
 
 d2.style;
 s2.style;
 m2.style;
-   
-is(d2.getAttribute("style"), "",
-   "Getting .style should parse style on HTML");
-is(s2.getAttribute("style"), "",
+
+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 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");
-is(m2.getAttribute("style"), "border:: invalid",
-   "Getting .style shouldn't parse style on MathML");
 
 d = document.adoptNode(d);
 s = document.adoptNode(s);
 m = document.adoptNode(m);
 
 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");
 
 $("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>
 </body>
 </html>
 
--- a/content/base/test/test_htmlcopyencoder.html
+++ b/content/base/test/test_htmlcopyencoder.html
@@ -44,24 +44,24 @@ function testHtmlCopyEncoder () {
   is(out, expected, "test node");
 
   var select = window.getSelection();
   select.selectAllChildren(node);
 
   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";
   encoder.init(document, "text/html", de.OutputLFLineBreak | de.OutputSelectionOnly);
   encoder.setSelection(select);
   out = encoder.encodeToString();
   expected = "<div id=\"draggable\" ondragstart=\"doDragStartSelection(event)\">This is a <em>draggable</em> bit of text.</div>";
   is(out, expected, "test selection with additional data");
--- a/content/xul/content/src/nsXULElement.cpp
+++ b/content/xul/content/src/nsXULElement.cpp
@@ -1939,19 +1939,22 @@ nsXULElement::EnsureLocalStyle()
         nsXULPrototypeAttribute *protoattr =
                   FindPrototypeAttribute(kNameSpaceID_None, nsGkAtoms::style);
         if (protoattr && protoattr->mValue.Type() == nsAttrValue::eCSSStyleRule) {
             nsCOMPtr<nsICSSRule> ruleClone;
             nsresult rv = protoattr->mValue.GetCSSStyleRuleValue()->
                 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);
         }
     }
 
     return NS_OK;
 }
@@ -2303,29 +2306,35 @@ nsresult nsXULElement::MakeHeavyweight()
         // We might have a local value for this attribute, in which case
         // we don't want to copy the prototype's value.
         if (hadAttributes &&
             mAttrsAndChildren.GetAttr(protoattr->mName.LocalName(),
                                       protoattr->mName.NamespaceID())) {
             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);
         }
         else {
             rv = mAttrsAndChildren.SetAndTakeAttr(protoattr->mName.NodeInfo(),
                                                   attrValue);
         }
         NS_ENSURE_SUCCESS(rv, rv);
@@ -2832,17 +2841,17 @@ nsXULPrototypeElement::SetAttrAt(PRUint3
         // XXX Get correct Base URI (need GetBaseURI on *prototype* element)
         parser.ParseStyleAttribute(aValue, aDocumentURI, aDocumentURI,
                                    // This is basically duplicating what
                                    // nsINode::NodePrincipal() does
                                    mNodeInfo->NodeInfoManager()->
                                      DocumentPrincipal(),
                                    getter_AddRefs(rule));
         if (rule) {
-            mAttributes[aPos].mValue.SetTo(rule);
+            mAttributes[aPos].mValue.SetTo(rule, &aValue);
 
             return NS_OK;
         }
         // Don't abort if parsing failed, it could just be malformed css.
     }
 
     mAttributes[aPos].mValue.ParseStringOrAtom(aValue);
 
--- a/content/xul/templates/tests/chrome/test_tmpl_storage_multiqueries.xul
+++ b/content/xul/templates/tests/chrome/test_tmpl_storage_multiqueries.xul
@@ -26,21 +26,21 @@ var testid ="storage listbox with multiq
 var queryType = "storage";
 var isTreeBuilder = false;
 var needsOpen = false;
 var notWorkingYet = false;
 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;"/>
     <listitem anyid="true" label="Bird: Emu"/>
     <listitem anyid="true" label="Bird: Raven"/>
 </output>;
 
 var changes = [];