Propagate scrolling/marginwidth/marginheight property values to child frame.

content/browser/site_per_process_browsertest.cc

Index: content/browser/site_per_process_browsertest.cc
diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc
index d500ce60e40f4ba222745b220de50392867d9365..8146a37d06ef6467d58eb02330590029857f306e 100644
--- a/content/browser/site_per_process_browsertest.cc
+++ b/content/browser/site_per_process_browsertest.cc
@@ -1812,6 +1812,173 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   }
 }
 
+// Verify that scrolling property propagates to child frames correctly.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       FrameOwnerPropertiesPropagationScrolling) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/frame_owner_properties_scrolling.html"));
+  NavigateToURL(shell(), main_url);
+
+  // It is safe to obtain the root frame tree node here, as it doesn't change.
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  ASSERT_EQ(1u, root->child_count());
+
+  EXPECT_EQ(
+      " Site A ------------ proxies for B\n"
+      "   +--Site B ------- proxies for A\n"
+      "Where A = http://a.com/\n"
+      "      B = http://b.com/",
+      DepictFrameTree(root));
+
+  FrameTreeNode* child = root->child_at(0);
+
+  auto has_scrollbar = [](RenderFrameHostImpl* rfh) {
+    int client_width;
+    EXPECT_TRUE(ExecuteScriptAndExtractInt(rfh,
+        "window.domAutomationController.send(document.body.clientWidth);",
+        &client_width));
+    const int THRESHOLD = 4;
+    const int FRAME_ELEMENT_WIDTH = 200;

[alexmos, 2015/09/23 22:38:27]

I wonder if there's an easier way to check if the scrollbar is present, rather
than hardcoding the width and using a threshold.  E.g., looking at
StackOverflow, this JS snippet may work?  
  var hasScrollbar = document.body.scrollHeight > document.body.clientHeight;

[lazyboy, 2015/09/30 23:37:54]

That gives us whether there's any content to scroll, which would always be true
for the test. We want to check whether we are
actually showing a scrollbar or not.

[alexmos, 2015/10/02 21:24:19]

Acknowledged.  Perhaps explain the need for threshold in a comment?

[lazyboy, 2015/10/05 22:16:08]

Done.

+    return client_width + THRESHOLD < FRAME_ELEMENT_WIDTH;
+  };
+
+  auto set_scrolling_property = [](RenderFrameHostImpl* parent_rfh,
+                                   const std::string& value) {
+    EXPECT_TRUE(ExecuteScript(
+        parent_rfh,
+        base::StringPrintf(
+            "window.domAutomationController.send("

[alexmos, 2015/09/23 22:38:27]

I don't think ExecuteScript needs the domAutomationController.send (see comment
on
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...)

[lazyboy, 2015/09/30 23:37:54]

Done.

+            "document.getElementById('child-1').setAttribute("
+            "    'scrolling', '%s'));", value.c_str())));
+  };
+
+  GURL urls[4] = {

[alexmos, 2015/09/23 22:38:27]

nit: can omit array size, here and below.

[lazyboy, 2015/09/30 23:37:54]

Done.

+    // Remote to remote.
+    embedded_test_server()->GetURL("b.com", "/tall_page1.html"),
+    // Remote to local.
+    embedded_test_server()->GetURL("a.com", "/tall_page1.html"),
+    // Local to local.
+    embedded_test_server()->GetURL("a.com", "/tall_page2.html"),

[alexmos, 2015/09/23 22:38:27]

Do we really need to test the local-to-local case?

[lazyboy, 2015/09/30 23:37:54]

Removed local to local case.

+    // Local to remote.
+    embedded_test_server()->GetURL("b.com", "/tall_page2.html")

[alexmos, 2015/09/23 22:38:27]

Can we get away with using just one tall page?  Does navigating to same URL
still update these properties? (I'm assuming so, since the very first navigation
is from b.com/tall_page1.html to b.com/tall_page1.html, and that works?)

[lazyboy, 2015/09/30 23:37:54]

Using one tall_page.html
That passes the test, but I meant to actually change the path. I've changed it
to go from b.com/title1.html to b.com/tall_page.html

+  };
+  const std::string scrolling_values[3] = {
+    "yes", "auto", "no"
+  };
+
+  for (size_t i = 0; i < arraysize(scrolling_values); ++i) {
+    bool expect_scrollbar = scrolling_values[i] != "no";
+    set_scrolling_property(root->current_frame_host(), scrolling_values[i]);
+    for (size_t j = 0; j < arraysize(urls); ++j) {
+      NavigateFrameToURL(child, urls[j]);
+
+      // TODO(alexmos): This can be removed once TestFrameNavigationObserver is
+      // fixed to use DidFinishLoad.
+      EXPECT_TRUE(

[alexmos, 2015/09/23 22:38:27]

nit: line break doesn't look necessary.

[lazyboy, 2015/09/30 23:37:54]

Done.

+          WaitForRenderFrameReady(child->current_frame_host()));
+
+      EXPECT_EQ(expect_scrollbar, has_scrollbar(child->current_frame_host()));
+    }
+  }
+}
+
+// Verify that marginwidth and marginheight properties propagate to child
+// frames correctly.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       FrameOwnerPropertiesPropagationMargin) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/frame_owner_properties_margin.html"));
+  NavigateToURL(shell(), main_url);
+
+  // It is safe to obtain the root frame tree node here, as it doesn't change.
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  ASSERT_EQ(1u, root->child_count());
+
+  EXPECT_EQ(
+      " Site A ------------ proxies for B\n"
+      "   +--Site B ------- proxies for A\n"
+      "Where A = http://a.com/\n"
+      "      B = http://b.com/",
+      DepictFrameTree(root));
+
+  FrameTreeNode* child = root->child_at(0);
+
+  std::string margin_width;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      child->current_frame_host(),
+      "window.domAutomationController.send("
+      "document.body.getAttribute('marginwidth'));",
+      &margin_width));
+  EXPECT_EQ("10", margin_width);
+
+  std::string margin_height;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      child->current_frame_host(),
+      "window.domAutomationController.send("
+      "document.body.getAttribute('marginheight'));",
+      &margin_height));
+  EXPECT_EQ("50", margin_height);
+
+  GURL urls[4] = {
+    // Remote to remote.
+    embedded_test_server()->GetURL("b.com", "/title1.html"),
+    // Remote to local.
+    embedded_test_server()->GetURL("a.com", "/title1.html"),
+    // Local to local.
+    embedded_test_server()->GetURL("a.com", "/title2.html"),
+    // Local to remote.
+    embedded_test_server()->GetURL("b.com", "/title2.html")
+  };
+
+  int current_margin_width = 15;
+  int current_margin_height = 25;
+  for (size_t i = 0; i < arraysize(urls); ++i) {
+    // Change marginwidth and marginheight before navigating.
+    EXPECT_TRUE(ExecuteScript(
+        root->current_frame_host(),
+        base::StringPrintf(
+            "window.domAutomationController.send("

[alexmos, 2015/09/23 22:38:27]

See earlier comment about domAutomationController.send not being necessary (here
and below)

[lazyboy, 2015/09/30 23:37:54]

Done.

+            "document.getElementById('child-1').setAttribute("
+            "    'marginwidth', '%d'));", current_margin_width)));
+    EXPECT_TRUE(ExecuteScript(
+        root->current_frame_host(),
+        base::StringPrintf(
+            "window.domAutomationController.send("
+            "document.getElementById('child-1').setAttribute("
+            "    'marginheight', '%d'));", current_margin_height)));
+
+    // Navigate.

[alexmos, 2015/09/23 22:38:27]

nit: no need for this comment.  Instead, maybe explain at a high level what's
going to happen in a comment on the for loop above.

[lazyboy, 2015/09/30 23:37:54]

Done.

+    NavigateFrameToURL(child, urls[i]);
+    // TODO(alexmos): This can be removed once TestFrameNavigationObserver is
+    // fixed to use DidFinishLoad.
+    EXPECT_TRUE(

[alexmos, 2015/09/23 22:38:27]

nit: no line break

[lazyboy, 2015/09/30 23:37:54]

Done.

+        WaitForRenderFrameReady(child->current_frame_host()));
+
+    std::string actual_margin_width;
+    EXPECT_TRUE(ExecuteScriptAndExtractString(
+        child->current_frame_host(),
+        "window.domAutomationController.send("
+        "document.body.getAttribute('marginwidth'));",
+        &actual_margin_width));
+    EXPECT_EQ(base::IntToString(current_margin_width), actual_margin_width);
+
+    std::string actual_margin_height;
+    EXPECT_TRUE(ExecuteScriptAndExtractString(

[alexmos, 2015/09/23 22:38:27]

Does ExecuteScriptAndExtractInt not work for these?

[lazyboy, 2015/09/30 23:37:54]

No. getAttribute returns string.

[alexmos, 2015/10/02 21:24:19]

Acknowledged.

+        child->current_frame_host(),
+        "window.domAutomationController.send("
+        "document.body.getAttribute('marginheight'));",
+        &actual_margin_height));
+    EXPECT_EQ(base::IntToString(current_margin_height), actual_margin_height);
+
+    current_margin_width += 5;
+    current_margin_height += 10;
+  }
+}
+
 // Verify origin replication with an A-embed-B-embed-C-embed-A hierarchy.
 IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, OriginReplication) {
   GURL main_url(embedded_test_server()->GetURL(