Bug 1348665 part 2 - Remove the ViewProperty and store the nsView* in a field on the relevant frame classes instead. r=tnikkel

The relevant frame classes are:
SubDocumentFrame
ListControlFrame (only when used for (non-e10s?) comboboxes)
PluginFrame
ViewportFrame
MenuPopupFrame

The view is now created in the frame's Init() method, except for
ViewportFrame which has its view assigned by the frame constructor
via a SetView() call.

MozReview-Commit-ID: 4O7Hm1yqwIp

layout/base/nsCSSFrameConstructor.cpp

@@ -3241,10 +3241,6 @@ nsCSSFrameConstructor::InitializeSelectFrame(nsFrameConstructorState& aState,
                     aStyleContext, aParentFrame);
   }
 
-  if (aBuildCombobox) {
-    nsFrame::CreateViewForFrame(scrollFrame, true);
-  }
-
   BuildScrollFrame(aState, aContent, aStyleContext, scrolledFrame,
                    geometricParent, scrollFrame);
 
@@ -3929,8 +3925,6 @@ nsCSSFrameConstructor::ConstructFrameFromItemInternal(FrameConstructionItem& aIt
       frameToAddToList = scrollframe;
     } else {
       InitAndRestoreFrame(aState, content, geometricParent, newFrame);
-      // See whether we need to create a view
-      nsFrame::CreateViewForFrame(newFrame, false);
       frameToAddToList = newFrame;
     }
 

layout/forms/nsListControlFrame.cpp

@@ -93,6 +93,7 @@ NS_IMPL_FRAMEARENA_HELPERS(nsListControlFrame)
 //---------------------------------------------------------
 nsListControlFrame::nsListControlFrame(nsStyleContext* aContext)
   : nsHTMLScrollFrame(aContext, false),
+    mView(nullptr),
     mMightNeedSecondPass(false),
     mHasPendingInterruptAtStartOfReflow(false),
     mDropdownCanGrow(false),
