Fix page zoom to be frame-centric for out-of-process frames.

third_party/WebKit/Source/web/WebViewImpl.cpp

Index: third_party/WebKit/Source/web/WebViewImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebViewImpl.cpp b/third_party/WebKit/Source/web/WebViewImpl.cpp
index 98b04285fcb22d795e62fbf56591a796e7734f21..5488add8eb3e0970aaadd3f6790c98b797e054dd 100644
--- a/third_party/WebKit/Source/web/WebViewImpl.cpp
+++ b/third_party/WebKit/Source/web/WebViewImpl.cpp
@@ -3022,7 +3022,13 @@ double WebViewImpl::zoomLevel()
     return m_zoomLevel;
 }
 
-double WebViewImpl::setZoomLevel(double zoomLevel)
+double WebViewImpl::setZoomLevelForFrame(WebLocalFrame* frame, double zoomLevel)
+{
+    LocalFrame* localFrame = static_cast<WebLocalFrameImpl*>(frame)->frame();
+    return setZoomLevelForFrame(localFrame, zoomLevel);
+}
+
+double WebViewImpl::setZoomLevelForFrame(LocalFrame* frame, double zoomLevel)

[alexmos, 2016/04/05 18:00:54]

Why is the LocalFrame version of the function needed?  Why not just have one
version that takes a WebLocalFrame and convert to LocalFrame in its body? 
setZoomLevel() can invoke it with mainFrameImpl() instead of
mainFrameImpl()->frame().

[wjmaclean, 2016/04/05 20:22:00]

Because there are callers who have WebLocalFrame* (and cannot access the
LocalFrame accessor in WebLocalFrameImpl, e.g. RenderFrameImpl), and there are
callers who only have access to LocalFrame* (e.g. within WebViewImpl).

 {
     if (zoomLevel < m_minimumZoomLevel)
         m_zoomLevel = m_minimumZoomLevel;
@@ -3031,12 +3037,6 @@ double WebViewImpl::setZoomLevel(double zoomLevel)
     else
         m_zoomLevel = zoomLevel;
 
-    // TODO(nasko): Setting zoom level needs to be refactored to support
-    // out-of-process iframes. See https://crbug.com/528407.
-    if (mainFrame()->isWebRemoteFrame())
-        return m_zoomLevel;
-
-    LocalFrame* frame = mainFrameImpl()->frame();
     if (!WebLocalFrameImpl::pluginContainerFromFrame(frame)) {
         float zoomFactor = m_zoomFactorOverride ? m_zoomFactorOverride : static_cast<float>(zoomLevelToZoomFactor(m_zoomLevel));
         if (m_zoomFactorForDeviceScaleFactor) {
@@ -3055,6 +3055,23 @@ double WebViewImpl::setZoomLevel(double zoomLevel)
     return m_zoomLevel;
 }
 
+double WebViewImpl::setZoomLevel(double zoomLevel)

[alexmos, 2016/04/05 18:00:54]

There seem to be some existing renderer-initiated call sites for this, such as
setCompositorDeviceScaleFactorOverride from devtools.  Would those need to
separately replicate the zoom update to any other processes for the FrameTree?

[wjmaclean, 2016/04/05 20:22:00]

They might, if it has to be done for something other than the main frame. As it
stands, that might still be broken for OOPIF (not sure, as all the tests seem to
pass), and in the non-OOPIF case all the mainFrame cases suffice I suspect.

+{
+    if (zoomLevel < m_minimumZoomLevel)
+        m_zoomLevel = m_minimumZoomLevel;
+    else if (zoomLevel > m_maximumZoomLevel)
+        m_zoomLevel = m_maximumZoomLevel;
+    else
+        m_zoomLevel = zoomLevel;

[alexmos, 2016/04/05 18:00:54]

Is it necessary to have this duplicated here in addition to what's in
setZoomLevelForFrame?  I also don't see any callers caring about the return
value.

[wjmaclean, 2016/04/05 20:22:00]

m_zoomLevel is accessed elsewhere in WebViewImpl, so I'm think we have to set it
here. Are you talking about removing all references to this class var and
replacing them with calls to (something like) get ZoomLevelForFrame?

[alexmos, 2016/04/07 01:20:56]

Sorry, I just meant that this work (clamping the zoom level to limits) also
seems to be done inside setZoomLevelForFrame, and since this always calls
setZoomLevelForFrame below, we end up doing it twice.  I was hoping we could
just delete this and rely on setZoomLevelForFrame doing it?

[wjmaclean, 2016/04/07 12:55:36]

I'd be happy to do that in a follow-on if I can, but I'm really trying to avoid
touching things if it's not central to the purpose of this CL. Is that ok?

[alexmos, 2016/04/07 23:48:08]

Yes, that's ok, though it doesn't look like this is an issue anymore in the
latest PS. :)

+
+    // Code that wishes to set zoom level on local frames when the main frame is
+    // remote should call setZoomLevelForFrame() instead.
+    if (mainFrame()->isWebRemoteFrame())

[alexmos, 2016/04/05 18:00:54]

How much would break if we ASSERT that mainFrame() is a local frame here, rather
than leaving the escape hatch in place?

[wjmaclean, 2016/04/05 20:22:00]

I'm not sure. I'll see if it passes the bots on a dry-run, but if it doesn't, I
suggest we make a separate CL out of it.

My changes to WebViewImpl in this CL are meant to be as close to a pure
re-factor as possible, with the intent that we can address some of these issues
separately (in a hopefully much smaller CL) as needed.

+        return m_zoomLevel;
+
+    return setZoomLevelForFrame(mainFrameImpl()->frame(), m_zoomLevel);
+}
+
 void WebViewImpl::zoomLimitsChanged(double minimumZoomLevel,
                                     double maximumZoomLevel)
 {