Propagate correct data to the Toolbar servers

chrome/browser/autofill/autofill_manager.cc

Index: chrome/browser/autofill/autofill_manager.cc
===================================================================
--- chrome/browser/autofill/autofill_manager.cc	(revision 71495)
+++ chrome/browser/autofill/autofill_manager.cc	(working copy)
@@ -220,13 +220,16 @@
   }
 
   DeterminePossibleFieldTypesForUpload(cached_upload_form_structure);
+
+  UploadFormData();
+
   // TODO(isherman): Consider uploading to server here rather than in
-  // HandleSubmit().
+  // SaveFormData().
 
   if (!upload_form_structure_->IsAutoFillable(true))
     return;
 
-  HandleSubmit();
+  SaveFormData();
 }
 
 void AutoFillManager::OnFormsSeen(const std::vector<FormData>& forms) {
@@ -486,62 +489,71 @@
 
 void AutoFillManager::DeterminePossibleFieldTypesForUpload(
     const FormStructure* cached_upload_form_structure) {
+  download_manager_.ClearPresence();
+
   for (size_t i = 0; i < upload_form_structure_->field_count(); i++) {
     const AutoFillField* field = upload_form_structure_->field(i);
     FieldTypeSet field_types;
     personal_data_->GetPossibleFieldTypes(field->value(), &field_types);
-    DCHECK(!field_types.empty());
-    upload_form_structure_->set_possible_types(i, field_types);
 
     if (field->form_control_type() == ASCIIToUTF16("select-one")) {
       // TODO(isherman): <select> fields don't support |is_autofilled()|. Since
       // this is heavily relied upon by our metrics, we just don't log anything
       // for all <select> fields. Better to have less data than misleading data.
-      continue;
-    }
+    } else {

[dhollowa, 2011/01/18 22:31:26]

As per our discussion, let's pull this logging stuff out into separate method. 
As in CL: http://codereview.chromium.org/6286003/

[GeorgeY, 2011/01/20 19:12:29]

Moved back (with a slight change) so you could commit your cl

+      // Log various quality metrics.
+      metric_logger_->Log(AutoFillMetrics::FIELD_SUBMITTED);
+      if (field_types.find(EMPTY_TYPE) == field_types.end() &&
+          field_types.find(UNKNOWN_TYPE) == field_types.end()) {
+        if (field->is_autofilled()) {
+          metric_logger_->Log(AutoFillMetrics::FIELD_AUTOFILLED);
+        } else {
+          metric_logger_->Log(AutoFillMetrics::FIELD_AUTOFILL_FAILED);
 
-    // Log various quality metrics.
-    metric_logger_->Log(AutoFillMetrics::FIELD_SUBMITTED);
-    if (field_types.find(EMPTY_TYPE) == field_types.end() &&
-        field_types.find(UNKNOWN_TYPE) == field_types.end()) {
-      if (field->is_autofilled()) {
-        metric_logger_->Log(AutoFillMetrics::FIELD_AUTOFILLED);
-      } else {
-        metric_logger_->Log(AutoFillMetrics::FIELD_AUTOFILL_FAILED);
+          AutoFillFieldType heuristic_type = cached_upload_form_structure?
+              cached_upload_form_structure->field(i)->heuristic_type() :
+              UNKNOWN_TYPE;
+          if (heuristic_type == UNKNOWN_TYPE)
+            metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_UNKNOWN);
+          else if (field_types.count(heuristic_type))
+            metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_MATCH);
+          else
+            metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_MISMATCH);
 
-        AutoFillFieldType heuristic_type = cached_upload_form_structure?
-            cached_upload_form_structure->field(i)->heuristic_type() :
-            UNKNOWN_TYPE;
-        if (heuristic_type == UNKNOWN_TYPE)
-          metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_UNKNOWN);
-        else if (field_types.count(heuristic_type))
-          metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_MATCH);
-        else
-          metric_logger_->Log(AutoFillMetrics::FIELD_HEURISTIC_TYPE_MISMATCH);
+          AutoFillFieldType server_type = cached_upload_form_structure?
+              cached_upload_form_structure->field(i)->server_type() :
+              NO_SERVER_DATA;
+          if (server_type == NO_SERVER_DATA)
+            metric_logger_->Log(AutoFillMetrics::FIELD_SERVER_TYPE_UNKNOWN);
+          else if (field_types.count(server_type))
+            metric_logger_->Log(AutoFillMetrics::FIELD_SERVER_TYPE_MATCH);
+          else
+            metric_logger_->Log(AutoFillMetrics::FIELD_SERVER_TYPE_MISMATCH);
+        }
 
-        AutoFillFieldType server_type = cached_upload_form_structure?
-            cached_upload_form_structure->field(i)->server_type() :
-            NO_SERVER_DATA;
-        if (server_type == NO_SERVER_DATA)
-          metric_logger_->Log(AutoFillMetrics::FIELD_SERVER_TYPE_UNKNOWN);
-        else if (field_types.count(server_type))
-          metric_logger_->Log(AutoFillMetrics::FIELD_SERVER_TYPE_MATCH);
-        else
-          metric_logger_->Log(AutoFillMetrics::FIELD_SERVER_TYPE_MISMATCH);
+
+        // TODO(isherman): Other things we might want to log here:
+        // * Per Vadim's email, a combination of (1) whether heuristics fired,
+        //   (2) whether the server returned something interesting, (3) whether
+        //   the user filled the field
+        // * Whether the server type matches the heursitic type
+        //   - Perhaps only if at least one of the types is not unknown/no data.
       }
