Bug 1746310 - Remove nsMenuPopupFrame::GenerateFrames and related code. r=tnikkel

The root of the problem is that nsMenuPopupFrame::GenerateFrames calls
into frame construction without making sure that styles are clean. So it
was pretty much working by chance, sorta.

I was going to fix this by adding the necessary flushes before calling
GenerateFrames, but on closer inspection, the front-end has effectively
already implemented this optimization by only generating the relevant
DOM on popupShowing:

  https://searchfox.org/mozilla-central/rev/a11b63915bd7810a03635d733123448ab5bfcad3/toolkit/content/widgets/menupopup.js#87-91

And for menulists on creation:

  https://searchfox.org/mozilla-central/rev/a11b63915bd7810a03635d733123448ab5bfcad3/toolkit/content/widgets/menupopup.js#151

After bug 1714846 we even destroy frames as needed, for panels.

So I think all of this complexity is unwarranted, and if we need some of
it we should implement it in the front-end like bug 1714846 did, and I'd
rather do this than flushing styles and so on.

There's one tweak I had to do to an nsPlaceholderFrame assertion. The
reason is that now the nsMenuPopupFrames do get their
NS_FRAME_FIRST_REFLOW bit cleared here:

  https://searchfox.org/mozilla-central/rev/bd25b1ca76dd5d323ffc69557f6cf759ba76ba23/layout/xul/nsMenuPopupFrame.cpp#557

Because the IsLeaf() condition here is no longer true:

  https://searchfox.org/mozilla-central/rev/bd25b1ca76dd5d323ffc69557f6cf759ba76ba23/layout/xul/nsMenuPopupFrame.cpp#532

It doesn't change anything though, because this condition never holded
for popups consistently.

Differential Revision: https://phabricator.services.mozilla.com/D134331

layout/base/nsCSSFrameConstructor.cpp

@@ -11700,31 +11700,6 @@ void nsCSSFrameConstructor::ReframeContainingBlock(nsIFrame* aFrame) {
                            InsertionKind::Async);
 }
 
-void nsCSSFrameConstructor::GenerateChildFrames(nsContainerFrame* aFrame) {
-  {
-    nsAutoScriptBlocker scriptBlocker;
-    nsFrameList childList;
-    nsFrameConstructorState state(mPresShell, nullptr, nullptr, nullptr);
-
-    nsFrameConstructorSaveState floatSaveState;
-    state.MaybePushFloatContainingBlock(aFrame, floatSaveState);
-
-    ProcessChildren(state, aFrame->GetContent(), aFrame->Style(), aFrame, false,
-                    childList, false);
-
-    aFrame->SetInitialChildList(kPrincipalList, childList);
-  }
-
-#ifdef ACCESSIBILITY
-  if (nsAccessibilityService* accService =
-          PresShell::GetAccessibilityService()) {
-    if (nsIContent* child = aFrame->GetContent()->GetFirstChild()) {
-      accService->ContentRangeInserted(mPresShell, child, nullptr);
-    }
-  }
-#endif
-}
-
 //////////////////////////////////////////////////////////
 // nsCSSFrameConstructor::FrameConstructionItem methods //
 //////////////////////////////////////////////////////////

layout/base/nsCSSFrameConstructor.h

