Fix the position of AutofillClient for PasswordAutofillManager when the focused form element is ins…

chrome/browser/chrome_site_per_process_browsertest.cc

Index: chrome/browser/chrome_site_per_process_browsertest.cc
diff --git a/chrome/browser/chrome_site_per_process_browsertest.cc b/chrome/browser/chrome_site_per_process_browsertest.cc
index de4374a469d5eb0f8268a137353cc3bfd927c6ff..324f3ef897293663865fe2b01e4e06138abda74a 100644
--- a/chrome/browser/chrome_site_per_process_browsertest.cc
+++ b/chrome/browser/chrome_site_per_process_browsertest.cc
@@ -7,16 +7,21 @@
 #include "base/macros.h"
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/external_protocol/external_protocol_handler.h"
 #include "chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h"
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.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/interstitial_page.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_service.h"
@@ -500,3 +505,143 @@ IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,
   EXPECT_TRUE(popup_handle_is_valid);
   ASSERT_EQ(2, browser()->tab_strip_model()->count());
 }
+
+namespace autofill {

[EhsanK, 2017/03/16 20:39:00]

not sure if this is better on top.

[Charlie Reis, 2017/03/17 18:43:07]

Yes, please move to the top.

[EhsanK, 2017/03/20 16:45:28]

Done.

+class AutofillPopupDelegate;
+struct Suggestion;
+}
+
+class ChromeSitePerProcessAutofillTest : public ChromeSitePerProcessTest {
+ public:
+  ChromeSitePerProcessAutofillTest() : ChromeSitePerProcessTest() {}
+  ~ChromeSitePerProcessAutofillTest() override{};
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ChromeSitePerProcessTest::SetUpCommandLine(command_line);
+    // We need to set the feature state before the render process is created,
+    // in order for it to inherit the feature state from the browser process.
+    // SetUp() runs too early, and SetUpOnMainThread() runs too late.
+    scoped_feature_list_.InitAndEnableFeature(
+        security_state::kHttpFormWarningFeature);
+  }
+
+  void SetUpOnMainThread() override {
+    ChromeSitePerProcessTest::SetUpOnMainThread();
+  }
+
+ protected:
+  class TestAutofillClient : public autofill::TestAutofillClient {
+   public:
+    TestAutofillClient() : popup_shown_(false){};
+    ~TestAutofillClient() override {}
+
+    void WaitForNextPopup() {
+      if (popup_shown_)
+        return;
+      loop_runner_ = new content::MessageLoopRunner();
+      loop_runner_->Run();
+    }
+
+    virtual void ShowAutofillPopup(
+        const gfx::RectF& element_bounds,
+        base::i18n::TextDirection text_direction,
+        const std::vector<autofill::Suggestion>& suggestions,
+        base::WeakPtr<autofill::AutofillPopupDelegate> delegate) {
+      element_bounds_ = element_bounds;
+      popup_shown_ = true;
+      if (loop_runner_)
+        loop_runner_->Quit();
+    }
+
+    const gfx::RectF& last_element_bounds() const { return element_bounds_; }
+
+   private:
+    gfx::RectF element_bounds_;
+    bool popup_shown_;
+    scoped_refptr<content::MessageLoopRunner> loop_runner_;
+
+    DISALLOW_COPY_AND_ASSIGN(TestAutofillClient);
+  };
+
+  const int kIframeTopDisplacement = 150;
+  const int kIframeLeftDisplacement = 200;
+
+  void SetupMainTab() {
+    // Add a fresh new WebContents for which we add our own version of the
+    // ChromePasswordManagerClient that uses a custom TestAutofillClient.
+    content::WebContents* new_contents = content::WebContents::Create(
+        content::WebContents::CreateParams(browser()
+                                               ->tab_strip_model()
+                                               ->GetActiveWebContents()
+                                               ->GetBrowserContext()));
+    ASSERT_TRUE(new_contents);
+    ASSERT_FALSE(ChromePasswordManagerClient::FromWebContents(new_contents));
+
+    // Create ChromePasswordManagerClient and verify it exists for the new
+    // WebContents.
+    ChromePasswordManagerClient::CreateForWebContentsWithAutofillClient(
+        new_contents, &test_autofill_client_);
+    ASSERT_TRUE(ChromePasswordManagerClient::FromWebContents(new_contents));
+
+    browser()->tab_strip_model()->AppendWebContents(new_contents, true);
+  }
+
+  TestAutofillClient& autofill_client() { return test_autofill_client_; }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+  TestAutofillClient test_autofill_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeSitePerProcessAutofillTest);
+};
+
+// 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(ChromeSitePerProcessAutofillTest,
+                       AutofillClientPoisitonWhenInsideOOPIF) {

[Charlie Reis, 2017/03/17 18:43:07]

nit: typo in Position

[EhsanK, 2017/03/20 16:45:28]

Done.

+  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';",
+                         kIframeTopDisplacement, kIframeLeftDisplacement)));
+
+  // Navigate the <iframe> to a page with <form>

[Charlie Reis, 2017/03/17 18:43:07]

nit: End with period, as above.

[EhsanK, 2017/03/20 16:45:27]

Done.

+  GURL frame_url = embedded_test_server()->GetURL("b.com", "/title1.html");
+  EXPECT_TRUE(NavigateIframeToURL(active_web_contents, "test", frame_url));
+
+  // Insert a password <input> inside the form.

[Charlie Reis, 2017/03/17 18:43:07]

The comments are suggesting that there's an existing form, but that's not the
case for title1.html.

[EhsanK, 2017/03/20 16:45:28]

Yes I forgot to fix the comments after changing my test a bit. Thanks! Changed
some of the comments.

+  content::RenderFrameHost* child_frame = content::FrameMatchingPredicate(
+      active_web_contents, base::Bind(&content::FrameIsChildOfMainFrame));
+
+  // 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);"
+                            "input.focus();"));
+
+  // The user gesture should lead to a security warning.
+  content::SimulateKeyPress(active_web_contents, ui::DomKey::FromCharacter('A'),
+                            ui::DomCode::US_A, ui::VKEY_A, false, false, false,
+                            false);
+  autofill_client().WaitForNextPopup();
+
+  // Verify the whereabouts of the pop-up.
+  EXPECT_GT(autofill_client().last_element_bounds().origin().y() /* top */,

[Charlie Reis, 2017/03/17 18:43:07]

This would miss cases that the popup is too low or too far right, but I guess we
don't know exactly where it will be.  I guess I'm ok with it, unless there's a
better approach in other autofill popup tests.

[EhsanK, 2017/03/20 16:45:28]

I am now using the notification about focused node change which contains bounds
in screen coordinates.

+            kIframeTopDisplacement);
+  EXPECT_GT(autofill_client().last_element_bounds().origin().x() /* left */,
+            kIframeLeftDisplacement);
+}