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..4cd8a922ca4a3e78b42f6879ec40f00cf80d143f
--- /dev/null
+++ b/content/browser/iframe_zoom_browsertest.cc
@@ -0,0 +1,358 @@
+// 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/public/test/test_navigation_observer.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 {

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

A comment with a high-level overview of how these tests work would be great to
have here. This can just contain what you wrote in the comments (i.e., that
scale changes within a frame cancel out the size change of the frameRect,
leading to constant innerWidth).

[wjmaclean, 2016/04/27 13:43:57]

I've tried to improve this ... let me know what you think.

[alexmos, 2016/04/27 23:39:51]

This is great - thanks for adding this!

+ 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) {

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

nit: s/GetMainframeZoomFactor/GetMainFrameZoomFactor/ for naming consistency in
other places.  Same below for kGetMainFrameZoomLevel.

[wjmaclean, 2016/04/27 13:43:57]

Done. I had used 'Mainframe' to match 'Subframe', but whatever.

+  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()                                            \

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

I'm not sure how I feel about this macro. :)  I'm concerned it makes the tests
harder to follow/modify and will screw up the reported line numbers if one of
the assertions fails.  Could we just repeat the setup in three tests?  You're
already doing it in the last two, plus see my comment about not needing the
|entry| verification everywhere.  Also, not sure it's worth it, but we could
extract out the bits common to all five tests (main_frame_window_border and
default_zoom_level verification) into a class-level helper/fields on
IFrameZoomBrowserTest to reduce some of the repetition.  

Actually, the other global helpers might make more sense as class helpers too --
or you might want to move them to an anonymous namespace, which seems to be the
typical pattern in other test files.

[wjmaclean, 2016/04/27 13:43:57]

I've replicated the macro, and moved the helpers into an anonymous namespace.

I didn't add anything to the main class, as I don't think it's right to load the
class down with stuff that's specific to certain tests.

+  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();   \
+  RenderFrameHostImpl* child = root->child_at(0)->current_frame_host();        \
+  RenderFrameHostImpl* grandchild =                                            \
+      root->child_at(0)->child_at(0)->current_frame_host();                    \
+                                                                               \
+  /* The following calls must be made when the page's scale factor = 1.0.*/    \
+  double scale_one_child_width = GetSubframeWidth(child);                      \
+  double scale_one_grandchild_width = GetSubframeWidth(grandchild);            \
+  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));
+
+// The tests in this file rely on the notion that, when a page zooms, that
+// subframes both (1) a change in their frame rect, and (2) a change in their
+// frame's scale. Since the page should scale as a unit, this means the
+// innerWidth value of any subframe should be the same before and after the
+// zoom (though it may transiently take on a different value). The
+// FrameSizeObserver serves to watch for onresize events, and observes when
+// the innerWidth is correctly set.
+struct FrameResizeObserver {
+  FrameResizeObserver(RenderFrameHost* host,
+                      std::string label,
+                      double inner_width,
+                      double tolerance)
+      : frame_host(host),
+        msg_label(std::move(label)),
+        zoomed_correctly(false),
+        expected_inner_width(inner_width),
+        tolerance(tolerance) {
+    SetupOnResizeCallback(host, 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));
+    zoomed_correctly = std::abs(expected_inner_width - inner_width) < tolerance;
+  }
+
+  FrameResizeObserver* toThis() {return this;}
+
+  RenderFrameHost* frame_host;
+  std::string msg_label;
+  bool zoomed_correctly;
+  double expected_inner_width;
+  double tolerance;
+};
+
+void WaitAndCheckFrameZoom(
+    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_zoomed_correctly = true;
+
+    // Use auto& to operate on a reference, and not a copy.
+    for (auto& observer : frame_observers) {
+      observer.Check(status);
+      all_zoomed_correctly = all_zoomed_correctly && observer.zoomed_correctly;
+    }
+
+    if (all_zoomed_correctly)
+      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(child, "child",
+                                 scale_one_child_width, kTolerance);
+    frame_observers.emplace_back(grandchild, "grandchild",
+                                 scale_one_grandchild_width, kTolerance);
+
+    const double new_zoom_level =
+        default_zoom_level + ZoomFactorToZoomLevel(new_zoom_factor);
+    host_zoom_map->SetZoomLevelForHost(top_level_host, new_zoom_level);
+
+    WaitAndCheckFrameZoom(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_child_width, GetSubframeWidth(child));
+  EXPECT_EQ(scale_one_grandchild_width, GetSubframeWidth(grandchild));
+
+  // 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(child, "child",
+                                 scale_one_child_width, kTolerance);
+    frame_observers.emplace_back(grandchild, "grandchild",
+                                 scale_one_grandchild_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);
+
+    WaitAndCheckFrameZoom(msg_queue, frame_observers);
+  }
+  EXPECT_DOUBLE_EQ(
+      new_default_zoom_factor,
+      GetMainframeZoomFactor(web_contents(), main_frame_window_border));
+}
+
+IN_PROC_BROWSER_TEST_F(IFrameZoomBrowserTest, SiblingFramesZoom) {
+  std::string top_level_host("a.com");
+  GURL main_url(embedded_test_server()->GetURL(
+      top_level_host, "/cross_site_iframe_factory.html?a(b,b)"));
+  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());

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

