Refactor PasswordAutofillAgent: methods to functions.

components/autofill/content/renderer/password_autofill_agent.cc

 // Copyright 2013 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 "components/autofill/content/renderer/password_autofill_agent.h"
 
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
@@ skipping 200 matching lines @@
 
 // Log a message including the name, method and action of |form|.
 void LogHTMLForm(SavePasswordProgressLogger* logger,
                  SavePasswordProgressLogger::StringID message_id,
                  const blink::WebFormElement& form) {
   logger->LogHTMLForm(message_id,
                       form.name().utf8(),
                       GURL(form.action().utf8()));
 }
 
+bool FillUserNameAndPassword(struct ParametersNeedUpdate* param,

[vabr (Chromium), 2014/09/25 10:25:49]

Please just pass a OtherPossibleUsernamesUsage* usernames_usage out param
(should be the last argument). You can change usernames_usage_ through that one
directly.

[vabr (Chromium), 2014/09/25 10:25:49]

(Just for future: if you passed a struct argument like this, you can drop
"struct".)

[vabr (Chromium), 2014/09/25 10:25:49]

nit: Please also document that the return value indicates whether the username
and password were filled.

(Slightly unrelated note: apparently, the return value of
FillUserNameAndPassword was not used (but we start to use it in this CL). If you
are looking for clean-up CLs in the future, changing return value type of
functions to void if the returned value is always ignored is a good thing to
look for.)

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

[Deepak, 2014/09/25 12:02:26]

I have made OtherPossibleUsernamesUsage enum as public so that we can use that
in the helper functions, as earlier it is private and that can not be accessed
outside class.

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+                             blink::WebInputElement* username_element,
+                             blink::WebInputElement* password_element,
+                             const PasswordFormFillData& fill_data,
+                             bool exact_username_match,
+                             bool set_selection) {
+  base::string16 current_username = username_element->value();
+  // username and password will contain the match found if any.
+  base::string16 username;
+  base::string16 password;
+
+  // Don't fill username if password can't be set.
+  if (!IsElementAutocompletable(*password_element))
+    return false;
+
+  // Look for any suitable matches to current field text.
+  if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
+                       current_username,
+                       exact_username_match)) {
+    username = fill_data.basic_data.fields[0].value;
+    password = fill_data.basic_data.fields[1].value;
+  } else {
+    // Scan additional logins for a match.
+    PasswordFormFillData::LoginCollection::const_iterator iter;
+    for (iter = fill_data.additional_logins.begin();
+         iter != fill_data.additional_logins.end();
+         ++iter) {
+      if (DoUsernamesMatch(
+              iter->first, current_username, exact_username_match)) {
+        username = iter->first;
+        password = iter->second.password;
+        break;
+      }
+    }
+
+    // Check possible usernames.
+    if (username.empty() && password.empty()) {
+      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 (DoUsernamesMatch(
+                  iter->second[i], current_username, exact_username_match)) {
+            param->usernames_usage = true;
+            username = iter->second[i];
+            password = iter->first.password;
+            break;
+          }
+        }
+        if (!username.empty() && !password.empty())
+          break;
+      }
+    }
+  }
+  if (password.empty())
+    return false;  // No match was found.
+
+  // Input matches the username, fill in required values.

[vabr (Chromium), 2014/09/25 10:25:49]

Why did you leave out the "TODO(kent)" present at this point in the original
code?

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+  if (IsElementAutocompletable(*username_element)) {
+    username_element->setValue(username, true);
+    username_element->setAutofilled(true);
+
+    if (set_selection) {
+      username_element->setSelectionRange(current_username.length(),
+                                          username.length());
+    }
+  } else if (current_username != username) {
+    // If the username can't be filled and it doesn't match a saved password
+    // as is, don't autofill a password.
+    return false;
+  }
+
+  // Wait to fill in the password until a user gesture occurs. This is to make
+  // sure that we do not fill in the DOM with a password until we believe the
+  // user is intentionally interacting with the page.
+  password_element->setSuggestedValue(password);
+  param->gate_keeper = true;

[vabr (Chromium), 2014/09/25 10:25:49]