@@ -975,6 +976,11 @@ nsListControlFrame::Init(nsIContent*       aContent,
 {
   nsHTMLScrollFrame::Init(aContent, aParent, aPrevInFlow);
 
+  if (IsInDropDownMode()) {
+    AddStateBits(NS_FRAME_IN_POPUP);
+    CreateView();
+  }
+
   // we shouldn't have to unregister this listener because when
   // our frame goes away all these content node go away as well
   // because our frame is the only one who references them.
@@ -996,10 +1002,6 @@ nsListControlFrame::Init(nsIContent*       aContent,
   mEndSelectionIndex = kNothingSelected;
 
   mLastDropdownBackstopColor = PresContext()->DefaultBackgroundColor();
-
-  if (IsInDropDownMode()) {
-    AddStateBits(NS_FRAME_IN_POPUP);
-  }
 }
 
 dom::HTMLOptionsCollection*

layout/forms/nsListControlFrame.h

@@ -239,11 +239,6 @@ public:
    */
   bool GetDropdownCanGrow() const { return mDropdownCanGrow; }
 
-  /**
-   * Dropdowns need views
-   */
-  virtual bool NeedsView() override { return IsInDropDownMode(); }
-
   /**
    * Frees statics owned by this class.
    */
@@ -398,16 +393,23 @@ protected:
    */
   uint32_t GetNumberOfRows();
 
+  nsView* GetViewInternal() const override { return mView; }
+  void SetViewInternal(nsView* aView) override { mView = aView; }
+
   // Data Members
   int32_t      mStartSelectionIndex;
   int32_t      mEndSelectionIndex;
 
-  nsIComboboxControlFrame *mComboboxFrame;
-  uint32_t     mNumDisplayRows;
+  nsIComboboxControlFrame* mComboboxFrame;
+
+  // The view is only created (& non-null) if IsInDropDownMode() is true.
+  nsView* mView;
+
+  uint32_t mNumDisplayRows;
   bool mChangesSinceDragStart:1;
   bool mButtonDown:1;
-  // Has the user selected a visible item since we showed the
-  // dropdown?
+
+  // Has the user selected a visible item since we showed the dropdown?
   bool mItemSelectionStarted:1;
 
   bool mIsAllContentHere:1;

layout/generic/ViewportFrame.cpp

@@ -39,6 +39,8 @@ ViewportFrame::Init(nsIContent*       aContent,
                     nsIFrame*         aPrevInFlow)
 {
   nsContainerFrame::Init(aContent, aParent, aPrevInFlow);
+  // No need to call CreateView() here - the frame ctor will call SetView()
+  // with the ViewManager's root view, so we'll assign it in SetViewInternal().
 
   nsIFrame* parent = nsLayoutUtils::GetCrossDocParentFrame(this);
   if (parent) {

layout/generic/ViewportFrame.h

@@ -31,6 +31,7 @@ public:
 
   explicit ViewportFrame(nsStyleContext* aContext)
     : nsContainerFrame(aContext)
+    , mView(nullptr)
   {}
   virtual ~ViewportFrame() { } // useful for debugging
 
@@ -83,9 +84,6 @@ public:
   virtual nsresult GetFrameName(nsAString& aResult) const override;
 #endif
 
-private:
-  virtual mozilla::layout::FrameChildListID GetAbsoluteListID() const override { return kFixedList; }
-
 protected:
   /**
    * Calculate how much room is available for fixed frames. That means
@@ -95,6 +93,14 @@ protected:
    * @return the current scroll position, or 0,0 if not scrollable
    */
   nsPoint AdjustReflowInputForScrollbars(ReflowInput* aReflowInput) const;
+
+  nsView* GetViewInternal() const override { return mView; }
+  void SetViewInternal(nsView* aView) override { mView = aView; }
+
+private:
+  virtual mozilla::layout::FrameChildListID GetAbsoluteListID() const override { return kFixedList; }
+
+  nsView* mView;
 };
 
 } // namespace mozilla

layout/generic/nsFrame.cpp

@@ -690,12 +690,8 @@ nsFrame::DestroyFrom(nsIFrame* aDestructRoot)
     }
   }
 
-  // Get the view pointer now before the frame properties disappear
-  // when we call NotifyDestroyingFrame()
-  nsView* view = GetView();
   nsPresContext* presContext = PresContext();
-
-  nsIPresShell *shell = presContext->GetPresShell();
+  nsIPresShell* shell = presContext->GetPresShell();
   if (mState & NS_FRAME_OUT_OF_FLOW) {
     nsPlaceholderFrame* placeholder =
       shell->FrameManager()->GetPlaceholderFrameFor(this);
@@ -785,11 +781,9 @@ nsFrame::DestroyFrom(nsIFrame* aDestructRoot)
     shell->ClearFrameRefs(this);
   }
 
+  nsView* view = GetView();
   if (view) {
-    // Break association between view and frame
     view->SetFrame(nullptr);
-
-    // Destroy the view
     view->Destroy();
   }
 
@@ -1072,31 +1066,22 @@ nsIFrame::SyncFrameViewProperties(nsPresContext*  aPresContext,
 }
 
 void
