Revert of Do not haul suggestions back to browser in...

Revert of Do not haul suggestions back to browser in AutofillHostMsg_ShowPasswordSuggestions (patchset #2 id:20001 of https://codereview.chromium.org/688633004/)

Reason for revert:
caused crashers. see bug 429576

Original issue's description:
> Do not haul suggestions back to browser in AutofillHostMsg_ShowPasswordSuggestions
>
> Currently, PasswordAutofillAgent (in renderer) prepares password autofill suggestions for PasswordAutofillManager (in browser) and sends them via IPC.
>
> But the manager has the data to generate the suggestions itself, so this CL makes it derive the suggestions from that data instead of hauling it through IPC.
>
> This CL also adds one more test case to the PasswordAutofillManager unittest, to compensate for the coverage lost in the PasswordAutofillAgent browsertest by moving some logic out of the agent to the manager.
>
> BUG=400186, 377422, 118601
>
> Committed: https://crrev.com/86cc798cc2947dd88aa73505d716f93538dffa5b
> Cr-Commit-Position: refs/heads/master@{#302245}

TBR=gcasto@chromium.org,tsepez@chromium.org,jww@chromium.org,vabr@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=400186, 377422, 118601

Review URL: https://codereview.chromium.org/695233002

Cr-Commit-Position: refs/heads/master@{#302413}

chrome/renderer/autofill/password_autofill_agent_browsertest.cc

@@ -417,22 +417,47 @@ class PasswordAutofillAgentTest : public ChromeRenderViewTest {
     EXPECT_EQ(end, username_element_.selectionEnd());
   }
 
-  // Checks the message sent to PasswordAutofillManager to build the suggestion
-  // list. |username| is the expected username field value, and |show_all| is
-  // the expected flag for the PasswordAutofillManager, whether to show all
-  // suggestions, or only those starting with |username|.
-  void CheckSuggestions(const std::string& username, bool show_all) {
+  void ExpectOneCredential(const base::string16& username) {
     const IPC::Message* message =
         render_thread_->sink().GetFirstMessageMatching(
             AutofillHostMsg_ShowPasswordSuggestions::ID);
-    EXPECT_TRUE(message);
-    Tuple4<autofill::FormFieldData, base::string16, bool, gfx::RectF> args;
+    ASSERT_TRUE(message);
+    Tuple4<autofill::FormFieldData,
+           gfx::RectF,
+           std::vector<base::string16>,
+           std::vector<base::string16> > args;
     AutofillHostMsg_ShowPasswordSuggestions::Read(message, &args);
-    EXPECT_EQ(2u, fill_data_.basic_data.fields.size());
-    EXPECT_EQ(fill_data_.basic_data.fields[0].name, args.a.name);
-    EXPECT_EQ(ASCIIToUTF16(username), args.a.value);
-    EXPECT_EQ(ASCIIToUTF16(username), args.b);
-    EXPECT_EQ(show_all, args.c);
+    ASSERT_EQ(1u, args.c.size());
+    EXPECT_TRUE(args.c[0] == username);
+  }
+
+  void ExpectAllCredentials() {
+    std::set<base::string16> usernames;
+    usernames.insert(username1_);
+    usernames.insert(username2_);
+    usernames.insert(username3_);
+    usernames.insert(alternate_username3_);
+
+    const IPC::Message* message =
+        render_thread_->sink().GetFirstMessageMatching(
+            AutofillHostMsg_ShowPasswordSuggestions::ID);
+    ASSERT_TRUE(message);
+    Tuple4<autofill::FormFieldData,
+           gfx::RectF,
+           std::vector<base::string16>,
+           std::vector<base::string16> > args;
+    AutofillHostMsg_ShowPasswordSuggestions::Read(message, &args);
+    ASSERT_EQ(4u, args.c.size());
+    std::set<base::string16>::iterator it;
+
+    for (int i = 0; i < 4; i++) {
+      it = usernames.find(args.c[i]);
+      EXPECT_TRUE(it != usernames.end());
+      if (it != usernames.end())
+        usernames.erase(it);
+    }
+
+    EXPECT_TRUE(usernames.empty());
 
     render_thread_->sink().ClearMessages();
   }
@@ -1430,7 +1455,7 @@ TEST_F(PasswordAutofillAgentTest, ClickAndSelect) {
   SimulateOnFillPasswordForm(fill_data_);
   SimulateElementClick(kUsernameName);
   SimulateSuggestionChoice(username_element_);
-  CheckSuggestions(kAliceUsername, true);
+  ExpectAllCredentials();
 
   CheckTextFieldsDOMState(kAliceUsername, true, kAlicePassword, true);
 }
@@ -1456,7 +1481,7 @@ TEST_F(PasswordAutofillAgentTest, CredentialsOnClick) {
   render_thread_->sink().ClearMessages();
   static_cast<PageClickListener*>(autofill_agent_)
       ->FormControlElementClicked(username_element_, false);
-  CheckSuggestions(std::string(), false);
+  ExpectAllCredentials();
 
   // Now simulate a user typing in an unrecognized username and then
   // clicking on the username element. This should also produce a message with
@@ -1465,7 +1490,7 @@ TEST_F(PasswordAutofillAgentTest, CredentialsOnClick) {
   render_thread_->sink().ClearMessages();
   static_cast<PageClickListener*>(autofill_agent_)
       ->FormControlElementClicked(username_element_, true);
-  CheckSuggestions("baz", true);
+  ExpectAllCredentials();
 
   // Now simulate a user typing in the first letter of the username and then
   // clicking on the username element. While the typing of the first letter will
@@ -1475,7 +1500,7 @@ TEST_F(PasswordAutofillAgentTest, CredentialsOnClick) {
   render_thread_->sink().ClearMessages();
   static_cast<PageClickListener*>(autofill_agent_)
       ->FormControlElementClicked(username_element_, true);
-  CheckSuggestions(kAliceUsername, true);
+  ExpectAllCredentials();
 }
 
 // The user types in a password, but then just before sending the form off, a

components/autofill/content/common/autofill_messages.h

@@ -285,12 +285,13 @@ IPC_MESSAGE_ROUTED2(AutofillHostMsg_AddPasswordFormMapping,
                     autofill::FormFieldData, /* the user name field */
                     autofill::PasswordFormFillData /* password pairings */)
 
-// Instruct the browser to show a popup with suggestions for the form field.
+// Instruct the browser to show a popup with the following suggestions from the
+// password manager.
 IPC_MESSAGE_ROUTED4(AutofillHostMsg_ShowPasswordSuggestions,
                     autofill::FormFieldData /* the form field */,
-                    base::string16 /* username typed by user */,
-                    bool /* show all suggestions */,
-                    gfx::RectF /* input field bounds, window-relative */)
+                    gfx::RectF /* input field bounds, window-relative */,
+                    std::vector<base::string16> /* suggestions */,
+                    std::vector<base::string16> /* realms */)
 
 // Inform browser of data list values for the curent field.
 IPC_MESSAGE_ROUTED2(AutofillHostMsg_SetDataList,

components/autofill/content/renderer/password_autofill_agent.cc

@@ -240,41 +240,45 @@ bool FillDataContainsUsername(const PasswordFormFillData& fill_data) {
   return !fill_data.basic_data.fields[0].name.empty();
 }
 
-// Sets |suggestions_present| to true if there are any suggestions to be derived
-// from |fill_data|. Unless |show_all| is true, only considers suggestions with
-// usernames having |current_username| as a prefix. Returns true if a username
-// from the |fill_data.other_possible_usernames| would be included in the
-// suggestions.
-bool GetSuggestionsStats(const PasswordFormFillData& fill_data,
-                         const base::string16& current_username,
-                         bool show_all,
-                         bool* suggestions_present) {
-  *suggestions_present = false;
-
-  for (const auto& usernames : fill_data.other_possible_usernames) {
-    for (size_t i = 0; i < usernames.second.size(); ++i) {
-      if (show_all ||
-          StartsWith(usernames.second[i], current_username, false)) {
-        *suggestions_present = true;
-        return true;
-      }
-    }
+// This function attempts to fill |suggestions| and |realms| form |fill_data|
+// based on |current_username|. Returns true when |suggestions| gets filled
+// from |fill_data.other_possible_usernames|, else returns false.
+bool GetSuggestions(const PasswordFormFillData& fill_data,
+                    const base::string16& current_username,
+                    std::vector<base::string16>* suggestions,
+                    std::vector<base::string16>* realms,
+                    bool show_all) {
+  bool other_possible_username_shown = false;
+  if (show_all ||
+      StartsWith(
+          fill_data.basic_data.fields[0].value, current_username, false)) {
+    suggestions->push_back(fill_data.basic_data.fields[0].value);
+    realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
   }
 
-  if (show_all || StartsWith(fill_data.basic_data.fields[0].value,
-                             current_username, false)) {
-    *suggestions_present = true;
-    return false;
+  for (PasswordFormFillData::LoginCollection::const_iterator iter =
+           fill_data.additional_logins.begin();
+       iter != fill_data.additional_logins.end();
+       ++iter) {
+    if (show_all || StartsWith(iter->first, current_username, false)) {
+      suggestions->push_back(iter->first);
+      realms->push_back(base::UTF8ToUTF16(iter->second.realm));
+    }
   }
 
-  for (const auto& login : fill_data.additional_logins) {
-    if (show_all || StartsWith(login.first, current_username, false)) {
-      *suggestions_present = true;
-      return false;
+  for (PasswordFormFillData::UsernamesCollection::const_iterator iter =
+           fill_data.other_possible_usernames.begin();
+       iter != fill_data.other_possible_usernames.end();
+       ++iter) {
+    for (size_t i = 0; i < iter->second.size(); ++i) {
+      if (show_all || StartsWith(iter->second[i], current_username, false)) {
+        other_possible_username_shown = true;
+        suggestions->push_back(iter->second[i]);
+        realms->push_back(base::UTF8ToUTF16(iter->first.realm));
+      }
     }
   }
-
-  return false;
+  return other_possible_username_shown;
 }
 
 // This function attempts to fill |username_element| and |password_element|
@@ -1104,6 +1108,15 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
   if (!webview)
     return false;
 
+  std::vector<base::string16> suggestions;
+  std::vector<base::string16> realms;
+  if (GetSuggestions(
+          fill_data, user_input.value(), &suggestions, &realms, show_all)) {
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SHOWN;
+  }
+
+  DCHECK_EQ(suggestions.size(), realms.size());
+
   FormData form;
   FormFieldData field;
   FindFormAndFieldForFormControlElement(
@@ -1118,14 +1131,8 @@ bool PasswordAutofillAgent::ShowSuggestionPopup(
                                  bounding_box.width() * scale,
                                  bounding_box.height() * scale);
   Send(new AutofillHostMsg_ShowPasswordSuggestions(
-      routing_id(), field, user_input.value(), show_all, bounding_box_scaled));
-
-  bool suggestions_present = false;
-  if (GetSuggestionsStats(fill_data, user_input.value(), show_all,
-                          &suggestions_present)) {
-    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SHOWN;
-  }
-  return suggestions_present;
+      routing_id(), field, bounding_box_scaled, suggestions, realms));
+  return !suggestions.empty();
 }
 
 void PasswordAutofillAgent::PerformInlineAutocomplete(

components/password_manager/core/browser/password_autofill_manager.cc

@@ -2,59 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/password_manager/core/browser/password_autofill_manager.h"
-
-#include <vector>
-
 #include "base/logging.h"
-#include "base/strings/string16.h"
-#include "base/strings/string_util.h"
-#include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autofill_driver.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/common/autofill_data_validation.h"
+#include "components/password_manager/core/browser/password_autofill_manager.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 
 namespace password_manager {
 
-namespace {
-
-// This function attempts to fill |suggestions| and |realms| form |fill_data|
-// based on |current_username|. Unless |show_all| is true, it only picks
-// suggestions where the username has |current_username| as a prefix.
-void GetSuggestions(const autofill::PasswordFormFillData& fill_data,
-                    const base::string16& current_username,
-                    std::vector<base::string16>* suggestions,
-                    std::vector<base::string16>* realms,
-                    bool show_all) {
-  if (show_all ||
-      StartsWith(
-          fill_data.basic_data.fields[0].value, current_username, false)) {
-    suggestions->push_back(fill_data.basic_data.fields[0].value);
-    realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
-  }
-
-  for (const auto& login : fill_data.additional_logins) {
-    if (show_all || StartsWith(login.first, current_username, false)) {
-      suggestions->push_back(login.first);
-      realms->push_back(base::UTF8ToUTF16(login.second.realm));
-    }
-  }
-
-  for (const auto& usernames : fill_data.other_possible_usernames) {
-    for (size_t i = 0; i < usernames.second.size(); ++i) {
-      if (show_all ||
-          StartsWith(usernames.second[i], current_username, false)) {
-        suggestions->push_back(usernames.second[i]);
-        realms->push_back(base::UTF8ToUTF16(usernames.first.realm));
-      }
-    }
-  }
-}
-
-}  // namespace
-
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillManager, public:
 
@@ -109,18 +66,9 @@ void PasswordAutofillManager::OnAddPasswordFormMapping(
 
 void PasswordAutofillManager::OnShowPasswordSuggestions(
     const autofill::FormFieldData& field,
-    const base::string16& typed_username,
-    bool show_all,
-    const gfx::RectF& bounds) {
-  std::vector<base::string16> suggestions;
-  std::vector<base::string16> realms;
-  LoginToPasswordInfoMap::const_iterator fill_data_it =
-      login_to_password_info_.find(field);
-  DCHECK(fill_data_it != login_to_password_info_.end());
-  GetSuggestions(fill_data_it->second, typed_username, &suggestions, &realms,
-                 show_all);
-  DCHECK_EQ(suggestions.size(), realms.size());
-
+    const gfx::RectF& bounds,
+    const std::vector<base::string16>& suggestions,
+    const std::vector<base::string16>& realms) {
   if (!autofill::IsValidString16Vector(suggestions) ||
       !autofill::IsValidString16Vector(realms) ||
       suggestions.size() != realms.size())

components/password_manager/core/browser/password_autofill_manager.h

@@ -44,10 +44,11 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate {
 
   // Handles a request from the renderer to show a popup with the given
   // |suggestions| from the password manager.
-  void OnShowPasswordSuggestions(const autofill::FormFieldData& field,
-                                 const base::string16& typed_username,
-                                 bool show_all,
-                                 const gfx::RectF& bounds);
+  void OnShowPasswordSuggestions(
+      const autofill::FormFieldData& field,
+      const gfx::RectF& bounds,
+      const std::vector<base::string16>& suggestions,
+      const std::vector<base::string16>& realms);
 
   // Invoked to clear any page specific cached values.
   void Reset();

components/password_manager/core/browser/password_autofill_manager_unittest.cc

@@ -2,16 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/password_manager/core/browser/password_autofill_manager.h"
-
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/popup_item_ids.h"
 #include "components/autofill/core/browser/test_autofill_client.h"
 #include "components/autofill/core/browser/test_autofill_driver.h"
-#include "components/autofill/core/common/form_field_data.h"
-#include "components/autofill/core/common/password_form_fill_data.h"
+#include "components/password_manager/core/browser/password_autofill_manager.h"
 #include "components/password_manager/core/browser/stub_password_manager_client.h"
 #include "components/password_manager/core/browser/stub_password_manager_driver.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -170,16 +167,10 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
   InitializePasswordAutofillManager(client.get(), autofill_client.get());
 
   gfx::RectF element_bounds;
-  autofill::PasswordFormFillData data;
-  data.basic_data.fields.resize(2);
-  data.basic_data.fields[0].value = test_username_;
-  data.basic_data.fields[1].value = test_password_;
-  data.preferred_realm = "http://foo.com/";
-  autofill::FormFieldData dummy_key;
-  password_autofill_manager_->OnAddPasswordFormMapping(dummy_key, data);
-
-  EXPECT_CALL(*client->mock_driver(),
-              FillSuggestion(test_username_, test_password_));
+  std::vector<base::string16> suggestions;
+  suggestions.push_back(test_username_);
+  std::vector<base::string16> realms;
+  realms.push_back(base::ASCIIToUTF16("http://foo.com/"));
 
   // The enums must be cast to ints to prevent compile errors on linux_rel.
   EXPECT_CALL(*autofill_client,
@@ -192,71 +183,12 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
                   testing::ElementsAre(autofill::POPUP_ITEM_ID_PASSWORD_ENTRY),
                   _));
   password_autofill_manager_->OnShowPasswordSuggestions(
-      dummy_key, base::string16(), false, element_bounds);
+      username_field_, element_bounds, suggestions, realms);
 
   // Accepting a suggestion should trigger a call to hide the popup.
   EXPECT_CALL(*autofill_client, HideAutofillPopup());
   password_autofill_manager_->DidAcceptSuggestion(
-      test_username_, autofill::POPUP_ITEM_ID_PASSWORD_ENTRY);
-}
-
-// Test that OnShowPasswordSuggestions correctly matches the given FormFieldData
-// to the known PasswordFormFillData, and extracts the right suggestions.
-TEST_F(PasswordAutofillManagerTest, ExtractSuggestions) {
-  scoped_ptr<TestPasswordManagerClient> client(new TestPasswordManagerClient);
-  scoped_ptr<MockAutofillClient> autofill_client(new MockAutofillClient);
-  InitializePasswordAutofillManager(client.get(), autofill_client.get());
-
-  gfx::RectF element_bounds;
-  autofill::PasswordFormFillData data;
-  data.basic_data.fields.resize(2);
-  data.basic_data.fields[0].value = test_username_;
-  data.basic_data.fields[1].value = test_password_;
-  data.preferred_realm = "http://foo.com/";
-
-  autofill::PasswordAndRealm additional;
-  additional.realm = "https://foobarrealm.org";
-  base::string16 additional_username(base::ASCIIToUTF16("John Foo"));
-  data.additional_logins[additional_username] = additional;
-
-  autofill::UsernamesCollectionKey usernames_key;
-  usernames_key.realm = "http://yetanother.net";
-  std::vector<base::string16> other_names;
-  base::string16 other_username(base::ASCIIToUTF16("John Different"));
-  other_names.push_back(other_username);
-  data.other_possible_usernames[usernames_key] = other_names;
-
-  autofill::FormFieldData dummy_key;
-  password_autofill_manager_->OnAddPasswordFormMapping(dummy_key, data);
-
-  // First, simulate displaying suggestions matching an empty prefix.
-  EXPECT_CALL(*autofill_client,
-              ShowAutofillPopup(
-                  element_bounds, _,
-                  testing::UnorderedElementsAre(
-                      test_username_, additional_username, other_username),
-                  _, _, _, _));
-  password_autofill_manager_->OnShowPasswordSuggestions(
-      dummy_key, base::string16(), false, element_bounds);
-
-  // Now simulate displaying suggestions matching "John".
-  EXPECT_CALL(*autofill_client,
-              ShowAutofillPopup(element_bounds, _,
-                                testing::UnorderedElementsAre(
-                                    additional_username, other_username),
-                                _, _, _, _));
-  password_autofill_manager_->OnShowPasswordSuggestions(
-      dummy_key, base::ASCIIToUTF16("John"), false, element_bounds);
-
-  // Finally, simulate displaying all suggestions, without any prefix matching.
-  EXPECT_CALL(*autofill_client,
-              ShowAutofillPopup(
-                  element_bounds, _,
-                  testing::UnorderedElementsAre(
-                      test_username_, additional_username, other_username),
-                  _, _, _, _));
-  password_autofill_manager_->OnShowPasswordSuggestions(
-      dummy_key, base::ASCIIToUTF16("xyz"), true, element_bounds);
+      suggestions[0], autofill::POPUP_ITEM_ID_PASSWORD_ENTRY);
 }
 
 }  // namespace password_manager