Protector strings and bubble/SettingsChange code refactoring.

chrome/browser/protector/default_search_provider_change.cc

Index: chrome/browser/protector/default_search_provider_change.cc
diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc
index 9075f4180f25ef521bb2adf4958d1f997d7787fb..9c2e88768ed1de0276f02406e354cde6e55bad2b 100644
--- a/chrome/browser/protector/default_search_provider_change.cc
+++ b/chrome/browser/protector/default_search_provider_change.cc
@@ -11,21 +11,33 @@
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/webdata/keyword_table.h"
 #include "chrome/common/url_constants.h"
+#include "grit/chromium_strings.h"
+#include "grit/generated_resources.h"
 #include "googleurl/src/gurl.h"
+#include "ui/base/l10n/l10n_util.h"
 
 namespace protector {
 
+namespace {
+
+// Maximum length of the search engine name to be displayed.
+const size_t kMaxDisplayedNameLength = 10;
+
+}

[whywhat, 2011/11/18 08:18:20]

// namespace

[Ivan Korotkov, 2011/11/18 10:44:25]

Done.

+
 class DefaultSearchProviderChange : public SettingChange {
  public:
   DefaultSearchProviderChange(const TemplateURL* old_url,
                               const TemplateURL* new_url);
 
   // SettingChange overrides:
-  virtual string16 GetOldSetting() const OVERRIDE;
-  virtual string16 GetNewSetting() const OVERRIDE;
-  virtual void Accept(Protector* protector) OVERRIDE;
-  virtual void Revert(Protector* protector) OVERRIDE;
-  virtual void DoDefault(Protector* protector) OVERRIDE;
+  virtual void Init(Protector* protector) OVERRIDE;
+  virtual void Apply(Protector* protector) OVERRIDE;
+  virtual void Discard(Protector* protector) OVERRIDE;
+  virtual string16 GetTitle() const OVERRIDE;
+  virtual string16 GetMessage() const OVERRIDE;
+  virtual string16 GetApplyButtonText() const OVERRIDE;
+  virtual string16 GetDiscardButtonText() const OVERRIDE;
 
  private:
   virtual ~DefaultSearchProviderChange();
@@ -34,10 +46,18 @@ class DefaultSearchProviderChange : public SettingChange {
   // guarding.
   void SetDefaultSearchProvider(Protector* protector, int64 id);
 
+  // Opens the Search engine settings page in a new tab.
+  void OpenSearchEngineSettings(Protector* protector);
+
   int64 old_id_;
   int64 new_id_;
+  // ID of the search engine that we fall back to if the backup is lost.
+  int64 fallback_id_;
   string16 old_name_;
   string16 new_name_;
+  // Name of the search engine that we fall back to if the backup is lost.
+  string16 fallback_name_;
+  string16 product_name_;
 
   DISALLOW_COPY_AND_ASSIGN(DefaultSearchProviderChange);
 };
@@ -45,12 +65,14 @@ class DefaultSearchProviderChange : public SettingChange {
 DefaultSearchProviderChange::DefaultSearchProviderChange(
     const TemplateURL* old_url,
     const TemplateURL* new_url)
-    : SettingChange(kSearchEngineChanged),
-      old_id_(0),
-      new_id_(0) {
-  DCHECK(new_url);
-  new_id_ = new_url->id();
-  new_name_ = new_url->short_name();
+    : old_id_(0),
+      new_id_(0),
+      fallback_id_(0),
+      product_name_(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME)) {
+  if (new_url) {
+    new_id_ = new_url->id();
+    new_name_ = new_url->short_name();
+  }
   if (old_url) {
     old_id_ = old_url->id();
     old_name_ = old_url->short_name();
@@ -60,33 +82,76 @@ DefaultSearchProviderChange::DefaultSearchProviderChange(
 DefaultSearchProviderChange::~DefaultSearchProviderChange() {
 }
 
-string16 DefaultSearchProviderChange::GetOldSetting() const {
-  return old_name_;
+void DefaultSearchProviderChange::Init(Protector* protector) {
+  // Initially reset the search engine to its previous setting.
+  SetDefaultSearchProvider(protector, old_id_);
+  // TODO(avayvod): Add histrogram.
 }
 
-string16 DefaultSearchProviderChange::GetNewSetting() const {
-  return new_name_;
+void DefaultSearchProviderChange::Apply(Protector* protector) {
+  // TODO(avayvod): Add histrogram.
+  if (!new_id_) {
+    // Open settings page in case the new setting is invalid.
+    OpenSearchEngineSettings(protector);
+  } else {
+    SetDefaultSearchProvider(protector, new_id_);
+  }
 }
 
-void DefaultSearchProviderChange::Accept(Protector* protector) {
-  SetDefaultSearchProvider(protector, new_id_);
+void DefaultSearchProviderChange::Discard(Protector* protector) {
   // TODO(avayvod): Add histrogram.
+  if (!old_id_) {
+    // Open settings page in case the old setting is invalid.
+    OpenSearchEngineSettings(protector);
+  }
+  // Nothing to do otherwise since we have already set the search engine
+  // to |old_id_| in |Init|.
 }
 
-void DefaultSearchProviderChange::Revert(Protector* protector) {
-  SetDefaultSearchProvider(protector, old_id_);
-  if (!old_id_) {
-    // Open settings page in case the original setting was lost.
-    protector->OpenTab(
-        GURL(std::string(chrome::kChromeUISettingsURL) +
-            chrome::kSearchEnginesSubPage));
+string16 DefaultSearchProviderChange::GetTitle() const {
+  return l10n_util::GetStringUTF16(IDS_SEARCH_ENGINE_CHANGE_TITLE);
+}
+
+string16 DefaultSearchProviderChange::GetMessage() const {
+  if (fallback_name_.empty())
+    return l10n_util::GetStringFUTF16(
+        IDS_SEARCH_ENGINE_CHANGE_MESSAGE, product_name_);
+  else
+    return l10n_util::GetStringFUTF16(
+        IDS_SEARCH_ENGINE_CHANGE_NO_BACKUP_MESSAGE,
+        product_name_, fallback_name_);
+}
+
+string16 DefaultSearchProviderChange::GetApplyButtonText() const {
+  if (new_id_) {
+    if (new_id_ == fallback_id_) {
+      // Old search engine is lost, the fallback search engine is the same as
+      // the new one so no need to show this button.
+      return string16();
+    }
+    if (new_name_.length() > kMaxDisplayedNameLength)
+      return l10n_util::GetStringUTF16(IDS_CHANGE_SEARCH_ENGINE_NO_NAME);
+    else
+      return l10n_util::GetStringFUTF16(IDS_CHANGE_SEARCH_ENGINE, new_name_);
+  } else if (old_id_) {
+    // New setting is lost, offer to go to settings.
+    return l10n_util::GetStringUTF16(IDS_SELECT_SEARCH_ENGINE);
+  } else {
+    // Both settings are lost: don't show this button.
+    return string16();
   }
-  // TODO(avayvod): Add histrogram.
 }
 
-void DefaultSearchProviderChange::DoDefault(Protector* protector) {
-  SetDefaultSearchProvider(protector, old_id_);
-  // TODO(avayvod): Add histrogram.
+string16 DefaultSearchProviderChange::GetDiscardButtonText() const {
+  if (old_id_) {
+    if (new_name_.length() > kMaxDisplayedNameLength)
+      return l10n_util::GetStringUTF16(IDS_KEEP_SETTING);
+    else
+      return l10n_util::GetStringFUTF16(IDS_KEEP_SEARCH_ENGINE, old_name_);
+  } else {
+    // Old setting is old, offer to go to settings.

[whywhat, 2011/11/18 08:18:20]

is old -> is lost

[Ivan Korotkov, 2011/11/18 10:44:25]

Done.

+    return l10n_util::GetStringUTF16(IDS_SELECT_SEARCH_ENGINE);
+  }
 }
 
 void DefaultSearchProviderChange::SetDefaultSearchProvider(
@@ -99,16 +164,30 @@ void DefaultSearchProviderChange::SetDefaultSearchProvider(
     return;
   }
   const TemplateURL* url = NULL;
-  const TemplateURLService::TemplateURLVector& urls =
-      url_service->GetTemplateURLs();
-  for (size_t i = 0; i < urls.size(); ++i)
-    if (urls[i]->id() == id) {
-      url = urls[i];
-      break;
+  if (id) {
+    const TemplateURLService::TemplateURLVector& urls =
+        url_service->GetTemplateURLs();
+    for (size_t i = 0; i < urls.size(); ++i) {
+      if (urls[i]->id() == id) {
+        url = urls[i];
+        break;
+      }
     }
-  if (!url)
+  } else {
     url = url_service->FindNewDefaultSearchProvider();
+    fallback_id_ = url->id();

[whywhat, 2011/11/18 08:18:20]

This code is logically part of initialization, not setting dsp to a value. I'd
prefer fallback_id_ and fallback_name_ being set explicitly in Init.

[Ivan Korotkov, 2011/11/18 10:44:25]

Makes sense.

+    fallback_name_ = url->short_name();
+  }
+  DCHECK(url);
   url_service->SetDefaultSearchProvider(url);
+  VLOG(1) << "Default search provider set to: " << url->short_name();
+}
+
+void DefaultSearchProviderChange::OpenSearchEngineSettings(
+    Protector* protector) {
+  protector->OpenTab(
+      GURL(std::string(chrome::kChromeUISettingsURL) +
+           chrome::kSearchEnginesSubPage));
 }
 
 SettingChange* CreateDefaultSearchProviderChange(