Bug 1378190: Try to make ServoRestyleManager easier to follow. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 04 Jul 2017 19:16:04 +0200
changeset 603863 da57c26963e0a3b6c57be2350e7fe3672f810ac7
parent 603801 032155fdd187dadf0917ed617d48f0c5def56ad5
child 604042 d0052c18f9567f821cec28d2b6f03043b125d03e
push id66897
push userbmo:emilio+bugs@crisal.io
push dateTue, 04 Jul 2017 23:57:58 +0000
reviewersheycam
bugs1378190
milestone56.0a1
Bug 1378190: Try to make ServoRestyleManager easier to follow. r?heycam MozReview-Commit-ID: B9bw23n2jUe
layout/base/ServoRestyleManager.cpp
layout/style/ServoStyleSet.cpp
layout/style/ServoStyleSet.h
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -950,97 +950,152 @@ ServoRestyleManager::ContentStateChanged
 
   ServoElementSnapshot& snapshot = SnapshotFor(aElement);
   EventStates previousState = aElement->StyleState() ^ aChangedBits;
   snapshot.AddState(previousState);
 
   if (Element* parent = aElement->GetFlattenedTreeParentElementForStyle()) {
     parent->NoteDirtyDescendantsForServo();
   }
-  PostRestyleEvent(aElement, restyleHint, changeHint);
+
+  if (restyleHint || changeHint) {
+    Servo_NoteExplicitHints(aElement, restyleHint, changeHint);
+  }
+}
+
+static inline bool
+AttributeInfluencesOtherPseudoClassState(const Element& aElement,
+                                         const nsIAtom* aAttribute)
+{
+  // We must record some state for :-moz-browser-frame and
+  // :-moz-table-border-nonzero.
+  if (aAttribute == nsGkAtoms::mozbrowser) {
+    return aElement.IsAnyOfHTMLElements(nsGkAtoms::iframe, nsGkAtoms::frame);
+  }
+
+  if (aAttribute == nsGkAtoms::border) {
+    return aElement.IsHTMLElement(nsGkAtoms::table);
+  }
+
+  return false;
 }
 
 static inline bool
