[Pinch-to-zoom] Moved scale factor into PinchViewport

Source/web/WebViewImpl.cpp

Index: Source/web/WebViewImpl.cpp
diff --git a/Source/web/WebViewImpl.cpp b/Source/web/WebViewImpl.cpp
index 9267de1b7f190ef35a802e331a0193ea4c870525..460a4dbc3573a410ff6c4ad03a1a7c0f9b5155ed 100644
--- a/Source/web/WebViewImpl.cpp
+++ b/Source/web/WebViewImpl.cpp
@@ -1570,7 +1570,7 @@ void WebViewImpl::resize(const WebSize& newSize)
         if (webFrame->frameView()) {
             webFrame->frameView()->resize(m_size);
             if (page()->settings().pinchVirtualViewportEnabled())
-                page()->frameHost().pinchViewport().setSize(m_size);
+                page()->frameHost().pinchViewport().mainFrameDidChangeSize();
         }
     }
 
@@ -2637,6 +2637,9 @@ float WebViewImpl::pageScaleFactor() const
     if (!page())
         return 1;
 
+    if (page()->settings().pinchVirtualViewportEnabled())
+        return page()->frameHost().pinchViewport().scale();
+
     return page()->pageScaleFactor();
 }
 
@@ -2654,6 +2657,19 @@ IntPoint WebViewImpl::clampOffsetAtScale(const IntPoint& offset, float scale)
     return view->clampOffsetAtScale(offset, scale);
 }
 
+void WebViewImpl::setPageScaleFactor(float scaleFactor)
+{
+    ASSERT(page()->settings().pinchVirtualViewportEnabled());
+
+    if (!page())
+        return;
+
+    scaleFactor = clampPageScaleFactorToLimits(scaleFactor);
+
+    page()->frameHost().pinchViewport().setScale(scaleFactor);
+    deviceOrPageScaleFactorChanged();
+}
+
 void WebViewImpl::setPageScaleFactor(float scaleFactor, const WebPoint& origin)
 {
     if (!page())
@@ -2663,7 +2679,20 @@ void WebViewImpl::setPageScaleFactor(float scaleFactor, const WebPoint& origin)
     scaleFactor = clampPageScaleFactorToLimits(scaleFactor);
     newScrollOffset = clampOffsetAtScale(newScrollOffset, scaleFactor);
 
-    page()->setPageScaleFactor(scaleFactor, newScrollOffset);
+    if (page()->settings().pinchVirtualViewportEnabled()) {
+        page()->frameHost().pinchViewport().setScale(scaleFactor);
+        deviceOrPageScaleFactorChanged();
+
+        if (FrameView* view = page()->mainFrame()->view()) {
+            view->notifyScrollPositionChanged(origin);
+
+            IntSize remaining = IntSize(origin.x, origin.y) - view->scrollOffset();

[aelias_OOO_until_Jul13, 2014/04/09 08:41:22]

Hmm, this is doing distribution which we would like to avoid.  This method
doesn't have a lot of callers -- probably the only important one is for
historyitem restore.  We know that (and only that) needs distribution, so let's
more this logic there.

I think setPageScaleFactor with a scroll offset is fundamentally ambiguous in
the new world, so we should probably plan to delete this method.

[bokan, 2014/04/09 15:30:59]

There's actually a few other uses (Fullscreen elements, page scale animations,
and DevTools pinch emulation) and HistoryItem uses Page::setPageScaleFactor. I
changed the call sites to set scale and offset in separate calls and will remove
this method. This will keep old-style pinch working. I'll address all the above
use cases in virtual viewport pinch later.

[aelias_OOO_until_Jul13, 2014/04/09 18:35:32]

Well, keep in mind that clamping can cause issue when updating scroll and scale
separately.  It's *probably* fine if you set the scroll position afterwards,
although it's still weird to update scroll position twice and I've sometimes
been surprised by bugs that can happen from this kind of thing.  So I'd suggest
not changing the behavior of the old pinch path for now.

[bokan, 2014/04/09 19:57:19]

Ok, I replaced all uses in unit tests since they all still pass (most cases were
just WebPoint(0,0)) and the simple cases in FullscreenController and
InspectorController. I've left the WebViewImpl-internal uses that are
higher-risk. This let me remove it from the WebView interface.

I replaced the other "pageScale" methods with "pinchViewportScale" named
methods. I think the naming is more consistent and less confusing (I've talked
to numerous people who confused page-scale with browser-zoom).

[bokan, 2014/04/09 20:21:53]

Erm, I had to leave setPageScale in WebView since it'll need a 3-sided patch due
to uses outside blink (webkit_test_runner).

+            page()->frameHost().pinchViewport().setLocation(
+                IntPoint(remaining.width(), remaining.height()));
+        }
+    } else {
+        page()->setPageScaleFactor(scaleFactor, newScrollOffset);
+    }
 }
 
 void WebViewImpl::setPageScaleFactorPreservingScrollOffset(float scaleFactor)
@@ -2912,7 +2941,7 @@ void WebViewImpl::resetSavedScrollAndScaleState()
 
 void WebViewImpl::resetScrollAndScaleState()
 {
-    page()->setPageScaleFactor(1, IntPoint());
+    setPageScaleFactor(1, IntPoint());
 
     // Clear out the values for the current history item. This will prevent the history item from clobbering the
     // value determined during page scale initialization, which may be less than 1.
@@ -3620,7 +3649,6 @@ void WebViewImpl::setRootGraphicsLayer(GraphicsLayer* layer)
     if (pinchVirtualViewportEnabled) {
         PinchViewport& pinchViewport = page()->frameHost().pinchViewport();
         pinchViewport.attachToLayerTree(layer, graphicsLayerFactory());
-        pinchViewport.setSize(mainFrameImpl()->frame()->view()->frameRect().size());
         if (layer) {
             m_rootGraphicsLayer = pinchViewport.rootGraphicsLayer();
             m_rootLayer = pinchViewport.rootGraphicsLayer()->platformLayer();
@@ -3812,12 +3840,13 @@ void WebViewImpl::applyScrollAndScale(const WebSize& scrollDelta, float pageScal
     if (!mainFrameImpl() || !mainFrameImpl()->frameView())
         return;
 
-    // With virtual viewport we need only set the scale (see TODO below).
     if (page()->settings().pinchVirtualViewportEnabled()) {
-        WebSize scrollOffset = mainFrame()->scrollOffset();
-        WebPoint scrollPoint(scrollOffset.width, scrollOffset.height);
-        setPageScaleFactor(pageScaleFactor() * pageScaleDelta, scrollPoint);
-        m_doubleTapZoomPending = false;
+        if (pageScaleDelta != 1) {
+            // When the virtual viewport is enabled, offsets are already set for us.
+            setPageScaleFactor(pageScaleFactor() * pageScaleDelta);
+            m_doubleTapZoomPending = false;
+        }
+
         return;
     }