Preserve page resize information when main frame is remote

third_party/WebKit/Source/core/frame/VisualViewport.cpp

Index: third_party/WebKit/Source/core/frame/VisualViewport.cpp
diff --git a/third_party/WebKit/Source/core/frame/VisualViewport.cpp b/third_party/WebKit/Source/core/frame/VisualViewport.cpp
index 5f61223528a847ff9b6a4834cd76ab341e7c3f3a..47e290f41ceaa5d7ae3b71c5549874d701b72abd 100644
--- a/third_party/WebKit/Source/core/frame/VisualViewport.cpp
+++ b/third_party/WebKit/Source/core/frame/VisualViewport.cpp
@@ -89,15 +89,11 @@ DEFINE_TRACE(VisualViewport)
 
 void VisualViewport::setSize(const IntSize& size)
 {
-    // When the main frame is remote, we won't have an associated frame.
-    if (!mainFrame())
-        return;
-
     if (m_size == size)
         return;
 
-    bool autosizerNeedsUpdating =
-        (size.width() != m_size.width())
+    bool autosizerNeedsUpdating = (size.width() != m_size.width())
+        && mainFrame()
         && mainFrame()->settings()
         && mainFrame()->settings()->textAutosizingEnabled();
 
@@ -194,9 +190,6 @@ void VisualViewport::setScale(float scale)
 
 void VisualViewport::setScaleAndLocation(float scale, const FloatPoint& location)

[dcheng, 2016/01/08 21:36:06]

What actually needs to run in here for a WebView with a remote main frame? Can
we do all the common work first and then early return halfway through?

[bokan, 2016/01/11 16:36:34]

Can a remote frame even be pinch-zoomed? I think maybe we don't need this method
to work for remote frames. e.g. If there's no frame than the clamping logic
below doesn't work (and will zero out the offset currently).

Perhaps the way this should work is that the root frame should get the pinch
gesture in its local process, then send some message to child frames' processes
where these values can be updated through some new method (e.g.
VisualViewport::updateValuesFromRemoteRoot)

[kenrb, 2016/01/11 17:07:24]

Pinch zoom doesn't really make sense on remote frames, so I think a lot of this
can be ignored, as Daniel suggests. I uploaded a new patchset that might make
more sense.

I think we need to preserve the scale here, though, because if the page scale
gets changed and then we swap the remote main frame back to a local main frame,
it probably needs to be correct in the same way as the size. Does that sound
right?

[bokan, 2016/01/11 18:18:58]

It sounds fine in principle but practically speaking, the only time we should be
swapping from remote to local is on a navigation right? That should always reset
the scale anyway.

My point earlier was that I don't think we should be changing the scale from a
remote frames' process at all. Pinch gestures should target the root frame and
other actions that cause zooming (such as focusing a text box on mobile) would
make more sense to send a message to the root and let it do the zoom. In that
case, we wouldn't have to worry about the VisualViewport's scale/location
changing in the remote process.

Anyway, I know this is all in flight so I'll stamp this change since it moves us
along. Just putting in my 2c for future direction :).

[kenrb, 2016/01/11 20:50:26]

I've removed this in the most recent patchset, since it isn't necessary to fix
current bugs and was speculative, that having stale values on VisualViewport
when a page is navigated in might be a problem in the same way as having a stale
size. Your comment makes it sounds like that is not likely to be possible, so it
is probably best to leave the method as is and investigate if we see any related
bugs pop up later on.

 {
-    if (!mainFrame())
-        return;
-
     bool valuesChanged = false;
 
     if (scale != m_scale) {
@@ -215,20 +208,23 @@ void VisualViewport::setScaleAndLocation(float scale, const FloatPoint& location
         if (ScrollingCoordinator* coordinator = frameHost().page().scrollingCoordinator())
             coordinator->scrollableAreaScrollLayerDidChange(this);
 
-        if (!frameHost().settings().inertVisualViewport()) {
+        if (!frameHost().settings().inertVisualViewport() && mainFrame()) {
             if (Document* document = mainFrame()->document())
                 document->enqueueScrollEventForNode(document);
         }
 
-        mainFrame()->loader().client()->didChangeScrollOffset();
+        if (mainFrame())
+            mainFrame()->loader().client()->didChangeScrollOffset();
         valuesChanged = true;
     }
 
     if (!valuesChanged)
         return;
 
-    InspectorInstrumentation::didUpdateLayout(mainFrame());
-    mainFrame()->loader().saveScrollState();
+    if (mainFrame()) {
+        InspectorInstrumentation::didUpdateLayout(mainFrame());
+        mainFrame()->loader().saveScrollState();
+    }
 
     clampToBoundaries();
 }