Replace history.options with history.scrollRestoration attribute

Source/core/loader/FrameLoader.cpp

Index: Source/core/loader/FrameLoader.cpp
diff --git a/Source/core/loader/FrameLoader.cpp b/Source/core/loader/FrameLoader.cpp
index 3d8ab8b84bc46b42a5857b61572df491501f33b4..f5d1f2473b32f5d7de3eac38178d8270b85757ea 100644
--- a/Source/core/loader/FrameLoader.cpp
+++ b/Source/core/loader/FrameLoader.cpp
@@ -377,6 +377,8 @@ void FrameLoader::setHistoryItemStateForCommit(HistoryCommitType historyCommitTy
     m_currentItem->setPinchViewportScrollPoint(oldItem->pinchViewportScrollPoint());
     m_currentItem->setPageScaleFactor(oldItem->pageScaleFactor());
     m_currentItem->setStateObject(oldItem->stateObject());
+    m_currentItem->setScrollRestorationType(oldItem->scrollRestorationType());
+
     // The item sequence number determines whether items are "the same", such back/forward navigation
     // between items with the same item sequence number is a no-op. Only treat this as identical if the
     // navigation did not create a back/forward entry and the url is identical or it was loaded via
@@ -571,6 +573,8 @@ void FrameLoader::checkCompleted()
         m_progressTracker->progressCompleted();
         // Retry restoring scroll offset since finishing loading disables content
         // size clamping.
+        // TODO(majidvp): Remove this call as it appears to be a no-op because
+        // we set load type to |FrameLoadTypeStandard| just above.
         restoreScrollPositionAndViewState();
         m_frame->localDOMWindow()->finishedLoading();
 
@@ -690,9 +694,6 @@ void FrameLoader::loadInSameDocument(const KURL& url, PassRefPtr<SerializedScrip
 
     m_frame->view()->setWasScrolledByUser(false);
 
-    // We need to scroll to the fragment whether or not a hash change occurred, since
-    // the user might have scrolled since the previous navigation.
-    processFragment(url, NavigationWithinSameDocument);
     checkCompleted();
 
     m_frame->localDOMWindow()->statePopped(stateObject ? stateObject : SerializedScriptValue::nullValue());
@@ -914,6 +915,10 @@ void FrameLoader::load(const FrameLoadRequest& passedRequest, FrameLoadType fram
 
         if (sameDocumentHistoryNavigation)
             restoreScrollPositionAndViewState();
+
+        // We need to scroll to the fragment whether or not a hash change occurred, since
+        // the user might have scrolled since the previous navigation.
+        processFragment(url, NavigationWithinSameDocument);

[Nate Chapin, 2015/07/16 21:34:01]

Why is this moving?

[majidvp, 2015/07/16 22:11:49]

The past logic was:
 1- scroll to fragment (checks didRestoreFromHistory flag)
 2- attempts to scroll to recorded scroll position overriding fragment scroll.

This does not work in "scrollRestoration=Manual" case because restoring scroll
position actually does not restore any scroll position but we still need to
prevent fragment scrolling. So I reversed the order and use an already existing
mechanism:

 1. attempt scroll to recorded scroll (set the didRestoreFromHistory if
successful)
 2. scroll to fragment (checks didRestoreFromHistory flag)

This can be made even cleaner but I think it needs to be done in a separate
patch. Here is my proposed refactoring:
https://docs.google.com/document/d/12RZGD2IsyA7Co9yAGb6f4r1YDIkPe7Nte0P82zWIz...

 

         return;
     }
 
@@ -1108,8 +1113,10 @@ void FrameLoader::restoreScrollPositionAndViewState()
     if (!needsHistoryItemRestore(m_loadType))
         return;
 
-    if (m_currentItem->scrollRestorationType() == ScrollRestorationManual)
+    if (m_currentItem->scrollRestorationType() == ScrollRestorationManual) {
+        documentLoader()->initialScrollState().didRestoreFromHistory = true;
         return;
+    }
 
     // This tries to balance 1. restoring as soon as possible, 2. detecting
     // clamping to avoid repeatedly popping the scroll position down as the
@@ -1236,9 +1243,13 @@ void FrameLoader::processFragment(const KURL& url, LoadStartType loadStartType)
     if (boundaryFrame && boundaryFrame->isLocalFrame())
         toLocalFrame(boundaryFrame.get())->view()->setSafeToPropagateScrollToParent(false);
 
-    view->processUrlFragment(url,
-        loadStartType == NavigationToDifferentDocument && documentLoader()->initialScrollState().didRestoreFromHistory ?
-            FrameView::UrlFragmentDontScroll : FrameView::UrlFragmentScroll);
+    // If scroll position is restored from history fragment then we should not override it unless
+    // this is a same document reload.
+    bool shouldScrollToFragment = (loadStartType == NavigationWithinSameDocument && !isBackForwardLoadType(m_loadType))
+        || !documentLoader()->initialScrollState().didRestoreFromHistory;
+
+    view->processUrlFragment(url, shouldScrollToFragment ?
+        FrameView::UrlFragmentScroll : FrameView::UrlFragmentDontScroll);
 
     if (boundaryFrame && boundaryFrame->isLocalFrame())
         toLocalFrame(boundaryFrame.get())->view()->setSafeToPropagateScrollToParent(true);