rAc: change how enabling/disabling Autofill affects requestAutocomplete().

Before, disabling Autofill would cause form.requestAutocomplete() to simply
dispatch an AutocompleteErrorEvent with a reason of "disabled". Now, disabling
Autofill and invoking rAc still shows the dialog with no ability to save or
display Autofill data. This is a step forward for web authors as they can use
our dialog with higher confidence of it always being there while privacy-centric
users can also be happy.

I should note that requestAutocomplete() still requires a user action to be
invoked, so a page will still need to do something like:

  button.onclick = function() { form.requestAutocomplete(); };

to invoke the dialog, which I believe will be mitigation for a majority of bad
actors attempting to misuse rAc.

BUG=342942
R=estade@chromium.org
TEST=unit_tests, chrome://settings/search#autofill, [ /x] Enable Autofill...
     while requestAutocomplete() dialog is showing

Review URL: https://codereview.chromium.org/145183004

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

8 files changed, 68 insertions, 25 deletions

diff --git a/components/autofill/content/browser/request_autocomplete_manager.cc b/components/autofill/content/browser/request_autocomplete_manager.cc
index 79b0121..9f70b93 100644
--- a/components/autofill/content/browser/request_autocomplete_manager.cc
+++ b/components/autofill/content/browser/request_autocomplete_manager.cc
@@ -30,13 +30,6 @@ void RequestAutocompleteManager::OnRequestAutocomplete(
   if (!IsValidFormData(form))
     return;
 
-  if (!autofill_driver_->autofill_manager()->IsAutofillEnabled()) {
-    ReturnAutocompleteResult(
-        blink::WebFormElement::AutocompleteResultErrorDisabled,
-        FormData());
-    return;
-  }
-
   base::Callback<void(const FormStructure*)> callback =
       base::Bind(&RequestAutocompleteManager::ReturnAutocompleteData,
                  weak_ptr_factory_.GetWeakPtr());
diff --git a/components/autofill/content/browser/request_autocomplete_manager_unittest.cc b/components/autofill/content/browser/request_autocomplete_manager_unittest.cc
index cf6ec40..72fae80 100644
--- a/components/autofill/content/browser/request_autocomplete_manager_unittest.cc
+++ b/components/autofill/content/browser/request_autocomplete_manager_unittest.cc
@@ -153,12 +153,12 @@ TEST_F(RequestAutocompleteManagerTest, OnRequestAutocompleteCancel) {
 }
 
 TEST_F(RequestAutocompleteManagerTest,
-       OnRequestAutocompleteWithAutocompleteDisabled) {
+       OnRequestAutocompleteWithAutofillDisabled) {
   blink::WebFormElement::AutocompleteResult result;
   driver_->mock_autofill_manager()->set_autofill_enabled(false);
   request_autocomplete_manager_->OnRequestAutocomplete(FormData(), GURL());
   EXPECT_TRUE(GetAutocompleteResultMessage(&result));
-  EXPECT_EQ(result, blink::WebFormElement::AutocompleteResultErrorDisabled);
+  EXPECT_EQ(result, blink::WebFormElement::AutocompleteResultSuccess);
 }
 
 }  // namespace autofill
diff --git a/components/autofill/core/browser/autofill_manager_unittest.cc b/components/autofill/core/browser/autofill_manager_unittest.cc
index 9ea7d9a..cf60753 100644
--- a/components/autofill/core/browser/autofill_manager_unittest.cc
+++ b/components/autofill/core/browser/autofill_manager_unittest.cc
@@ -75,7 +75,7 @@ class TestPersonalDataManager : public PersonalDataManager {
   }
 
   using PersonalDataManager::set_database;