There is no need to signal this through |gate_keeper| -- observe that
gate_keeper is true if and only if the whole method returns true.

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+
+  password_element->setAutofilled(true);
+  return true;
+}
+
+void GetSuggestions(struct ParametersNeedUpdate* param,

[vabr (Chromium), 2014/09/25 10:25:49]

Please keep the order of the 3 functions as it was -- it made sense to group the
Fill* methods together. Also, keeping the same order makes it easier to diff
your change against the ToT when reviewing it.

[Deepak, 2014/09/25 12:02:26]

We are calling FillUserNameAndPassword() from FillFormOnPasswordRecieved(), so
FillUserNameAndPassword() should be before FillFormOnPasswordRecieved().

+                    const PasswordFormFillData& fill_data,
+                    const base::string16& input,
+                    std::vector<base::string16>* suggestions,
+                    std::vector<base::string16>* realms,
+                    bool show_all) {
+  if (show_all ||
+      StartsWith(fill_data.basic_data.fields[0].value, input, false)) {
+    suggestions->push_back(fill_data.basic_data.fields[0].value);
+    realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
+  }
+
+  for (PasswordFormFillData::LoginCollection::const_iterator iter =
+           fill_data.additional_logins.begin();
+       iter != fill_data.additional_logins.end();
+       ++iter) {
+    if (show_all || StartsWith(iter->first, input, false)) {
+      suggestions->push_back(iter->first);
+      realms->push_back(base::UTF8ToUTF16(iter->second.realm));
+    }
+  }
+
+  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], input, false)) {
+        param->usernames_usage = true;
+        suggestions->push_back(iter->second[i]);
+        realms->push_back(base::UTF8ToUTF16(iter->first.realm));
+      }
+    }
+  }
+}
+
+// Attempts to fill |username_element| and |password_element| with the
+// |fill_data|.  Will use the data corresponding to the preferred username,
+// unless the |username_element| already has a value set.  In that case,
+// attempts to fill the password matching the already filled username, if
+// such a password exists.
+void FillFormOnPasswordRecieved(struct ParametersNeedUpdate* param,
+                                const PasswordFormFillData& fill_data,
+                                blink::WebInputElement username_element,
+                                blink::WebInputElement password_element) {
+  // Do not fill if the password field is in an iframe.
+  DCHECK(password_element.document().frame());
+  if (password_element.document().frame()->parent())
+    return;
+
+  if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
+      !username_element.form().autoComplete())
+    return;
+
+  // If we can't modify the password, don't try to set the username
+  if (!IsElementAutocompletable(password_element))
+    return;
+
+  // Try to set the username to the preferred name, but only if the field
+  // can be set and isn't prefilled.
+  if (IsElementAutocompletable(username_element) &&
+      username_element.value().isEmpty()) {
+    // TODO(tkent): Check maxlength and pattern.
+    username_element.setValue(fill_data.basic_data.fields[0].value, true);
+  }
+
+  // Fill if we have an exact match for the username. Note that this sets
+  // username to autofilled.
+  FillUserNameAndPassword(param,

[vabr (Chromium), 2014/09/25 10:25:49]

Forward the return value here, to signal out if the gatekeeper should register
the password element.

[Deepak, 2014/09/25 12:02:26]

Acknowledged.

+                          &username_element,
+                          &password_element,
+                          fill_data,
+                          true /* exact_username_match */,
+                          false /* set_selection */);
+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillAgent, public:
 
 PasswordAutofillAgent::PasswordAutofillAgent(content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
       usernames_usage_(NOTHING_TO_AUTOFILL),
       web_view_(render_view->GetWebView()),
       logging_state_active_(false),
@@ skipping 65 matching lines @@
     return false;
 
   blink::WebInputElement password = password_info.password_field;
   if (!IsElementEditable(password))
     return false;
 
   blink::WebInputElement username = element;  // We need a non-const.
 
   // Do not set selection when ending an editing session, otherwise it can
   // mess with focus.
-  FillUserNameAndPassword(&username,
+  struct ParametersNeedUpdate param;
+  FillUserNameAndPassword(&param,
+                          &username,
                           &password,
                           fill_data,
                           true /* exact_username_match */,
                           false /* set_selection */);
+  if (param.usernames_usage)
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+  if (param.gate_keeper)
+    gatekeeper_.RegisterElement(&password);
+
   return true;
 }
 
 bool PasswordAutofillAgent::TextDidChangeInTextField(
     const blink::WebInputElement& element) {
   // TODO(vabr): Get a mutable argument instead. http://crbug.com/397083
   blink::WebInputElement mutable_element = element;  // We need a non-const.
 
   if (element.isPasswordField()) {
     // Some login forms have event handlers that put a hash of the password into
@@ skipping 485 matching lines @@
     blink::WebInputElement username_element =
         form_elements->input_elements[form_data.basic_data.fields[0].name];
 
     // Get pointer to password element. (We currently only support single
     // password forms).
     blink::WebInputElement password_element =
         form_elements->input_elements[form_data.basic_data.fields[1].name];
 
     // If wait_for_username is true, we don't want to initially fill the form
     // until the user types in a valid username.
-    if (!form_data.wait_for_username)
-      FillFormOnPasswordRecieved(form_data, username_element, password_element);
+    if (!form_data.wait_for_username) {
+      struct ParametersNeedUpdate param;
+      FillFormOnPasswordRecieved(
+          &param, form_data, username_element, password_element);
+      if (param.usernames_usage)
+        usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+      if (param.gate_keeper)
+        gatekeeper_.RegisterElement(&password_element);
+    }
 
     // We might have already filled this form if there are two <form> elements
     // with identical markup.
     if (login_to_password_info_.find(username_element) !=
         login_to_password_info_.end())
       continue;
 
     PasswordInfo password_info;
     password_info.fill_data = form_data;
     password_info.password_field = password_element;
@@ skipping 13 matching lines @@
   logging_state_active_ = active;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillAgent, private:
 
 PasswordAutofillAgent::PasswordInfo::PasswordInfo()
     : backspace_pressed_last(false), password_was_edited_last(false) {
 }
 
-void PasswordAutofillAgent::GetSuggestions(
-    const PasswordFormFillData& fill_data,
-    const base::string16& input,
-    std::vector<base::string16>* suggestions,
-    std::vector<base::string16>* realms,
-    bool show_all) {
-  if (show_all ||
-      StartsWith(fill_data.basic_data.fields[0].value, input, false)) {
-    suggestions->push_back(fill_data.basic_data.fields[0].value);
-    realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
-  }
-
-  for (PasswordFormFillData::LoginCollection::const_iterator iter =
-           fill_data.additional_logins.begin();
-       iter != fill_data.additional_logins.end();
-       ++iter) {
-    if (show_all || StartsWith(iter->first, input, false)) {
-      suggestions->push_back(iter->first);
-      realms->push_back(base::UTF8ToUTF16(iter->second.realm));
-    }
-  }
-
-  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], input, false)) {
-        usernames_usage_ = OTHER_POSSIBLE_USERNAME_SHOWN;
-        suggestions->push_back(iter->second[i]);
-        realms->push_back(base::UTF8ToUTF16(iter->first.realm));
-      }
-    }
-  }
-}
-
 bool PasswordAutofillAgent::ShowSuggestionPopup(
     const PasswordFormFillData& fill_data,
     const blink::WebInputElement& user_input,
     bool show_all) {
   blink::WebFrame* frame = user_input.document().frame();
   if (!frame)
     return false;
 
   blink::WebView* webview = frame->view();
   if (!webview)
     return false;
 
   std::vector<base::string16> suggestions;
   std::vector<base::string16> realms;
+  struct ParametersNeedUpdate param;
   GetSuggestions(
-      fill_data, user_input.value(), &suggestions, &realms, show_all);
+      &param, fill_data, user_input.value(), &suggestions, &realms, show_all);
+  if (param.usernames_usage)
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SHOWN;
   DCHECK_EQ(suggestions.size(), realms.size());
 
   FormData form;
   FormFieldData field;
   FindFormAndFieldForFormControlElement(
       user_input, &form, &field, REQUIRE_NONE);
 
   blink::WebInputElement selected_element = user_input;
   gfx::Rect bounding_box(selected_element.boundsInViewportSpace());
 
   float scale = web_view_->pageScaleFactor();
   gfx::RectF bounding_box_scaled(bounding_box.x() * scale,
                                  bounding_box.y() * scale,
                                  bounding_box.width() * scale,
                                  bounding_box.height() * scale);
   Send(new AutofillHostMsg_ShowPasswordSuggestions(
       routing_id(), field, bounding_box_scaled, suggestions, realms));
   return !suggestions.empty();
 }
 
