Bug 1531582 - Don't drop the change hint for ContentStateChanged on the floor. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Mar 2019 02:04:12 +0000
changeset 519812 635dd2d69386b9f98b5d23b8feb54de32616e2d4
parent 519811 91d3c9e78ce7d6f4cc7f91617290933c060f7c21
child 519813 234f0dde00661d41a6641e950ded8bd93aabf9a9
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
layout/reftests/forms/input/range/reftest.list
--- 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;
--- a/layout/reftests/forms/input/range/reftest.list
+++ b/layout/reftests/forms/input/range/reftest.list
@@ -9,23 +9,23 @@ fuzzy-if(skiaContent,0-1,0-40) == to-ran
 == from-range-to-other-type-unthemed-1.html from-range-to-other-type-unthemed-1-ref.html
 
 # for different values:
 != different-fraction-of-range-unthemed-1.html different-fraction-of-range-unthemed-1-notref.html
 == same-fraction-of-range-unthemed-1.html same-fraction-of-range-unthemed-1-ref.html
 
 # dynamic value changes:
 fuzzy-if(skiaContent,0-1,0-40) == value-prop-unthemed.html 75pct-unthemed-common-ref.html
-fuzzy-if(webrender&&gtkWidget,79-79,138-138) == value-prop.html 75pct-common-ref.html
+== value-prop.html 75pct-common-ref.html
 fuzzy-if(skiaContent,0-1,0-40) == valueAsNumber-prop-unthemed.html 75pct-unthemed-common-ref.html
-fuzzy-if(webrender&&gtkWidget,79-79,138-138) == valueAsNumber-prop.html 75pct-common-ref.html
+== valueAsNumber-prop.html 75pct-common-ref.html
 fuzzy-if(skiaContent,0-1,0-40) == stepDown-unthemed.html 75pct-unthemed-common-ref.html
-fuzzy-if(webrender&&gtkWidget,79-79,138-138) == stepDown.html 75pct-common-ref.html
+== stepDown.html 75pct-common-ref.html
 fuzzy-if(skiaContent,0-1,0-40) == stepUp-unthemed.html 75pct-unthemed-common-ref.html
-fuzzy-if(webrender&&gtkWidget,79-79,138-138) == stepUp.html 75pct-common-ref.html
+== stepUp.html 75pct-common-ref.html
 == max-prop.html 100pct-common-ref.html
 == reset-value.html reset-value-ref.html
 
 # 'direction' property:
 == direction-unthemed-1.html direction-unthemed-1-ref.html
 
 # ::-moz-range-progress pseudo-element:
 == moz-range-progress-1.html moz-range-progress-1-ref.html