Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild(); r?emilio draft
authorManish Goregaokar <manishearth@gmail.com>
Thu, 16 Mar 2017 14:10:22 -0700
changeset 502449 8983883e4bbeb0ac91009102f6ba5d88d60c577e
parent 502448 f7e392fe6671b38b613644a087885557314a8eb1
child 502450 ac9d3d041f4805f9a1f8547547bcdb98d519974d
push id50276
push userbmo:manishearth@gmail.com
push dateTue, 21 Mar 2017 19:27:24 +0000
reviewersemilio
bugs1341086
milestone55.0a1
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild(); r?emilio MozReview-Commit-ID: 3l8cNwDHKFl
dom/base/FragmentOrElement.cpp
dom/base/FragmentOrElement.h
dom/base/nsGenericDOMDataNode.cpp
dom/base/nsGenericDOMDataNode.h
dom/base/nsIContent.h
layout/style/ServoBindings.cpp
layout/style/nsCSSRuleProcessor.cpp
layout/style/nsCSSRuleProcessor.h
layout/style/nsStyleUtil.cpp
layout/style/nsStyleUtil.h
--- a/dom/base/FragmentOrElement.cpp
+++ b/dom/base/FragmentOrElement.cpp
@@ -2067,16 +2067,22 @@ FragmentOrElement::AppendText(const char
 
 bool
 FragmentOrElement::TextIsOnlyWhitespace()
 {
   return false;
 }
 
 bool
+FragmentOrElement::ThreadSafeTextIsOnlyWhitespace() const
+{
+  return false;
+}
+
+bool
 FragmentOrElement::HasTextForTranslation()
 {
   return false;
 }
 
 void
 FragmentOrElement::AppendTextTo(nsAString& aResult)
 {
--- a/dom/base/FragmentOrElement.h
+++ b/dom/base/FragmentOrElement.h
@@ -138,16 +138,17 @@ public:
   // Need to implement this here too to avoid hiding.
   nsresult SetText(const nsAString& aStr, bool aNotify)
   {
     return SetText(aStr.BeginReading(), aStr.Length(), aNotify);
   }
   virtual nsresult AppendText(const char16_t* aBuffer, uint32_t aLength,
                               bool aNotify) override;
   virtual bool TextIsOnlyWhitespace() override;
+  virtual bool ThreadSafeTextIsOnlyWhitespace() const override;
   virtual bool HasTextForTranslation() override;
   virtual void AppendTextTo(nsAString& aResult) override;
   MOZ_MUST_USE
   virtual bool AppendTextTo(nsAString& aResult, const mozilla::fallible_t&) override;
   virtual nsIContent *GetBindingParent() const override;
   virtual nsXBLBinding *GetXBLBinding() const override;
   virtual void SetXBLBinding(nsXBLBinding* aBinding,
                              nsBindingManager* aOldBindingManager = nullptr) override;
--- a/dom/base/nsGenericDOMDataNode.cpp
+++ b/dom/base/nsGenericDOMDataNode.cpp
@@ -978,16 +978,31 @@ nsGenericDOMDataNode::AppendText(const c
                                  bool aNotify)
 {
   return SetTextInternal(mText.GetLength(), 0, aBuffer, aLength, aNotify);
 }
 
 bool
 nsGenericDOMDataNode::TextIsOnlyWhitespace()
 {
+
+  MOZ_ASSERT(NS_IsMainThread());
+  if (!ThreadSafeTextIsOnlyWhitespace()) {
+    UnsetFlags(NS_TEXT_IS_ONLY_WHITESPACE);
+    SetFlags(NS_CACHED_TEXT_IS_ONLY_WHITESPACE);
+    return false;
+  }
+
+  SetFlags(NS_CACHED_TEXT_IS_ONLY_WHITESPACE | NS_TEXT_IS_ONLY_WHITESPACE);
+  return true;
+}
+
+bool
+nsGenericDOMDataNode::ThreadSafeTextIsOnlyWhitespace() const
+{
   // FIXME: should this method take content language into account?
   if (mText.Is2b()) {
     // The fragment contains non-8bit characters and such characters
     // are never considered whitespace.
     return false;
   }
 
   if (HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE)) {
@@ -996,25 +1011,22 @@ nsGenericDOMDataNode::TextIsOnlyWhitespa
 
   const char* cp = mText.Get1b();
   const char* end = cp + mText.GetLength();
 
   while (cp < end) {
     char ch = *cp;
 
     if (!dom::IsSpaceCharacter(ch)) {
-      UnsetFlags(NS_TEXT_IS_ONLY_WHITESPACE);
-      SetFlags(NS_CACHED_TEXT_IS_ONLY_WHITESPACE);
       return false;
     }
 
     ++cp;
   }
 
-  SetFlags(NS_CACHED_TEXT_IS_ONLY_WHITESPACE | NS_TEXT_IS_ONLY_WHITESPACE);
   return true;
 }
 
 bool
 nsGenericDOMDataNode::HasTextForTranslation()
 {
   if (NodeType() != nsIDOMNode::TEXT_NODE &&
       NodeType() != nsIDOMNode::CDATA_SECTION_NODE) {
--- a/dom/base/nsGenericDOMDataNode.h
+++ b/dom/base/nsGenericDOMDataNode.h
@@ -131,16 +131,17 @@ public:
   // Need to implement this here too to avoid hiding.
   nsresult SetText(const nsAString& aStr, bool aNotify)
   {
     return SetText(aStr.BeginReading(), aStr.Length(), aNotify);
   }
   virtual nsresult AppendText(const char16_t* aBuffer, uint32_t aLength,
                               bool aNotify) override;
   virtual bool TextIsOnlyWhitespace() override;
+  virtual bool ThreadSafeTextIsOnlyWhitespace() const final;
   virtual bool HasTextForTranslation() override;
   virtual void AppendTextTo(nsAString& aResult) override;
   MOZ_MUST_USE
   virtual bool AppendTextTo(nsAString& aResult,
                             const mozilla::fallible_t&) override;
   virtual void SaveSubtreeState() override;
 
 #ifdef DEBUG
--- a/dom/base/nsIContent.h
+++ b/dom/base/nsIContent.h
@@ -554,16 +554,21 @@ public:
 
   /**
    * Query method to see if the frame is nothing but whitespace
    * NOTE: Always returns false for elements
    */
   virtual bool TextIsOnlyWhitespace() = 0;
 
   /**
+   * Thread-safe version of TextIsOnlyWhitespace.
+   */
+  virtual bool ThreadSafeTextIsOnlyWhitespace() const = 0;
+
+  /**
    * Method to see if the text node contains data that is useful
    * for a translation: i.e., it consists of more than just whitespace,
    * digits and punctuation.
    * NOTE: Always returns false for elements.
    */
   virtual bool HasTextForTranslation() = 0;
 
   /**
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -492,21 +492,20 @@ nscolor Gecko_GetLookAndFeelSystemColor(
 }
 
 bool
 Gecko_MatchStringArgPseudo(RawGeckoElementBorrowed aElement,
                            CSSPseudoClassType aType,
                            const char16_t* aIdent,
                            bool* aSetSlowSelectorFlag)
 {
-  MOZ_ASSERT(aElement->OwnerDoc() == aElement->GetComposedDoc());
   EventStates dummyMask; // mask is never read because we pass aDependence=nullptr
   return nsCSSRuleProcessor::StringPseudoMatches(aElement, aType, aIdent,
                                                  aElement->OwnerDoc(), true,
-                                                 dummyMask, aSetSlowSelectorFlag, nullptr);
+                                                 dummyMask, false, aSetSlowSelectorFlag, nullptr);
 }
 
 template <typename Implementor>
 static nsIAtom*
 AtomAttrValue(Implementor* aElement, nsIAtom* aName)
 {
   const nsAttrValue* attr = aElement->GetParsedAttr(aName);
   return attr ? attr->GetAtomValue() : nullptr;
--- a/layout/style/nsCSSRuleProcessor.cpp
+++ b/layout/style/nsCSSRuleProcessor.cpp
@@ -1632,23 +1632,44 @@ StateSelectorMatches(Element* aElement,
                               aTreeMatchContext, aSelectorFlags, nullptr,
                               statesToCheck)) {
       return false;
     }
   }
   return true;
 }
 
+// Chooses the thread safe version in Servo mode, and
+// the non-thread safe one in Gecko mode. The non thread safe one does
+// some extra caching, and is preferred when possible.
+static inline bool
+IsSignificantChildMaybeThreadSafe(const nsIContent* aContent,
+                                  bool aTextIsSignificant,
+                                  bool aWhitespaceIsSignificant,
+                                  bool aIsGecko)
+{
+  if (aIsGecko) {
+    auto content = const_cast<nsIContent*>(aContent);
+    return IsSignificantChild(content, aTextIsSignificant, aWhitespaceIsSignificant);
+  } else {
+    // See bug 1349100 for optimizing this
+    return nsStyleUtil::ThreadSafeIsSignificantChild(aContent,
+                                                     aTextIsSignificant,
+                                                     aWhitespaceIsSignificant);
+  }
+}
+
 /* static */ bool
 nsCSSRuleProcessor::StringPseudoMatches(const mozilla::dom::Element* aElement,
                                         CSSPseudoClassType aPseudo,
                                         const char16_t* aString,
                                         const nsIDocument* aDocument,
                                         bool aForStyling,
                                         EventStates aStateMask,
+                                        bool aIsGecko,
                                         bool* aSetSlowSelectorFlag,
                                         bool* const aDependence)
 {
   MOZ_ASSERT(aSetSlowSelectorFlag);
 
   switch (aPseudo) {
     case CSSPseudoClassType::mozLocaleDir:
       {
@@ -1686,31 +1707,31 @@ nsCSSRuleProcessor::StringPseudoMatches(
           return false;
         }
       }
       break;
 
     case CSSPseudoClassType::mozEmptyExceptChildrenWithLocalname:
       {
         NS_ASSERTION(aString, "Must have string!");
-        nsIContent *child = nullptr;
+        const nsIContent *child = nullptr;
         int32_t index = -1;
 
         if (aForStyling) {
           // FIXME:  This isn't sufficient to handle:
           //   :-moz-empty-except-children-with-localname() + E
           //   :-moz-empty-except-children-with-localname() ~ E
           // because we don't know to restyle the grandparent of the
           // inserted/removed element (as in bug 534804 for :empty).
           *aSetSlowSelectorFlag = true;
         }
         do {
           child = aElement->GetChildAt(++index);
         } while (child &&
-                  (!IsSignificantChild(child, true, false) ||
+                  (!IsSignificantChildMaybeThreadSafe(child, true, false, aIsGecko) ||
                   (child->GetNameSpaceID() == aElement->GetNameSpaceID() &&
                     child->NodeInfo()->NameAtom()->Equals(nsDependentString(aString)))));
         if (child) {
           return false;
         }
       }
       break;
 
@@ -2152,16 +2173,17 @@ static bool SelectorMatches(Element* aEl
         MOZ_ASSERT(nsCSSPseudoClasses::HasStringArg(pseudoClass->mType));
         bool setSlowSelectorFlag = false;
         bool matched = nsCSSRuleProcessor::StringPseudoMatches(aElement,
                                                                pseudoClass->mType,
                                                                pseudoClass->u.mString,
                                                                aTreeMatchContext.mDocument,
                                                                aTreeMatchContext.mForStyling,
                                                                aNodeMatchContext.mStateMask,
+                                                               true,
                                                                &setSlowSelectorFlag,
                                                                aDependence);
         if (setSlowSelectorFlag) {
           aElement->SetFlags(NODE_HAS_SLOW_SELECTOR);
         }
 
         if (!matched) {
           return false;
--- a/layout/style/nsCSSRuleProcessor.h
+++ b/layout/style/nsCSSRuleProcessor.h
@@ -147,25 +147,29 @@ public:
    * @param aElement The element we are trying to match
    * @param aPseudo The name of the pseudoselector
    * @param aString The identifier inside the pseudoselector (cannot be null)
    * @param aDocument The document
    * @param aForStyling Is this matching operation for the creation of a style context?
    *                    (For setting the slow selector flag)
    * @param aStateMask Mask containing states which we should exclude.
    *                   Ignored if aDependence is null
+   * @param aIsGecko Set if Gecko.
+   * @param aSetSlowSelectorFlag Outparam, set if the caller is
+   *                             supposed to set the slow selector flag.
    * @param aDependence Pointer to be set to true if we ignored a state due to
    *                    aStateMask. Can be null.
    */
   static bool StringPseudoMatches(const mozilla::dom::Element* aElement,
                                   mozilla::CSSPseudoClassType aPseudo,
                                   const char16_t* aString,
                                   const nsIDocument* aDocument,
                                   bool aForStyling,
                                   mozilla::EventStates aStateMask,
+                                  bool aIsGecko,
                                   bool* aSetSlowSelectorFlag,
                                   bool* const aDependence = nullptr);
 
   // nsIStyleRuleProcessor
   virtual void RulesMatching(ElementRuleProcessorData* aData) override;
 
   virtual void RulesMatching(PseudoElementRuleProcessorData* aData) override;
 
--- a/layout/style/nsStyleUtil.cpp
+++ b/layout/style/nsStyleUtil.cpp
@@ -736,16 +736,36 @@ nsStyleUtil::IsSignificantChild(nsIConte
     return true;
   }
 
   return aTextIsSignificant && isText && aChild->TextLength() != 0 &&
          (aWhitespaceIsSignificant ||
           !aChild->TextIsOnlyWhitespace());
 }
 
+/* static */ bool
+nsStyleUtil::ThreadSafeIsSignificantChild(const nsIContent* aChild,
+                                          bool aTextIsSignificant,
+                                          bool aWhitespaceIsSignificant)
+{
+  NS_ASSERTION(!aWhitespaceIsSignificant || aTextIsSignificant,
+               "Nonsensical arguments");
+
+  bool isText = aChild->IsNodeOfType(nsINode::eTEXT);
+
+  if (!isText && !aChild->IsNodeOfType(nsINode::eCOMMENT) &&
+      !aChild->IsNodeOfType(nsINode::ePROCESSING_INSTRUCTION)) {
+    return true;
+  }
+
+  return aTextIsSignificant && isText && aChild->TextLength() != 0 &&
+         (aWhitespaceIsSignificant ||
+          !aChild->ThreadSafeTextIsOnlyWhitespace());
+}
+
 // For a replaced element whose concrete object size is no larger than the
 // element's content-box, this method checks whether the given
 // "object-position" coordinate might cause overflow in its dimension.
 static bool
 ObjectPositionCoordMightCauseOverflow(const Position::Coord& aCoord)
 {
   // Any nonzero length in "object-position" can push us to overflow
   // (particularly if our concrete object size is exactly the same size as the
--- a/layout/style/nsStyleUtil.h
+++ b/layout/style/nsStyleUtil.h
@@ -133,16 +133,23 @@ public:
   static float ColorComponentToFloat(uint8_t aAlpha);
 
   /*
    * Does this child count as significant for selector matching?
    */
   static bool IsSignificantChild(nsIContent* aChild,
                                    bool aTextIsSignificant,
                                    bool aWhitespaceIsSignificant);
+
+  /*
+   * Thread-safe version of IsSignificantChild()
+   */
+  static bool ThreadSafeIsSignificantChild(const nsIContent* aChild,
+                                           bool aTextIsSignificant,
+                                           bool aWhitespaceIsSignificant);
   /**
    * Returns true if our object-fit & object-position properties might cause
    * a replaced element's contents to overflow its content-box (requiring
    * clipping), or false if we can be sure that this won't happen.
    *
    * This lets us optimize by skipping clipping when we can tell it's
    * unnecessary (particularly with the default values of these properties).
    *