[protector] Refactoring of --no-protector code.

*) On DSE change, new provider is not pushed to Sync.
*) Simplified code in BrowserInit.


BUG=None
TEST=protector.py


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

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

chrome/browser/protector/base_prefs_change.cc

@@ -5,8 +5,10 @@
 #include "chrome/browser/prefs/pref_set_observer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/protector/base_prefs_change.h"
+#include "chrome/browser/protector/protected_prefs_watcher.h"
 #include "chrome/browser/protector/protector_service.h"
 #include "chrome/browser/protector/protector_service_factory.h"
+#include "chrome/browser/protector/protector_utils.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/notification_service.h"
 
@@ -25,6 +27,13 @@ bool BasePrefsChange::Init(Profile* profile) {
   return true;
 }
 
+void BasePrefsChange::InitWhenDisabled(Profile* profile) {
+  // Forcibly set backup to match the actual settings so that no changes are
+  // detected on future runs.
+  ProtectorServiceFactory::GetForProfile(profile)->GetPrefsWatcher()->
+      ForceUpdateBackup();
+}
+
 void BasePrefsChange::DismissOnPrefChange(const std::string& pref_name) {
   DCHECK(!pref_observer_->IsObserved(pref_name));
   pref_observer_->AddPref(pref_name);

chrome/browser/protector/base_prefs_change.h

@@ -25,6 +25,7 @@ class BasePrefsChange : public BaseSettingChange,
 
   // BaseSettingChange overrides:
   virtual bool Init(Profile* profile) OVERRIDE;
+  virtual void InitWhenDisabled(Profile* profile) OVERRIDE;
 
  protected:
   // Marks |this| to be dismissed when |pref_name| is changed. Should only be

chrome/browser/protector/base_setting_change.cc

@@ -39,11 +39,15 @@ bool BaseSettingChange::Contains(const BaseSettingChange* other) const {
 }
 
 bool BaseSettingChange::Init(Profile* profile) {
-  DCHECK(profile);
+  DCHECK(profile && !profile_);
   profile_ = profile;
   return true;
 }
 
+void BaseSettingChange::InitWhenDisabled(Profile* profile) {
+  DCHECK(profile && !profile_);
+}
+
 void BaseSettingChange::Apply(Browser* browser) {
 }
 

chrome/browser/protector/base_setting_change.h

@@ -55,6 +55,10 @@ class BaseSettingChange {
   // base method.
   virtual bool Init(Profile* profile);
 
+  // Called instead of Init when ProtectorService is disabled. No other members
+  // are called in that case.
+  virtual void InitWhenDisabled(Profile* profile);
+
   // Persists new setting if needed. |browser| is the Browser instance from
   // which the user action originates.
   virtual void Apply(Browser* browser);

chrome/browser/protector/default_search_provider_change.cc

@@ -87,6 +87,7 @@ class DefaultSearchProviderChange : public BaseSettingChange,
 
   // BaseSettingChange overrides:
   virtual bool Init(Profile* profile) OVERRIDE;
+  virtual void InitWhenDisabled(Profile* profile) OVERRIDE;
   virtual void Apply(Browser* browser) OVERRIDE;
   virtual void Discard(Browser* browser) OVERRIDE;
   virtual void Timeout() OVERRIDE;
@@ -232,6 +233,12 @@ bool DefaultSearchProviderChange::Init(Profile* profile) {
   return true;
 }
 
+void DefaultSearchProviderChange::InitWhenDisabled(Profile* profile) {
+  // The --no-protector case is handled in TemplateURLService internals.
+  // TODO(ivankr): move it here.
+  NOTREACHED();
+}
+
 void DefaultSearchProviderChange::Apply(Browser* browser) {
   UMA_HISTOGRAM_ENUMERATION(
       kProtectorHistogramSearchProviderApplied,

chrome/browser/protector/histograms.h

@@ -23,6 +23,7 @@ enum ProtectorError {
   kProtectorErrorValueChanged,
   kProtectorErrorValueValid,
   kProtectorErrorValueValidZero,
+  kProtectorErrorForcedUpdate,
 
   // This is for convenience only, must always be the last.
   kProtectorErrorCount
@@ -55,7 +56,7 @@ extern const char kProtectorHistogramStartupSettingsApplied[];
 // Histogram name to report the new startup settings when the backup is
 // valid and a change is detected.
 extern const char kProtectorHistogramStartupSettingsChanged[];
-// Histogram name to report when keeps previous startup settings.
+// Histogram name to report when user keeps previous startup settings.
 extern const char kProtectorHistogramStartupSettingsDiscarded[];
 // Histogram name to report when user ignores startup settings change.
 extern const char kProtectorHistogramStartupSettingsTimeout[];

chrome/browser/protector/mock_setting_change.h

@@ -20,6 +20,7 @@ class MockSettingChange : public BaseSettingChange {
   virtual bool Init(Profile* profile) OVERRIDE;
 
   MOCK_METHOD1(MockInit, bool(Profile* profile));
+  MOCK_METHOD1(InitWhenDisabled, void(Profile*));
   MOCK_METHOD1(Apply, void(Browser*));
   MOCK_METHOD1(Discard, void(Browser*));
   MOCK_METHOD0(Timeout, void());

chrome/browser/protector/prefs_backup_invalid_change.cc

@@ -30,6 +30,7 @@ class PrefsBackupInvalidChange : public BasePrefsChange {
 
   // BasePrefsChange overrides:
   virtual bool Init(Profile* profile) OVERRIDE;
+  virtual void InitWhenDisabled(Profile* profile) OVERRIDE;
   virtual void Apply(Browser* browser) OVERRIDE;
   virtual void Discard(Browser* browser) OVERRIDE;
   virtual void Timeout() OVERRIDE;
@@ -74,6 +75,10 @@ bool PrefsBackupInvalidChange::Init(Profile* profile) {
   return true;
 }
 
+void PrefsBackupInvalidChange::InitWhenDisabled(Profile* profile) {
+  // Nothing to do here since the backup has been already reset.
+}
+
 void PrefsBackupInvalidChange::Apply(Browser* browser) {
   NOTREACHED();
 }

chrome/browser/protector/protected_prefs_watcher.cc

@@ -86,7 +86,7 @@ void ProtectedPrefsWatcher::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterStringPref(kBackupHomePage, "",
                             PrefService::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(kBackupHomePageIsNewTabPage, false,
-                            PrefService::UNSYNCABLE_PREF);
+                             PrefService::UNSYNCABLE_PREF);
   prefs->RegisterBooleanPref(kBackupShowHomeButton, false,
                              PrefService::UNSYNCABLE_PREF);
   prefs->RegisterIntegerPref(kBackupRestoreOnStartup, 0,
@@ -131,6 +131,14 @@ const base::Value* ProtectedPrefsWatcher::GetBackupForPref(
   return backup_pref->GetValue();
 }
 
+void ProtectedPrefsWatcher::ForceUpdateBackup() {
+  UMA_HISTOGRAM_ENUMERATION(
+      kProtectorHistogramPrefs,
+      kProtectorErrorForcedUpdate,
+      kProtectorErrorCount);
+  InitBackup();
+}
+
 void ProtectedPrefsWatcher::Observe(
     int type,
     const content::NotificationSource& source,

chrome/browser/protector/protected_prefs_watcher.h

@@ -42,8 +42,9 @@ class ProtectedPrefsWatcher : public content::NotificationObserver {
   // instance is owned by the PrefService.
   const base::Value* GetBackupForPref(const std::string& path) const;
 
-  // Updates the backup signature.
-  void UpdateBackupSignature();
+  // Forces a valid backup, matching actual preferences (overwriting any
+  // previous data). Should only be when protector service is disabled.
+  void ForceUpdateBackup();
 
   // True if the backup was valid at the profile load time.
   bool is_backup_valid() { return is_backup_valid_; }
@@ -84,6 +85,9 @@ class ProtectedPrefsWatcher : public content::NotificationObserver {
   // Returns |true| if backup signature is valid.
   bool IsSignatureValid() const;
 
+  // Updates the backup signature.
+  void UpdateBackupSignature();
+
   // Returns data to be signed as string.
   std::string GetSignatureData(PrefService* prefs) const;
 

chrome/browser/protector/protector_service.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/protector/composite_settings_change.h"
 #include "chrome/browser/protector/keys.h"
 #include "chrome/browser/protector/protected_prefs_watcher.h"
+#include "chrome/browser/protector/protector_utils.h"
 #include "chrome/browser/protector/settings_change_global_error.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -46,19 +47,24 @@ ProtectorService::~ProtectorService() {
 
 void ProtectorService::ShowChange(BaseSettingChange* change) {
   DCHECK(change);
+  // Change instance will either be owned by |this| or deleted after this call.
+  scoped_ptr<BaseSettingChange> change_ptr(change);
 
   DVLOG(1) << "Init change";
-  if (!change->Init(profile_)) {
+  if (!protector::IsEnabled()) {
+    change_ptr->InitWhenDisabled(profile_);
+    return;
+  } else if (!change_ptr->Init(profile_)) {
     LOG(WARNING) << "Error while initializing, dismissing change";
-    delete change;
     return;
   }
 
-  Item* item_to_merge_with = FindItemToMergeWith(change);
+  Item* item_to_merge_with = FindItemToMergeWith(change_ptr.get());
   if (item_to_merge_with) {
     // CompositeSettingsChange takes ownership of merged changes.
     BaseSettingChange* existing_change = item_to_merge_with->change.release();
-    CompositeSettingsChange* merged_change = existing_change->MergeWith(change);
+    CompositeSettingsChange* merged_change =
+        existing_change->MergeWith(change_ptr.release());
     item_to_merge_with->change.reset(merged_change);
     item_to_merge_with->was_merged = true;
     if (item_to_merge_with->error->GetBubbleView())
@@ -69,9 +75,9 @@ void ProtectorService::ShowChange(BaseSettingChange* change) {
   } else {
     Item new_item;
     SettingsChangeGlobalError* error =
-        new SettingsChangeGlobalError(change, this);
+        new SettingsChangeGlobalError(change_ptr.get(), this);
     new_item.error.reset(error);
-    new_item.change.reset(change);
+    new_item.change.reset(change_ptr.release());
     items_.push_back(new_item);
     // Do not show the bubble immediately if another one is active.
     error->AddToProfile(profile_, !has_active_change_);

chrome/browser/search_engines/template_url_service.cc

@@ -654,7 +654,7 @@ void TemplateURLService::OnWebDataServiceRequestDone(
     UpdateKeywordSearchTermsForURL(visits_to_add_[i]);
   visits_to_add_.clear();
 
-  if (new_resource_keyword_version && service_.get())
+  if (new_resource_keyword_version)
     service_->SetBuiltinKeywordVersion(new_resource_keyword_version);
 
 #if defined(ENABLE_PROTECTOR_SERVICE)
@@ -676,8 +676,7 @@ void TemplateURLService::OnWebDataServiceRequestDone(
       // Protector is turned off: set the current default search to itself
       // to update the backup and sign it. Otherwise, change will be reported
       // every time when keywords are loaded until a search provider is added.
-      // Note that this saves the default search provider to prefs.
-      SetDefaultSearchProviderNoNotify(default_search_provider_);
+      service_->SetDefaultSearchProvider(default_search_provider_);
     }
   }
 #endif

chrome/browser/ui/browser_init.cc

@@ -1582,18 +1582,11 @@ void BrowserInit::LaunchWithProfile::CheckPreferencesBackup(Profile* profile) {
       ProtectorServiceFactory::GetForProfile(profile);
   ProtectedPrefsWatcher* prefs_watcher = protector_service->GetPrefsWatcher();
 
-  // BaseSettingChange instances are always created, even when Protector is
-  // disabled, to report corresponding histograms. With Protector disabled,
-  // the backup is updated to match the new setting value, otherwise histograms
-  // would be reported on each run.
-  // TODO(ivankr): move IsEnabled() check to ProtectorService::ShowChange().
-
   // Check if backup is valid.
   if (!prefs_watcher->is_backup_valid()) {
     scoped_ptr<BaseSettingChange> change(
         protector::CreatePrefsBackupInvalidChange());
-    if (protector::IsEnabled())
-      protector_service->ShowChange(change.release());
+    protector_service->ShowChange(change.release());
     // Further checks make no sense.
     return;
   }
@@ -1612,12 +1605,7 @@ void BrowserInit::LaunchWithProfile::CheckPreferencesBackup(Profile* profile) {
             new_tabs,
             SessionStartupPref::GetStartupPrefBackup(profile),
             PinnedTabCodec::ReadPinnedTabs(tabs_backup)));
-    if (protector::IsEnabled()) {
-      protector_service->ShowChange(change.release());
-    } else {
-      SessionStartupPref::SetStartupPref(profile, new_pref);
-      PinnedTabCodec::WritePinnedTabs(profile, new_tabs);
-    }
+    protector_service->ShowChange(change.release());
   }
 }