[Password Generation] Always query password forms via Autofill

components/autofill/core/browser/form_structure.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/core/browser/form_structure.h"
 
 #include <utility>
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
@@ skipping 334 matching lines @@
 
 }  // namespace
 
 FormStructure::FormStructure(const FormData& form)
     : form_name_(form.name),
       source_url_(form.origin),
       target_url_(form.action),
       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 =
            form.fields.begin();
        field != form.fields.end(); ++field) {
     if (!ShouldSkipField(*field)) {
       // Add all supported form fields (including with empty names) to the
       // signature.  This is a requirement for Autofill servers.
       form_signature_field_names_.append("&");
       form_signature_field_names_.append(StripDigitsIfRequired(field->name));
 
       ++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.
     if (!unique_names.count(field->name))
       unique_names[field->name] = 1;
     else
       ++unique_names[field->name];
     base::string16 unique_name = field->name + base::ASCIIToUTF16("_") +
         base::IntToString16(unique_names[field->name]);
     fields_.push_back(new AutofillField(*field, unique_name));
@@ skipping 173 matching lines @@
                                        &upload_required);
   buzz::XmlParser parser(&parse_handler);
   parser.Parse(response_xml.c_str(), response_xml.length(), true);
   if (!parse_handler.succeeded())
     return;
 
   metric_logger.LogServerQueryMetric(AutofillMetrics::QUERY_RESPONSE_PARSED);
 
   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 =
       field_infos.begin();
   for (std::vector<FormStructure*>::const_iterator iter = forms.begin();
        iter != forms.end(); ++iter) {
     FormStructure* form = *iter;
     form->upload_required_ = upload_required;
 
     for (std::vector<AutofillField*>::iterator field = form->fields_.begin();
          field != form->fields_.end(); ++field) {
       if (form->ShouldSkipField(**field))
         continue;
 
       // In some cases *successful* response does not return all the fields.
       // Quit the update of the types then.
       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;
     }
 
     form->UpdateAutofillCount();
     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
 void FormStructure::GetFieldTypePredictions(
     const std::vector<FormStructure*>& form_structures,
     std::vector<FormDataPredictions>* forms) {
   forms->clear();
   forms->reserve(form_structures.size());
   for (size_t i = 0; i < form_structures.size(); ++i) {
     FormStructure* form_structure = form_structures[i];
@@ skipping 74 matching lines @@
   bool has_text_field = false;
   for (std::vector<AutofillField*>::const_iterator it = begin();
        it != end() && !has_text_field; ++it) {
     has_text_field |= (*it)->form_control_type != "select-one";
   }
 
   return has_text_field;
 }
 
 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) {
   // Map from field signatures to cached fields.
   std::map<std::string, const AutofillField*> cached_fields;
   for (size_t i = 0; i < cached_form.field_count(); ++i) {
     const AutofillField* field = cached_form.field(i);
     cached_fields[field->FieldSignature()] = field;
   }
 
@@ skipping 508 matching lines @@
        field != fields_.end(); ++field) {
     FieldTypeGroup field_type_group = (*field)->Type().group();
     if (field_type_group == CREDIT_CARD)
       (*field)->set_section((*field)->section() + "-cc");
     else
       (*field)->set_section((*field)->section() + "-default");
   }
 }
 
 }  // namespace autofill