Bug 1379505: Less fishyness when resolving the style of the document element. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 10 Jul 2017 14:32:39 +0200
changeset 417142 183625671488778f2321cb569ae57068be379010
parent 417141 b6728735c6f67859230523f877fa2ebb5c6fb701
child 417143 039a8c4306acc3f385490cef5e42222fd8af16fe
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1379505
milestone56.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 1379505: Less fishyness when resolving the style of the document element. r=heycam Previous to these patches, the style resolution happening on [1] made the style data stick on the element, so we'd never think it was the initial style, even if it was. This was wallpapering the fact that, if that was the initial style, we'd never have another chance of traversing the document when [2] kicked in. This somehow just happened to work, but is a very fishy way to get it to work. Instead, call StyleDocument() properly _before_, and rely on the fact that it will stop when it has a non-null binding, and only if it fails explicitly style the children. This fixes the few XBL test-cases with this patch series that exercise -moz-bindings on the root element without the root being styled, and makes the -moz-binding code more consistent in both places of the frame constructor. [1]: http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/layout/base/nsCSSFrameConstructor.cpp#2526 [2]: https://github.com/servo/servo/blob/3330653dc80cc8947af17f1989fb7fbf390c4d2f/components/style/traversal.rs#L439 MozReview-Commit-ID: HbjsD6nYsvX
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsPresContext.cpp
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -2514,23 +2514,30 @@ nsCSSFrameConstructor::ConstructDocEleme
   // the future.  We need this, because the document element might
   // have stale restyle bits from a previous frame constructor for
   // this document.  Unlike in AddFrameConstructionItems, it's safe to
   // unset all element restyle flags, since we don't have any
   // siblings.
   aDocElement->UnsetRestyleFlagsIfGecko();
 
   // --------- CREATE AREA OR BOX FRAME -------
+  if (ServoStyleSet* set = mPresShell->StyleSet()->GetAsServo()) {
+    // NOTE(emilio): If the root has a non-null binding, we'll stop at the
+    // document element and won't process any children, loading the bindings (or
+    // failing to do so) will take care of the rest.
+    set->StyleDocument(TraversalRestyleBehavior::Normal);
+  }
+
   // FIXME: Should this use ResolveStyleContext?  (The calls in this
   // function are the only case in nsCSSFrameConstructor where we don't
   // do so for the construction of a style context for an element.)
-  RefPtr<nsStyleContext> styleContext;
-  styleContext = mPresShell->StyleSet()->ResolveStyleFor(aDocElement,
-                                                         nullptr,
-                                                         LazyComputeBehavior::Allow);
+  RefPtr<nsStyleContext> styleContext =
+    mPresShell->StyleSet()->ResolveStyleFor(aDocElement,
+                                            nullptr,
+                                            LazyComputeBehavior::Assert);
 
   const nsStyleDisplay* display = styleContext->StyleDisplay();
 
   // Ensure that our XBL bindings are installed.
   if (display->mBinding) {
     // Get the XBL loader.
     nsresult rv;
     bool resolveStyle;
@@ -2539,42 +2546,48 @@ nsCSSFrameConstructor::ConstructDocEleme
     if (!xblService) {
       return nullptr;
     }
 
     RefPtr<nsXBLBinding> binding;
     rv = xblService->LoadBindings(aDocElement, display->mBinding->GetURI(),
                                   display->mBinding->mExtraData->GetPrincipal(),
                                   getter_AddRefs(binding), &resolveStyle);
-    if (NS_FAILED(rv) && rv != NS_ERROR_XBL_BLOCKED)
-      return nullptr; // Binding will load asynchronously.
+    if (NS_FAILED(rv) && rv != NS_ERROR_XBL_BLOCKED) {
+      // Binding will load asynchronously.
+      if (aDocElement->IsStyledByServo()) {
+        ServoRestyleManager::ClearServoDataFromSubtree(aDocElement);
+      }
+      return nullptr;
+    }
 
     if (binding) {
       // For backwards compat, keep firing the root's constructor
       // after all of its kids' constructors.  So tell the binding
       // manager about it right now.
       mDocument->BindingManager()->AddToAttachedQueue(binding);
     }
 
     if (resolveStyle) {
       // FIXME: Should this use ResolveStyleContext?  (The calls in this
       // function are the only case in nsCSSFrameConstructor where we
       // don't do so for the construction of a style context for an
       // element.)
-      styleContext = mPresShell->StyleSet()->ResolveStyleFor(aDocElement,
-                                                             nullptr,
-                                                             LazyComputeBehavior::Allow);
+      //
+      // FIXME(emilio): This looks fishy. It really wants to fully re-resolve
+      // the style, but it wont if the element is already styled afaict... It
+      // seems we handle it on a subsequent restyle?
+      styleContext = mPresShell->StyleSet()->ResolveStyleFor(
+          aDocElement, nullptr, LazyComputeBehavior::Assert);
       display = styleContext->StyleDisplay();
     }
-  }
-
-  // We delay traversing the entire document until here, since we per above we
-  // may invalidate the root style when we load doc stylesheets.
-  if (ServoStyleSet* set = mPresShell->StyleSet()->GetAsServo()) {
-    set->StyleDocument(TraversalRestyleBehavior::Normal);
+  } else if (display->mBinding.ForceGet() && aDocElement->IsStyledByServo()) {
+    // See the comment in AddFrameConstructionItemsInternal for why this is
+    // needed.
+    mPresShell->StyleSet()->AsServo()->StyleNewChildren(aDocElement);
   }
 
   // --------- IF SCROLLABLE WRAP IN SCROLLFRAME --------
 
   NS_ASSERTION(!display->IsScrollableOverflow() ||
                state.mPresContext->IsPaginated() ||
                propagatedScrollFrom == aDocElement,
                "Scrollbars should have been propagated to the viewport");