-  using PersonalDataManager::set_pref_service;
+  using PersonalDataManager::SetPrefService;
 
   // Factory method for keyed service.  PersonalDataManager is NULL for testing.
   static BrowserContextKeyedService* Build(content::BrowserContext* profile) {
@@ -620,7 +620,7 @@ class AutofillManagerTest : public ChromeRenderViewHostTestHarness {
     autofill::TabAutofillManagerDelegate* manager_delegate =
         autofill::TabAutofillManagerDelegate::FromWebContents(web_contents());
     personal_data_.set_database(manager_delegate->GetDatabase());
-    personal_data_.set_pref_service(profile()->GetPrefs());
+    personal_data_.SetPrefService(profile()->GetPrefs());
     autofill_driver_.reset(new MockAutofillDriver());
     autofill_manager_.reset(new TestAutofillManager(
         autofill_driver_.get(), manager_delegate, &personal_data_));
@@ -639,11 +639,13 @@ class AutofillManagerTest : public ChromeRenderViewHostTestHarness {
     // be destroyed at the destruction of the WebContents.
     autofill_manager_.reset();
     autofill_driver_.reset();
-    ChromeRenderViewHostTestHarness::TearDown();
 
     // Remove the AutofillWebDataService so TestPersonalDataManager does not
     // need to care about removing self as an observer in destruction.
     personal_data_.set_database(scoped_refptr<AutofillWebDataService>(NULL));
+    personal_data_.SetPrefService(NULL);
+
+    ChromeRenderViewHostTestHarness::TearDown();
   }
 
   void GetAutofillSuggestions(int query_id,
diff --git a/components/autofill/core/browser/autofill_metrics_unittest.cc b/components/autofill/core/browser/autofill_metrics_unittest.cc
index 33d5e66..85aab19 100644
--- a/components/autofill/core/browser/autofill_metrics_unittest.cc
+++ b/components/autofill/core/browser/autofill_metrics_unittest.cc
@@ -97,7 +97,7 @@ class TestPersonalDataManager : public PersonalDataManager {
   }
 
   using PersonalDataManager::set_database;
-  using PersonalDataManager::set_pref_service;
+  using PersonalDataManager::SetPrefService;
 
   // Overridden to avoid a trip to the database. This should be a no-op except
   // for the side-effect of logging the profile count.
@@ -290,7 +290,7 @@ void AutofillMetricsTest::SetUp() {
 
   personal_data_.reset(new TestPersonalDataManager());
   personal_data_->set_database(manager_delegate->GetDatabase());
-  personal_data_->set_pref_service(profile()->GetPrefs());
+  personal_data_->SetPrefService(profile()->GetPrefs());
   autofill_driver_.reset(new TestAutofillDriver());
   autofill_manager_.reset(new TestAutofillManager(
       autofill_driver_.get(), manager_delegate, personal_data_.get()));
diff --git a/components/autofill/core/browser/personal_data_manager.cc b/components/autofill/core/browser/personal_data_manager.cc
index 0e19ac4..da70ce2 100644
--- a/components/autofill/core/browser/personal_data_manager.cc
+++ b/components/autofill/core/browser/personal_data_manager.cc
@@ -146,6 +146,7 @@ PersonalDataManager::PersonalDataManager(const std::string& app_locale)
       pending_creditcards_query_(0),
       app_locale_(app_locale),
       metric_logger_(new AutofillMetrics),
+      pref_service_(NULL),
       is_off_the_record_(false),
       has_logged_profile_count_(false) {}
 
@@ -153,7 +154,7 @@ void PersonalDataManager::Init(scoped_refptr<AutofillWebDataService> database,
                                PrefService* pref_service,
                                bool is_off_the_record) {
   database_ = database;
-  pref_service_ = pref_service;
+  SetPrefService(pref_service);
   is_off_the_record_ = is_off_the_record;
 
   if (!is_off_the_record_)
@@ -205,8 +206,7 @@ void PersonalDataManager::OnWebDataServiceRequestDone(
   // If both requests have responded, then all personal data is loaded.
   if (pending_profiles_query_ == 0 && pending_creditcards_query_ == 0) {
     is_data_loaded_ = true;
-    FOR_EACH_OBSERVER(PersonalDataManagerObserver, observers_,
-                      OnPersonalDataChanged());
+    NotifyPersonalDataChanged();
   }
 }
 
@@ -679,6 +679,7 @@ void PersonalDataManager::GetCreditCardSuggestions(
 }
 
 bool PersonalDataManager::IsAutofillEnabled() const {
+  DCHECK(pref_service_);
   return pref_service_->GetBoolean(prefs::kAutofillEnabled);
 }
 
@@ -686,6 +687,17 @@ std::string PersonalDataManager::CountryCodeForCurrentTimezone() const {
   return base::CountryCodeForCurrentTimezone();
 }
 
+void PersonalDataManager::SetPrefService(PrefService* pref_service) {
+  enabled_pref_.reset(new BooleanPrefMember);
+  pref_service_ = pref_service;
+  // |pref_service_| can be NULL in tests.
+  if (pref_service_) {
+    enabled_pref_->Init(prefs::kAutofillEnabled, pref_service_,
+        base::Bind(&PersonalDataManager::EnabledPrefChanged,
+                   base::Unretained(this)));
+  }
+}
+
 // static
 bool PersonalDataManager::IsValidLearnableProfile(
     const AutofillProfile& profile,
@@ -983,6 +995,10 @@ std::string PersonalDataManager::SaveImportedProfile(
   return guid;
 }
 
+void PersonalDataManager::NotifyPersonalDataChanged() {
+  FOR_EACH_OBSERVER(PersonalDataManagerObserver, observers_,
+                    OnPersonalDataChanged());
+}
 
 std::string PersonalDataManager::SaveImportedCreditCard(
     const CreditCard& imported_card) {
@@ -1024,6 +1040,9 @@ void PersonalDataManager::LogProfileCount() const {
 }
 
 std::string PersonalDataManager::MostCommonCountryCodeFromProfiles() const {
+  if (!IsAutofillEnabled())
+    return std::string();
+
   // Count up country codes from existing profiles.
   std::map<std::string, int> votes;
   // TODO(estade): can we make this GetProfiles() instead? It seems to cause
@@ -1052,4 +1071,9 @@ std::string PersonalDataManager::MostCommonCountryCodeFromProfiles() const {
   return std::string();
 }
 
+void PersonalDataManager::EnabledPrefChanged() {
+  default_country_code_.clear();
+  NotifyPersonalDataChanged();
+}
+
 }  // namespace autofill
diff --git a/components/autofill/core/browser/personal_data_manager.h b/components/autofill/core/browser/personal_data_manager.h
index e44c85d..42d80f4 100644
--- a/components/autofill/core/browser/personal_data_manager.h
+++ b/components/autofill/core/browser/personal_data_manager.h
@@ -12,6 +12,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/observer_list.h"
+#include "base/prefs/pref_member.h"
 #include "base/strings/string16.h"
 #include "components/autofill/core/browser/autofill_metrics.h"
 #include "components/autofill/core/browser/autofill_profile.h"
@@ -254,6 +255,9 @@ class PersonalDataManager : public WebDataServiceConsumer,
   // query handle.
   void CancelPendingQuery(WebDataServiceBase::Handle* handle);
 
+  // Notifies observers that personal data has changed.
+  void NotifyPersonalDataChanged();
+
   // The first time this is called, logs an UMA metrics for the number of
   // profiles the user has. On subsequent calls, does nothing.
   void LogProfileCount() const;
@@ -264,6 +268,10 @@ class PersonalDataManager : public WebDataServiceConsumer,
   // Overrideable for testing.
   virtual std::string CountryCodeForCurrentTimezone() const;
 
+  // Sets which PrefService to use and observe. |pref_service| is not owned by
+  // this class and must outlive |this|.
+  void SetPrefService(PrefService* pref_service);
+
   // For tests.
   const AutofillMetrics* metric_logger() const { return metric_logger_.get(); }
 
@@ -275,10 +283,6 @@ class PersonalDataManager : public WebDataServiceConsumer,
     metric_logger_.reset(metric_logger);
   }
 
-  void set_pref_service(PrefService* pref_service) {
-    pref_service_ = pref_service;
-  }
-
   // The backing database that this PersonalDataManager uses.
   scoped_refptr<AutofillWebDataService> database_;
 
@@ -313,6 +317,9 @@ class PersonalDataManager : public WebDataServiceConsumer,
   // Prefers verified profiles over unverified ones.
   std::string MostCommonCountryCodeFromProfiles() const;
 
+  // Called when the value of prefs::kAutofillEnabled changes.
+  void EnabledPrefChanged();
+
   const std::string app_locale_;
 
   // The default country code for new addresses.
@@ -331,6 +338,9 @@ class PersonalDataManager : public WebDataServiceConsumer,
   // Whether we have already logged the number of profiles this session.
   mutable bool has_logged_profile_count_;
 
+  // An observer to listen for changes to prefs::kAutofillEnabled.
+  scoped_ptr<BooleanPrefMember> enabled_pref_;
+
   DISALLOW_COPY_AND_ASSIGN(PersonalDataManager);
 };
 
diff --git a/components/autofill/core/browser/personal_data_manager_unittest.cc b/components/autofill/core/browser/personal_data_manager_unittest.cc
index 6264cc2..f0d32ea 100644
--- a/components/autofill/core/browser/personal_data_manager_unittest.cc
+++ b/components/autofill/core/browser/personal_data_manager_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/guid.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
 #include "chrome/test/base/testing_profile.h"
@@ -19,6 +20,7 @@
 #include "components/autofill/core/browser/personal_data_manager_observer.h"
 #include "components/autofill/core/browser/webdata/autofill_table.h"
 #include "components/autofill/core/browser/webdata/autofill_webdata_service.h"
+#include "components/autofill/core/common/autofill_pref_names.h"
 #include "components/autofill/core/common/form_data.h"
 #include "components/webdata/common/web_data_service_base.h"
 #include "components/webdata/common/web_database_service.h"
@@ -2500,6 +2502,20 @@ TEST_F(PersonalDataManagerTest, DefaultCountryCodeIsCached) {
   // The value is cached and doesn't change even after adding an address.
   EXPECT_EQ(default_country,
             personal_data_->GetDefaultCountryCodeForNewAddress());
+
+  EXPECT_CALL(personal_data_observer_, OnPersonalDataChanged()).Times(2);
+
+  // Disabling Autofill blows away this cache and shouldn't account for Autofill
+  // profiles.
+  profile_->GetPrefs()->SetBoolean(prefs::kAutofillEnabled, false);
+  EXPECT_EQ(default_country,
+            personal_data_->GetDefaultCountryCodeForNewAddress());
+
+  // Enabling Autofill blows away the cached value and should reflect the new
+  // value (accounting for profiles).
+  profile_->GetPrefs()->SetBoolean(prefs::kAutofillEnabled, true);
+  EXPECT_EQ(base::UTF16ToUTF8(moose.GetRawInfo(ADDRESS_HOME_COUNTRY)),
+            personal_data_->GetDefaultCountryCodeForNewAddress());
 }
 
 TEST_F(PersonalDataManagerTest, DefaultCountryCodeComesFromProfiles) {
diff --git a/components/autofill/core/browser/test_personal_data_manager.cc b/components/autofill/core/browser/test_personal_data_manager.cc
index 65542fa..2349cda 100644
--- a/components/autofill/core/browser/test_personal_data_manager.cc
+++ b/components/autofill/core/browser/test_personal_data_manager.cc
@@ -15,14 +15,12 @@ TestPersonalDataManager::~TestPersonalDataManager() {}
 
 void TestPersonalDataManager::AddTestingProfile(AutofillProfile* profile) {
   profiles_.push_back(profile);
-  FOR_EACH_OBSERVER(PersonalDataManagerObserver, observers_,
-                    OnPersonalDataChanged());
+  NotifyPersonalDataChanged();
 }
 
 void TestPersonalDataManager::AddTestingCreditCard(CreditCard* credit_card) {
   credit_cards_.push_back(credit_card);
-  FOR_EACH_OBSERVER(PersonalDataManagerObserver, observers_,
-                    OnPersonalDataChanged());
+  NotifyPersonalDataChanged();
 }
 
 const std::vector<AutofillProfile*>& TestPersonalDataManager::GetProfiles()