Maybe it's enough to have coverage for lines 252-256 in one of the tests?  It
might be a bit redundant in all tests.

[alexmos, 2016/04/27 23:39:51]

Just making sure you saw this -- though it's also minor and ok to ignore. :)

+
+  FrameTreeNode* root =
+      static_cast<WebContentsImpl*>(web_contents())->GetFrameTree()->root();
+  RenderFrameHostImpl* child1 = root->child_at(0)->current_frame_host();
+  RenderFrameHostImpl* child2 = root->child_at(1)->current_frame_host();
+
+  /* The following calls must be made when the page's scale factor = 1.0.*/

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

Any reason not to use // instead of /* */ here for consistency?  Same below.

[wjmaclean, 2016/04/27 13:43:57]

This migrated from a macro, hence the old-style comment.
Done.

+  double scale_one_child1_width = GetSubframeWidth(child1);
+  double scale_one_child2_width = GetSubframeWidth(child2);
+  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));
+
+  const double new_zoom_factor = 2.5;
+  {
+    DOMMessageQueue msg_queue;
+
+    const double kTolerance = 0.1;

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

How about moving this to be declared only once somewhere on top?  And also
adding a comment that this is in pixels.

[wjmaclean, 2016/04/27 13:43:57]

Done.

+    std::vector<FrameResizeObserver> frame_observers;
+    frame_observers.emplace_back(child1, "child1",
+                                 scale_one_child1_width, kTolerance);
+    frame_observers.emplace_back(child2, "child2",
+                                 scale_one_child2_width, kTolerance);
+
+    const double new_zoom_level =
+        default_zoom_level + ZoomFactorToZoomLevel(new_zoom_factor);
+    host_zoom_map->SetZoomLevelForHost(top_level_host, new_zoom_level);
+
+    WaitAndCheckFrameZoom(msg_queue, frame_observers);
+  }
+
+  EXPECT_DOUBLE_EQ(
+      new_zoom_factor,
+      GetMainframeZoomFactor(web_contents(), main_frame_window_border));
+}
+
+IN_PROC_BROWSER_TEST_F(IFrameZoomBrowserTest, SubframeRetainsZoomOnNavigation) {
+  std::string top_level_host("a.com");
+  GURL main_url(embedded_test_server()->GetURL(
+      top_level_host, "/cross_site_iframe_factory.html?a(b)"));
+  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();
+  RenderFrameHostImpl* child = root->child_at(0)->current_frame_host();
+
+  /* The following calls must be made when the page's scale factor = 1.0.*/
+  double scale_one_child_width = GetSubframeWidth(child);
+  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));
+
+  const double new_zoom_factor = 2.5;

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

It probably doesn't make any difference, but maybe it's worth covering the zoom
factor being less than 1.0 in one of the tests?

[wjmaclean, 2016/04/27 13:43:57]

Actually, I don't think it makes any difference, as the zoom system never really
looks at this value, other than to test when it equals the default zoom level.
But I'll change one test anyways.

+  {
+    DOMMessageQueue msg_queue;
+
+    const double kTolerance = 0.1;
+    std::vector<FrameResizeObserver> frame_observers;
+    frame_observers.emplace_back(child, "child",
+                                 scale_one_child_width, kTolerance);
+
+    const double new_zoom_level =
+        default_zoom_level + ZoomFactorToZoomLevel(new_zoom_factor);
+    host_zoom_map->SetZoomLevelForHost(top_level_host, new_zoom_level);
+
+    WaitAndCheckFrameZoom(msg_queue, frame_observers);
+  }
+
+  EXPECT_DOUBLE_EQ(
+      new_zoom_factor,
+      GetMainframeZoomFactor(web_contents(), main_frame_window_border));
+
+  // Navigate child frame, and make sure zoom is the same.

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

nit: "Navigate the child frame cross-site"

[wjmaclean, 2016/04/27 13:43:57]

Done.

+  TestNavigationObserver observer(web_contents());
+  GURL url = embedded_test_server()->GetURL("c.com", "/title1.html");
+  NavigateFrameToURL(root->child_at(0), url);
+  EXPECT_TRUE(observer.last_navigation_succeeded());
+  EXPECT_EQ(url, observer.last_navigation_url());
+
+  // After the navigation, we expect the child sub-frame to have the same scale,
+  // even if its domain has a different value specified.

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

nit: "Check that the child frame maintained the same scale after navigating
cross-site."

[wjmaclean, 2016/04/27 13:43:57]

Done.

+  double new_child_width =
+      GetSubframeWidth(root->child_at(0)->current_frame_host());
+  EXPECT_EQ(scale_one_child_width, new_child_width);
+}
+
+}  // namespace content