@@ -5868,19 +5881,18 @@ nsCSSFrameConstructor::AddFrameConstruct
           // XXX: We should have a better way to restyle ourselves.
           ServoRestyleManager::ClearServoDataFromSubtree(element);
           styleSet->StyleNewSubtree(element);
 
           // Servo's should_traverse_children() in traversal.rs skips
           // styling descendants of elements with a -moz-binding the
           // first time. Thus call StyleNewChildren() again.
           styleSet->StyleNewChildren(element);
-
           styleContext =
-            styleSet->ResolveStyleFor(element, nullptr, LazyComputeBehavior::Allow);
+            styleSet->ResolveStyleFor(element, nullptr, LazyComputeBehavior::Assert);
         } else {
           styleContext =
             ResolveStyleContext(styleContext->GetParent(), aContent, &aState);
         }
 
         display = styleContext->StyleDisplay();
         aStyleContext = styleContext;
       }
--- a/layout/base/nsPresContext.cpp
+++ b/layout/base/nsPresContext.cpp
@@ -1480,19 +1480,18 @@ GetPropagatedScrollbarStylesForViewport(
 
   // docElement might be null if we're doing this after removing it.
   if (!docElement) {
     return nullptr;
   }
 
   // Check the style on the document root element
   StyleSetHandle styleSet = aPresContext->StyleSet();
-  RefPtr<nsStyleContext> rootStyle;
-  rootStyle = styleSet->ResolveStyleFor(docElement, nullptr,
-                                        LazyComputeBehavior::Allow);
+  RefPtr<nsStyleContext> rootStyle =
+    styleSet->ResolveStyleFor(docElement, nullptr, LazyComputeBehavior::Allow);
   if (CheckOverflow(rootStyle->StyleDisplay(), aStyles)) {
     // tell caller we stole the overflow style from the root element
     return docElement;
   }
 
   // Don't look in the BODY for non-HTML documents or HTML documents
   // with non-HTML roots
   // XXX this should be earlier; we shouldn't even look at the document root
@@ -1509,19 +1508,19 @@ GetPropagatedScrollbarStylesForViewport(
   nsCOMPtr<nsIContent> bodyElement = do_QueryInterface(body);
 
   if (!bodyElement ||
       !bodyElement->NodeInfo()->Equals(nsGkAtoms::body)) {
     // The body is not a <body> tag, it's a <frameset>.
     return nullptr;
   }
 
-  RefPtr<nsStyleContext> bodyStyle;
-  bodyStyle = styleSet->ResolveStyleFor(bodyElement->AsElement(), rootStyle,
-                                        LazyComputeBehavior::Allow);
+  RefPtr<nsStyleContext> bodyStyle =
+    styleSet->ResolveStyleFor(bodyElement->AsElement(), rootStyle,
+                              LazyComputeBehavior::Allow);
 
   if (CheckOverflow(bodyStyle->StyleDisplay(), aStyles)) {
     // tell caller we stole the overflow style from the body element
     return bodyElement;
   }
 
   return nullptr;
 }