[Password Generation] Always query password forms via Autofill

components/autofill/core/browser/form_structure.cc

Index: components/autofill/core/browser/form_structure.cc
diff --git a/components/autofill/core/browser/form_structure.cc b/components/autofill/core/browser/form_structure.cc
index e8fd37a40f564ac0043910473491d1d014010a16..24289503f68d0321009a2499fc1d8d03718c2e1f 100644
--- a/components/autofill/core/browser/form_structure.cc
+++ b/components/autofill/core/browser/form_structure.cc
@@ -352,7 +352,8 @@ FormStructure::FormStructure(const FormData& form)
       autofill_count_(0),
       active_field_count_(0),
       upload_required_(USE_UPLOAD_RATES),
-      has_author_specified_types_(false) {
+      has_author_specified_types_(false),
+      has_password_field_(false) {
   // Copy the form fields.
   std::map<base::string16, size_t> unique_names;
   for (std::vector<FormFieldData>::const_iterator field =
@@ -367,6 +368,9 @@ FormStructure::FormStructure(const FormData& form)
       ++active_field_count_;
     }
 
+    if (field->form_control_type == "password")
+      has_password_field_ = true;

[Ilya Sherman, 2014/10/24 22:42:30]

Now that this is only used in one place, I wonder if it's really worth caching
as a member variable.  I don't feel strongly either way, but I lean slightly
toward computing on the fly.

[Garrett Casto, 2014/10/24 23:19:52]

I think that I slightly prefer keeping it here just so we don't have to loop
through all the fields whenever ShouldBeCrowdsourced() is called. Performance
obviously isn't a concern here, but it doesn't seem clearly cleaner to me the
other way. Your call.

+
     // Generate a unique name for this field by appending a counter to the name.
     // Make sure to prepend the counter with a non-numeric digit so that we are
     // guaranteed to avoid collisions.
@@ -579,20 +583,29 @@ void FormStructure::ParseQueryResponse(
       if (current_info == field_infos.end())
         break;
 
-      // UNKNOWN_TYPE is reserved for use by the client.
-      DCHECK_NE(current_info->field_type, UNKNOWN_TYPE);
+      // If |form->has_author_specified_types| only password fields should be
+      // updated.
+      if (form->has_author_specified_types_) {
+        // Heuristic values can be ignored for password fields as Autofill
+        // doesn't handle them. UMA stats can also be skipped in that case.

[Ilya Sherman, 2014/10/24 22:42:30]

nit: This comment talks about UMA stats, but I don't think it's relevant
anymore.

[Garrett Casto, 2014/10/24 23:19:52]

Removed.

+        if ((*field)->form_control_type == "password")
+          (*field)->set_server_type(current_info->field_type);

[Ilya Sherman, 2014/10/24 22:42:30]

I think it would be simpler to write this code as a continue stmt, i.e.

if (form->has_author_specified_types_ &&
    (*field)->form_control_type != "password")
  continue;

You can then reduce the indentation level for the remainder of the code, which
improves readability IMO.

[Garrett Casto, 2014/10/24 23:19:52]

I had originally done it this way to avoid influencing the metrics and to avoid
having to duplicate the loop iteration. We are punting on the former, but I
generally try to avoid early exits with manual iteration as it's a source of
bugs. I switched this to one level of indentation without an "else" statement.
WDYT?

[Ilya Sherman, 2014/10/24 23:40:14]

I really prefer less indentation over avoiding continue stmts.  Note that
there's already a continue and a break stmt right above this check, so it fits
with the theme of this loop.

[Garrett Casto, 2014/10/25 00:04:54]

Done.

+      } else {
+        // UNKNOWN_TYPE is reserved for use by the client.
+        DCHECK_NE(current_info->field_type, UNKNOWN_TYPE);
 
-      ServerFieldType heuristic_type = (*field)->heuristic_type();
-      if (heuristic_type != UNKNOWN_TYPE)
-        heuristics_detected_fillable_field = true;
+        ServerFieldType heuristic_type = (*field)->heuristic_type();
+        if (heuristic_type != UNKNOWN_TYPE)
+          heuristics_detected_fillable_field = true;
 
-      (*field)->set_server_type(current_info->field_type);
-      if (heuristic_type != (*field)->Type().GetStorableType())
-        query_response_overrode_heuristics = true;
+        (*field)->set_server_type(current_info->field_type);
+        if (heuristic_type != (*field)->Type().GetStorableType())
+          query_response_overrode_heuristics = true;
 
-      // Copy default value into the field if available.
-      if (!current_info->default_value.empty())
-        (*field)->set_default_value(current_info->default_value);
+        // Copy default value into the field if available.
+        if (!current_info->default_value.empty())
+          (*field)->set_default_value(current_info->default_value);
+      }
 
       ++current_info;
     }
@@ -706,7 +719,8 @@ bool FormStructure::ShouldBeParsed() const {
 }
 
 bool FormStructure::ShouldBeCrowdsourced() const {
-  return !has_author_specified_types_ && ShouldBeParsed();
+  return (has_password_field_ || !has_author_specified_types_) &&
+      ShouldBeParsed();
 }
 
 void FormStructure::UpdateFromCache(const FormStructure& cached_form) {