+    }
+    DCHECK(!field_types.empty());
+    if (field_types.size() > 1) {
+      // If there is more than one possibility, do not poison the crowd-sourced
+      // data by returning UNKNOWN_TYPE.
+      field_types.clear();
+      field_types.insert(UNKNOWN_TYPE);
+    }
 
-
-      // TODO(isherman): Other things we might want to log here:
-      // * Per Vadim's email, a combination of (1) whether heuristics fired,
-      //   (2) whether the server returned something interesting, (3) whether
-      //   the user filled the field
-      // * Whether the server type matches the heursitic type
-      //   - Perhaps only if at least one of the types is not unknown/no data.
-    }
+    upload_form_structure_->set_possible_types(i, field_types);
+    download_manager_.SetPresenceBit(*(field_types.begin()));
   }
 }
 
-void AutoFillManager::HandleSubmit() {
+void AutoFillManager::SaveFormData() {
   // If there wasn't enough data to import then we don't want to send an upload
   // to the server.
   // TODO(jhawkins): Import form data from |form_structures_|.  That will
@@ -556,10 +568,8 @@
   CreditCard* credit_card;
   personal_data_->GetImportedFormData(&profile, &credit_card);
 
-  if (!credit_card) {
-    UploadFormData();
+  if (!credit_card)
     return;
-  }
 
   // Show an infobar to offer to save the credit card info.
   if (tab_contents_) {
@@ -578,8 +588,9 @@
     for (it = autofilled_forms_signatures_.begin();
          it != autofilled_forms_signatures_.end() && total_form_checked < 3;
          ++it, ++total_form_checked) {
-      if (*it == upload_form_structure_->FormSignature())
+      if (*it == upload_form_structure_->FormSignature()) {

[dhollowa, 2011/01/18 22:31:26]

nit: Style guide says no braces if the body and conditional are single lines.

[GeorgeY, 2011/01/20 19:12:29]

Done.

         was_autofilled = true;
+      }
     }
     // Remove outdated form signatures.
     if (total_form_checked == 3 && it != autofilled_forms_signatures_.end()) {
@@ -599,7 +610,6 @@
 void AutoFillManager::OnInfoBarClosed(bool should_save) {
   if (should_save)
     personal_data_->SaveImportedCreditCard();
-  UploadFormData();
 }
 
 AutoFillManager::AutoFillManager()