Bug 1377902: Trivially cleanup a bit of the FC code. r=mats draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 05 Jul 2017 16:30:24 +0200
changeset 604332 44ba5caf31e890759b2b68dfa8d507ca149a542e
parent 604331 207dd6b9d716c5dcf8d577faf1b2f7f42a38d657
child 636149 38966ea22dca85aa22053e5511ea6cae8cfd8a2f
push id67025
push userbmo:emilio+bugs@crisal.io
push dateWed, 05 Jul 2017 18:45:23 +0000
reviewersmats
bugs1377902
milestone56.0a1
Bug 1377902: Trivially cleanup a bit of the FC code. r=mats MozReview-Commit-ID: HJKVk7lWp4p
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -4273,17 +4273,18 @@ ConnectAnonymousTreeDescendants(nsIConte
     NS_ASSERTION(content, "null anonymous content?");
 
     ConnectAnonymousTreeDescendants(content, aContent[i].mChildren);
 
     aParent->AppendChildTo(content, false);
   }
 }
 
-void SetNativeAnonymousBitOnDescendants(nsIContent *aRoot)
+static void
+SetNativeAnonymousBitOnDescendants(nsIContent* aRoot)
 {
   for (nsIContent* curr = aRoot; curr; curr = curr->GetNextNode(aRoot)) {
     curr->SetFlags(NODE_IS_NATIVE_ANONYMOUS);
   }
 }
 
 nsresult
 nsCSSFrameConstructor::GetAnonymousContent(nsIContent* aParent,
@@ -5893,17 +5894,17 @@ nsCSSFrameConstructor::AddFrameConstruct
         // children after they have been rearranged in the flattened tree.
         // If the URL couldn't be resolved, we still need to style the children,
         // so we do that here.
         mPresShell->StyleSet()->AsServo()->StyleNewChildren(aContent->AsElement());
       }
     }
   }
 
-  bool isGeneratedContent = ((aFlags & ITEM_IS_GENERATED_CONTENT) != 0);
+  const bool isGeneratedContent = !!(aFlags & ITEM_IS_GENERATED_CONTENT);
 
   // Pre-check for display "none" - if we find that, don't create
   // any frame at all
   if (StyleDisplay::None == display->mDisplay) {
     SetAsUndisplayedContent(aState, aItems, aContent, styleContext, isGeneratedContent);
     return;
   }
 
@@ -7337,27 +7338,29 @@ nsCSSFrameConstructor::CreateNeededFrame
       TreeMatchContext::AutoAncestorPusher pusher(&aTreeMatchContext);
       pusher.PushAncestorAndStyleScope(child);
 
       CreateNeededFrames(child, aTreeMatchContext);
     }
   }
 }
 
-void nsCSSFrameConstructor::CreateNeededFrames()
+void
+nsCSSFrameConstructor::CreateNeededFrames()
 {
   NS_ASSERTION(!nsContentUtils::IsSafeToRunScript(),
                "Someone forgot a script blocker");
 
   Element* rootElement = mDocument->GetRootElement();
   NS_ASSERTION(!rootElement || !rootElement->HasFlag(NODE_NEEDS_FRAME),
     "root element should not have frame created lazily");
   if (rootElement && rootElement->HasFlag(NODE_DESCENDANTS_NEED_FRAMES)) {
     BeginUpdate();
-    TreeMatchContext treeMatchContext(mDocument, TreeMatchContext::ForFrameConstruction);
+    TreeMatchContext treeMatchContext(
+        mDocument, TreeMatchContext::ForFrameConstruction);
     treeMatchContext.InitAncestors(rootElement);
     CreateNeededFrames(rootElement, treeMatchContext);
     EndUpdate();
   }
 }
 
 void
 nsCSSFrameConstructor::IssueSingleInsertNofications(nsIContent* aContainer,
@@ -7944,36 +7947,32 @@ nsCSSFrameConstructor::ContentRangeInser
   MOZ_ASSERT(!aAllowLazyConstruction || !RestyleManager()->IsInStyleRefresh());
 
   AUTO_LAYOUT_PHASE_ENTRY_POINT(mPresShell->GetPresContext(), FrameC);
   NS_PRECONDITION(mUpdateCount != 0,
                   "Should be in an update while creating frames");
 
   NS_PRECONDITION(aStartChild, "must always pass a child");
 
-  // XXXldb Do we need to re-resolve style to handle the CSS2 + combinator and
-  // the :empty pseudo-class?
 #ifdef DEBUG
   if (gNoisyContentUpdates) {
     printf("nsCSSFrameConstructor::ContentRangeInserted container=%p "
            "start-child=%p end-child=%p lazy=%d\n",
            static_cast<void*>(aContainer),
            static_cast<void*>(aStartChild), static_cast<void*>(aEndChild),
            aAllowLazyConstruction);
     if (gReallyNoisyContentUpdates) {
       if (aContainer) {
         aContainer->List(stdout,0);
       } else {
         aStartChild->List(stdout, 0);
       }
     }
   }
-#endif
-
-#ifdef DEBUG
+
   for (nsIContent* child = aStartChild;
        child != aEndChild;
        child = child->GetNextSibling()) {
     // XXX the GetContent() != child check is needed due to bug 135040.
     // Remove it once that's fixed.
     NS_ASSERTION(!child->GetPrimaryFrame() ||
                  child->GetPrimaryFrame()->GetContent() != child,
                  "asked to construct a frame for a node that already has a frame");
@@ -8005,55 +8004,49 @@ nsCSSFrameConstructor::ContentRangeInser
       return;
     }
   }
 #endif // MOZ_XUL
 
   // If we have a null parent, then this must be the document element being
   // inserted, or some other child of the document in the DOM (might be a PI,
   // say).
