Bug 1269539 - Ensure that the scroll position is restored properly on reloading a page which loads incrementally. r=tnikkel

MozReview-Commit-ID: 1qVA5yU7a7g

--HG--
extra : rebase_source : 032336d7552ece6dbeafa4eb07eceb7ed2a096ba

docshell/base/nsIContentViewer.idl

@@ -41,6 +41,7 @@ interface nsIContentViewer : nsISupports
 
   [noscript,notxpcom,nostdcall] void loadStart(in nsIDocument aDoc);
   void loadComplete(in nsresult aStatus);
+  [noscript] readonly attribute boolean loadCompleted;
 
   /**
    * Checks if the document wants to prevent unloading by firing beforeunload on

layout/base/nsDocumentViewer.cpp

@@ -1050,6 +1050,13 @@ nsDocumentViewer::LoadComplete(nsresult aStatus)
   return rv;
 }
 
+NS_IMETHODIMP
+nsDocumentViewer::GetLoadCompleted(bool *aOutLoadCompleted)
+{
+  *aOutLoadCompleted = mLoaded;
+  return NS_OK;
+}
+
 NS_IMETHODIMP
 nsDocumentViewer::PermitUnload(bool *aPermitUnload)
 {

layout/base/nsPresShell.cpp

@@ -2417,7 +2417,6 @@ PresShell::RestoreRootScrollPosition()
 {
   nsIScrollableFrame* scrollableFrame = GetRootScrollFrameAsScrollable();
   if (scrollableFrame) {
-    scrollableFrame->SetRestoringHistoryScrollPosition(true);
     scrollableFrame->ScrollToRestoredPosition();
   }
 }
@@ -2475,11 +2474,6 @@ PresShell::EndLoad(nsIDocument *aDocument)
 void
 PresShell::LoadComplete()
 {
-  nsIScrollableFrame* scrollableFrame = GetRootScrollFrameAsScrollable();
-  if (scrollableFrame) {
-    scrollableFrame->SetRestoringHistoryScrollPosition(false);
-  }
-
   gfxTextPerfMetrics *tp = nullptr;
   if (mPresContext) {
     tp = mPresContext->GetTextPerfMetrics();

layout/base/nsPresState.h

@@ -21,7 +21,6 @@ public:
   nsPresState()
     : mContentData(nullptr)
     , mScrollState(0, 0)
-    , mRestoringHistoryScrollPosition(true)
     , mResolution(1.0)
     , mScaleToResolution(false)
     , mDisabledSet(false)
@@ -29,10 +28,9 @@ public:
     , mDroppedDown(false)
   {}
 
-  void SetScrollState(const nsPoint& aState, bool aRestoringHistoryScrollPosition = true)
+  void SetScrollState(const nsPoint& aState)
   {
     mScrollState = aState;
-    mRestoringHistoryScrollPosition = aRestoringHistoryScrollPosition;
   }
 
   nsPoint GetScrollPosition() const
@@ -40,11 +38,6 @@ public:
     return mScrollState;
   }
 
-  bool GetRestoringHistoryScrollPosition() const
-  {
-    return mRestoringHistoryScrollPosition;
-  }
-
   void SetResolution(float aSize)
   {
     mResolution = aSize;

layout/generic/nsGfxScrollFrame.cpp

@@ -1891,7 +1891,6 @@ ScrollFrameHelper::ScrollFrameHelper(nsContainerFrame* aOuter,
   , mTransformingByAPZ(false)
   , mScrollableByAPZ(false)
   , mZoomableByAPZ(false)
-  , mRestoringHistoryScrollPosition(true)
   , mVelocityQueue(aOuter->PresContext())
   , mAsyncScrollEvent(END_DOM)
 {
@@ -4065,7 +4064,7 @@ ScrollFrameHelper::ScrollToRestoredPosition()
       if (!weakFrame.IsAlive()) {
         return;
       }
-      if (mRestoringHistoryScrollPosition) {
+      if (PageIsStillLoading()) {
         // If we're trying to do a history scroll restore, then we want to
         // keep trying this until we succeed, because the page can be loading
         // incrementally. So re-get the scroll position for the next iteration,
@@ -4081,19 +4080,24 @@ ScrollFrameHelper::ScrollToRestoredPosition()
     mRestorePos.y = -1;
     mLastPos.x = -1;
     mLastPos.y = -1;
-    mRestoringHistoryScrollPosition = false;
   } else {
     // user moved the position, so we won't need to restore
     mLastPos.x = -1;
     mLastPos.y = -1;
-    mRestoringHistoryScrollPosition = false;
   }
 }
 
-void
-ScrollFrameHelper::SetRestoringHistoryScrollPosition(bool aValue)
+bool
+ScrollFrameHelper::PageIsStillLoading()
 {
-  mRestoringHistoryScrollPosition = aValue;
+  bool loadCompleted = false;
+  nsCOMPtr<nsIDocShell> ds = mOuter->GetContent()->GetComposedDoc()->GetDocShell();
+  if (ds) {
+    nsCOMPtr<nsIContentViewer> cv;
+    ds->GetContentViewer(getter_AddRefs(cv));
+    cv->GetLoadCompleted(&loadCompleted);
+  }
+  return !loadCompleted;
 }
 
 nsresult
@@ -5677,7 +5681,7 @@ ScrollFrameHelper::SaveState() const
   if (mRestorePos.y != -1 && pt == mLastPos) {
     pt = mRestorePos;
   }
-  state->SetScrollState(pt, mRestoringHistoryScrollPosition);
+  state->SetScrollState(pt);
   if (mIsRoot) {
     // Only save resolution properties for root scroll frames
     nsIPresShell* shell = mOuter->PresContext()->PresShell();
@@ -5691,7 +5695,6 @@ void
 ScrollFrameHelper::RestoreState(nsPresState* aState)
 {
   mRestorePos = aState->GetScrollPosition();
-  mRestoringHistoryScrollPosition = aState->GetRestoringHistoryScrollPosition();
   mDidHistoryRestore = true;
   mLastPos = mScrolledFrame ? GetLogicalScrollPosition() : nsPoint(0,0);
 

layout/generic/nsGfxScrollFrame.h

@@ -257,7 +257,7 @@ public:
    */
   void ScrollToRestoredPosition();
 
