[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;

[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.
     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 192 matching lines @@
     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->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;
     }
 
     form->UpdateAutofillCount();
     form->IdentifySections(false);
   }
 
   AutofillMetrics::ServerQueryMetric metric;
   if (query_response_overrode_heuristics) {
@@ skipping 93 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();
 }
 
 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