[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..5517951a672450c3e90f71242b32bd79967a27fb 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;
+
     // 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.
@@ -560,6 +564,7 @@ void FormStructure::ParseQueryResponse(
 
   bool heuristics_detected_fillable_field = false;
   bool query_response_overrode_heuristics = false;
+  bool non_password_field_processed = false;
 
   // Copy the field types into the actual form.
   std::vector<AutofillServerFieldInfo>::iterator current_info =
@@ -579,20 +584,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->ShouldSkipProcessingNonPasswordFields()) {
+        // Heuristic values can be ignored for password fields as Autofill
+        // doesn't handle them. UMA stats can also be skipped in that case.
+        if ((*field)->Type().group() == PASSWORD_FIELD)
+          (*field)->set_server_type(current_info->field_type);
+      } else {
+        non_password_field_processed = true;

[Ilya Sherman, 2014/10/24 01:24:45]

Wait, where do you check that a non-password field was processed?  This could
still be a password form, even if no autocomplete types are manually specified
by the site author, right?

I think this is a useful change, but I'd recommend splitting it off to a
separate CL.

[Garrett Casto, 2014/10/24 21:13:09]

I agree that the name doesn't do a great job of expressing the intent here.
Basically I was trying to keep the current behavior for metrics so that the new
requests wouldn't be logged. The percentage of such requests is likely very low
though, so it probably doesn't matter. I'll just pull this out for now and file
a bug.

 
-      ServerFieldType heuristic_type = (*field)->heuristic_type();
-      if (heuristic_type != UNKNOWN_TYPE)
-        heuristics_detected_fillable_field = true;
+        // UNKNOWN_TYPE is reserved for use by the client.
+        DCHECK_NE(current_info->field_type, UNKNOWN_TYPE);
 
-      (*field)->set_server_type(current_info->field_type);
-      if (heuristic_type != (*field)->Type().GetStorableType())
-        query_response_overrode_heuristics = true;
+        ServerFieldType heuristic_type = (*field)->heuristic_type();
+        if (heuristic_type != UNKNOWN_TYPE)
+          heuristics_detected_fillable_field = true;
 
-      // Copy default value into the field if available.
-      if (!current_info->default_value.empty())
-        (*field)->set_default_value(current_info->default_value);
+        (*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);
+      }
 
       ++current_info;
     }
@@ -601,17 +615,21 @@ void FormStructure::ParseQueryResponse(
     form->IdentifySections(false);
   }
 
-  AutofillMetrics::ServerQueryMetric metric;
-  if (query_response_overrode_heuristics) {
-    if (heuristics_detected_fillable_field) {
-      metric = AutofillMetrics::QUERY_RESPONSE_OVERRODE_LOCAL_HEURISTICS;
+  // Only log metrics if response was for non-password fields, since password
+  // fields don't have local heuristics to measure.
+  if (non_password_field_processed) {
+    AutofillMetrics::ServerQueryMetric metric;
+    if (query_response_overrode_heuristics) {
+      if (heuristics_detected_fillable_field) {
+        metric = AutofillMetrics::QUERY_RESPONSE_OVERRODE_LOCAL_HEURISTICS;
+      } else {
+        metric = AutofillMetrics::QUERY_RESPONSE_WITH_NO_LOCAL_HEURISTICS;
+      }
     } else {
-      metric = AutofillMetrics::QUERY_RESPONSE_WITH_NO_LOCAL_HEURISTICS;
+      metric = AutofillMetrics::QUERY_RESPONSE_MATCHED_LOCAL_HEURISTICS;
     }
-  } else {
-    metric = AutofillMetrics::QUERY_RESPONSE_MATCHED_LOCAL_HEURISTICS;
+    metric_logger.LogServerQueryMetric(metric);
   }
-  metric_logger.LogServerQueryMetric(metric);
 }
 
 // static
@@ -706,7 +724,12 @@ bool FormStructure::ShouldBeParsed() const {
 }
 
 bool FormStructure::ShouldBeCrowdsourced() const {
-  return !has_author_specified_types_ && ShouldBeParsed();
+  return (has_password_field_ || !has_author_specified_types_) &&
+      ShouldBeParsed();
+}
+
+bool FormStructure::ShouldSkipProcessingNonPasswordFields() const {

[Ilya Sherman, 2014/10/24 01:24:45]

This method name is pretty long and, IMO, still a little bit confusing.  Since
you only use this in one place (afaict), I'd recommend just inlining the check.

[Garrett Casto, 2014/10/24 21:13:09]

Done.

+  return has_password_field_ && has_author_specified_types_;
 }
 
 void FormStructure::UpdateFromCache(const FormStructure& cached_form) {