-  void SetRestoringHistoryScrollPosition(bool aValue);
+  bool PageIsStillLoading();
 
   /**
    * GetSnapPointForDestination determines which point to snap to after
@@ -581,11 +581,6 @@ public:
   // True if we don't want the scrollbar to repaint itself right now.
   bool mSuppressScrollbarRepaints:1;
 
-  // True if the calls to ScrollToRestoredPosition() are trying to restore the
-  // scroll position from history, and need to account for incremental page
-  // load.
-  bool mRestoringHistoryScrollPosition:1;
-
   mozilla::layout::ScrollVelocityQueue mVelocityQueue;
 
 protected:
@@ -850,9 +845,6 @@ public:
   virtual void ScrollToRestoredPosition() override {
     mHelper.ScrollToRestoredPosition();
   }
-  virtual void SetRestoringHistoryScrollPosition(bool aValue) override {
-    mHelper.SetRestoringHistoryScrollPosition(aValue);
-  }
   virtual void AddScrollPositionListener(nsIScrollPositionListener* aListener) override {
     mHelper.AddScrollPositionListener(aListener);
   }
@@ -1262,9 +1254,6 @@ public:
   virtual void ScrollToRestoredPosition() override {
     mHelper.ScrollToRestoredPosition();
   }
-  virtual void SetRestoringHistoryScrollPosition(bool aValue) override {
-    mHelper.SetRestoringHistoryScrollPosition(aValue);
-  }
   virtual void AddScrollPositionListener(nsIScrollPositionListener* aListener) override {
     mHelper.AddScrollPositionListener(aListener);
   }

layout/generic/nsIScrollableFrame.h

@@ -287,15 +287,6 @@ public:
    */
   virtual void ScrollToRestoredPosition() = 0;
 
-  /**
-   * Set whether or not the scrollframe should treat scroll position restoration
-   * as coming from history or not. This changes behaviour slightly, as history-
-   * basied scroll position restorations need to deal with incremental page
-   * loading, where the restore attempt might not work until more of the page
-   * is loaded.
-   */
-  virtual void SetRestoringHistoryScrollPosition(bool aValue) = 0;
-
   /**
    * Add a scroll position listener. This listener must be removed
    * before it is destroyed.

layout/generic/test/mochitest.ini

@@ -141,3 +141,5 @@ skip-if = buildapp == 'mulet'
 [test_selection_touchevents.html]
 [test_taintedfilters.html]
 support-files = file_taintedfilters_feDisplacementMap-tainted-1.svg file_taintedfilters_feDisplacementMap-tainted-2.svg file_taintedfilters_feDisplacementMap-tainted-3.svg file_taintedfilters_feDisplacementMap-tainted-ref.svg file_taintedfilters_feDisplacementMap-untainted-ref.svg file_taintedfilters_feDisplacementMap-untainted-1.svg file_taintedfilters_feDisplacementMap-untainted-2.svg file_taintedfilters_red-flood-for-feImage-cors.svg file_taintedfilters_red-flood-for-feImage-cors.svg^headers^ file_taintedfilters_red-flood-for-feImage.svg
+[test_scroll_position_restore.html]
+support-files = file_scroll_position_restore.html

layout/generic/test/test_scroll_position_restore.html

@@ -0,0 +1,41 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1269539
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1269539</title>
+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/paint_listener.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1269539">Mozilla Bug 1269539</a>
+<p id="display"></p>
+<script>
+SimpleTest.waitForExplicitFinish();
+
+var loadCount = 0;
+var childWin = window.open('file_scroll_position_restore.html', '_blank');
+
+function handleLoad() {
+  if (loadCount == 0) {
+    loadCount++;
+    childWin.scrollTo(0, childWin.scrollMaxY);
+    childWin.waitForAllPaintsFlushed(function() {
+      childWin.location.reload();
+    });
+  } else {
+    childWin.waitForAllPaintsFlushed(function() {
+      // Verify that the scroll position was retained
+      is(childWin.scrollY, childWin.scrollMaxY);
+      childWin.close();
+      SimpleTest.finish();
+    });
+  }
+}
+
+</script>
+</body>
+</html>