Restore scale even if scroll is not restored

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 ffb912ae0aadef4c4e3f2f76f2054050709cb0d2..5e2662b9a3e637f905588c8aa2e30381d9622fcb 100644
--- a/Source/core/loader/FrameLoader.cpp
+++ b/Source/core/loader/FrameLoader.cpp
@@ -1107,10 +1107,8 @@ void FrameLoader::restoreScrollPositionAndViewState()
     if (!needsHistoryItemRestore(m_loadType))
         return;
 
-    if (m_currentItem->scrollRestorationType() == ScrollRestorationManual) {
-        documentLoader()->initialScrollState().didRestoreFromHistory = true;
-        return;
-    }
+    bool shouldRestoreScroll = m_currentItem->scrollRestorationType() != ScrollRestorationManual;
+    bool shouldRestoreScale = m_currentItem->pageScaleFactor();
 
     // This tries to balance 1. restoring as soon as possible, 2. detecting
     // clamping to avoid repeatedly popping the scroll position down as the
@@ -1119,32 +1117,32 @@ void FrameLoader::restoreScrollPositionAndViewState()
     // height.
     bool canRestoreWithoutClamping = view->clampOffsetAtScale(m_currentItem->scrollPoint(), 1) == m_currentItem->scrollPoint();
     bool canRestoreWithoutAnnoyingUser = !view->wasScrolledByUser() && (canRestoreWithoutClamping || m_frame->isLoading());
-    if (!canRestoreWithoutAnnoyingUser)
+    if (shouldRestoreScroll && !canRestoreWithoutAnnoyingUser)

[bokan, 2015/08/07 15:08:01]

Are you sure we want to restore scale if it would "annoy the user"? If the user
started scrolling I think we shouldn't do anything.

[majidvp, 2015/08/07 18:24:06]

That is reasonable. Updated the logic notto restore anything if user has 
scrolled. Perhaps we should do the same if user has scaled. 
I added a TODO for this.

         return;
 
-    if (m_frame->isMainFrame() && m_currentItem->pageScaleFactor()) {
-        FloatPoint visualViewportOffset(m_currentItem->visualViewportScrollPoint());
-        IntPoint frameScrollOffset(m_currentItem->scrollPoint());
+    if (shouldRestoreScroll)
+        view->setScrollPosition(m_currentItem->scrollPoint(), ProgrammaticScroll);

[bokan, 2015/08/07 15:08:01]

This should be view->layoutViewportScrollableArea()->setScrollPosition(...)

[majidvp, 2015/08/07 18:24:06]

Done.

 
-        m_frame->page()->setPageScaleFactor(m_currentItem->pageScaleFactor(), frameScrollOffset);
+    // For main frame restore scale and visual viewport position
+    if (m_frame->isMainFrame()) {
+        FloatPoint visualViewportOffset(m_currentItem->visualViewportScrollPoint());
 
         // If the visual viewport's offset is (-1, -1) it means the history item
         // is an old version of HistoryItem so distribute the scroll between
         // the main frame and the visual viewport as best as we can.
         if (visualViewportOffset.x() == -1 && visualViewportOffset.y() == -1)
-            visualViewportOffset = FloatPoint(frameScrollOffset - view->scrollPosition());
-
-        m_frame->host()->visualViewport().setLocation(visualViewportOffset);
-    } else {
-        IntPoint adjustedScrollPosition = view->clampScrollPosition(m_currentItem->scrollPoint());
-        if (adjustedScrollPosition != view->scrollPosition())
-            view->setScrollPosition(adjustedScrollPosition, ProgrammaticScroll);
+            visualViewportOffset = FloatPoint(m_currentItem->scrollPoint() - view->scrollPosition());
+
+        VisualViewport& visualViewport = m_frame->host()->visualViewport();
+        if (shouldRestoreScale && shouldRestoreScroll) {

[Nate Chapin, 2015/08/06 21:02:00]

Style nit: this if/else block is all single line, so {} aren't necessary.

[majidvp, 2015/08/06 21:25:36]

Acknowledged.

+            visualViewport.setScaleAndLocation(m_currentItem->pageScaleFactor(), visualViewportOffset);
+        } else if (shouldRestoreScale) {
+            visualViewport.setScale(m_currentItem->pageScaleFactor());
+        } else if (shouldRestoreScroll) {
+            visualViewport.setLocation(visualViewportOffset);
+        }
     }
 
-    if (m_frame->isMainFrame()) {
-        if (ScrollingCoordinator* scrollingCoordinator = m_frame->page()->scrollingCoordinator())
-            scrollingCoordinator->frameViewRootLayerDidChange(view);

[bokan, 2015/08/07 15:08:01]

Why is this no longer needed?

[majidvp, 2015/08/07 18:24:06]

Added back. Actually this was a mistake as I thought VisualViewport will notify
scrollingcoordinator.
The call does more work that is needed and it may indeed be redundant 
(no test failed!) but I will look into that at a later point.

-    }
     documentLoader()->initialScrollState().didRestoreFromHistory = true;
 }