-void PasswordAutofillAgent::FillFormOnPasswordRecieved(
-    const PasswordFormFillData& fill_data,
-    blink::WebInputElement username_element,
-    blink::WebInputElement password_element) {
-  // Do not fill if the password field is in an iframe.
-  DCHECK(password_element.document().frame());
-  if (password_element.document().frame()->parent())
-    return;
-
-  if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
-      !username_element.form().autoComplete())
-    return;
-
-  // If we can't modify the password, don't try to set the username
-  if (!IsElementAutocompletable(password_element))
-    return;
-
-  // Try to set the username to the preferred name, but only if the field
-  // can be set and isn't prefilled.
-  if (IsElementAutocompletable(username_element) &&
-      username_element.value().isEmpty()) {
-    // TODO(tkent): Check maxlength and pattern.
-    username_element.setValue(fill_data.basic_data.fields[0].value, true);
-  }
-
-  // Fill if we have an exact match for the username. Note that this sets
-  // username to autofilled.
-  FillUserNameAndPassword(&username_element,
-                          &password_element,
-                          fill_data,
-                          true /* exact_username_match */,
-                          false /* set_selection */);
-}
-
-bool PasswordAutofillAgent::FillUserNameAndPassword(
-    blink::WebInputElement* username_element,
-    blink::WebInputElement* password_element,
-    const PasswordFormFillData& fill_data,
-    bool exact_username_match,
-    bool set_selection) {
-  base::string16 current_username = username_element->value();
-  // username and password will contain the match found if any.
-  base::string16 username;
-  base::string16 password;
-
-  // Don't fill username if password can't be set.
-  if (!IsElementAutocompletable(*password_element))
-    return false;
-
-  // Look for any suitable matches to current field text.
-  if (DoUsernamesMatch(fill_data.basic_data.fields[0].value,
-                       current_username,
-                       exact_username_match)) {
-    username = fill_data.basic_data.fields[0].value;
-    password = fill_data.basic_data.fields[1].value;
-  } else {
-    // Scan additional logins for a match.
-    PasswordFormFillData::LoginCollection::const_iterator iter;
-    for (iter = fill_data.additional_logins.begin();
-         iter != fill_data.additional_logins.end();
-         ++iter) {
-      if (DoUsernamesMatch(
-              iter->first, current_username, exact_username_match)) {
-        username = iter->first;
-        password = iter->second.password;
-        break;
-      }
-    }
-
-    // Check possible usernames.
-    if (username.empty() && password.empty()) {
-      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 (DoUsernamesMatch(
-                  iter->second[i], current_username, exact_username_match)) {
-            usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
-            username = iter->second[i];
-            password = iter->first.password;
-            break;
-          }
-        }
-        if (!username.empty() && !password.empty())
-          break;
-      }
-    }
-  }
-  if (password.empty())
-    return false;  // No match was found.
-
-  // TODO(tkent): Check maxlength and pattern for both username and password
-  // fields.
-
-  // Input matches the username, fill in required values.
-  if (IsElementAutocompletable(*username_element)) {
-    username_element->setValue(username, true);
-    username_element->setAutofilled(true);
-
-    if (set_selection) {
-      username_element->setSelectionRange(current_username.length(),
-                                          username.length());
-    }
-  } else if (current_username != username) {
-    // If the username can't be filled and it doesn't match a saved password
-    // as is, don't autofill a password.
-    return false;
-  }
-
-  // Wait to fill in the password until a user gesture occurs. This is to make
-  // sure that we do not fill in the DOM with a password until we believe the
-  // user is intentionally interacting with the page.
-  password_element->setSuggestedValue(password);
-  gatekeeper_.RegisterElement(password_element);
-
-  password_element->setAutofilled(true);
-  return true;
-}
-
 void PasswordAutofillAgent::PerformInlineAutocomplete(
     const blink::WebInputElement& username_input,
     const blink::WebInputElement& password_input,
     const PasswordFormFillData& fill_data) {
   DCHECK(!fill_data.wait_for_username);
 
   // We need non-const versions of the username and password inputs.
   blink::WebInputElement username = username_input;
   blink::WebInputElement password = password_input;
 
   // Don't inline autocomplete if the caret is not at the end.
   // TODO(jcivelli): is there a better way to test the caret location?
   if (username.selectionStart() != username.selectionEnd() ||
       username.selectionEnd() != static_cast<int>(username.value().length())) {
     return;
   }
 
   // Show the popup with the list of available usernames.
   ShowSuggestionPopup(fill_data, username, false);
 
 #if !defined(OS_ANDROID)
   // Fill the user and password field with the most relevant match. Android
   // only fills in the fields after the user clicks on the suggestion popup.
-  FillUserNameAndPassword(&username,
+  struct ParametersNeedUpdate param;

[tfarina, 2014/09/25 20:00:23]

no 'struct' here as well. This is a C[ism].

+  FillUserNameAndPassword(&param,
+                          &username,
                           &password,
                           fill_data,
                           false /* exact_username_match */,
                           true /* set_selection */);
+  if (param.usernames_usage)
+    usernames_usage_ = OTHER_POSSIBLE_USERNAME_SELECTED;
+  if (param.gate_keeper)
+    gatekeeper_.RegisterElement(&password);
+
 #endif
 }
 
 void PasswordAutofillAgent::FrameClosing(const blink::WebFrame* frame) {
   for (LoginToPasswordInfoMap::iterator iter = login_to_password_info_.begin();
        iter != login_to_password_info_.end();) {
     if (iter->first.document().frame() == frame) {
       password_to_username_.erase(iter->second.password_field);
       login_to_password_info_.erase(iter++);
     } else {
@@ skipping 53 matching lines @@
   scoped_ptr<PasswordForm> password_form(CreatePasswordForm(form));
   if (!password_form || (restriction == RESTRICTION_NON_EMPTY_PASSWORD &&
                          password_form->password_value.empty() &&
                          password_form->new_password_value.empty())) {
     return;
   }
   provisionally_saved_forms_[frame].reset(password_form.release());
 }
 
 }  // namespace autofill