Protector bubble cancels itself if user changes default search engine manually.

BUG=None
TEST=Manual: trigger protector bubble, change search engine manually; bubble should vanish.

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

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

chrome/browser/protector/base_setting_change.cc

@@ -4,22 +4,30 @@
 
 #include "chrome/browser/protector/base_setting_change.h"
 
+#include "base/logging.h"
+
 namespace protector {
 
-BaseSettingChange::BaseSettingChange() {
+BaseSettingChange::BaseSettingChange()
+    : protector_(NULL) {
 }
 
 BaseSettingChange::~BaseSettingChange() {
 }
 
 bool BaseSettingChange::Init(Protector* protector) {
+  DCHECK(protector);
+  protector_ = protector;
   return true;
 }
 
-void BaseSettingChange::Apply(Protector* protector) {
+void BaseSettingChange::Apply() {
+}
+
+void BaseSettingChange::Discard() {
 }
 
-void BaseSettingChange::Discard(Protector* protector) {
+void BaseSettingChange::OnBeforeRemoved() {
 }
 
 }  // namespace protector

chrome/browser/protector/base_setting_change.h

@@ -25,15 +25,21 @@ class BaseSettingChange {
   virtual ~BaseSettingChange();
 
   // Applies initial actions to the setting if needed. Must be called before
-  // any other calls are made, including text getters. Returns true if
-  // initialization was successful.
+  // any other calls are made, including text getters.
+  // Returns true if initialization was successful. Otherwise, no other
+  // calls should be made.
+  // Associates this change with |protector_| instance so overrides must
+  // call the base method.
   virtual bool Init(Protector* protector);
 
   // Persists new setting if needed.
-  virtual void Apply(Protector* protector);
+  virtual void Apply();
 
   // Restores old setting if needed.
-  virtual void Discard(Protector* protector);
+  virtual void Discard();
+
+  // Called before the change is removed from the protector instance.
+  virtual void OnBeforeRemoved() = 0;
 
   // Returns the wrench menu item and bubble title.
   virtual string16 GetBubbleTitle() const = 0;
@@ -48,7 +54,12 @@ class BaseSettingChange {
   // Returns text for the button to discard the change with |Discard|.
   virtual string16 GetDiscardButtonText() const = 0;
 
+  // Protector instance we've been associated with by an |Init| call.
+  Protector* protector() { return protector_; }
+
  private:
+  Protector* protector_;
+
   DISALLOW_COPY_AND_ASSIGN(BaseSettingChange);
 };
 

chrome/browser/protector/default_search_provider_change.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/protector/protector.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/template_url_service_observer.h"
 #include "chrome/browser/webdata/keyword_table.h"
 #include "chrome/common/url_constants.h"
 #include "grit/chromium_strings.h"
@@ -25,33 +26,37 @@ const size_t kMaxDisplayedNameLength = 10;
 
 }  // namespace
 
-class DefaultSearchProviderChange : public BaseSettingChange {
+class DefaultSearchProviderChange : public BaseSettingChange,
+                                    public TemplateURLServiceObserver {
  public:
   DefaultSearchProviderChange(const TemplateURL* old_url,
                               const TemplateURL* new_url);
 
   // BaseSettingChange overrides:
   virtual bool Init(Protector* protector) OVERRIDE;
-  virtual void Apply(Protector* protector) OVERRIDE;
-  virtual void Discard(Protector* protector) OVERRIDE;
+  virtual void Apply() OVERRIDE;
+  virtual void Discard() OVERRIDE;
+  virtual void OnBeforeRemoved() OVERRIDE;
   virtual string16 GetBubbleTitle() const OVERRIDE;
   virtual string16 GetBubbleMessage() const OVERRIDE;
   virtual string16 GetApplyButtonText() const OVERRIDE;
   virtual string16 GetDiscardButtonText() const OVERRIDE;
 
+  // TemplateURLServiceObserver overrides:
+  virtual void OnTemplateURLServiceChanged() OVERRIDE;
+
  private:
   virtual ~DefaultSearchProviderChange();
 
-  // Sets the given default search provider to profile that |protector| is
-  // guarding. Returns the |TemplateURL| instance the default search provider
-  // has been set to. If no search provider with |id| exists and
-  // |allow_fallback| is true, sets one of the prepoluated search providers.
-  const TemplateURL* SetDefaultSearchProvider(Protector* protector,
-                                              int64 id,
+  // Sets the given default search provider to profile that this change is
+  // related to. Returns the |TemplateURL| instance of the new default search
+  // provider. If no search provider with |id| exists and |allow_fallback| is
+  // true, sets one of the prepopulated search providers.
+  const TemplateURL* SetDefaultSearchProvider(int64 id,
                                               bool allow_fallback);
 
   // Opens the Search engine settings page in a new tab.
-  void OpenSearchEngineSettings(Protector* protector);
+  void OpenSearchEngineSettings();
 
   int64 old_id_;
   int64 new_id_;
@@ -62,6 +67,12 @@ class DefaultSearchProviderChange : public BaseSettingChange {
   // Name of the search engine that we fall back to if the backup is lost.
   string16 fallback_name_;
   string16 product_name_;
+  // Default search provider set by |Init| for the period until user makes a
+  // choice and either |Apply| or |Discard| is performed. Should only be used
+  // for comparison with the current default search provider and never
+  // dereferenced other than in |Init| because it may be deallocated by
+  // TemplateURLService at any time.
+  const TemplateURL* default_search_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(DefaultSearchProviderChange);
 };
@@ -72,7 +83,8 @@ DefaultSearchProviderChange::DefaultSearchProviderChange(
     : old_id_(0),
       new_id_(0),
       fallback_id_(0),
-      product_name_(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)) {
+      product_name_(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)),
+      default_search_provider_(NULL) {
   if (new_url) {
     new_id_ = new_url->id();
     new_name_ = new_url->short_name();
@@ -87,41 +99,50 @@ DefaultSearchProviderChange::~DefaultSearchProviderChange() {
 }
 
 bool DefaultSearchProviderChange::Init(Protector* protector) {
+  BaseSettingChange::Init(protector);
+
   // Initially reset the search engine to its previous setting.
-  const TemplateURL* current_url =
-      SetDefaultSearchProvider(protector, old_id_, true);
-  if (!current_url)
+  default_search_provider_ = SetDefaultSearchProvider(old_id_, true);
+  if (!default_search_provider_)
     return false;
-  if (!old_id_ || current_url->id() != old_id_) {
+
+  if (!old_id_ || default_search_provider_->id() != old_id_) {
     // Old settings is lost or invalid, so we had to fall back to one of the
     // prepopulated search engines.
-    fallback_id_ = current_url->id();
-    fallback_name_ = current_url->short_name();
+    fallback_id_ = default_search_provider_->id();
+    fallback_name_ = default_search_provider_->short_name();
     VLOG(1) << "Fallback to " << fallback_name_;
   }
+
+  protector->GetTemplateURLService()->AddObserver(this);
+
   return true;
 }
 
-void DefaultSearchProviderChange::Apply(Protector* protector) {
+void DefaultSearchProviderChange::Apply() {
   // TODO(avayvod): Add histrogram.
   if (!new_id_) {
     // Open settings page in case the new setting is invalid.
-    OpenSearchEngineSettings(protector);
+    OpenSearchEngineSettings();
   } else {
-    SetDefaultSearchProvider(protector, new_id_, false);
+    SetDefaultSearchProvider(new_id_, false);
   }
 }
 
-void DefaultSearchProviderChange::Discard(Protector* protector) {
+void DefaultSearchProviderChange::Discard() {
   // TODO(avayvod): Add histrogram.
   if (!old_id_) {
     // Open settings page in case the old setting is invalid.
-    OpenSearchEngineSettings(protector);
+    OpenSearchEngineSettings();
   }
   // Nothing to do otherwise since we have already set the search engine
   // to |old_id_| in |Init|.
 }
 
+void DefaultSearchProviderChange::OnBeforeRemoved() {
+  protector()->GetTemplateURLService()->RemoveObserver(this);
+}
+
 string16 DefaultSearchProviderChange::GetBubbleTitle() const {
   return l10n_util::GetStringUTF16(IDS_SEARCH_ENGINE_CHANGE_TITLE);
 }
@@ -168,11 +189,20 @@ string16 DefaultSearchProviderChange::GetDiscardButtonText() const {
   }
 }
 
+void DefaultSearchProviderChange::OnTemplateURLServiceChanged() {
+  if (protector()->GetTemplateURLService()->GetDefaultSearchProvider() !=
+      default_search_provider_) {
+    default_search_provider_ = NULL;
+    VLOG(1) << "Default search provider has been changed by user";
+    // This will delete the Protector instance and |this|.
+    protector()->DismissChange();
+  }
+}
+
 const TemplateURL* DefaultSearchProviderChange::SetDefaultSearchProvider(
-    Protector* protector,
     int64 id,
     bool allow_fallback) {
-  TemplateURLService* url_service = protector->GetTemplateURLService();
+  TemplateURLService* url_service = protector()->GetTemplateURLService();
   if (!url_service) {
     NOTREACHED() << "Can't get TemplateURLService object.";
     return NULL;
@@ -199,9 +229,8 @@ const TemplateURL* DefaultSearchProviderChange::SetDefaultSearchProvider(
   return url;
 }
 
-void DefaultSearchProviderChange::OpenSearchEngineSettings(
-    Protector* protector) {
-  protector->OpenTab(
+void DefaultSearchProviderChange::OpenSearchEngineSettings() {
+  protector()->OpenTab(
       GURL(std::string(chrome::kChromeUISettingsURL) +
            chrome::kSearchEnginesSubPage));
 }

chrome/browser/protector/protector.cc

@@ -26,6 +26,8 @@ Protector::Protector(Profile* profile)
 }
 
 Protector::~Protector() {
+  if (change_.get())
+    change_->OnBeforeRemoved();
 }
 
 void Protector::OpenTab(const GURL& url) {
@@ -48,37 +50,44 @@ void Protector::ShowChange(BaseSettingChange* change) {
                  base::Unretained(this), change));
 }
 
-void Protector::InitAndShowChange(BaseSettingChange* change) {
-  VLOG(1) << "Init change";
-  if (!change->Init(this)) {
-    VLOG(1) << "Error while initializing, removing ourselves";
-    delete change;
-    OnRemovedFromProfile();
-    return;
-  }
-  error_.reset(new SettingsChangeGlobalError(change, this));
-  error_->ShowForProfile(profile_);
+void Protector::DismissChange() {
+  DCHECK(error_.get());
+  error_->RemoveFromProfile();
 }
 
 void Protector::OnApplyChange() {
-  VLOG(1) << "Apply change";
-  error_->mutable_change()->Apply(this);
+  DVLOG(1) << "Apply change";
+  change_->Apply();
 }
 
 void Protector::OnDiscardChange() {
-  VLOG(1) << "Discard change";
-  error_->mutable_change()->Discard(this);
+  DVLOG(1) << "Discard change";
+  change_->Discard();
 }
 
 void Protector::OnDecisionTimeout() {
   // TODO(ivankr): Add histogram.
-  VLOG(1) << "Timeout";
+  DVLOG(1) << "Timeout";
 }
 
 void Protector::OnRemovedFromProfile() {
   BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
 }
 
+void Protector::InitAndShowChange(BaseSettingChange* change) {
+  DVLOG(1) << "Init change";
+  if (!change->Init(this)) {
+    LOG(WARNING) << "Error while initializing, removing ourselves";
+    delete change;
+    delete this;
+    return;
+  }
+  // |change_| should not be set until a successful |Init| call.
+  change_.reset(change);
+  error_.reset(new SettingsChangeGlobalError(change, this));
+  error_->ShowForProfile(profile_);
+}
+
 
 std::string SignSetting(const std::string& value) {
   crypto::HMAC hmac(crypto::HMAC::SHA256);

chrome/browser/protector/protector.h

@@ -31,14 +31,16 @@ class Protector : public SettingsChangeGlobalErrorDelegate {
   // bubble for.
   void OpenTab(const GURL& url);
 
-  // Returns TemplateURLService for the profile we've shown error bubble
-  // for.
+  // Returns TemplateURLService for the profile we've shown error bubble for.
   TemplateURLService* GetTemplateURLService();
 
-  // Shows global error about the specified change. Ownership of the change
-  // is passed to the GlobalError object.
+  // Shows global error about the specified change. Owns |change|.
   void ShowChange(BaseSettingChange* change);
 
+  // Silently discards any change previously shown (without calling Discard),
+  // removes global error and deletes itself.
+  void DismissChange();
+
   // SettingsChangeGlobalErrorDelegate implementation.
   virtual void OnApplyChange() OVERRIDE;
   virtual void OnDiscardChange() OVERRIDE;
@@ -53,13 +55,17 @@ class Protector : public SettingsChangeGlobalErrorDelegate {
 
   // Performs the initial action on settings change and shows it. This is run
   // asynchronously on UI thread because |ShowChange| may be called in the
-  // middle of some operations on settings that have changed.
+  // middle of some operations on settings that have changed. If the initial
+  // action fails, deletes itself.
   void InitAndShowChange(BaseSettingChange* change);
 
   // Pointer to error bubble controller. Indicates if we're showing change
-  // notification to user. Owns itself.
+  // notification to user.
   scoped_ptr<SettingsChangeGlobalError> error_;
 
+  // Setting change which we're showing.
+  scoped_ptr<BaseSettingChange> change_;
+
   // Profile which settings we are protecting.
   Profile* profile_;
 

chrome/browser/protector/settings_change_global_error.cc

@@ -10,6 +10,7 @@
 #include "base/stl_util.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/protector/base_setting_change.h"
 #include "chrome/browser/protector/settings_change_global_error_delegate.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/global_error_service.h"
@@ -103,8 +104,6 @@ void SettingsChangeGlobalError::BubbleViewCancelButtonPressed() {
 void SettingsChangeGlobalError::RemoveFromProfile() {
   if (profile_)
     GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError(this);
-  if (!closed_by_button_)
-    delegate_->OnDecisionTimeout();
   delegate_->OnRemovedFromProfile();
 }
 
@@ -113,7 +112,7 @@ void SettingsChangeGlobalError::BubbleViewDidClose() {
   if (!closed_by_button_) {
     BrowserThread::PostDelayedTask(
         BrowserThread::UI, FROM_HERE,
-        base::Bind(&SettingsChangeGlobalError::RemoveFromProfile,
+        base::Bind(&SettingsChangeGlobalError::OnInactiveTimeout,
                    weak_factory_.GetWeakPtr()),
         kMenuItemDisplayPeriodMs);
   } else {
@@ -148,4 +147,9 @@ void SettingsChangeGlobalError::Show() {
     ShowBubbleView(browser_);
 }
 
+void SettingsChangeGlobalError::OnInactiveTimeout() {
+  delegate_->OnDecisionTimeout();
+  RemoveFromProfile();
+}
+
 }  // namespace protector

chrome/browser/protector/settings_change_global_error.h

@@ -18,13 +18,15 @@ class Profile;
 
 namespace protector {
 
+class BaseSettingChange;
 class SettingsChangeGlobalErrorDelegate;
 
 // Global error about unwanted settings changes.
 class SettingsChangeGlobalError : public GlobalError {
  public:
-  // Creates new global error about setting changes |change| and takes
-  // ownership over it. Uses |delegate| to notify about user decision.
+  // Creates new global error about setting changes |change| which must not be
+  // deleted until |delegate->OnRemovedFromProfile| is called. Uses |delegate|
+  // to notify about user decision.
   SettingsChangeGlobalError(BaseSettingChange* change,
                             SettingsChangeGlobalErrorDelegate* delegate);
   virtual ~SettingsChangeGlobalError();
@@ -33,11 +35,12 @@ class SettingsChangeGlobalError : public GlobalError {
   // Can be called from any thread.
   void ShowForProfile(Profile* profile);
 
+  // Removes global error from its profile.
+  void RemoveFromProfile();
+
   // Browser that the bubble has been last time shown for.
   Browser* browser() const { return browser_; }
 
-  BaseSettingChange* mutable_change() { return change_.get(); }
-
   // GlobalError implementation.
   virtual bool HasBadge() OVERRIDE;
   virtual bool HasMenuItem() OVERRIDE;
@@ -61,11 +64,12 @@ class SettingsChangeGlobalError : public GlobalError {
   // Displays global error bubble. Must be called on the UI thread.
   void Show();
 
-  // Removes global error from its profile and deletes |this| later.
-  void RemoveFromProfile();
+  // Called when the wrench menu item has been displayed for enough time
+  // without user interaction.
+  void OnInactiveTimeout();
 
   // Change to show.
-  scoped_ptr<BaseSettingChange> change_;
+  BaseSettingChange* change_;
 
   // Delegate to notify about user actions.
   SettingsChangeGlobalErrorDelegate* delegate_;