Add some basic success/failure UMA logging for autofill.

BUG=none
TEST=unit_tests --gtest_filter=AutoFillMetricsTest.*

Review URL: http://codereview.chromium.org/5703002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69232 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 409 insertions, 82 deletions

diff --git a/chrome/browser/autofill/autofill_download.cc b/chrome/browser/autofill/autofill_download.cc
index 9b2e747..94d9129 100644
--- a/chrome/browser/autofill/autofill_download.cc
+++ b/chrome/browser/autofill/autofill_download.cc
@@ -67,7 +67,8 @@ void AutoFillDownloadManager::SetObserver(
 }
 
 bool AutoFillDownloadManager::StartQueryRequest(
-    const ScopedVector<FormStructure>& forms) {
+    const ScopedVector<FormStructure>& forms,
+    const AutoFillMetrics& metric_logger) {
   if (next_query_request_ > base::Time::Now()) {
     // We are in back-off mode: do not do the request.
     return false;
@@ -79,7 +80,7 @@ bool AutoFillDownloadManager::StartQueryRequest(
     return false;
 
   request_data.request_type = AutoFillDownloadManager::REQUEST_QUERY;
-  autofill_metrics::LogServerQueryMetric(autofill_metrics::QUERY_SENT);
+  metric_logger.Log(AutoFillMetrics::QUERY_SENT);
 
   return StartRequest(form_xml, request_data);
 }
diff --git a/chrome/browser/autofill/autofill_download.h b/chrome/browser/autofill/autofill_download.h
index 5987c9d..488c3b3 100644
--- a/chrome/browser/autofill/autofill_download.h
+++ b/chrome/browser/autofill/autofill_download.h
@@ -16,6 +16,7 @@
 #include "chrome/browser/autofill/form_structure.h"
 #include "chrome/common/net/url_fetcher.h"
 
+class AutoFillMetrics;
 class Profile;
 
 // Handles getting and updating AutoFill heuristics.
@@ -60,7 +61,8 @@ class AutoFillDownloadManager : public URLFetcher::Delegate {
   // Starts a query request to AutoFill servers. The observer is called with the
   // list of the fields of all requested forms.
   // |forms| - array of forms aggregated in this request.
-  bool StartQueryRequest(const ScopedVector<FormStructure>& forms);
+  bool StartQueryRequest(const ScopedVector<FormStructure>& forms,
+                         const AutoFillMetrics& metric_logger);
 
   // Start upload request if necessary. The probability of request going
   // over the wire are GetPositiveUploadRate() if it was matched by
diff --git a/chrome/browser/autofill/autofill_download_unittest.cc b/chrome/browser/autofill/autofill_download_unittest.cc
index 68173c6..a2f9d78 100644
--- a/chrome/browser/autofill/autofill_download_unittest.cc
+++ b/chrome/browser/autofill/autofill_download_unittest.cc
@@ -8,9 +8,11 @@
 #include "base/test/test_timeouts.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/autofill_download.h"
+#include "chrome/browser/autofill/autofill_metrics.h"
 #include "chrome/common/net/test_url_fetcher_factory.h"
 #include "chrome/test/testing_profile.h"
 #include "net/url_request/url_request_status.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebInputElement.h"
 #include "webkit/glue/form_data.h"
@@ -18,6 +20,19 @@
 using webkit_glue::FormData;
 using WebKit::WebInputElement;
 
+namespace {
+
+class MockAutoFillMetrics : public AutoFillMetrics {
+ public:
+  MockAutoFillMetrics() {}
+  MOCK_CONST_METHOD1(Log, void(ServerQueryMetric metric));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockAutoFillMetrics);
+};
+
+}  // namespace
+
 // This tests AutoFillDownloadManager. AutoFillDownloadTestHelper implements
 // AutoFillDownloadManager::Observer and creates an instance of
 // AutoFillDownloadManager. Then it records responses to different initiated
@@ -86,8 +101,6 @@ class AutoFillDownloadTestHelper : public AutoFillDownloadManager::Observer {
   AutoFillDownloadManager download_manager;
 };
 
-namespace {
-
 TEST(AutoFillDownloadTest, QueryAndUploadTest) {
   MessageLoopForUI message_loop;
   // Create and register factory.
@@ -173,7 +186,10 @@ TEST(AutoFillDownloadTest, QueryAndUploadTest) {
   form_structures.push_back(form_structure);
 
   // Request with id 0.
-  EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures));
+  MockAutoFillMetrics mock_metric_logger;
+  EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+  EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures,
+                                                        mock_metric_logger));
   // Set upload to 100% so requests happen.
   helper.download_manager.SetPositiveUploadRate(1.0);
   helper.download_manager.SetNegativeUploadRate(1.0);
@@ -258,7 +274,9 @@ TEST(AutoFillDownloadTest, QueryAndUploadTest) {
   EXPECT_EQ(NULL, fetcher);
 
   // Request with id 3.
-  EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures));
+  EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(1);
+  EXPECT_TRUE(helper.download_manager.StartQueryRequest(form_structures,
+                                                        mock_metric_logger));
   fetcher = factory.GetFetcherByID(3);
   ASSERT_TRUE(fetcher);
   fetcher->set_backoff_delay(
@@ -274,7 +292,9 @@ TEST(AutoFillDownloadTest, QueryAndUploadTest) {
   helper.responses_.pop_front();
 
   // Query requests should be ignored for the next 10 seconds.
-  EXPECT_FALSE(helper.download_manager.StartQueryRequest(form_structures));
+  EXPECT_CALL(mock_metric_logger, Log(AutoFillMetrics::QUERY_SENT)).Times(0);
+  EXPECT_FALSE(helper.download_manager.StartQueryRequest(form_structures,
+                                                         mock_metric_logger));
   fetcher = factory.GetFetcherByID(4);
   EXPECT_EQ(NULL, fetcher);
 
@@ -304,5 +324,3 @@ TEST(AutoFillDownloadTest, QueryAndUploadTest) {
   // Make sure consumer of URLFetcher does the right thing.
   URLFetcher::set_factory(NULL);
 }
-
-}  // namespace
diff --git a/chrome/browser/autofill/autofill_manager.cc b/chrome/browser/autofill/autofill_manager.cc
index e74f6b3..bb19e5f 100644
--- a/chrome/browser/autofill/autofill_manager.cc
+++ b/chrome/browser/autofill/autofill_manager.cc
@@ -13,6 +13,7 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/autofill/autofill_cc_infobar_delegate.h"
 #include "chrome/browser/autofill/autofill_dialog.h"
+#include "chrome/browser/autofill/autofill_metrics.h"
 #include "chrome/browser/autofill/form_structure.h"
 #include "chrome/browser/autofill/phone_number.h"
 #include "chrome/browser/autofill/select_control_handler.h"
@@ -128,6 +129,7 @@ AutoFillManager::AutoFillManager(TabContents* tab_contents)
       personal_data_(NULL),
       download_manager_(tab_contents_->profile()),
       disable_download_manager_requests_(false),
+      metric_logger_(new AutoFillMetrics),
       cc_infobar_(NULL) {
   DCHECK(tab_contents);
 
@@ -174,12 +176,17 @@ void AutoFillManager::FormSubmitted(const FormData& form) {
   // Grab a copy of the form data.
   upload_form_structure_.reset(new FormStructure(form));
 
+  // Disregard forms that we wouldn't ever autofill in the first place.
+  if (!upload_form_structure_->ShouldBeParsed(true))
+    return;
+
+  DeterminePossibleFieldTypesForUpload();
+  // TODO(isherman): Consider uploading to server here rather than in
+  // HandleSubmit().
+
   if (!upload_form_structure_->IsAutoFillable(true))
     return;
 
-  // Determine the possible field types and upload the form structure to the
-  // PersonalDataManager.
-  DeterminePossibleFieldTypes(upload_form_structure_.get());
   HandleSubmit();
 }
 
@@ -398,7 +405,8 @@ void AutoFillManager::OnLoadedAutoFillHeuristics(
   UploadRequired upload_required;
   FormStructure::ParseQueryResponse(heuristic_xml,
                                     form_structures_.get(),
-                                    &upload_required);
+                                    &upload_required,
+                                    *metric_logger_);
 }
 
 void AutoFillManager::OnUploadedAutoFillHeuristics(
@@ -425,13 +433,38 @@ bool AutoFillManager::IsAutoFillEnabled() const {
   return prefs->GetBoolean(prefs::kAutoFillEnabled);
 }
 
-void AutoFillManager::DeterminePossibleFieldTypes(
-    FormStructure* form_structure) {
-  for (size_t i = 0; i < form_structure->field_count(); i++) {
-    const AutoFillField* field = form_structure->field(i);
+void AutoFillManager::DeterminePossibleFieldTypesForUpload() {
+  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);
-    form_structure->set_possible_types(i, 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;
+    }
+
+    // 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);
+
+      // TODO(isherman): Other things we might want to log here:
+      // * Did the field's heuristic type match the PDM type?
+      // * 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.
+    }
   }
 }
 
@@ -495,7 +528,8 @@ AutoFillManager::AutoFillManager()
     : tab_contents_(NULL),
       personal_data_(NULL),
       download_manager_(NULL),
-      disable_download_manager_requests_(false),
+      disable_download_manager_requests_(true),
+      metric_logger_(new AutoFillMetrics),
       cc_infobar_(NULL) {
 }
 
@@ -504,11 +538,17 @@ AutoFillManager::AutoFillManager(TabContents* tab_contents,
     : tab_contents_(tab_contents),
       personal_data_(personal_data),
       download_manager_(NULL),
-      disable_download_manager_requests_(false),
+      disable_download_manager_requests_(true),
+      metric_logger_(new AutoFillMetrics),
       cc_infobar_(NULL) {
   DCHECK(tab_contents);
 }
 
+void AutoFillManager::set_metric_logger(
+    const AutoFillMetrics* metric_logger) {
+  metric_logger_.reset(metric_logger);
+}
+
 bool AutoFillManager::GetHost(const std::vector<AutoFillProfile*>& profiles,
                               const std::vector<CreditCard*>& credit_cards,
                               RenderViewHost** host) {
@@ -709,8 +749,10 @@ void AutoFillManager::ParseForms(const std::vector<FormData>& forms) {
   }
 
   // If none of the forms were parsed, no use querying the server.
-  if (!form_structures_.empty() && !disable_download_manager_requests_)
-    download_manager_.StartQueryRequest(form_structures_);
+  if (!form_structures_.empty() && !disable_download_manager_requests_) {
+    download_manager_.StartQueryRequest(form_structures_,
+                                        *metric_logger_);
+  }
 
   for (std::vector<FormStructure *>::const_iterator iter =
            non_queryable_forms.begin();
diff --git a/chrome/browser/autofill/autofill_manager.h b/chrome/browser/autofill/autofill_manager.h
index 29e6b23..3a68336 100644
--- a/chrome/browser/autofill/autofill_manager.h
+++ b/chrome/browser/autofill/autofill_manager.h
@@ -20,6 +20,7 @@
 
 class AutoFillCCInfoBarDelegate;
 class AutoFillProfile;
+class AutoFillMetrics;
 class CreditCard;
 class FormStructure;
 class PrefService;
@@ -74,10 +75,6 @@ class AutoFillManager : public RenderViewHostDelegate::AutoFill,
   // Returns the value of the AutoFillEnabled pref.
   virtual bool IsAutoFillEnabled() const;
 
-  // Uses heuristics and existing personal data to determine the possible field
-  // types.
-  void DeterminePossibleFieldTypes(FormStructure* form_structure);
-
   // Handles the form data submitted by the user.
   void HandleSubmit();
 
@@ -94,6 +91,11 @@ class AutoFillManager : public RenderViewHostDelegate::AutoFill,
     personal_data_ = personal_data;
   }
 
+  const AutoFillMetrics* metric_logger() const {
+    return metric_logger_.get();
+  }
+  void set_metric_logger(const AutoFillMetrics* metric_logger);
+
   // Maps GUIDs to and from IDs that are used to identify profiles and credit
   // cards sent to and from the renderer process.
   virtual int GUIDToID(const std::string& guid);
@@ -160,10 +162,9 @@ class AutoFillManager : public RenderViewHostDelegate::AutoFill,
   // Parses the forms using heuristic matching and querying the AutoFill server.
   void ParseForms(const std::vector<webkit_glue::FormData>& forms);
 
-  // The following function is meant to be called from unit-test only.
-  void set_disable_download_manager_requests(bool value) {
-    disable_download_manager_requests_ = value;
-  }
+  // Uses existing personal data to determine possible field types for the
+  // |upload_form_structure_|.
+  void DeterminePossibleFieldTypesForUpload();
 
   // The TabContents hosting this AutoFillManager.
   // Weak reference.
@@ -182,9 +183,13 @@ class AutoFillManager : public RenderViewHostDelegate::AutoFill,
 
   // Should be set to true in AutoFillManagerTest and other tests, false in
   // AutoFillDownloadManagerTest and in non-test environment. Is false by
-  // default.
+  // default for the public constructor, and true by default for the test-only
+  // constructors.
   bool disable_download_manager_requests_;
 
+  // For logging UMA metrics. Overridden by metrics tests.
+  scoped_ptr<const AutoFillMetrics> metric_logger_;
+
   // Our copy of the form data.
   ScopedVector<FormStructure> form_structures_;
 
@@ -200,7 +205,6 @@ class AutoFillManager : public RenderViewHostDelegate::AutoFill,
   std::map<int, std::string> id_guid_map_;
 
   friend class FormStructureBrowserTest;
-  friend class TestAutoFillManager;
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillCreditCardForm);
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillAddressForm);
   FRIEND_TEST_ALL_PREFIXES(AutoFillManagerTest, FillAddressAndCreditCardForm);
diff --git a/chrome/browser/autofill/autofill_manager_unittest.cc b/chrome/browser/autofill/autofill_manager_unittest.cc
index ffaa536..bf6877f 100644
--- a/chrome/browser/autofill/autofill_manager_unittest.cc
+++ b/chrome/browser/autofill/autofill_manager_unittest.cc
@@ -183,7 +183,7 @@ void CreateTestAddressFormData(FormData* form) {
   form->fields.push_back(field);
 }
 
-// Populates |form| with data corresponding to a simple credit card form, with.
+// Populates |form| with data corresponding to a simple credit card form.
 // Note that this actually appends fields to the form data, which can be useful
 // for building up more complex test forms.
 void CreateTestCreditCardFormData(FormData* form, bool is_https) {
@@ -364,19 +364,13 @@ void ExpectFilledCreditCardFormElvis(int page_id,
                    has_address_fields, true);
 }
 
-}  // namespace
-
 class TestAutoFillManager : public AutoFillManager {
  public:
   TestAutoFillManager(TabContents* tab_contents,
                       TestPersonalDataManager* personal_manager)
-      : AutoFillManager(tab_contents, NULL),
+      : AutoFillManager(tab_contents, personal_manager),
         autofill_enabled_(true) {
     test_personal_data_ = personal_manager;
-    set_personal_data_manager(personal_manager);
-    // Download manager requests are disabled for purposes of this unit test.
-    // These requests are tested in autofill_download_unittest.cc.
-    set_disable_download_manager_requests(true);
   }
 
   virtual bool IsAutoFillEnabled() const { return autofill_enabled_; }
@@ -425,6 +419,8 @@ class TestAutoFillManager : public AutoFillManager {
   DISALLOW_COPY_AND_ASSIGN(TestAutoFillManager);
 };
 
+}  // namespace
+
 class AutoFillManagerTest : public RenderViewHostTestHarness {
  public:
   AutoFillManagerTest() {}
diff --git a/chrome/browser/autofill/autofill_metrics.cc b/chrome/browser/autofill/autofill_metrics.cc
index 85b5517..bae11d7 100644
--- a/chrome/browser/autofill/autofill_metrics.cc
+++ b/chrome/browser/autofill/autofill_metrics.cc
@@ -6,13 +6,22 @@
 
 #include "base/metrics/histogram.h"
 
-namespace autofill_metrics {
+AutoFillMetrics::AutoFillMetrics() {
+}
+
+AutoFillMetrics::~AutoFillMetrics() {
+}
 
-void LogServerQueryMetric(ServerQueryMetricType type) {
-  DCHECK(type < NUM_SERVER_QUERY_METRICS);
+void AutoFillMetrics::Log(ServerQueryMetric metric) const {
+  DCHECK(metric < NUM_SERVER_QUERY_METRICS);
 
-  UMA_HISTOGRAM_ENUMERATION("AutoFill.ServerQueryResponse", type,
+  UMA_HISTOGRAM_ENUMERATION("AutoFill.ServerQueryResponse", metric,
                             NUM_SERVER_QUERY_METRICS);
 }
 
-}  // namespace autofill_metrics
+void AutoFillMetrics::Log(QualityMetric metric) const {
+  DCHECK(metric < NUM_QUALITY_METRICS);
+
+  UMA_HISTOGRAM_ENUMERATION("AutoFill.Quality", metric,
+                            NUM_QUALITY_METRICS);
+}
diff --git a/chrome/browser/autofill/autofill_metrics.h b/chrome/browser/autofill/autofill_metrics.h
index d461668..6318274f 100644
--- a/chrome/browser/autofill/autofill_metrics.h
+++ b/chrome/browser/autofill/autofill_metrics.h
@@ -6,32 +6,56 @@
 #define CHROME_BROWSER_AUTOFILL_AUTOFILL_METRICS_H_
 #pragma once
 
-namespace autofill_metrics {
-
-// Each of these should be logged at most once per query to the server, which in
-// turn should occur at most once per page load.
-enum ServerQueryMetricType {
-  // Logged for each query sent to the server
-  QUERY_SENT = 0,
-  // Logged for each query response received from the server
-  QUERY_RESPONSE_RECEIVED,
-  // Logged for each parsable response received from the server
-  QUERY_RESPONSE_PARSED,
-  // Logged for each parsable response that provided no improvements relative to
-  // our heuristics.
-  QUERY_RESPONSE_MATCHED_LOCAL_HEURISTICS,
-  // Logged for each page for which our heuristics detected at least one
-  // auto-fillable field, but the server response overrode the type of at least
-  // one field
-  QUERY_RESPONSE_OVERRODE_LOCAL_HEURISTICS,
-  // Logged for each page for which our heuristics did not detect any
-  // auto-fillable fields, but the server response did detect some.
-  QUERY_RESPONSE_WITH_NO_LOCAL_HEURISTICS,
-  NUM_SERVER_QUERY_METRICS
-};
+#include "base/basictypes.h"
+
+class AutoFillMetrics {
+ public:
+  // Each of these is logged at most once per query to the server, which in turn
+  // occurs at most once per page load.
+  enum ServerQueryMetric {
+    // Logged for each query sent to the server.
+    QUERY_SENT = 0,
+    // Logged for each query response received from the server.
+    QUERY_RESPONSE_RECEIVED,
+    // Logged for each parsable response received from the server.
+    QUERY_RESPONSE_PARSED,
+    // Logged for each parsable response that provided no improvements relative
+    // to our heuristics.
+    QUERY_RESPONSE_MATCHED_LOCAL_HEURISTICS,
+    // Logged for each page for which our heuristics detected at least one
+    // auto-fillable field, but the server response overrode the type of at
+    // least one field.
+    QUERY_RESPONSE_OVERRODE_LOCAL_HEURISTICS,
+    // Logged for each page for which our heuristics did not detect any
+    // auto-fillable fields, but the server response did detect some.
+    QUERY_RESPONSE_WITH_NO_LOCAL_HEURISTICS,
+    NUM_SERVER_QUERY_METRICS
+  };
+
+  // Each of these is logged at most once per form submission.
+  enum QualityMetric {
+    // Logged for each field in a submitted form.
+    FIELD_SUBMITTED = 0,
+    // A simple successs metric, logged for each field that returns true for
+    // |is_autofilled()| and has a value that is present in the personal data
+    // manager. There is a small chance of false positives from filling via
+    // autocomplete rather than autofill.
+    FIELD_AUTOFILLED,
+    // A simple failure metric, logged for each field that returns false for
+    // |is_autofilled()| but as a value that is present in the personal data
+    // manager.
+    FIELD_AUTOFILL_FAILED,
+    NUM_QUALITY_METRICS
+  };
 
-void LogServerQueryMetric(ServerQueryMetricType type);
+  AutoFillMetrics();
+  virtual ~AutoFillMetrics();
 
-}  // namespace autofill_metrics
+  virtual void Log(ServerQueryMetric metric) const;
+  virtual void Log(QualityMetric metric) const;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AutoFillMetrics);
+};
 
 #endif  // CHROME_BROWSER_AUTOFILL_AUTOFILL_METRICS_H_
diff --git a/chrome/browser/autofill/autofill_metrics_unittest.cc b/chrome/browser/autofill/autofill_metrics_unittest.cc
new file mode 100644
index 0000000..4521296
--- /dev/null
+++ b/chrome/browser/autofill/autofill_metrics_unittest.cc
@@ -0,0 +1,229 @@
+// Copyright (c) 2010 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 "base/ref_counted.h"
+#include "base/scoped_ptr.h"
+#include "base/string16.h"
+#include "base/utf_string_conversions.h"
+#include "chrome/browser/autofill/autofill_common_test.h"
+#include "chrome/browser/autofill/autofill_manager.h"
+#include "chrome/browser/autofill/autofill_metrics.h"
+#include "chrome/browser/autofill/personal_data_manager.h"
+#include "chrome/browser/renderer_host/test/test_render_view_host.h"
+#include "chrome/browser/tab_contents/test_tab_contents.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webkit/glue/form_data.h"
+#include "webkit/glue/form_field.h"
+
+using webkit_glue::FormData;
+using webkit_glue::FormField;
+
+namespace {
+
+// TODO(isherman): Move this into autofill_common_test.h?
+class TestPersonalDataManager : public PersonalDataManager {
+ public:
+  TestPersonalDataManager() {
+    CreateTestAutoFillProfiles(&web_profiles_);
+    CreateTestCreditCards(&credit_cards_);
+  }
+
+  virtual void InitializeIfNeeded() {}
+  virtual void SaveImportedFormData() {}
+  virtual bool IsDataLoaded() const { return true; }
+
+ private:
+  void CreateTestAutoFillProfiles(ScopedVector<AutoFillProfile>* profiles) {
+    AutoFillProfile* profile = new AutoFillProfile;
+    autofill_test::SetProfileInfo(profile, "Home", "Elvis", "Aaron",
+                                  "Presley", "theking@gmail.com", "RCA",
+                                  "3734 Elvis Presley Blvd.", "Apt. 10",
+                                  "Memphis", "Tennessee", "38116", "USA",
+                                  "12345678901", "");
+    profile->set_guid("00000000-0000-0000-0000-000000000001");
+    profiles->push_back(profile);
+    profile = new AutoFillProfile;
+    autofill_test::SetProfileInfo(profile, "Work", "Charles", "Hardin",
+                                  "Holley", "buddy@gmail.com", "Decca",
+                                  "123 Apple St.", "unit 6", "Lubbock",
+                                  "Texas", "79401", "USA", "23456789012",
+                                  "");
+    profile->set_guid("00000000-0000-0000-0000-000000000002");
+    profiles->push_back(profile);
+    profile = new AutoFillProfile;
+    autofill_test::SetProfileInfo(profile, "Empty", "", "", "", "", "", "", "",
+                                  "", "", "", "", "", "");
+    profile->set_guid("00000000-0000-0000-0000-000000000003");
+    profiles->push_back(profile);
+  }
+
+  void CreateTestCreditCards(ScopedVector<CreditCard>* credit_cards) {
+    CreditCard* credit_card = new CreditCard;
+    autofill_test::SetCreditCardInfo(credit_card, "First", "Elvis Presley",
+                                     "4234567890123456", // Visa
+                                     "04", "2012");
+    credit_card->set_guid("00000000-0000-0000-0000-000000000004");
+    credit_cards->push_back(credit_card);
+    credit_card = new CreditCard;
+    autofill_test::SetCreditCardInfo(credit_card, "Second", "Buddy Holly",
+                                     "5187654321098765", // Mastercard
+                                     "10", "2014");
+    credit_card->set_guid("00000000-0000-0000-0000-000000000005");
+    credit_cards->push_back(credit_card);
+    credit_card = new CreditCard;
+    autofill_test::SetCreditCardInfo(credit_card, "Empty", "", "", "", "");
+    credit_card->set_guid("00000000-0000-0000-0000-000000000006");
+    credit_cards->push_back(credit_card);
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(TestPersonalDataManager);
+};
+
+class MockAutoFillMetrics : public AutoFillMetrics {
+ public:
+  MockAutoFillMetrics() {}
+  MOCK_CONST_METHOD1(Log, void(ServerQueryMetric metric));
+  MOCK_CONST_METHOD1(Log, void(QualityMetric metric));
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockAutoFillMetrics);
+};
+
+class TestAutoFillManager : public AutoFillManager {
+ public:
+  TestAutoFillManager(TabContents* tab_contents,
+                      TestPersonalDataManager* personal_manager)
+      : AutoFillManager(tab_contents, personal_manager),
+        autofill_enabled_(true) {
+    set_metric_logger(new MockAutoFillMetrics);
+  }
+  virtual ~TestAutoFillManager() {}
+
+  virtual bool IsAutoFillEnabled() const { return autofill_enabled_; }
+
+  void set_autofill_enabled(bool autofill_enabled) {
+    autofill_enabled_ = autofill_enabled;
+  }
+
+  const MockAutoFillMetrics* metric_logger() const {
+    return static_cast<const MockAutoFillMetrics*>(
+        AutoFillManager::metric_logger());
+  }
+
+ private:
+  bool autofill_enabled_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestAutoFillManager);
+};
+
+}  // namespace
+
+class AutoFillMetricsTest : public RenderViewHostTestHarness {
+ public:
+  AutoFillMetricsTest() {}
+  virtual ~AutoFillMetricsTest() {
+    // Order of destruction is important as AutoFillManager relies on
+    // PersonalDataManager to be around when it gets destroyed.
+    autofill_manager_.reset(NULL);
+    test_personal_data_ = NULL;
+  }
+
+  virtual void SetUp() {
+    RenderViewHostTestHarness::SetUp();
+    test_personal_data_ = new TestPersonalDataManager();
+    autofill_manager_.reset(new TestAutoFillManager(contents(),
+                                                    test_personal_data_.get()));
+  }
+
+ protected:
+  scoped_ptr<TestAutoFillManager> autofill_manager_;
+  scoped_refptr<TestPersonalDataManager> test_personal_data_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AutoFillMetricsTest);
+};
+
+// Test that we log quality metrics appropriately.
+TEST_F(AutoFillMetricsTest, QualityMetrics) {
+  // Set up our form data.
+  FormData form;
+  form.name = ASCIIToUTF16("TestForm");
+  form.method = ASCIIToUTF16("POST");
+  form.origin = GURL("http://example.com/form.html");
+  form.action = GURL("http://example.com/submit.html");
+  form.user_submitted = true;
+
+  FormField field;
+  autofill_test::CreateTestFormField(
+      "Autofilled", "autofilled", "Elvis Presley", "text", &field);
+  field.set_autofilled(true);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Autofill Failed", "autofillfailed", "buddy@gmail.com", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Empty", "empty", "", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Unknown", "unknown", "garbage", "text", &field);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Select", "select", "USA", "select-one", &field);
+  form.fields.push_back(field);
+
+  // Establish our expectations.
+  ::testing::InSequence dummy;
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_AUTOFILLED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_AUTOFILL_FAILED ));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED));
+
+  // Simulate form submission.
+  EXPECT_NO_FATAL_FAILURE(autofill_manager_->FormSubmitted(form));
+}
+
+// Test that we don't log quality metrics for non-autofillable forms.
+TEST_F(AutoFillMetricsTest, NoQualityMetricsForNonAutoFillableForms) {
+  // Forms must include at least three fields to be auto-fillable.
+  FormData form;
+  form.name = ASCIIToUTF16("TestForm");
+  form.method = ASCIIToUTF16("POST");
+  form.origin = GURL("http://example.com/form.html");
+  form.action = GURL("http://example.com/submit.html");
+  form.user_submitted = true;
+
+  FormField field;
+  autofill_test::CreateTestFormField(
+      "Autofilled", "autofilled", "Elvis Presley", "text", &field);
+  field.set_autofilled(true);
+  form.fields.push_back(field);
+  autofill_test::CreateTestFormField(
+      "Autofill Failed", "autofillfailed", "buddy@gmail.com", "text", &field);
+  form.fields.push_back(field);
+
+  // Simulate form submission.
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED)).Times(0);
+  EXPECT_NO_FATAL_FAILURE(autofill_manager_->FormSubmitted(form));
+
+  // Search forms are not auto-fillable.
+  form.action = GURL("http://example.com/search?q=Elvis%20Presley");
+  autofill_test::CreateTestFormField(
+      "Empty", "empty", "", "text", &field);
+  form.fields.push_back(field);
+
+  // Simulate form submission.
+  EXPECT_CALL(*autofill_manager_->metric_logger(),
+              Log(AutoFillMetrics::FIELD_SUBMITTED)).Times(0);
+  EXPECT_NO_FATAL_FAILURE(autofill_manager_->FormSubmitted(form));
+}
diff --git a/chrome/browser/autofill/form_structure.cc b/chrome/browser/autofill/form_structure.cc
index 2958df6..8ae8eb2 100644
--- a/chrome/browser/autofill/form_structure.cc
+++ b/chrome/browser/autofill/form_structure.cc
@@ -194,9 +194,9 @@ bool FormStructure::EncodeQueryRequest(const ScopedVector<FormStructure>& forms,
 // static
 void FormStructure::ParseQueryResponse(const std::string& response_xml,
                                        const std::vector<FormStructure*>& forms,
-                                       UploadRequired* upload_required) {
-  autofill_metrics::LogServerQueryMetric(
-      autofill_metrics::QUERY_RESPONSE_RECEIVED);
+                                       UploadRequired* upload_required,
+                                       const AutoFillMetrics& metric_logger) {
+  metric_logger.Log(AutoFillMetrics::QUERY_RESPONSE_RECEIVED);
 
   // Parse the field types from the server response to the query.
   std::vector<AutoFillFieldType> field_types;
@@ -206,8 +206,7 @@ void FormStructure::ParseQueryResponse(const std::string& response_xml,
   if (!parse_handler.succeeded())
     return;
 
-  autofill_metrics::LogServerQueryMetric(
-      autofill_metrics::QUERY_RESPONSE_PARSED);
+  metric_logger.Log(AutoFillMetrics::QUERY_RESPONSE_PARSED);
 
   bool heuristics_detected_fillable_field = false;
   bool query_response_overrode_heuristics = false;
@@ -250,17 +249,17 @@ void FormStructure::ParseQueryResponse(const std::string& response_xml,
     form->UpdateAutoFillCount();
   }
 
-  autofill_metrics::ServerQueryMetricType metric_type;
+  AutoFillMetrics::ServerQueryMetric metric;
   if (query_response_overrode_heuristics) {
     if (heuristics_detected_fillable_field) {
-      metric_type = autofill_metrics::QUERY_RESPONSE_OVERRODE_LOCAL_HEURISTICS;
+      metric = AutoFillMetrics::QUERY_RESPONSE_OVERRODE_LOCAL_HEURISTICS;
     } else {
-      metric_type = autofill_metrics::QUERY_RESPONSE_WITH_NO_LOCAL_HEURISTICS;
+      metric = AutoFillMetrics::QUERY_RESPONSE_WITH_NO_LOCAL_HEURISTICS;
     }
   } else {
-    metric_type = autofill_metrics::QUERY_RESPONSE_MATCHED_LOCAL_HEURISTICS;
+    metric = AutoFillMetrics::QUERY_RESPONSE_MATCHED_LOCAL_HEURISTICS;
   }
-  autofill_metrics::LogServerQueryMetric(metric_type);
+  metric_logger.Log(metric);
 }
 
 std::string FormStructure::FormSignature() const {
diff --git a/chrome/browser/autofill/form_structure.h b/chrome/browser/autofill/form_structure.h
index fe4c5da..ad0c411 100644
--- a/chrome/browser/autofill/form_structure.h
+++ b/chrome/browser/autofill/form_structure.h
@@ -31,6 +31,8 @@ enum UploadRequired {
   USE_UPLOAD_RATES
 };
 
+class AutoFillMetrics;
+
 // FormStructure stores a single HTML form together with the values entered
 // in the fields along with additional information needed by AutoFill.
 class FormStructure {
@@ -55,7 +57,8 @@ class FormStructure {
   // same as the one passed to EncodeQueryRequest when constructing the query.
   static void ParseQueryResponse(const std::string& response_xml,
                                  const std::vector<FormStructure*>& forms,
-                                 UploadRequired* upload_required);
+                                 UploadRequired* upload_required,
+                                 const AutoFillMetrics& metric_logger);
 
   // The unique signature for this form, composed of the target url domain,
   // the form name, and the form field names in a 64-bit hash.