Modify a test to wait for compositor frame of OOPIF synced with CSS modifications in parent process

chrome/browser/site_per_process_interactive_browsertest.cc

Index: chrome/browser/site_per_process_interactive_browsertest.cc
diff --git a/chrome/browser/site_per_process_interactive_browsertest.cc b/chrome/browser/site_per_process_interactive_browsertest.cc
index 79c4ca0ffa2b3f4522b4d812573ddd075019a55f..728c7f2f835143842d7d103d084afbb71b132239 100644
--- a/chrome/browser/site_per_process_interactive_browsertest.cc
+++ b/chrome/browser/site_per_process_interactive_browsertest.cc
@@ -5,6 +5,7 @@
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/time/time.h"
 #include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h"
 #include "chrome/browser/ui/browser.h"
@@ -19,6 +20,7 @@
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
 #include "components/security_state/core/security_state.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/focused_node_details.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/notification_details.h"
@@ -1068,6 +1070,30 @@ class FocusedEditableNodeChangedObserver : content::NotificationObserver {
   DISALLOW_COPY_AND_ASSIGN(FocusedEditableNodeChangedObserver);
 };
 
+// Waits until transforming |sample_point| from |render_frame_host| coordinates
+// to its root frame's view's coordinates matches |transformed_point| within a
+// reasonable error margin less than or equal to |bound|. This method is used to
+// verify CSS changes on OOPIFs have been applied properly and the corresponding
+// compositor frame is updated as well. This way we can rest assured that the
+// future transformed and reported bounds for the elements inside
+// |render_frame_host| are correct.
+void WaitForFramePositionUpdated(content::RenderFrameHost* render_frame_host,
+                                 const gfx::Point& sample_point,
+                                 const gfx::Point& transformed_point,
+                                 float bound) {
+  const int64_t kCheckAgainDelay = 60U;
+  while ((transformed_point -
+          render_frame_host->GetView()->TransformPointToRootCoordSpace(
+              sample_point))
+             .Length() > bound) {
+    scoped_refptr<content::MessageLoopRunner> loop_runner_ =

[alexmos, 2017/03/28 18:54:25]

content::MessageLoopRunner is marked as deprecated.  Could you use base::RunLoop
instead, similarly to what's done in
https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser...
?

[EhsanK, 2017/03/28 19:37:46]

Sure. Thanks for pointing this out. Done.

+        new content::MessageLoopRunner();
+    content::BrowserThread::PostDelayedTask(
+        content::BrowserThread::IO, FROM_HERE, loop_runner_->QuitClosure(),
+        base::TimeDelta::FromMilliseconds(kCheckAgainDelay));

[alexmos, 2017/03/28 18:54:25]

nit: you can use TestTimeouts::tiny_timeout()

[EhsanK, 2017/03/28 19:37:46]

Done.

+    loop_runner_->Run();
+  }
+}
 // This test verifies that displacements (margin, etc) in the position of an
 // OOPIF is considered when showing an AutofillClient warning pop-up for
 // unsecure web sites.
@@ -1086,8 +1112,10 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessAutofillTest,
   ASSERT_TRUE(content::ExecuteScript(
       active_web_contents,
       base::StringPrintf("var iframe = document.querySelector('iframe');"
-                         "iframe.style.marginTop = '%dpx';"
-                         "iframe.style.marginLeft = '%dpx';",
+                         "iframe.style.position = 'fixed';"
+                         "iframe.style.border = 'none';"
+                         "iframe.style.top = '%dpx';"
+                         "iframe.style.left = '%dpx';",

[alexmos, 2017/03/28 18:54:24]

I wonder if declaring the CSS statically before navigation could avoid this
problem too - e.g., similarly to page_with_positioned_frame.html.

[EhsanK, 2017/03/28 19:37:46]

It might stop flaking but we are not removing the underlying issue which is the
compositor frame update. It could theoretically still flake if the SurfaceId for
the OOPIF is not registered in time.

Also looking at a helper method which uses the file you mentioned:
https://cs.chromium.org/chromium/src/content/browser/site_per_process_browser...
it seems like we also wait for surface to be ready (which is I guess what the
wait method below is doing indirectly).

[alexmos, 2017/03/28 19:48:13]

Acknowledged.

                          kIframeTopDisplacement, kIframeLeftDisplacement)));
 
   // Navigate the <iframe> to a simple page.
@@ -1096,6 +1124,10 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessAutofillTest,
   content::RenderFrameHost* child_frame = content::FrameMatchingPredicate(
       active_web_contents, base::Bind(&content::FrameIsChildOfMainFrame));
 
+  WaitForFramePositionUpdated(
+      child_frame, gfx::Point(),
+      gfx::Point(kIframeLeftDisplacement, kIframeTopDisplacement), 1.4143f);
+
   // We will need to listen to focus changes to find out about the container
   // bounds of any focused <input> elements on the page.
   FocusedEditableNodeChangedObserver focus_observer;