-AttributeInfluencesOtherPseudoClassState(Element* aElement, nsIAtom* aAttribute)
+NeedToRecordAttrChange(const ServoStyleSet& aStyleSet,
+                       const Element& aElement,
+                       int32_t aNameSpaceID,
+                       nsIAtom* aAttribute,
+                       bool* aInfluencesOtherPseudoClassState)
 {
-  // We must record some state for :-moz-browser-frame and
-  // :-moz-table-border-nonzero.
-  return (aAttribute == nsGkAtoms::mozbrowser &&
-          aElement->IsAnyOfHTMLElements(nsGkAtoms::iframe, nsGkAtoms::frame)) ||
-         (aAttribute == nsGkAtoms::border &&
-          aElement->IsHTMLElement(nsGkAtoms::table));
+  *aInfluencesOtherPseudoClassState =
+    AttributeInfluencesOtherPseudoClassState(aElement, aAttribute);
+
+  // If the attribute influences one of the pseudo-classes that are backed by
+  // attributes, we just record it.
+  if (*aInfluencesOtherPseudoClassState) {
+    return true;
+  }
+
+  // We assume that id and class attributes are used in class/id selectors, and
+  // thus record them.
+  //
+  // TODO(emilio): We keep a filter of the ids in use somewhere in the StyleSet,
+  // presumably we could try to filter the old and new id, but it's not clear
+  // it's worth it.
+  if (aNameSpaceID == kNameSpaceID_None &&
+      (aAttribute == nsGkAtoms::id || aAttribute == nsGkAtoms::_class)) {
+    return true;
+  }
+
+  // We always record lang="", even though we force a subtree restyle when it
+  // changes, since it can change how its siblings match :lang(..) due to
+  // selectors like :lang(..) + div.
+  if (aAttribute == nsGkAtoms::lang) {
+    return true;
+  }
+
+  // Otherwise, just record the attribute change if a selector in the page may
+  // reference it from an attribute selector.
+  return aStyleSet.MightHaveAttributeDependency(aElement, aAttribute);
 }
 
 void
 ServoRestyleManager::AttributeWillChange(Element* aElement,
                                          int32_t aNameSpaceID,
                                          nsIAtom* aAttribute, int32_t aModType,
                                          const nsAttrValue* aNewValue)
 {
   MOZ_ASSERT(!mInStyleRefresh);
 
   if (!aElement->HasServoData()) {
     return;
   }
 
-  bool influencesOtherPseudoClassState =
-    AttributeInfluencesOtherPseudoClassState(aElement, aAttribute);
-
-  if (!influencesOtherPseudoClassState &&
-      !((aNameSpaceID == kNameSpaceID_None &&
-         (aAttribute == nsGkAtoms::id ||
-          aAttribute == nsGkAtoms::_class)) ||
-        aAttribute == nsGkAtoms::lang ||
-        StyleSet()->MightHaveAttributeDependency(*aElement, aAttribute))) {
+  bool influencesOtherPseudoClassState;
+  if (!NeedToRecordAttrChange(*StyleSet(),
+                              *aElement,
+                              aNameSpaceID,
+                              aAttribute,
+                              &influencesOtherPseudoClassState)) {
     return;
   }
 
   ServoElementSnapshot& snapshot = SnapshotFor(aElement);
   snapshot.AddAttrs(aElement, aNameSpaceID, aAttribute);
 
   if (influencesOtherPseudoClassState) {
     snapshot.AddOtherPseudoClassState(aElement);
   }
 
   if (Element* parent = aElement->GetFlattenedTreeParentElementForStyle()) {
     parent->NoteDirtyDescendantsForServo();
   }
 }
 
+// For some attribute changes we must restyle the whole subtree:
+//
+// * <td> is affected by the cellpadding on its ancestor table
+// * lang="" and xml:lang="" can affect all descendants due to :lang()
+//
+static inline bool
+AttributeChangeRequiresSubtreeRestyle(const Element& aElement, nsIAtom* aAttr)
+{
+  if (aAttr == nsGkAtoms::cellpadding) {
+    return aElement.IsHTMLElement(nsGkAtoms::table);
+  }
+
+  return aAttr == nsGkAtoms::lang;
+}
+
 void
 ServoRestyleManager::AttributeChanged(Element* aElement, int32_t aNameSpaceID,
                                       nsIAtom* aAttribute, int32_t aModType,
                                       const nsAttrValue* aOldValue)
 {
   MOZ_ASSERT(!mInStyleRefresh);
 
-  nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
-  if (primaryFrame) {
+  if (nsIFrame* primaryFrame = aElement->GetPrimaryFrame()) {
     primaryFrame->AttributeChanged(aNameSpaceID, aAttribute, aModType);
   }
 
-  nsChangeHint hint = aElement->GetAttributeChangeHint(aAttribute, aModType);
-  if (hint) {
-    PostRestyleEvent(aElement, nsRestyleHint(0), hint);
-  }
+  auto changeHint = nsChangeHint(0);
+  auto restyleHint = nsRestyleHint(0);
+
+  changeHint |= aElement->GetAttributeChangeHint(aAttribute, aModType);
 
   if (aAttribute == nsGkAtoms::style) {
-    PostRestyleEvent(aElement, eRestyle_StyleAttribute, nsChangeHint(0));
+    restyleHint |= eRestyle_StyleAttribute;
+  } else if (AttributeChangeRequiresSubtreeRestyle(*aElement, aAttribute)) {
+    restyleHint |= eRestyle_Subtree;
+  } else if (aElement->IsAttributeMapped(aAttribute)) {
+    restyleHint |= eRestyle_Self;
   }
 
-  // For some attribute changes we must restyle the whole subtree:
-  //
-  // * <td> is affected by the cellpadding on its ancestor table
-  // * lang="" and xml:lang="" can affect all descendants due to :lang()
-  if ((aAttribute == nsGkAtoms::cellpadding &&
-       aElement->IsHTMLElement(nsGkAtoms::table)) ||
-      aAttribute == nsGkAtoms::lang) {
-    PostRestyleEvent(aElement, eRestyle_Subtree, nsChangeHint(0));
-  } else if (aElement->IsAttributeMapped(aAttribute)) {
-    Servo_NoteExplicitHints(aElement, eRestyle_Self, nsChangeHint(0));
+  if (restyleHint || changeHint) {
+    Servo_NoteExplicitHints(aElement, restyleHint, changeHint);
   }
 }
 
 nsresult
 ServoRestyleManager::ReparentStyleContext(nsIFrame* aFrame)
 {
   NS_WARNING("stylo: ServoRestyleManager::ReparentStyleContext not implemented");
   return NS_OK;
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -1420,22 +1420,23 @@ ServoStyleSet::StyleRuleMap()
     doc->AddObserver(mStyleRuleMap);
     doc->CSSLoader()->AddObserver(mStyleRuleMap);
   }
   return mStyleRuleMap;
 }
 
 bool
 ServoStyleSet::MightHaveAttributeDependency(const Element& aElement,
-                                            nsIAtom* aAttribute)
+                                            nsIAtom* aAttribute) const
 {
   return Servo_StyleSet_MightHaveAttributeDependency(
       mRawSet.get(), &aElement, aAttribute);
 }
 
 bool
-ServoStyleSet::HasStateDependency(const Element& aElement, EventStates aState)
+ServoStyleSet::HasStateDependency(const Element& aElement,
+                                  EventStates aState) const
 {
   return Servo_StyleSet_HasStateDependency(
       mRawSet.get(), &aElement, aState.ServoValue());
 }
 
 ServoStyleSet* ServoStyleSet::sInServoTraversal = nullptr;
--- a/layout/style/ServoStyleSet.h
+++ b/layout/style/ServoStyleSet.h
@@ -454,27 +454,28 @@ public:
    * Returns true if a modification to an an attribute with the specified
    * local name might require us to restyle the element.
    *
    * This function allows us to skip taking a an attribute snapshot when
    * the modified attribute doesn't appear in an attribute selector in
    * a style sheet.
    */
   bool MightHaveAttributeDependency(const dom::Element& aElement,
-                                    nsIAtom* aAttribute);
+                                    nsIAtom* aAttribute) const;
 
   /**
    * Returns true if a change in event state on an element might require
    * us to restyle the element.
    *
    * This function allows us to skip taking a state snapshot when
    * the changed state isn't depended upon by any pseudo-class selectors
    * in a style sheet.
    */
-  bool HasStateDependency(const dom::Element& aElement, EventStates aState);
+  bool HasStateDependency(const dom::Element& aElement,
+                          EventStates aState) const;
 
 private:
   // On construction, sets sInServoTraversal to the given ServoStyleSet.
   // On destruction, clears sInServoTraversal and calls RunPostTraversalTasks.
   class MOZ_STACK_CLASS AutoSetInServoTraversal
   {
   public:
     explicit AutoSetInServoTraversal(ServoStyleSet* aSet)