From 32579060423f2d2a8d7fc7a75157d2cf88720cf5 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 10 Dec 2009 14:36:02 -0800 Subject: [PATCH] Bug 523148. Speed up AddImportantRules. r=dbaron --- layout/style/nsCSSStyleRule.cpp | 5 +- layout/style/nsICSSStyleRule.h | 7 +-- layout/style/nsIStyleRule.h | 7 ++- layout/style/nsRuleWalker.h | 13 ++++- layout/style/nsStyleSet.cpp | 100 ++++++++++++++++++++++---------- 5 files changed, 91 insertions(+), 41 deletions(-) diff --git a/layout/style/nsCSSStyleRule.cpp b/layout/style/nsCSSStyleRule.cpp index bd27ff10d8ee..2da21e8c8512 100644 --- a/layout/style/nsCSSStyleRule.cpp +++ b/layout/style/nsCSSStyleRule.cpp @@ -1231,7 +1231,7 @@ public: virtual nsCSSDeclaration* GetDeclaration(void) const; - virtual already_AddRefed GetImportantRule(void); + virtual nsIStyleRule* GetImportantRule(void); NS_IMETHOD GetStyleSheet(nsIStyleSheet*& aSheet) const; NS_IMETHOD SetStyleSheet(nsICSSStyleSheet* aSheet); @@ -1382,7 +1382,7 @@ nsCSSDeclaration* CSSStyleRuleImpl::GetDeclaration(void) const return mDeclaration; } -already_AddRefed CSSStyleRuleImpl::GetImportantRule(void) +nsIStyleRule* CSSStyleRuleImpl::GetImportantRule(void) { if (!mDeclaration->HasImportantData()) { NS_ASSERTION(!mImportantRule, "immutable, so should be no important rule"); @@ -1395,7 +1395,6 @@ already_AddRefed CSSStyleRuleImpl::GetImportantRule(void) return nsnull; NS_ADDREF(mImportantRule); } - NS_ADDREF(mImportantRule); return mImportantRule; } diff --git a/layout/style/nsICSSStyleRule.h b/layout/style/nsICSSStyleRule.h index c6a36a76d5ad..93b095b29f58 100644 --- a/layout/style/nsICSSStyleRule.h +++ b/layout/style/nsICSSStyleRule.h @@ -260,9 +260,10 @@ private: nsCSSSelectorList& operator=(const nsCSSSelectorList& aCopy); }; -// IID for the nsICSSStyleRule interface {00803ccc-66e8-4ec8-a037-45e901bb5304} +// IID for the nsICSSStyleRule interface {3ffbd89e-3c83-4e9b-9b1f-424c6cebac1b} #define NS_ICSS_STYLE_RULE_IID \ -{0x00803ccc, 0x66e8, 0x4ec8, {0xa0, 0x37, 0x45, 0xe9, 0x01, 0xbb, 0x53, 0x04}} +{ 0x3ffbd89e, 0x3c83, 0x4e9b, \ + { 0x9b, 0x1f, 0x42, 0x4c, 0x6c, 0xeb, 0xac, 0x1b } } class nsICSSStyleRule : public nsICSSRule { public: @@ -288,8 +289,6 @@ public: virtual already_AddRefed DeclarationChanged(PRBool aHandleContainer) = 0; - virtual already_AddRefed GetImportantRule(void) = 0; - // hooks for DOM rule virtual nsresult GetCssText(nsAString& aCssText) = 0; virtual nsresult SetCssText(const nsAString& aCssText) = 0; diff --git a/layout/style/nsIStyleRule.h b/layout/style/nsIStyleRule.h index bc9525336319..6a20ed61e6dd 100644 --- a/layout/style/nsIStyleRule.h +++ b/layout/style/nsIStyleRule.h @@ -52,9 +52,10 @@ class nsPresContext; class nsIContent; struct nsRuleData; -// IID for the nsIStyleRule interface {40ae5c90-ad6a-11d1-8031-006008159b5a} +// IID for the nsIStyleRule interface {f75f3f70-435d-43a6-a01b-65970489ca26} #define NS_ISTYLE_RULE_IID \ -{0x40ae5c90, 0xad6a, 0x11d1, {0x80, 0x31, 0x00, 0x60, 0x08, 0x15, 0x9b, 0x5a}} +{ 0xf75f3f70, 0x435d, 0x43a6, \ + { 0xa0, 0x1b, 0x65, 0x97, 0x04, 0x89, 0xca, 0x26 } } /** * An object implementing |nsIStyleRule| (henceforth, a rule) represents @@ -105,6 +106,8 @@ public: */ NS_IMETHOD MapRuleInfoInto(nsRuleData* aRuleData)=0; + virtual nsIStyleRule* GetImportantRule(void) { return nsnull; } + #ifdef DEBUG NS_IMETHOD List(FILE* out = stdout, PRInt32 aIndent = 0) const = 0; #endif diff --git a/layout/style/nsRuleWalker.h b/layout/style/nsRuleWalker.h index 17ebd6484ec9..fd238579d866 100644 --- a/layout/style/nsRuleWalker.h +++ b/layout/style/nsRuleWalker.h @@ -51,6 +51,8 @@ public: void Forward(nsIStyleRule* aRule) { if (mCurrent) { // check for OOM from previous step mCurrent = mCurrent->Transition(aRule, mLevel, mImportance); + mCheckForImportantRules = + mCheckForImportantRules && !aRule->GetImportantRule(); } } @@ -58,18 +60,27 @@ public: PRBool AtRoot() { return mCurrent == mRoot; } - void SetLevel(PRUint8 aLevel, PRBool aImportance) { + void SetLevel(PRUint8 aLevel, PRBool aImportance, + PRBool aCheckForImportantRules) { + NS_ASSERTION(!aCheckForImportantRules || !aImportance, + "Shouldn't be checking for important rules while walking " + "important rules"); mLevel = aLevel; mImportance = aImportance; + mCheckForImportantRules = aCheckForImportantRules; } PRUint8 GetLevel() const { return mLevel; } PRBool GetImportance() const { return mImportance; } + PRBool GetCheckForImportantRules() const { return mCheckForImportantRules; } private: nsRuleNode* mCurrent; // Our current position. nsRuleNode* mRoot; // The root of the tree we're walking. PRUint8 mLevel; // an nsStyleSet::sheetType PRPackedBool mImportance; + PRPackedBool mCheckForImportantRules; // If true, check for important rules as + // we walk and set to false if we find + // one. public: nsRuleWalker(nsRuleNode* aRoot) :mCurrent(aRoot), mRoot(aRoot) { MOZ_COUNT_CTOR(nsRuleWalker); } diff --git a/layout/style/nsStyleSet.cpp b/layout/style/nsStyleSet.cpp index 5571b0733e85..3436861e545a 100644 --- a/layout/style/nsStyleSet.cpp +++ b/layout/style/nsStyleSet.cpp @@ -460,21 +460,23 @@ nsStyleSet::AddImportantRules(nsRuleNode* aCurrLevelNode, nsRuleNode* aLastPrevLevelNode, nsRuleWalker* aRuleWalker) { + // The rulewalker will move itself to a null node on OOM. if (!aCurrLevelNode) return; - nsAutoTArray, 16> importantRules; + nsAutoTArray importantRules; for (nsRuleNode *node = aCurrLevelNode; node != aLastPrevLevelNode; node = node->GetParent()) { - nsIStyleRule *rule = node->GetRule(); - nsCOMPtr cssRule(do_QueryInterface(rule)); - if (cssRule) { - nsCOMPtr impRule = cssRule->GetImportantRule(); - if (impRule) - importantRules.AppendElement(impRule); - } + // We guarantee that we never walk the root node here, so no need + // to null-check GetRule(). + nsIStyleRule* impRule = node->GetRule()->GetImportantRule(); + if (impRule) + importantRules.AppendElement(impRule); } + NS_ASSERTION(importantRules.Length() != 0, + "Why did we think there were important rules?"); + for (PRUint32 i = importantRules.Length(); i-- != 0; ) { aRuleWalker->Forward(importantRules[i]); } @@ -490,11 +492,9 @@ nsStyleSet::AssertNoImportantRules(nsRuleNode* aCurrLevelNode, for (nsRuleNode *node = aCurrLevelNode; node != aLastPrevLevelNode; node = node->GetParent()) { - nsIStyleRule *rule = node->GetRule(); - nsCOMPtr cssRule(do_QueryInterface(rule)); - if (cssRule) { - nsCOMPtr impRule = cssRule->GetImportantRule(); - NS_ASSERTION(!impRule, "Unexpected important rule"); + nsIStyleRule* rule = node->GetRule(); + if (rule) { + NS_ASSERTION(!rule->GetImportantRule(), "Unexpected important rule"); } } } @@ -538,29 +538,31 @@ nsStyleSet::FileRules(nsIStyleRuleProcessor::EnumFunc aCollectorFunc, SheetCount(eHTMLPresHintSheet) == 0, "Can't have both types of preshint sheets at once!"); - aRuleWalker->SetLevel(eAgentSheet, PR_FALSE); + aRuleWalker->SetLevel(eAgentSheet, PR_FALSE, PR_TRUE); if (mRuleProcessors[eAgentSheet]) (*aCollectorFunc)(mRuleProcessors[eAgentSheet], aData); nsRuleNode* lastAgentRN = aRuleWalker->GetCurrentNode(); + PRBool haveImportantUARules = !aRuleWalker->GetCheckForImportantRules(); - aRuleWalker->SetLevel(ePresHintSheet, PR_FALSE); + aRuleWalker->SetLevel(ePresHintSheet, PR_FALSE, PR_FALSE); if (mRuleProcessors[ePresHintSheet]) (*aCollectorFunc)(mRuleProcessors[ePresHintSheet], aData); nsRuleNode* lastPresHintRN = aRuleWalker->GetCurrentNode(); - aRuleWalker->SetLevel(eUserSheet, PR_FALSE); + aRuleWalker->SetLevel(eUserSheet, PR_FALSE, PR_TRUE); PRBool skipUserStyles = aData->mContent && aData->mContent->IsInNativeAnonymousSubtree(); if (!skipUserStyles && mRuleProcessors[eUserSheet]) // NOTE: different (*aCollectorFunc)(mRuleProcessors[eUserSheet], aData); nsRuleNode* lastUserRN = aRuleWalker->GetCurrentNode(); + PRBool haveImportantUserRules = !aRuleWalker->GetCheckForImportantRules(); - aRuleWalker->SetLevel(eHTMLPresHintSheet, PR_FALSE); + aRuleWalker->SetLevel(eHTMLPresHintSheet, PR_FALSE, PR_FALSE); if (mRuleProcessors[eHTMLPresHintSheet]) (*aCollectorFunc)(mRuleProcessors[eHTMLPresHintSheet], aData); nsRuleNode* lastHTMLPresHintRN = aRuleWalker->GetCurrentNode(); - aRuleWalker->SetLevel(eDocSheet, PR_FALSE); + aRuleWalker->SetLevel(eDocSheet, PR_FALSE, PR_TRUE); PRBool cutOffInheritance = PR_FALSE; if (mBindingManager) { // We can supply additional document-level sheets that should be walked. @@ -569,37 +571,73 @@ nsStyleSet::FileRules(nsIStyleRuleProcessor::EnumFunc aCollectorFunc, if (!skipUserStyles && !cutOffInheritance && mRuleProcessors[eDocSheet]) // NOTE: different (*aCollectorFunc)(mRuleProcessors[eDocSheet], aData); - aRuleWalker->SetLevel(eStyleAttrSheet, PR_FALSE); + aRuleWalker->SetLevel(eStyleAttrSheet, PR_FALSE, + aRuleWalker->GetCheckForImportantRules()); if (mRuleProcessors[eStyleAttrSheet]) (*aCollectorFunc)(mRuleProcessors[eStyleAttrSheet], aData); nsRuleNode* lastDocRN = aRuleWalker->GetCurrentNode(); + PRBool haveImportantDocRules = !aRuleWalker->GetCheckForImportantRules(); - aRuleWalker->SetLevel(eOverrideSheet, PR_FALSE); + aRuleWalker->SetLevel(eOverrideSheet, PR_FALSE, PR_TRUE); if (mRuleProcessors[eOverrideSheet]) (*aCollectorFunc)(mRuleProcessors[eOverrideSheet], aData); nsRuleNode* lastOvrRN = aRuleWalker->GetCurrentNode(); + PRBool haveImportantOverrideRules = !aRuleWalker->GetCheckForImportantRules(); + + if (haveImportantDocRules) { + aRuleWalker->SetLevel(eDocSheet, PR_TRUE, PR_FALSE); + AddImportantRules(lastDocRN, lastHTMLPresHintRN, aRuleWalker); // doc + } +#ifdef DEBUG + else { + AssertNoImportantRules(lastDocRN, lastHTMLPresHintRN); + } +#endif + + if (haveImportantOverrideRules) { + aRuleWalker->SetLevel(eOverrideSheet, PR_TRUE, PR_FALSE); + AddImportantRules(lastOvrRN, lastDocRN, aRuleWalker); // override + } +#ifdef DEBUG + else { + AssertNoImportantRules(lastOvrRN, lastDocRN); + } +#endif - aRuleWalker->SetLevel(eDocSheet, PR_TRUE); - AddImportantRules(lastDocRN, lastHTMLPresHintRN, aRuleWalker); // doc - aRuleWalker->SetLevel(eOverrideSheet, PR_TRUE); - AddImportantRules(lastOvrRN, lastDocRN, aRuleWalker); // override #ifdef DEBUG AssertNoCSSRules(lastHTMLPresHintRN, lastUserRN); AssertNoImportantRules(lastHTMLPresHintRN, lastUserRN); // HTML preshints #endif - aRuleWalker->SetLevel(eUserSheet, PR_TRUE); - AddImportantRules(lastUserRN, lastPresHintRN, aRuleWalker); //user + + if (haveImportantUserRules) { + aRuleWalker->SetLevel(eUserSheet, PR_TRUE, PR_FALSE); + AddImportantRules(lastUserRN, lastPresHintRN, aRuleWalker); //user + } +#ifdef DEBUG + else { + AssertNoImportantRules(lastUserRN, lastPresHintRN); + } +#endif + #ifdef DEBUG AssertNoCSSRules(lastPresHintRN, lastAgentRN); AssertNoImportantRules(lastPresHintRN, lastAgentRN); // preshints #endif - aRuleWalker->SetLevel(eAgentSheet, PR_TRUE); - AddImportantRules(lastAgentRN, nsnull, aRuleWalker); //agent + + if (haveImportantUARules) { + aRuleWalker->SetLevel(eAgentSheet, PR_TRUE, PR_FALSE); + AddImportantRules(lastAgentRN, mRuleTree, aRuleWalker); //agent + } +#ifdef DEBUG + else { + AssertNoImportantRules(lastAgentRN, mRuleTree); + } +#endif #ifdef DEBUG nsRuleNode *lastImportantRN = aRuleWalker->GetCurrentNode(); #endif - aRuleWalker->SetLevel(eTransitionSheet, PR_FALSE); + aRuleWalker->SetLevel(eTransitionSheet, PR_FALSE, PR_FALSE); (*aCollectorFunc)(mRuleProcessors[eTransitionSheet], aData); #ifdef DEBUG AssertNoCSSRules(aRuleWalker->GetCurrentNode(), lastImportantRN); @@ -719,7 +757,7 @@ nsStyleSet::ResolveStyleForRules(nsStyleContext* aParentContext, ruleWalker.SetCurrentNode(aRuleNode); // FIXME: Perhaps this should be passed in, but it probably doesn't // matter. - ruleWalker.SetLevel(eDocSheet, PR_FALSE); + ruleWalker.SetLevel(eDocSheet, PR_FALSE, PR_FALSE); for (PRInt32 i = 0; i < aRules.Count(); i++) { ruleWalker.Forward(aRules.ObjectAt(i)); } @@ -749,7 +787,7 @@ nsStyleSet::WalkRestrictionRule(nsIAtom* aPseudoType, { // This needs to match GetPseudoRestriction in nsRuleNode.cpp. if (aPseudoType) { - aRuleWalker->SetLevel(eAgentSheet, PR_FALSE); + aRuleWalker->SetLevel(eAgentSheet, PR_FALSE, PR_FALSE); if (aPseudoType == nsCSSPseudoElements::firstLetter) aRuleWalker->Forward(mFirstLetterRule); else if (aPseudoType == nsCSSPseudoElements::firstLine)