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

 // 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 "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"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller_test.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/autofill/core/browser/autofill_client.h"
 #include "components/autofill/core/browser/test_autofill_client.h"
 #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"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
@@ skipping 1029 matching lines @@
 
  private:
   content::NotificationRegistrar registrar_;
   bool observed_;
   gfx::Point focused_node_bounds_in_screen_;
   scoped_refptr<content::MessageLoopRunner> loop_runner_;
 
   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.
 IN_PROC_BROWSER_TEST_F(SitePerProcessAutofillTest,
                        PasswordAutofillPopupPositionInsideOOPIF) {
   SetupMainTab();
   ASSERT_TRUE(
       base::FeatureList::IsEnabled(security_state::kHttpFormWarningFeature));
 
   GURL main_url(embedded_test_server()->GetURL("a.com", "/iframe.html"));
   ui_test_utils::NavigateToURL(browser(), main_url);
   content::WebContents* active_web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
 
   // Add some displacement for <iframe>.
   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.
   GURL frame_url = embedded_test_server()->GetURL("b.com", "/title1.html");
   EXPECT_TRUE(NavigateIframeToURL(active_web_contents, "test", frame_url));
   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;
 
   // Focus the child frame, add an <input> with type "password", and focus it.
   ASSERT_TRUE(ExecuteScript(child_frame,
                             "window.focus();"
                             "var input = document.createElement('input');"
                             "input.type = 'password';"
                             "document.body.appendChild(input);"
@@ skipping 21 matching lines @@
 
   // Ideally, the length of the error vector should be 0.0f. But due to
   // potential rounding errors, we assume a larger limit (which is slightly
   // larger than square root of 2).
   EXPECT_LT(error.Length(), 1.4143f)
       << "Origin of bounds from focused node changed event is '"
       << focus_observer.focused_node_bounds_in_screen().ToString()
       << "' but AutofillClient is reporting '" << bounds_origin.ToString()
       << "'";
 }