Bug 1296556: Don't store change hints for non-elements. r?heycam draft
authorEmilio Cobos Álvarez <ecoal95@gmail.com>
Sat, 20 Aug 2016 00:20:36 -0700
changeset 408963 2010051c631710db08241d69c27605e6a917d833
parent 408962 9cbddf9d357a2234ebca67b9f21f62c698c146aa
child 409636 d57bf15ed3146a224f10cb3481f62567ae718bba
push id28335
push userbmo:ecoal95@gmail.com
push dateThu, 01 Sep 2016 22:18:00 +0000
reviewersheycam
bugs1296556
milestone51.0a1
Bug 1296556: Don't store change hints for non-elements. r?heycam MozReview-Commit-ID: 1pIajBpt4CT
layout/style/ServoBindings.cpp
layout/style/nsStyleContext.h
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -270,37 +270,35 @@ Gecko_CalcStyleDifference(nsStyleContext
 
   return result;
 }
 
 void
 Gecko_StoreStyleDifference(RawGeckoNode* aNode, nsChangeHint aChangeHintToStore)
 {
 #ifdef MOZ_STYLO
-  MOZ_ASSERT(aNode->IsContent());
+  MOZ_ASSERT(aNode->IsElement());
   MOZ_ASSERT(aNode->IsDirtyForServo(),
              "Change hint stored in a not-dirty node");
 
-  // For elements, we need to store the change hint in the proper style context.
-  // For text nodes, we want to store the change hint in the parent element,
-  // since Gecko's change list only operates on Elements, and we'll fail to
-  // compute the change hint for the element properly because the property won't
-  // always be in the cache of the parent's nsStyleContext.
-  //
-  // For Gecko this is not a problem, because they access the inherited structs
-  // from the parent style context in order to inherit them, so they're found in
-  // the cache and get compared.
-  Element* aElement =
-    aNode->IsElement() ? aNode->AsElement() : aNode->GetParentElement();
-
+  Element* aElement = aNode->AsElement();
   nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
   if (!primaryFrame) {
-    // TODO: Pick the undisplayed content map from the frame-constructor, and
-    // stick it there. For now we're generating ReconstructFrame
-    // unconditionally, which is suboptimal.
+    // If there's no primary frame, that means that either this content is
+    // undisplayed (so we only need to check at the restyling phase for the
+    // display value on the element), or is a display: contents element.
+    //
+    // In this second case, we should store it in the frame constructor display
+    // contents map. Note that while this operation looks hairy, this would be
+    // thread-safe because the content should be there already (we'd only need
+    // to read the map and modify our entry).
+    //
+    // That being said, we still don't support display: contents anyway, so it's
+    // probably not worth it to do all the roundtrip just yet until we have a
+    // more concrete plan.
     return;
   }
 
   if ((aChangeHintToStore & nsChangeHint_ReconstructFrame) &&
       aNode->IsInNativeAnonymousSubtree())
   {
     NS_WARNING("stylo: Removing forbidden frame reconstruction hint on native "
                "anonymous content. Fix this in bug 1297857!");
--- a/layout/style/nsStyleContext.h
+++ b/layout/style/nsStyleContext.h
@@ -536,17 +536,17 @@ public:
   // NOTE: It'd be great to assert here that the previous change hint is always
   // consumed.
   //
   // This is not the case right now, since the changes of childs of frames that
   // go through frame construction are not consumed.
   void StoreChangeHint(nsChangeHint aHint)
   {
     MOZ_ASSERT(!IsShared());
-    mStoredChangeHint |= aHint;
+    mStoredChangeHint = aHint;
 #ifdef DEBUG
     mConsumedChangeHint = false;
 #endif
   }
 
   nsChangeHint ConsumeStoredChangeHint()
   {
     MOZ_ASSERT(!mConsumedChangeHint, "Re-consuming the same change hint!");