-  if (! aContainer) {
+  if (!aContainer) {
     NS_ASSERTION(isSingleInsert,
                  "root node insertion should be a single insertion");
-    Element *docElement = mDocument->GetRootElement();
+    Element* docElement = mDocument->GetRootElement();
 
     if (aStartChild != docElement) {
       // Not the root element; just bail out
       return;
     }
 
-    NS_PRECONDITION(nullptr == mRootElementFrame,
-                    "root element frame already created");
+    NS_PRECONDITION(!mRootElementFrame, "root element frame already created");
 
     // Create frames for the document element and its child elements
-    nsIFrame* docElementFrame =
-      ConstructDocElementFrame(docElement, aFrameState);
-
-    if (docElementFrame) {
+    if (ConstructDocElementFrame(docElement, aFrameState)) {
       InvalidateCanvasIfNeeded(mPresShell, aStartChild);
 #ifdef DEBUG
       if (gReallyNoisyContentUpdates) {
         printf("nsCSSFrameConstructor::ContentRangeInserted: resulting frame "
                "model:\n");
-        docElementFrame->List(stdout, 0);
+        mRootElementFrame->List(stdout, 0);
       }
 #endif
     }
 
     if (aFrameState) {
       // Restore frame state for the root scroll frame if there is one
-      nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame();
-      if (rootScrollFrame) {
+      if (nsIFrame* rootScrollFrame = mPresShell->GetRootScrollFrame()) {
         RestoreFrameStateFor(rootScrollFrame, aFrameState);
       }
     }
 
 #ifdef ACCESSIBILITY
-    nsAccessibilityService* accService = nsIPresShell::AccService();
-    if (accService) {
+    if (nsAccessibilityService* accService = nsIPresShell::AccService()) {
       accService->ContentRangeInserted(mPresShell, aContainer,
                                        aStartChild, aEndChild);
     }
 #endif
 
     return;
   }
 
@@ -8544,37 +8537,42 @@ nsCSSFrameConstructor::ContentRemoved(ns
   NS_PRECONDITION(mUpdateCount != 0,
                   "Should be in an update while destroying frames");
 
   *aDidReconstruct = false;
   if (aDestroyedFramesFor) {
     *aDestroyedFramesFor = aChild;
   }
 
-  // We're destroying our frame(s). This normally happens either when the content
-  // is being removed from the DOM (in which case we'll drop all Servo data in
-  // UnbindFromTree), or when we're recreating frames (usually in response to
-  // having retrieved a ReconstructFrame change hint after restyling). In both of
-  // those cases, there are no pending restyles we need to worry about.
+  // We're destroying our frame(s). This normally happens either when the
+  // content is being removed from the DOM (in which case we'll drop all Servo
+  // data in UnbindFromTree), or when we're recreating frames (usually in
+  // response to having retrieved a ReconstructFrame change hint after
+  // restyling). In both of those cases, there are no pending restyles we need
+  // to worry about.
   //
   // However, there is also the (rare) DestroyFramesFor path, in which we tear
   // down (and usually recreate) the frames for a subtree. In this case, leaving
   // the style data on the elements is problematic for our invariants, because
-  // there might be pending restyles in the subtree. If we simply leave them as-is,
-  // the subsequent traversal when recreating frames will generate a bunch of bogus
-  // change hints to update frames that no longer exist.
+  // there might be pending restyles in the subtree. If we simply leave them
+  // as-is, the subsequent traversal when recreating frames will generate a
+  // bunch of bogus change hints to update frames that no longer exist.
   //
-  // So the two obvious options are to (1) process all pending restyles and take all
-  // the change hints before destroying the frames, or (2) drop all the style data.
-  // We chose the latter, since that matches the performance characteristics of the
-  // old Gecko style system.
+  // So the two obvious options are to (1) process all pending restyles and take
+  // all the change hints before destroying the frames, or (2) drop all the
+  // style data.  We chose the latter, since that matches the performance
+  // characteristics of the old Gecko style system.
   //
-  // That said, it's almost certainly possible to optimize this if it turns out to be
-  // hot. It's just not a priority at the moment.
-  if (aFlags == REMOVE_DESTROY_FRAMES && aChild->IsElement() && aChild->IsStyledByServo()) {
+  // That said, it's almost certainly possible to optimize this if it turns out
+  // to be hot. It's just not a priority at the moment.
+  //
+  // FIXME(emilio): This really really feels like a hack, and it's only for the
+  // XBL/Shadow DOM path, so we should do this there instead.
+  if (aFlags == REMOVE_DESTROY_FRAMES && aChild->IsElement() &&
+      aChild->IsStyledByServo()) {
     ServoRestyleManager::ClearServoDataFromSubtree(aChild->AsElement());
   }
 
   nsPresContext* presContext = mPresShell->GetPresContext();
   MOZ_ASSERT(presContext, "Our presShell should have a valid presContext");
 
   if (aChild->IsHTMLElement(nsGkAtoms::body) ||
       (!aContainer && aChild->IsElement())) {
@@ -8583,19 +8581,16 @@ nsCSSFrameConstructor::ContentRemoved(ns
     // possible scrollbar-propagation sources: the <body> [as aChild or a
     // descendant] and the root node. The other possible scrollbar-propagation
     // source is a fullscreen element, and we have code elsewhere to update
     // scrollbars after fullscreen elements are removed -- specifically, it's
     // part of the fullscreen cleanup code called by Element::UnbindFromTree.)
     presContext->UpdateViewportScrollbarStylesOverride();
   }
 
-  // XXXldb Do we need to re-resolve style to handle the CSS2 + combinator and
-  // the :empty pseudo-class?
-
 #ifdef DEBUG
   if (gNoisyContentUpdates) {
     printf("nsCSSFrameConstructor::ContentRemoved container=%p child=%p "
            "old-next-sibling=%p\n",
            static_cast<void*>(aContainer),
            static_cast<void*>(aChild),
            static_cast<void*>(aOldNextSibling));
     if (gReallyNoisyContentUpdates) {
@@ -8650,17 +8645,16 @@ nsCSSFrameConstructor::ContentRemoved(ns
 #ifdef MOZ_XUL
   if (NotifyListBoxBody(presContext, aContainer, aChild, aOldNextSibling,
                         childFrame, CONTENT_REMOVED)) {
     if (aFlags == REMOVE_DESTROY_FRAMES) {
       CaptureStateForFramesOf(aChild, mTempFrameTreeState);
     }
     return;
   }
-
 #endif // MOZ_XUL
 
   // If we're removing the root, then make sure to remove things starting at
   // the viewport's child instead of the primary frame (which might even be
   // null if the root had an XBL binding or display:none, even though the
   // frames above it got created).  We do the adjustment after the childFrame
   // check above, because we do want to clear any undisplayed content we might
   // have for the root.  Detecting removal of a root is a little exciting; in
@@ -8679,20 +8673,20 @@ nsCSSFrameConstructor::ContentRemoved(ns
         NS_ASSERTION(!childFrame->GetNextSibling(), "How did that happen?");
       }
     }
   }
 
   if (aContainer && aContainer->HasFlag(NODE_IS_IN_SHADOW_TREE) &&
       !aContainer->IsInNativeAnonymousSubtree() &&
       !aChild->IsInNativeAnonymousSubtree()) {
-    // Recreate frames if content is removed from a ShadowRoot
-    // because it may contain an insertion point which can change
-    // how the host is rendered.
-    //XXXsmaug This is super unefficient!
+    // Recreate frames if content is removed from a ShadowRoot because it may
+    // contain an insertion point which can change how the host is rendered.
+    //
+    // XXXsmaug This is super unefficient!
     nsIContent* bindingParent = aContainer->GetBindingParent();
     *aDidReconstruct = true;
     LAYOUT_PHASE_TEMP_EXIT();
     RecreateFramesForContent(bindingParent, false, aFlags, aDestroyedFramesFor);
     LAYOUT_PHASE_TEMP_REENTER();
     return;
   }
 
@@ -9097,33 +9091,38 @@ nsCSSFrameConstructor::WillDestroyFrameT
 
   nsFrameManager::Destroy();
 }
 
 //STATIC
 
 // XXXbz I'd really like this method to go away. Once we have inline-block and
 // I can just use that for sized broken images, that can happen, maybe.
-void nsCSSFrameConstructor::GetAlternateTextFor(nsIContent*    aContent,
-                                                nsIAtom*       aTag,  // content object's tag
-                                                nsXPIDLString& aAltText)
+void
+nsCSSFrameConstructor::GetAlternateTextFor(nsIContent*    aContent,
+                                           nsIAtom*       aTag,
+                                           nsXPIDLString& aAltText)
 {
   // The "alt" attribute specifies alternate text that is rendered
-  // when the image can not be displayed
-
-  // If there's no "alt" attribute, and aContent is an input
-  // element, then use the value of the "value" attribute
-  if (!aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::alt, aAltText) &&
-      nsGkAtoms::input == aTag) {
-    // If there's no "value" attribute either, then use the localized string
-    // for "Submit" as the alternate text.
-    if (!aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::value, aAltText)) {
-      nsContentUtils::GetLocalizedString(nsContentUtils::eFORMS_PROPERTIES,
-                                         "Submit", aAltText);
-    }
+  // when the image can not be displayed.
+  if (aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::alt, aAltText)) {
+    return;
+  }
+
+  if (nsGkAtoms::input == aTag) {
+    // If there's no "alt" attribute, and aContent is an input element, then use
+    // the value of the "value" attribute
+    if (aContent->GetAttr(kNameSpaceID_None, nsGkAtoms::value, aAltText)) {
+      return;
+    }
+
+    // If there's no "value" attribute either, then use the localized string for
+    // "Submit" as the alternate text.
+    nsContentUtils::GetLocalizedString(nsContentUtils::eFORMS_PROPERTIES,
+                                       "Submit", aAltText);
   }
 }
 
 nsIFrame*
 nsCSSFrameConstructor::CreateContinuingOuterTableFrame(nsIPresShell*     aPresShell,
                                                        nsPresContext*    aPresContext,
                                                        nsIFrame*         aFrame,
                                                        nsContainerFrame* aParentFrame,
@@ -10849,17 +10848,18 @@ nsCSSFrameConstructor::WrapItemsInPseudo
 
   // Eat up all items between |aIter| and |aEndIter| and put them in our
   // wrapper Advances |aIter| to point to |aEndIter|.
   aIter.AppendItemsToList(aEndIter, newItem->mChildItems);
 
   aIter.InsertItem(newItem);
 }
 
-void nsCSSFrameConstructor::CreateNeededPseudoSiblings(
+void
+nsCSSFrameConstructor::CreateNeededPseudoSiblings(
     nsFrameConstructorState& aState,
     FrameConstructionItemList& aItems,
     nsIFrame* aParentFrame)
 {
   if (aItems.IsEmpty() ||
       GetParentType(aParentFrame) != eTypeRuby) {
     return;
   }
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -55,17 +55,17 @@ public:
   typedef mozilla::CSSPseudoElementType CSSPseudoElementType;
   typedef mozilla::dom::Element Element;
 
   friend class mozilla::RestyleManager;
   friend class mozilla::GeckoRestyleManager;
   friend class mozilla::ServoRestyleManager;
 
   nsCSSFrameConstructor(nsIDocument* aDocument, nsIPresShell* aPresShell);
-  ~nsCSSFrameConstructor(void) {
+  ~nsCSSFrameConstructor() {
     MOZ_ASSERT(mUpdateCount == 0, "Dying in the middle of our own update?");
   }
 
   // get the alternate text for a content node
   static void GetAlternateTextFor(nsIContent*    aContent,
                                   nsIAtom*       aTag,  // content object's tag
                                   nsXPIDLString& aAltText);