@@ -275,9 +275,6 @@ class nsCSSFrameConstructor final : public nsFrameManager {
   bool EnsureFrameForTextNodeIsCreatedAfterFlush(
       mozilla::dom::CharacterData* aContent);
 
-  // Generate the child frames and process bindings
-  void GenerateChildFrames(nsContainerFrame* aFrame);
-
   // Should be called when a frame is going to be destroyed and
   // WillDestroyFrameTree hasn't been called yet.
   void NotifyDestroyingFrame(nsIFrame* aFrame);

layout/generic/FrameClass.py

@@ -6,7 +6,6 @@
 # Leaf constants to pass to Frame's leafness argument.
 LEAF = "Leaf"
 NOT_LEAF = "NotLeaf"
-DYNAMIC_LEAF = "DynamicLeaf"
 
 
 class FrameClass:

layout/generic/FrameClasses.py

@@ -3,7 +3,7 @@
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 # Frame class definitions, used to generate FrameIdList.h and FrameTypeList.h
 
-from FrameClass import Frame, AbstractFrame, LEAF, NOT_LEAF, DYNAMIC_LEAF
+from FrameClass import Frame, AbstractFrame, LEAF, NOT_LEAF
 
 FRAME_CLASSES = [
     Frame("BRFrame", "Br", LEAF),
@@ -68,7 +68,7 @@ FRAME_CLASSES = [
     Frame("nsMathMLTokenFrame", "None", NOT_LEAF),
     Frame("nsMenuBarFrame", "Box", NOT_LEAF),
     Frame("nsMenuFrame", "Menu", NOT_LEAF),
-    Frame("nsMenuPopupFrame", "MenuPopup", DYNAMIC_LEAF),
+    Frame("nsMenuPopupFrame", "MenuPopup", NOT_LEAF),
     Frame("nsMeterFrame", "Meter", LEAF),
     Frame("nsNumberControlFrame", "TextInput", LEAF),
     Frame("nsPageBreakFrame", "PageBreak", LEAF),

layout/generic/nsIFrame.cpp

@@ -171,13 +171,11 @@ const nsIFrame::FrameClassBits nsIFrame::sFrameClassBits[
     0] = {
 #define Leaf eFrameClassBitsLeaf
 #define NotLeaf eFrameClassBitsNone
-#define DynamicLeaf eFrameClassBitsDynamicLeaf
 #define FRAME_ID(class_, type_, leaf_, ...) leaf_,
 #define ABSTRACT_FRAME_ID(...)
 #include "mozilla/FrameIdList.h"
 #undef Leaf
 #undef NotLeaf
-#undef DynamicLeaf
 #undef FRAME_ID
 #undef ABSTRACT_FRAME_ID
 };

layout/generic/nsIFrame.h

@@ -3364,9 +3364,6 @@ class nsIFrame : public nsQueryFrame {
   bool IsLeaf() const {
     MOZ_ASSERT(uint8_t(mClass) < mozilla::ArrayLength(sFrameClassBits));
     FrameClassBits bits = sFrameClassBits[uint8_t(mClass)];
-    if (MOZ_UNLIKELY(bits & eFrameClassBitsDynamicLeaf)) {
-      return IsLeafDynamic();
-    }
     return bits & eFrameClassBitsLeaf;
   }
 
@@ -4964,13 +4961,6 @@ class nsIFrame : public nsQueryFrame {
   void ReparentFrameViewTo(nsViewManager* aViewManager, nsView* aNewParentView,
                            nsView* aOldParentView);
 
-  /**
-   * To be overridden by frame classes that have a varying IsLeaf() state and
-   * is indicating that with DynamicLeaf in FrameIdList.h.
-   * @see IsLeaf()
-   */
-  virtual bool IsLeafDynamic() const { return false; }
-
   // Members
   nsRect mRect;
   nsCOMPtr<nsIContent> mContent;
@@ -5385,7 +5375,6 @@ class nsIFrame : public nsQueryFrame {
   enum FrameClassBits {
     eFrameClassBitsNone = 0x0,
     eFrameClassBitsLeaf = 0x1,
-    eFrameClassBitsDynamicLeaf = 0x2,
   };
   // Maps mClass to IsLeaf() flags.
   static const FrameClassBits sFrameClassBits[

layout/generic/nsPlaceholderFrame.cpp

@@ -110,10 +110,16 @@ void nsPlaceholderFrame::Reflow(nsPresContext* aPresContext,
   // (See bug 1367711.)
 
 #ifdef DEBUG
-  // We should be getting reflowed before our out-of-flow.
-  // If this is our first reflow, and our out-of-flow has already received its
-  // first reflow (before us), complain.
+  // We should be getting reflowed before our out-of-flow. If this is our first
+  // reflow, and our out-of-flow has already received its first reflow (before
+  // us), complain.
+  //
+  // Popups are an exception though, because their position doesn't depend on
+  // the placeholder, so they don't have this requirement (and this condition
+  // doesn't hold anyways because the default popupgroup goes before than the
+  // default tooltip, for example).
   if (HasAnyStateBits(NS_FRAME_FIRST_REFLOW) &&
+      !HasAnyStateBits(PLACEHOLDER_FOR_POPUP) &&
       !mOutOfFlowFrame->HasAnyStateBits(NS_FRAME_FIRST_REFLOW)) {
     // Unfortunately, this can currently happen when the placeholder is in a
     // later continuation or later IB-split sibling than its out-of-flow (as

layout/xul/nsMenuFrame.cpp

@@ -1096,13 +1096,8 @@ nsMenuFrame::GetActiveChild(dom::Element** aResult) {
 NS_IMETHODIMP
 nsMenuFrame::SetActiveChild(dom::Element* aChild) {
   nsMenuPopupFrame* popupFrame = GetPopup();
-  if (!popupFrame) return NS_ERROR_FAILURE;
-
-  // Force the child frames within the popup to be generated.
-  AutoWeakFrame weakFrame(popupFrame);
-  popupFrame->GenerateFrames();
-  if (!weakFrame.IsAlive()) {
-    return NS_OK;
+  if (!popupFrame) {
+    return NS_ERROR_FAILURE;
   }
 
   if (!aChild) {
@@ -1112,7 +1107,9 @@ nsMenuFrame::SetActiveChild(dom::Element* aChild) {
   }
 
   nsMenuFrame* menu = do_QueryFrame(aChild->GetPrimaryFrame());
-  if (menu) popupFrame->ChangeMenuItem(menu, false, false);
+  if (menu) {
+    popupFrame->ChangeMenuItem(menu, false, false);
+  }
   return NS_OK;
 }
 

layout/xul/nsMenuPopupFrame.cpp

@@ -117,7 +117,6 @@ nsMenuPopupFrame::nsMenuPopupFrame(ComputedStyle* aStyle,
       mIsOpenChanged(false),
       mIsContextMenu(false),
       mAdjustOffsetForContextMenu(false),
-      mGeneratedChildren(false),
       mMenuCanOverlapOSBar(false),
       mShouldAutoPosition(true),
       mInContentShell(true),
@@ -136,6 +135,41 @@ nsMenuPopupFrame::nsMenuPopupFrame(ComputedStyle* aStyle,
 
 nsMenuPopupFrame::~nsMenuPopupFrame() = default;
 
+bool nsMenuPopupFrame::ShouldCreateWidgetUpfront() const {
+  if (mPopupType != ePopupTypeMenu) {
+    // Any panel with a type attribute, such as the autocomplete popup, is
+    // always generated right away.
+    return mContent->AsElement()->HasAttr(nsGkAtoms::type);
+  }
+  // Generate the widget up-front if the following is true:
+  //
+  // - If the parent menu is a <menulist> unless its sizetopopup is set to
+  // "none".
+  // - For other elements, if the parent menu has a sizetopopup attribute.
+  //
+  // In these cases the size of the parent menu is dependent on the size of the
+  // popup, so the widget needs to exist in order to calculate this size.
+  nsIContent* parentContent = mContent->GetParent();
+  if (!parentContent) {
+    return true;
+  }
+
+  if (parentContent->IsXULElement(nsGkAtoms::menulist)) {
+    Element* parent = parentContent->AsElement();
+    nsAutoString sizedToPopup;
+    if (!parent->GetAttr(nsGkAtoms::sizetopopup, sizedToPopup)) {
+      // No prop set, generate child frames normally for the default value
+      // ("pref").
+      return true;
+    }
+    // Don't generate child frame only if the property is set to none.
+    return !sizedToPopup.EqualsLiteral("none");
+  }
+
+  return parentContent->IsElement() &&
+         parentContent->AsElement()->HasAttr(nsGkAtoms::sizetopopup);
+}
+
 void nsMenuPopupFrame::Init(nsIContent* aContent, nsContainerFrame* aParent,
                             nsIFrame* aPrevInFlow) {
   nsBoxFrame::Init(aContent, aParent, aPrevInFlow);
@@ -178,10 +212,13 @@ void nsMenuPopupFrame::Init(nsIContent* aContent, nsContainerFrame* aParent,
       mInContentShell = false;
     }
   }
-  // To improve performance, create the widget for the popup only if it is not
-  // a leaf. Leaf popups such as menus will create their widgets later when
-  // the popup opens.
-  if (!IsLeaf() && !ourView->HasWidget()) {
+
+  // To improve performance, create the widget for the popup if needed. Popups
+  // such as menus will create their widgets later when the popup opens.
+  //
+  // FIXME(emilio): Doing this up-front for all menupopups causes a bunch of
+  // assertions, while it's supposed to be just an optimization.
+  if (!ourView->HasWidget() && ShouldCreateWidgetUpfront()) {
     CreateWidgetForView(ourView);
   }
 
@@ -267,9 +304,6 @@ void nsMenuPopupFrame::EnsureWidget(bool aRecreate) {
     ourView->DestroyWidget();
   }
   if (!ourView->HasWidget()) {
-    NS_ASSERTION(aRecreate || (!mGeneratedChildren &&
-                               !PrincipalChildList().FirstChild()),
-                 "Creating widget for MenuPopupFrame with children");
     CreateWidgetForView(ourView);
   }
 }
@@ -449,49 +483,6 @@ void nsXULPopupShownEvent::CancelListener() {
 NS_IMPL_ISUPPORTS_INHERITED(nsXULPopupShownEvent, Runnable,
                             nsIDOMEventListener);
 
-bool nsMenuPopupFrame::IsLeafDynamic() const {
-  if (mGeneratedChildren) return false;
-
-  if (mPopupType != ePopupTypeMenu) {
-    // any panel with a type attribute, such as the autocomplete popup,
-    // is always generated right away.
-    return !mContent->AsElement()->HasAttr(kNameSpaceID_None, nsGkAtoms::type);
-  }
-
-  // menu popups generate their child frames lazily only when opened, so
-  // behave like a leaf frame. However, generate child frames normally if the
-  // following is true:
-  //
-  // - If the parent menu is a <menulist> unless its sizetopopup is set to
-  // "none".
-  // - For other elements, if the parent menu has a sizetopopup attribute.
-  //
-  // In these cases the size of the parent menu is dependent on the size of
-  // the popup, so the frames need to exist in order to calculate this size.
-  nsIContent* parentContent = mContent->GetParent();
-  if (!parentContent) {
-    return false;
-  }
-
-  if (parentContent->IsXULElement(nsGkAtoms::menulist)) {
-    Element* parent = parentContent->AsElement();
-    if (!parent->HasAttr(kNameSpaceID_None, nsGkAtoms::sizetopopup)) {
-      // No prop set, generate child frames normally for the
-      // default value ("pref").
-      return false;
-    }
-
-    nsAutoString sizedToPopup;
-    parent->GetAttr(kNameSpaceID_None, nsGkAtoms::sizetopopup, sizedToPopup);
-    // Don't generate child frame only if the property is set to none.
-    return sizedToPopup.EqualsLiteral("none");
-  }
-
-  return (!parentContent->IsElement() ||
-          !parentContent->AsElement()->HasAttr(kNameSpaceID_None,
-                                               nsGkAtoms::sizetopopup));
-}
-
 void nsMenuPopupFrame::DidSetComputedStyle(ComputedStyle* aOldStyle) {
   nsBoxFrame::DidSetComputedStyle(aOldStyle);
 
@@ -529,7 +520,7 @@ void nsMenuPopupFrame::DidSetComputedStyle(ComputedStyle* aOldStyle) {
 
 void nsMenuPopupFrame::LayoutPopup(nsBoxLayoutState& aState,
                                    nsIFrame* aParentMenu, bool aSizedToPopup) {
-  if (IsLeaf() || IsNativeMenu()) {
+  if (IsNativeMenu()) {
     return;
   }
 
@@ -1790,17 +1781,6 @@ void nsMenuPopupFrame::WidgetPositionOrSizeDidChange() {
   }
 }
 
-void nsMenuPopupFrame::GenerateFrames() {
-  const bool generateFrames = IsLeaf();
-  MOZ_ASSERT_IF(generateFrames, !mGeneratedChildren);
-  mGeneratedChildren = true;
-  if (generateFrames) {
-    MOZ_ASSERT(PrincipalChildList().IsEmpty());
-    RefPtr<mozilla::PresShell> presShell = PresContext()->PresShell();
-    presShell->FrameConstructor()->GenerateChildFrames(this);
-  }
-}
-
 /* virtual */
 nsMenuFrame* nsMenuPopupFrame::GetCurrentMenuItem() { return mCurrentMenu; }
 

layout/xul/nsMenuPopupFrame.h

@@ -193,7 +193,10 @@ class nsMenuPopupFrame final : public nsBoxFrame,
 
   bool HasRemoteContent() const;
 
-  // returns true if the popup is a panel with the noautohide attribute set to
+  // Whether we should create a widget on Init().
+  bool ShouldCreateWidgetUpfront() const;
+
+  // Returns true if the popup is a panel with the noautohide attribute set to
   // true. These panels do not roll up automatically.
   bool IsNoAutoHide() const;
 
@@ -207,8 +210,6 @@ class nsMenuPopupFrame final : public nsBoxFrame,
   nsresult CreateWidgetForView(nsView* aView);
   mozilla::StyleWindowShadow GetShadowStyle();
 
-  bool IsLeafDynamic() const override;
-
   void DidSetComputedStyle(ComputedStyle* aOldStyle) override;
 
   // layout, position and display the popup as needed
@@ -224,9 +225,6 @@ class nsMenuPopupFrame final : public nsBoxFrame,
   nsresult SetPopupPosition(nsIFrame* aAnchorFrame, bool aIsMove,
                             bool aSizedToPopup);
 
-  // Force the children to be generated if they have not already been generated.
-  void GenerateFrames();
-
   // called when the Enter key is pressed while the popup is open. This will
   // just pass the call down to the current menu, if any. If a current menu
   // should be opened as a result, this method should return the frame for
@@ -615,7 +613,6 @@ class nsMenuPopupFrame final : public nsBoxFrame,
   bool mIsContextMenu;  // true for context menus
   // true if we need to offset the popup to ensure it's not under the mouse
   bool mAdjustOffsetForContextMenu;
-  bool mGeneratedChildren;  // true if the contents have been created
 
   bool mMenuCanOverlapOSBar;  // can we appear over the taskbar/menubar?
   bool mShouldAutoPosition;   // Should SetPopupPosition be allowed to auto

layout/xul/nsXULPopupManager.cpp

@@ -1560,14 +1560,11 @@ void nsXULPopupManager::BeginShowingPopup(const PendingPopup& aPendingPopup,
                                           bool aSelectFirstItem) {
   nsCOMPtr<nsIContent> popup = aPendingPopup.mPopup;
 
-  nsMenuPopupFrame* popupFrame = do_QueryFrame(popup->GetPrimaryFrame());
-  if (!popupFrame) return;
-
-  popupFrame->GenerateFrames();
-
-  // get the frame again
-  popupFrame = do_QueryFrame(popup->GetPrimaryFrame());
-  if (!popupFrame) return;
+  nsMenuPopupFrame* popupFrame =
+      do_QueryFrame(popup->GetPrimaryFrame());
+  if (!popupFrame) {
+    return;
+  }
 
   nsPresContext* presContext = popupFrame->PresContext();
   RefPtr<PresShell> presShell = presContext->PresShell();

layout/xul/test/test_bug372685.xhtml

@@ -12,7 +12,7 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=372685
 
 <menuitem id="a" style="display: -moz-box;">
 <box id="b" style="display: -moz-popup; ">
-<box id="c" style="position: fixed;"></box>
+<box id="c" style="position: fixed; display: block;"></box>
 </box>
 </menuitem>