-nsFrame::CreateViewForFrame(nsIFrame* aFrame,
-                            bool aForce)
+nsFrame::CreateView()
 {
-  if (aFrame->HasView()) {
-    return;
-  }
+  MOZ_ASSERT(!HasView());
 
-  // If we don't yet have a view, see if we need a view
-  if (!aForce && !aFrame->NeedsView()) {
-    // don't need a view
-    return;
-  }
-
-  nsView* parentView = aFrame->GetParent()->GetClosestView();
+  nsView* parentView = GetParent()->GetClosestView();
   NS_ASSERTION(parentView, "no parent with view");
 
   nsViewManager* viewManager = parentView->GetViewManager();
   NS_ASSERTION(viewManager, "null view manager");
 
   // Create a view
-  nsView* view = viewManager->CreateView(aFrame->GetRect(), parentView);
+  nsView* view = viewManager->CreateView(GetRect(), parentView);
 
-  SyncFrameViewProperties(aFrame->PresContext(), aFrame, nullptr, view);
+  SyncFrameViewProperties(PresContext(), this, nullptr, view);
 
-  nsView* insertBefore = nsLayoutUtils::FindSiblingViewFor(parentView, aFrame);
+  nsView* insertBefore = nsLayoutUtils::FindSiblingViewFor(parentView, this);
   // we insert this view 'above' the insertBefore view, unless insertBefore is null,
   // in which case we want to call with aAbove == false to insert at the beginning
   // in document order
@@ -1110,14 +1095,14 @@ nsFrame::CreateViewForFrame(nsIFrame* aFrame,
   // we know this frame has no view, so it will crawl the children. Also,
   // we know that any descendants with views must have 'parentView' as their
   // parent view.
-  ReparentFrameViewTo(aFrame, viewManager, view, parentView);
+  ReparentFrameViewTo(this, viewManager, view, parentView);
 
   // Remember our view
-  aFrame->SetView(view);
+  SetView(view);
 
   NS_FRAME_LOG(NS_FRAME_TRACE_CALLS,
-               ("nsContainerFrame::CreateViewForFrame: frame=%p view=%p",
-                aFrame, view));
+               ("nsFrame::CreateView: frame=%p view=%p",
+                this, view));
 }
 
 // MSVC fails with link error "one or more multiply defined symbols found",
@@ -5984,23 +5969,8 @@ nsIFrame* nsIFrame::GetTailContinuation()
   return frame;
 }
 
-NS_DECLARE_FRAME_PROPERTY_WITHOUT_DTOR(ViewProperty, nsView)
-
 // Associated view object
-nsView*
-nsIFrame::GetView() const
-{
-  // Check the frame state bit and see if the frame has a view
-  if (!(GetStateBits() & NS_FRAME_HAS_VIEW))
-    return nullptr;
-
-  // Check for a property on the frame
-  nsView* value = Properties().Get(ViewProperty());
-  NS_ASSERTION(value, "frame state bit was set but frame has no view");
-  return value;
-}
-
-nsresult
+void
 nsIFrame::SetView(nsView* aView)
 {
   if (aView) {
@@ -6008,8 +5978,7 @@ nsIFrame::SetView(nsView* aView)
 
 #ifdef DEBUG
     nsIAtom* frameType = GetType();
-    NS_ASSERTION(frameType == nsGkAtoms::scrollFrame ||
-                 frameType == nsGkAtoms::subDocumentFrame ||
+    NS_ASSERTION(frameType == nsGkAtoms::subDocumentFrame ||
                  frameType == nsGkAtoms::listControlFrame ||
                  frameType == nsGkAtoms::objectFrame ||
                  frameType == nsGkAtoms::viewportFrame ||
@@ -6017,8 +5986,8 @@ nsIFrame::SetView(nsView* aView)
                  "Only specific frame types can have an nsView");
 #endif
 
-    // Set a property on the frame
-    Properties().Set(ViewProperty(), aView);
+    // Store the view on the frame.
+    SetViewInternal(aView);
 
     // Set the frame state bit that says the frame has a view
     AddStateBits(NS_FRAME_HAS_VIEW);
@@ -6028,9 +5997,11 @@ nsIFrame::SetView(nsView* aView)
          f && !(f->GetStateBits() & NS_FRAME_HAS_CHILD_WITH_VIEW);
          f = f->GetParent())
       f->AddStateBits(NS_FRAME_HAS_CHILD_WITH_VIEW);
+  } else {
+    MOZ_ASSERT_UNREACHABLE("Destroying a view while the frame is alive?");
+    RemoveStateBits(NS_FRAME_HAS_VIEW);
+    SetViewInternal(nullptr);
   }
-
-  return NS_OK;
 }
 
 // Find the first geometric parent that has a view

