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

third_party/WebKit/Source/web/WebViewImpl.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 3004 matching lines @@
     // TODO(alexmos): Pass in proper with sourceCapabilities.
     page()->focusController().advanceFocusAcrossFrames(
         type, toWebRemoteFrameImpl(from)->frame(), toWebLocalFrameImpl(to)->frame());
 }
 
 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;
     else if (zoomLevel > m_maximumZoomLevel)
         m_zoomLevel = m_maximumZoomLevel;
     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) {
             if (m_compositorDeviceScaleFactorOverride) {
                 // Adjust the page's DSF so that DevicePixelRatio becomes m_zoomFactorForDeviceScaleFactor.
                 page()->setDeviceScaleFactor(m_zoomFactorForDeviceScaleFactor / m_compositorDeviceScaleFactorOverride);
                 zoomFactor *= m_compositorDeviceScaleFactorOverride;
             } else {
                 page()->setDeviceScaleFactor(1.f);
                 zoomFactor *= m_zoomFactorForDeviceScaleFactor;
             }
         }
         frame->setPageZoomFactor(zoomFactor);

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

This calls LocalFrame::setPageAndTextZoomFactors, which also updates the zoom
parameters for all local child frames:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
 Isn't this redundant, since with one IPC per frame,
WebViewImpl::setZoomLevelForFrame will also be called separately for those child
frames?

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

Yes, I suppose there is some redundancy. This could be avoided if we bail out
for any local frame that is not a local root (since all frames in a page should
have the same zoom), though I'm not sure how to determine that.

     }
 
     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)
 {
     m_minimumZoomLevel = minimumZoomLevel;
     m_maximumZoomLevel = maximumZoomLevel;
     m_client->zoomLimitsChanged(m_minimumZoomLevel, m_maximumZoomLevel);
 }
 
 float WebViewImpl::textZoomFactor()
 {
@@ skipping 1496 matching lines @@
 {
     // TODO(oshima): Investigate if this should return the ScreenInfo's scale factor rather than
     // page's scale factor, which can be 1 in use-zoom-for-dsf mode.
     if (!page())
         return 1;
 
     return page()->deviceScaleFactor();
 }
 
 } // namespace blink