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

content/browser/iframe_zoom_browsertest.cc

Index: content/browser/iframe_zoom_browsertest.cc
diff --git a/content/browser/iframe_zoom_browsertest.cc b/content/browser/iframe_zoom_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..aec2df88e7a39e366f7efa8ba7066b57130bf989
--- /dev/null
+++ b/content/browser/iframe_zoom_browsertest.cc
@@ -0,0 +1,238 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <vector>
+
+#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/host_zoom_map.h"
+#include "content/public/browser/navigation_entry.h"
+#include "content/public/common/page_zoom.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/shell/browser/shell.h"
+#include "content/test/content_browser_test_utils_internal.h"
+#include "net/dns/mock_host_resolver.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace content {
+
+class IFrameZoomBrowserTest : public ContentBrowserTest {
+ public:
+  IFrameZoomBrowserTest() {}
+
+ protected:
+  void SetUpOnMainThread() override {
+    host_resolver()->AddRule("*", "127.0.0.1");
+    ASSERT_TRUE(embedded_test_server()->Start());
+    SetupCrossSiteRedirector(embedded_test_server());
+  }
+
+  WebContentsImpl* web_contents() {
+    return static_cast<WebContentsImpl*>(shell()->web_contents());
+  }
+};
+
+double GetMainframeWindowBorder(const ToRenderFrameHost& adapter) {
+  double border;
+  const char kGetMainframeBorder[] = "window.domAutomationController.send("
+      "window.outerWidth - window.innerWidth"
+      ");";
+  EXPECT_TRUE(
+      ExecuteScriptAndExtractDouble(adapter, kGetMainframeBorder, &border));
+  return border;
+}
+
+double GetMainframeZoomFactor(const ToRenderFrameHost& adapter, double border) {
+  const char kGetMainframeZoomLevel[] =
+      "window.domAutomationController.send("
+      "(window.outerWidth - %f)/window.innerWidth"
+      ");";
+  double zoom_factor;
+  EXPECT_TRUE(ExecuteScriptAndExtractDouble(
+      adapter, base::StringPrintf(kGetMainframeZoomLevel, border),
+      &zoom_factor));
+  return zoom_factor;
+}
+
+double GetSubframeWidth(const ToRenderFrameHost& adapter) {
+  double width;
+  EXPECT_TRUE(ExecuteScriptAndExtractDouble(
+      adapter, "window.domAutomationController.send(window.innerWidth);",
+      &width));
+  return width;
+}
+
+#define SETUP_A_B_A_NESTED_FRAMES() \
+  std::string top_level_host("a.com");                                         \
+  GURL main_url(embedded_test_server()->GetURL(                                \
+      top_level_host, "/cross_site_iframe_factory.html?a(b(a))"));             \
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));                               \
+  NavigationEntry* entry =                                                     \
+      web_contents()->GetController().GetLastCommittedEntry();                 \
+  ASSERT_TRUE(entry);                                                          \
+  GURL loaded_url = HostZoomMap::GetURLFromEntry(entry);                       \
+  EXPECT_EQ(top_level_host, loaded_url.host());                                \
+                                                                               \
+  FrameTreeNode* root =                                                        \
+      static_cast<WebContentsImpl*>(web_contents())->GetFrameTree()->root();   \
+  auto subframe1 = root->child_at(0)->current_frame_host();                    \

[alexmos, 2016/04/21 17:35:35]

I'd spell out the RenderFrameHost* type for better readability.  Otherwise
someone might think it's a FrameTreeNode* later on and misuse it after a
navigation.

[alexmos, 2016/04/21 17:35:35]

"child" and "grandchild" might be better names, since subframe1 and subframe2
seem like they are siblings (this is the convention we've loosely followed in
other tests).

[wjmaclean, 2016/04/22 19:57:01]

Done & Done.

+  auto subframe2 = root->child_at(0)->child_at(0)->current_frame_host();       \

[alexmos, 2016/04/21 17:35:35]

nit: subframe1->child_at(0)->current_frame_host()

[wjmaclean, 2016/04/22 19:57:01]

subframe1 is a RenderFrameHostImpl*, so this doesn't work :-)

[alexmos, 2016/04/25 22:43:27]

Acknowledged.  I'm too used to these kinds of things being FrameTreeNodes in our
other tests. :)