layout/generic/nsFrame.h

@@ -596,11 +596,10 @@ protected:
 
 public:
   /**
-   * Helper method to wrap views around frames. Used by containers
-   * under special circumstances (can be used by leaf frames as well)
+   * Helper method to create a view for a frame.  Only used by a few sub-classes
+   * that need a view.
    */
-  static void CreateViewForFrame(nsIFrame* aFrame,
-                                 bool aForce);
+  void CreateView();
 
   //given a frame five me the first/last leaf available
   //XXX Robert O'Callahan wants to move these elsewhere

layout/generic/nsIFrame.h

@@ -613,8 +613,8 @@ public:
    * If the frame is a continuing frame, then aPrevInFlow indicates the previous
    * frame (the frame that was split).
    *
-   * If you want a view associated with your frame, you should create the view
-   * after Init() has returned.
+   * Each subclass that need a view should override this method and call
+   * CreateView() after calling its base class Init().
    *
    * @param   aContent the content object associated with the frame
    * @param   aParent the parent frame
@@ -1607,11 +1607,6 @@ public:
                                 const nsDisplayListSet& aLists,
                                 uint32_t                aFlags = 0);
 
-  /**
-   * Does this frame need a view?
-   */
-  virtual bool NeedsView() { return false; }
-
   bool RefusedAsyncAnimation() const
   {
     return Properties().Get(RefusedAsyncAnimationProperty());
@@ -2445,14 +2440,31 @@ public:
   virtual bool HasAnyNoncollapsedCharacters()
   { return false; }
 
-  /**
-   * Accessor functions to get/set the associated view object
-   *
-   * GetView returns non-null if and only if |HasView| returns true.
-   */
+  //
+  // Accessor functions to an associated view object:
+  //
   bool HasView() const { return !!(mState & NS_FRAME_HAS_VIEW); }
-  nsView* GetView() const;
-  nsresult SetView(nsView* aView);
+protected:
+  virtual nsView* GetViewInternal() const
+  {
+    MOZ_ASSERT_UNREACHABLE("method should have been overridden by subclass");
+    return nullptr;
+  }
+  virtual void SetViewInternal(nsView* aView)
+  {
+    MOZ_ASSERT_UNREACHABLE("method should have been overridden by subclass");
+  }
+public:
+  nsView* GetView() const
+  {
+    if (MOZ_LIKELY(!HasView())) {
+      return nullptr;
+    }
+    nsView* view = GetViewInternal();
+    MOZ_ASSERT(view, "GetViewInternal() should agree with HasView()");
+    return view;
+  }
+  void SetView(nsView* aView);
 
   /**
    * Find the closest view (on |this| or an ancestor).

layout/generic/nsPluginFrame.cpp

@@ -150,6 +150,7 @@ protected:
 nsPluginFrame::nsPluginFrame(nsStyleContext* aContext)
   : nsFrame(aContext)
   , mInstanceOwner(nullptr)
+  , mOuterView(nullptr)
   , mInnerView(nullptr)
   , mBackgroundSink(nullptr)
   , mReflowCallbackPosted(false)
@@ -194,6 +195,7 @@ nsPluginFrame::Init(nsIContent*       aContent,
          ("Initializing nsPluginFrame %p for content %p\n", this, aContent));
 
   nsFrame::Init(aContent, aParent, aPrevInFlow);
+  CreateView();
 }
 
 void

layout/generic/nsPluginFrame.h

@@ -96,8 +96,6 @@ public:
       ~(nsIFrame::eReplaced | nsIFrame::eReplacedSizing));
   }
 
-  virtual bool NeedsView() override { return true; }
-
 #ifdef DEBUG_FRAME_DUMP
   virtual nsresult GetFrameName(nsAString& aResult) const override;
 #endif
@@ -271,6 +269,9 @@ protected:
   friend class nsDisplayPlugin;
   friend class PluginBackgroundSink;
 
+  nsView* GetViewInternal() const override { return mOuterView; }
+  void SetViewInternal(nsView* aView) override { mOuterView = aView; }
+
 private:
   // Registers the plugin for a geometry update, and requests a geometry
   // update. This caches the root pres context in
@@ -303,7 +304,8 @@ private:
   };
 
   nsPluginInstanceOwner*          mInstanceOwner; // WEAK
-  nsView*                        mInnerView;
+  nsView*                         mOuterView;
+  nsView*                         mInnerView;
   nsCOMPtr<nsIWidget>             mWidget;
   nsIntRect                       mWindowlessRect;
   /**

layout/generic/nsSubDocumentFrame.cpp

@@ -60,6 +60,7 @@ GetDocumentFromView(nsView* aView)
 
 nsSubDocumentFrame::nsSubDocumentFrame(nsStyleContext* aContext)
   : nsAtomicContainerFrame(aContext)
+  , mOuterView(nullptr)
   , mInnerView(nullptr)
   , mIsInline(false)
   , mPostedReflowCallback(false)
@@ -121,17 +122,10 @@ nsSubDocumentFrame::Init(nsIContent*       aContent,
 
   nsAtomicContainerFrame::Init(aContent, aParent, aPrevInFlow);
 
-  // We are going to create an inner view.  If we need a view for the
-  // OuterFrame but we wait for the normal view creation path in
-  // nsCSSFrameConstructor, then we will lose because the inner view's
-  // parent will already have been set to some outer view (e.g., the
-  // canvas) when it really needs to have this frame's view as its
-  // parent. So, create this frame's view right away, whether we
-  // really need it or not, and the inner view will get it as the
-  // parent.
-  if (!HasView()) {
-    nsFrame::CreateViewForFrame(this, true);
-  }
+  // CreateView() creates this frame's view, stored in mOuterView.  It needs to
+  // be created first since it's the parent of the inner view, stored in
+  // mInnerView.
+  CreateView();
   EnsureInnerView();
 
   // Set the primary frame now so that nsDocumentViewer::FindContainerView

layout/generic/nsSubDocumentFrame.h

@@ -157,7 +157,11 @@ protected:
    */
   nsIFrame* ObtainIntrinsicSizeFrame();
 
+  nsView* GetViewInternal() const override { return mOuterView; }
+  void SetViewInternal(nsView* aView) override { mOuterView = aView; }
+
   RefPtr<nsFrameLoader> mFrameLoader;
+  nsView* mOuterView;
   nsView* mInnerView;
   bool mIsInline;
   bool mPostedReflowCallback;

layout/xul/nsMenuPopupFrame.cpp

@@ -94,6 +94,7 @@ NS_QUERYFRAME_TAIL_INHERITING(nsBoxFrame)
 nsMenuPopupFrame::nsMenuPopupFrame(nsStyleContext* aContext)
   :nsBoxFrame(aContext),
   mCurrentMenu(nullptr),
+  mView(nullptr),
   mPrefSize(-1, -1),
   mLastClientOffset(0, 0),
   mPopupType(ePopupTypePanel),

layout/xul/nsMenuPopupFrame.h

@@ -533,6 +533,9 @@ protected:
   // view, and is initially hidden.
   void CreatePopupView();
 
+  nsView* GetViewInternal() const override { return mView; }
+  void SetViewInternal(nsView* aView) override { mView = aView; }
+
   // Returns true if the popup should try to remain at the same relative
   // location as the anchor while it is open. If the anchor becomes hidden
   // either directly or indirectly because a parent popup or other element
@@ -555,6 +558,7 @@ protected:
   nsCOMPtr<nsIContent> mTriggerContent;
 
   nsMenuFrame* mCurrentMenu; // The current menu that is active.
+  nsView* mView;
 
   RefPtr<nsXULPopupShownEvent> mPopupShownDispatcher;