Bug 1531582 - Don't drop the change hint for ContentStateChanged on the floor. r=heycam
☠☠ backed out by 7c3df2c8c8a7 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Mar 2019 02:04:12 +0000
changeset 519731 60128088f86203d665367e195c5fafefd94fe6e7
parent 519730 26021d8ebb2772d070b99969757ef22b73a1d21f
child 519732 6b8e4909d303024cef8f404fc1d2ae4f3ecce055
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1531582, 1472637
milestone67.0a1
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 1531582 - Don't drop the change hint for ContentStateChanged on the floor. r=heycam This doesn't matter yet because all the states that return a change hint are on stylesheets, but will matter with bug 1472637. Differential Revision: https://phabricator.services.mozilla.com/D21616
layout/base/RestyleManager.cpp
layout/base/RestyleManager.h
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -459,65 +459,61 @@ void RestyleManager::ContentRemoved(nsIC
       }
     }
   }
 }
 
 /**
  * Calculates the change hint and the restyle hint for a given content state
  * change.
- *
- * This is called from both Restyle managers.
  */
-void RestyleManager::ContentStateChangedInternal(const Element& aElement,
-                                                 EventStates aStateMask,
-                                                 nsChangeHint* aOutChangeHint) {
-  MOZ_ASSERT(!mInStyleRefresh);
-  MOZ_ASSERT(aOutChangeHint);
-
-  *aOutChangeHint = nsChangeHint(0);
+static nsChangeHint ChangeForContentStateChange(const Element& aElement,
+                                                EventStates aStateMask) {
+  auto changeHint = nsChangeHint(0);
+
   // Any change to a content state that affects which frames we construct
   // must lead to a frame reconstruct here if we already have a frame.
   // Note that we never decide through non-CSS means to not create frames
   // based on content states, so if we already don't have a frame we don't
   // need to force a reframe -- if it's needed, the HasStateDependentStyle
   // call will handle things.
-  nsIFrame* primaryFrame = aElement.GetPrimaryFrame();
-  if (primaryFrame) {
+  if (nsIFrame* primaryFrame = aElement.GetPrimaryFrame()) {
     // If it's generated content, ignore LOADING/etc state changes on it.
     if (!primaryFrame->IsGeneratedContentFrame() &&
         aStateMask.HasAtLeastOneOfStates(
             NS_EVENT_STATE_BROKEN | NS_EVENT_STATE_USERDISABLED |
             NS_EVENT_STATE_SUPPRESSED | NS_EVENT_STATE_LOADING)) {
-      *aOutChangeHint = nsChangeHint_ReconstructFrame;
-    } else {
-      auto* disp = primaryFrame->StyleDisplay();
-      if (disp->HasAppearance()) {
-        nsITheme* theme = PresContext()->GetTheme();
-        if (theme && theme->ThemeSupportsWidget(PresContext(), primaryFrame,
-                                                disp->mAppearance)) {
-          bool repaint = false;
-          theme->WidgetStateChanged(primaryFrame, disp->mAppearance, nullptr,
-                                    &repaint, nullptr);
-          if (repaint) {
-            *aOutChangeHint |= nsChangeHint_RepaintFrame;
-          }
+      return nsChangeHint_ReconstructFrame;
+    }
+
+    auto* disp = primaryFrame->StyleDisplay();
+    if (disp->HasAppearance()) {
+      nsPresContext* pc = primaryFrame->PresContext();
+      nsITheme* theme = pc->GetTheme();
+      if (theme &&
+          theme->ThemeSupportsWidget(pc, primaryFrame, disp->mAppearance)) {
+        bool repaint = false;
+        theme->WidgetStateChanged(primaryFrame, disp->mAppearance, nullptr,
+                                  &repaint, nullptr);
+        if (repaint) {
+          changeHint |= nsChangeHint_RepaintFrame;
         }
       }
     }
-
     primaryFrame->ContentStatesChanged(aStateMask);
   }
 
   if (aStateMask.HasState(NS_EVENT_STATE_VISITED)) {
     // Exposing information to the page about whether the link is
     // visited or not isn't really something we can worry about here.
     // FIXME: We could probably do this a bit better.
-    *aOutChangeHint |= nsChangeHint_RepaintFrame;
+    changeHint |= nsChangeHint_RepaintFrame;
   }
+
+  return changeHint;
 }
 
 /* static */ nsCString RestyleManager::RestyleHintToString(
     nsRestyleHint aHint) {
   nsCString result;
   bool any = false;
   const char* names[] = {"Self",           "SomeDescendants",
                          "Subtree",        "LaterSiblings",
@@ -3215,40 +3211,34 @@ void RestyleManager::ContentStateChanged
   if (aChangedBits.HasAllStates(kVisitedAndUnvisited) &&
       !Gecko_VisitedStylesEnabled(element.OwnerDoc())) {
     aChangedBits &= ~kVisitedAndUnvisited;
     if (aChangedBits.IsEmpty()) {
       return;
     }
   }
 
-  nsChangeHint changeHint;
-  ContentStateChangedInternal(element, aChangedBits, &changeHint);
+  if (auto changeHint = ChangeForContentStateChange(element, aChangedBits)) {
+    Servo_NoteExplicitHints(&element, nsRestyleHint(0), changeHint);
+  }
 
   // Don't bother taking a snapshot if no rules depend on these state bits.
   //
   // We always take a snapshot for the LTR/RTL event states, since Servo doesn't
   // track those bits in the same way, and we know that :dir() rules are always
   // present in UA style sheets.
-  //
-  // FIXME(emilio): Doesn't this early-return drop the change hint on the floor?
-  // Should it?
   if (!aChangedBits.HasAtLeastOneOfStates(DIRECTION_STATES) &&
       !StyleSet()->HasStateDependency(element, aChangedBits)) {
     return;
   }
 
   ServoElementSnapshot& snapshot = SnapshotFor(element);
   EventStates previousState = element.StyleState() ^ aChangedBits;
   snapshot.AddState(previousState);
 
-  if (changeHint) {
-    Servo_NoteExplicitHints(&element, nsRestyleHint(0), changeHint);
-  }
-
   // Assuming we need to invalidate cached style in getComputedStyle for
   // undisplayed elements, since we don't know if it is needed.
   IncrementUndisplayedRestyleGeneration();
 }
 
 static inline bool AttributeInfluencesOtherPseudoClassState(
     const Element& aElement, const nsAtom* aAttribute) {
   // We must record some state for :-moz-browser-frame and
--- a/layout/base/RestyleManager.h
+++ b/layout/base/RestyleManager.h
@@ -451,21 +451,16 @@ class RestyleManager {
                                    ServoPostTraversalFlags aFlags);
 
   ServoStyleSet* StyleSet() const { return PresContext()->StyleSet(); }
 
   void RestyleForEmptyChange(Element* aContainer);
   void MaybeRestyleForEdgeChildChange(Element* aContainer,
                                       nsIContent* aChangedChild);
 
-  // TODO(emilio): there's no good reason this isn't part of ContentStateChanged
-  // now, or the change hint isn't returned instead of via an out-param, really.
-  void ContentStateChangedInternal(const Element&, EventStates aStateMask,
-                                   nsChangeHint* aOutChangeHint);
-
   bool IsDisconnected() const { return !mPresContext; }
 
   void IncrementRestyleGeneration() {
     if (++mRestyleGeneration == 0) {
       // Keep mRestyleGeneration from being 0, since that's what
       // nsPresContext::GetRestyleGeneration returns when it no
       // longer has a RestyleManager.
       ++mRestyleGeneration;