Fix for bug #96736: [FLOATER]The "Top 1000 Reviewer" is overwritten by text

mozilla/layout/base/src/nsSpaceManager.cpp
  mozilla/layout/base/src/nsSpaceManager.h

    - Implemented nsSpaceManager methods PushState()
      and PopState().

  mozilla/layout/html/base/src/nsBlockReflowContext.cpp

    - Modified nsBlockReflowContext::DoReflowBlock() to
      call PushState() and PopState() to remove any
      floaters that may have been added to the SpaceManager
      during the intermediate "unconstrained" Reflow()
      call, used to calculate the max block width. This
      allows the Reflow() call that immediately follows
      it to properly place the floaters.


r=waterson@netscape.com  sr=dbaron@fas.harvard.edu

layout/base/src/nsSpaceManager.cpp

@@ -111,6 +111,7 @@ nsSpaceManager::nsSpaceManager(nsIPresShell* aPresShell, nsIFrame* aFrame)
   MOZ_COUNT_CTOR(nsSpaceManager);
   mX = mY = 0;
   mFrameInfoMap = nsnull;
+  mSavedStates = nsnull;
 }
 
 void
@@ -128,6 +129,14 @@ nsSpaceManager::~nsSpaceManager()
   MOZ_COUNT_DTOR(nsSpaceManager);
   mBandList.Clear();
   ClearFrameInfo();
+
+  NS_ASSERTION(!mSavedStates, "states remaining on state stack");
+
+  while (mSavedStates){
+    SpaceManagerState *state = mSavedStates;
+    mSavedStates = state->mNext;
+    delete state;
+  }
 }
 
 // static
@@ -983,6 +992,92 @@ nsSpaceManager::ClearRegions()
   mBandList.Clear();
 }
 
+void
+nsSpaceManager::PushState()
+{
+  // This is a quick and dirty push implementation, which
+  // only saves the (x,y) and last frame in the mFrameInfoMap
+  // which is enough info to get us back to where we should be
+  // when pop is called.
+  //
+  // The alternative would be to make full copies of the contents
+  // of mBandList and mFrameInfoMap and restore them when pop is
+  // called, but I'm not sure it's worth the effort/bloat at this
+  // point, since this push/pop mechanism is only used to undo any
+  // floaters that were added during the unconstrained reflow
+  // in nsBlockReflowContext::DoReflowBlock(). (See bug 96736)
+  //
+  // It should also be noted that the state for mFloatDamage is
+  // intentionally not saved or restored in PushState() and PopState(),
+  // since that could lead to bugs where damage is missed/dropped when
+  // we move from position A to B (during the intermediate incremental
+  // reflow mentioned above) and then from B to C during the subsequent
+  // reflow. In the typical case A and C will be the same, but not always.
+  // Allowing mFloatDamage to accumulate the damage incurred during both
+  // reflows ensures that nothing gets missed.
+
+  SpaceManagerState *state = new SpaceManagerState;
+
+  NS_ASSERTION(state, "PushState() failed!");
+
+  if (!state) {
+    return;
+  }
+
+  state->mX = mX;
+  state->mY = mY;
+
+  if (mFrameInfoMap) {
+    state->mLastFrame = mFrameInfoMap->mFrame;
+  }
+
+  // Now that we've saved our state, add it to mSavedStates.
+
+  state->mNext = mSavedStates;
+  mSavedStates = state;
+}
+
+void
+nsSpaceManager::PopState()
+{
+  // This is a quick and dirty pop implementation, to
+  // match the current implementation of PushState(). The
+  // idea here is to remove any frames that have been added
+  // to the mFrameInfoMap since the last call to PushState().
+
+  NS_ASSERTION(mSavedStates, "Invalid call to PopState()!");
+
+  if (!mSavedStates) {
+    return;
+  }
+
+  // mFrameInfoMap is LIFO so keep removing what it points
+  // to until we hit mLastFrame.
+
+  while (mFrameInfoMap && mFrameInfoMap->mFrame != mSavedStates->mLastFrame) {
+    RemoveRegion(mFrameInfoMap->mFrame);
+  }
+
+  // If we trip this assertion it means that someone added
+  // PushState()/PopState() calls around code that actually
+  // removed mLastFrame from mFrameInfoMap, which means our
+  // state is now out of sync with what we thought it should be.
+
+  NS_ASSERTION(((mSavedStates->mLastFrame && mFrameInfoMap) ||
+               (!mSavedStates->mLastFrame && !mFrameInfoMap)),
+               "Unexpected outcome!");
+
+  mX = mSavedStates->mX;
+  mY = mSavedStates->mY;
+
+  // Now that we've restored our state, pop the topmost
+  // state and delete it.
+
+  SpaceManagerState *state = mSavedStates;
+  mSavedStates = mSavedStates->mNext;
+  delete state;
+}
+
 #ifdef DEBUG
 void
 DebugListSpaceManager(nsSpaceManager *aSpaceManager)