+                                                                               \
+  /* The following calls must be made when the page's scale factor = 1.0.*/    \
+  double scale_one_subframe1_width = GetSubframeWidth(subframe1);              \
+  double scale_one_subframe2_width = GetSubframeWidth(subframe2);              \
+  double main_frame_window_border = GetMainframeWindowBorder(web_contents());  \
+                                                                               \
+  HostZoomMap* host_zoom_map = HostZoomMap::GetForWebContents(web_contents()); \
+  double default_zoom_level = host_zoom_map->GetDefaultZoomLevel();            \
+  EXPECT_EQ(0.0, default_zoom_level);                                          \
+                                                                               \
+  EXPECT_DOUBLE_EQ(1.0, GetMainframeZoomFactor(web_contents(),                 \
+                                               main_frame_window_border));
+
+struct FrameResizeObserver {
+  FrameResizeObserver(RenderFrameHost* adapter,

[alexmos, 2016/04/21 17:35:35]

nit: rename adapter to host or frame or similar.  adapter doesn't really make
sense here, as this always just takes a RFH* (as opposed to ToRenderFrameHost).

[wjmaclean, 2016/04/22 19:57:01]

Done.

+                      std::string label,
+                      double inner_width,
+                      double tol)

[alexmos, 2016/04/21 17:35:35]

nit: s/tol/tolerance/

[wjmaclean, 2016/04/22 19:57:01]

Done.

+      : frame_adapter(adapter),
+        msg_label(std::move(label)),
+        resized(false),
+        expected_inner_width(inner_width),
+        tolerance(tol) {
+    SetupOnResizeCallback(adapter, msg_label);
+  }
+
+  void SetupOnResizeCallback(const ToRenderFrameHost& adapter,
+                             const std::string& label) {
+    const char kOnResizeCallbackSetup[] =
+        "document.body.onresize = function(){"
+        "  window.domAutomationController.setAutomationId(0);"
+        "  window.domAutomationController.send('%s ' + window.innerWidth);"
+        "};";
+    EXPECT_TRUE(ExecuteScript(
+        adapter, base::StringPrintf(kOnResizeCallbackSetup, label.c_str())));
+  }
+
+  void Check(const std::string& status_msg) {
+    if (status_msg.find(msg_label) != 0)
+      return;
+
+    double inner_width = std::stod(status_msg.substr(msg_label.length() + 1));
+    resized = std::abs(expected_inner_width - inner_width) < tolerance;

[alexmos, 2016/04/21 17:35:35]

So this won't check whether the subframes zoomed to the correct level, just
whether they did or did not zoom?  Did the idea with assigning percentage zoom
width to subframes and then measuring the window.innerWidth change not work out?
 That would allow verifying actual zoom level as well.  Perhaps we could try
that for just A-B if A-B-A is too complicated.

Also, this seems backwards to me: if the width change was below tolerance,
resized is set to true?  (I'd expect it to be false)  I'm a bit confused about
how this would work.  It seems the inner widths for subframes won't change
regardless of whether or not they are zoomed (unless you also did the percentage
widths, and then the resized value would be flipped).  Is this only about
receiving the onresize event?

[wjmaclean, 2016/04/21 17:59:35]

I think you've got it backwards: this method allows for a *precise* check that
the correct zoom level was achieved, since the innerWidth has to match exactly
(the scale change within the frame cancels out the size change of the frameRect,
leading to constant innerWidth). The other method, with percentage-width, kept
leading to slight changes in innerWidth that were too hard to reason about,
which is why I abandoned it. Even for a single sub-frame it was annoying hard to
reason about.
We expect the width change to be zero, give-or-take any floating point error.
Hence change below tolerance is accepted. Here I've specified .1 pixels, which
should be fine.

[alexmos, 2016/04/21 19:02:40]

Ah, I see.  Thanks for clarifying! (Let's explain this strategy in test
comments.)  I assume before any changes in your CL, if you zoom a page with an
OOPIF, the OOPIF's window.innerWidth would keep increasing, since the frameRect
size change is propagated but the scale isn't.

I like this.  Is it guaranteed that the onresize event will always fire for
zoom?  Also, there's a danger that a spurious onresize event (unrelated to zoom)
might cause the test to still pass if the width didn't change at all, but I
suppose that shouldn't be possible with the test setup?
Acknowledged.

[alexmos, 2016/04/21 19:08:45]

Also, maybe rename |resized| to |zoomed_correctly| or something like that, since
this was what caused my confusion initially (this is about whether or not the
frame maintained its old innerWidth after zoom, not about whether or not it
resized).

[wjmaclean, 2016/04/22 19:57:01]

It *seems* like it always fires ... but I can't point to a codepath that
provides a hard guarantee. But I suspect any call to setFrameRect() will force
an onresize event. A spurious onresize would only cause us grief if it happened
to cause the innerWidth to be our expected value, which I don't think can happen
spuriously. Depending on timing, we may get one or two onresize events, but it's
only the last one that has the correct size we are looking for.

[alexmos, 2016/04/25 22:43:27]

Acknowledged.  It might be good to document the fact that we could get one or
two onresize events and that we need to wait for the last one on top of the
WaitForMessage loop.

+  }
+
+  FrameResizeObserver* toThis() {return this;}
+
+  RenderFrameHost* frame_adapter;
+  std::string msg_label;
+  bool resized;
+  double expected_inner_width;
+  double tolerance;
+};
+
+void WaitAndCheckFrameResize(
+    DOMMessageQueue& msg_queue,
+    std::vector<FrameResizeObserver>& frame_observers) {
+  std::string status;
+  while (msg_queue.WaitForMessage(&status)) {
+    // Strip the double quotes from the message.
+    status = status.substr(1, status.length() -2);
+
+    bool all_resized = true;
+
+    // Use auto& to operate on a reference, and not a copy.
+    for (auto& observer : frame_observers) {
+      observer.Check(status);
+      all_resized = all_resized && observer.resized;
+    }
+
+    if (all_resized)
+      break;
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(IFrameZoomBrowserTest, SubframesZoomProperly) {
+  SETUP_A_B_A_NESTED_FRAMES();
+
+  const double new_zoom_factor = 2.5;
+  {
+    DOMMessageQueue msg_queue;
+
+    const double kTolerance = 0.1;
+    std::vector<FrameResizeObserver> frame_observers;
+    frame_observers.emplace_back(subframe1, "subframe1",
+                                 scale_one_subframe1_width, kTolerance);
+    frame_observers.emplace_back(subframe2, "subframe2",
+                                 scale_one_subframe2_width, kTolerance);
+
+    const double new_zoom_level =
+        default_zoom_level + ZoomFactorToZoomLevel(new_zoom_factor);
+    host_zoom_map->SetZoomLevelForHost(top_level_host, new_zoom_level);
+
+    WaitAndCheckFrameResize(msg_queue, frame_observers);
+  }
+
+  EXPECT_DOUBLE_EQ(
+      new_zoom_factor,
+      GetMainframeZoomFactor(web_contents(), main_frame_window_border));
+}
+
+IN_PROC_BROWSER_TEST_F(IFrameZoomBrowserTest, SubframesDontZoomIndependently) {
+  SETUP_A_B_A_NESTED_FRAMES();
+
+  const double new_zoom_factor = 2.0;
+  const double new_zoom_level =
+      default_zoom_level + ZoomFactorToZoomLevel(new_zoom_factor);
+
+  // This should not cause the nested iframe to change its zoom.
+  host_zoom_map->SetZoomLevelForHost("b.com", new_zoom_level);
+
+  EXPECT_DOUBLE_EQ(
+      1.0, GetMainframeZoomFactor(web_contents(), main_frame_window_border));
+  EXPECT_EQ(scale_one_subframe1_width, GetSubframeWidth(subframe1));
+  EXPECT_EQ(scale_one_subframe2_width, GetSubframeWidth(subframe2));
+
+  // When we navigate so that b.com is the top-level site, then it has the
+  // expected zoom.
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL(
+                   "b.com", "/cross_site_iframe_factory.html?b")));
+  EXPECT_DOUBLE_EQ(
+      new_zoom_factor,
+      GetMainframeZoomFactor(web_contents(), main_frame_window_border));
+}
+
+IN_PROC_BROWSER_TEST_F(IFrameZoomBrowserTest, AllFramesGetDefaultZoom) {
+  SETUP_A_B_A_NESTED_FRAMES();
+
+  const double new_default_zoom_factor = 2.0;
+  {
+    DOMMessageQueue msg_queue;
+
+    const double kTolerance = 0.1;
+    std::vector<FrameResizeObserver> frame_observers;
+    frame_observers.emplace_back(subframe1, "subframe1",
+                                 scale_one_subframe1_width, kTolerance);
+    frame_observers.emplace_back(subframe2, "subframe2",
+                                 scale_one_subframe2_width, kTolerance);
+
+    const double new_default_zoom_level =
+        default_zoom_level + ZoomFactorToZoomLevel(new_default_zoom_factor);
+
+    host_zoom_map->SetZoomLevelForHost("b.com", new_default_zoom_level + 1.0);
+    host_zoom_map->SetDefaultZoomLevel(new_default_zoom_level);
+
+    WaitAndCheckFrameResize(msg_queue, frame_observers);
+  }
+  EXPECT_DOUBLE_EQ(
+      new_default_zoom_factor,
+      GetMainframeZoomFactor(web_contents(), main_frame_window_border));
+}

[alexmos, 2016/04/21 17:35:35]

So far, these are are about A-B-A without subframe navigations, but I think we
should test at least a couple of other scenarios:
- A-embed-B, zoom the page, and navigate subframe B cross-site to C.  Make sure
C still has correct zoom.
- A embeds two children (B, B).  Make sure both subframes have correct zoom.

[wjmaclean, 2016/04/21 17:59:35]

I'm ok with adding some tests to test subframe navigation, and the two-child
case ... I'll add these.

+
+}  // namespace content