layout/base/src/nsSpaceManager.h

@@ -293,6 +293,17 @@ public:
     return mFloatDamage.Intersects(aIntervalBegin + mY, aIntervalEnd + mY);
   }
 
+  /**
+   * Pushes the current state of the space manager onto a state stack.
+   */
+  void PushState();
+
+  /**
+   * Restores the space manager to the state at the top of the state stack,
+   * then pops this state off the stack.
+   */
+  void PopState();
+
 #ifdef DEBUG
   /**
    * Dump the state of the spacemanager out to a file
@@ -316,6 +327,17 @@ protected:
 #endif
   };
 
+  // Structure that stores the current state of a frame manager for
+  // Save/Restore purposes.
+  struct SpaceManagerState {
+    nscoord mX, mY;
+    nsIFrame *mLastFrame;
+    SpaceManagerState *mNext;
+
+    SpaceManagerState() : mX(0), mY(0), mLastFrame(nsnull), mNext(nsnull) {}
+    ~SpaceManagerState() {}
+  };
+
 public:
   // Doubly linked list of band rects
   struct BandRect : PRCListStr {
@@ -387,6 +409,8 @@ protected:
   FrameInfo*      mFrameInfoMap;
   nsIntervalSet   mFloatDamage;
 
+  SpaceManagerState *mSavedStates;
+
 protected:
   FrameInfo* GetFrameInfoFor(nsIFrame* aFrame);
   FrameInfo* CreateFrameInfo(nsIFrame* aFrame, const nsRect& aRect);

layout/generic/nsBlockReflowContext.cpp

@@ -544,6 +544,8 @@ nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState &aReflowState,
   // See if this is the child's initial reflow and we are supposed to
   // compute our maximum width
   if (mComputeMaximumWidth && (eReflowReason_Initial == aReason)) {
+    mOuterReflowState.mSpaceManager->PushState();
+
     nscoord oldAvailableWidth = aReflowState.availableWidth;
     nscoord oldComputedWidth = aReflowState.mComputedWidth;
 
@@ -563,6 +565,8 @@ nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState &aReflowState,
     aReflowState.availableWidth = oldAvailableWidth;
     aReflowState.mComputedWidth = oldComputedWidth;
     aReason = eReflowReason_Resize;
+
+    mOuterReflowState.mSpaceManager->PopState();
   }
 
   rv = aFrame->Reflow(mPresContext, mMetrics, aReflowState,

layout/generic/nsSpaceManager.cpp

@@ -111,6 +111,7 @@ nsSpaceManager::nsSpaceManager(nsIPresShell* aPresShell, nsIFrame* aFrame)
   MOZ_COUNT_CTOR(nsSpaceManager);
   mX = mY = 0;
   mFrameInfoMap = nsnull;
+  mSavedStates = nsnull;
 }
 
 void
@@ -128,6 +129,14 @@ nsSpaceManager::~nsSpaceManager()
   MOZ_COUNT_DTOR(nsSpaceManager);
   mBandList.Clear();
   ClearFrameInfo();
+
+  NS_ASSERTION(!mSavedStates, "states remaining on state stack");
+
+  while (mSavedStates){
+    SpaceManagerState *state = mSavedStates;
+    mSavedStates = state->mNext;
+    delete state;
+  }
 }
 
 // static
@@ -983,6 +992,92 @@ nsSpaceManager::ClearRegions()
   mBandList.Clear();
 }
 
+void
+nsSpaceManager::PushState()
+{
+  // This is a quick and dirty push implementation, which
+  // only saves the (x,y) and last frame in the mFrameInfoMap
+  // which is enough info to get us back to where we should be
+  // when pop is called.
+  //
+  // The alternative would be to make full copies of the contents
+  // of mBandList and mFrameInfoMap and restore them when pop is
+  // called, but I'm not sure it's worth the effort/bloat at this
+  // point, since this push/pop mechanism is only used to undo any
+  // floaters that were added during the unconstrained reflow
+  // in nsBlockReflowContext::DoReflowBlock(). (See bug 96736)
+  //
+  // It should also be noted that the state for mFloatDamage is
+  // intentionally not saved or restored in PushState() and PopState(),
+  // since that could lead to bugs where damage is missed/dropped when
+  // we move from position A to B (during the intermediate incremental
+  // reflow mentioned above) and then from B to C during the subsequent
+  // reflow. In the typical case A and C will be the same, but not always.
+  // Allowing mFloatDamage to accumulate the damage incurred during both
+  // reflows ensures that nothing gets missed.
+
+  SpaceManagerState *state = new SpaceManagerState;
+
+  NS_ASSERTION(state, "PushState() failed!");
+
+  if (!state) {
+    return;
+  }
+
+  state->mX = mX;
+  state->mY = mY;
+
+  if (mFrameInfoMap) {
+    state->mLastFrame = mFrameInfoMap->mFrame;
+  }
+
+  // Now that we've saved our state, add it to mSavedStates.
+
+  state->mNext = mSavedStates;
+  mSavedStates = state;
+}
+
+void
+nsSpaceManager::PopState()
+{
+  // This is a quick and dirty pop implementation, to
+  // match the current implementation of PushState(). The
+  // idea here is to remove any frames that have been added
+  // to the mFrameInfoMap since the last call to PushState().
+
+  NS_ASSERTION(mSavedStates, "Invalid call to PopState()!");
+
+  if (!mSavedStates) {
+    return;
+  }
+
+  // mFrameInfoMap is LIFO so keep removing what it points
+  // to until we hit mLastFrame.
+
+  while (mFrameInfoMap && mFrameInfoMap->mFrame != mSavedStates->mLastFrame) {
+    RemoveRegion(mFrameInfoMap->mFrame);
+  }
+
+  // If we trip this assertion it means that someone added
+  // PushState()/PopState() calls around code that actually
+  // removed mLastFrame from mFrameInfoMap, which means our
+  // state is now out of sync with what we thought it should be.
+
+  NS_ASSERTION(((mSavedStates->mLastFrame && mFrameInfoMap) ||
+               (!mSavedStates->mLastFrame && !mFrameInfoMap)),
+               "Unexpected outcome!");
+
+  mX = mSavedStates->mX;
+  mY = mSavedStates->mY;
+
+  // Now that we've restored our state, pop the topmost
+  // state and delete it.
+
+  SpaceManagerState *state = mSavedStates;
+  mSavedStates = mSavedStates->mNext;
+  delete state;
+}
+
 #ifdef DEBUG
 void
 DebugListSpaceManager(nsSpaceManager *aSpaceManager)

layout/generic/nsSpaceManager.h

@@ -293,6 +293,17 @@ public:
     return mFloatDamage.Intersects(aIntervalBegin + mY, aIntervalEnd + mY);
   }
 
+  /**
+   * Pushes the current state of the space manager onto a state stack.
+   */
+  void PushState();
+
+  /**
+   * Restores the space manager to the state at the top of the state stack,
+   * then pops this state off the stack.
+   */
+  void PopState();
+
 #ifdef DEBUG
   /**
    * Dump the state of the spacemanager out to a file
@@ -316,6 +327,17 @@ protected:
 #endif
   };
 
+  // Structure that stores the current state of a frame manager for
+  // Save/Restore purposes.
+  struct SpaceManagerState {
+    nscoord mX, mY;
+    nsIFrame *mLastFrame;
+    SpaceManagerState *mNext;
+
+    SpaceManagerState() : mX(0), mY(0), mLastFrame(nsnull), mNext(nsnull) {}
+    ~SpaceManagerState() {}
+  };
+
 public:
   // Doubly linked list of band rects
   struct BandRect : PRCListStr {
@@ -387,6 +409,8 @@ protected:
   FrameInfo*      mFrameInfoMap;
   nsIntervalSet   mFloatDamage;
 
+  SpaceManagerState *mSavedStates;
+
 protected:
   FrameInfo* GetFrameInfoFor(nsIFrame* aFrame);
   FrameInfo* CreateFrameInfo(nsIFrame* aFrame, const nsRect& aRect);

layout/html/base/src/nsBlockReflowContext.cpp

@@ -544,6 +544,8 @@ nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState &aReflowState,
   // See if this is the child's initial reflow and we are supposed to
   // compute our maximum width
   if (mComputeMaximumWidth && (eReflowReason_Initial == aReason)) {
+    mOuterReflowState.mSpaceManager->PushState();
+
     nscoord oldAvailableWidth = aReflowState.availableWidth;
     nscoord oldComputedWidth = aReflowState.mComputedWidth;
 
@@ -563,6 +565,8 @@ nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState &aReflowState,
     aReflowState.availableWidth = oldAvailableWidth;
     aReflowState.mComputedWidth = oldComputedWidth;
     aReason = eReflowReason_Resize;
+
+    mOuterReflowState.mSpaceManager->PopState();
   }
 
   rv = aFrame->Reflow(mPresContext, mMetrics, aReflowState,