[protector] Homepage UI polishing.

chrome/browser/protector/homepage_change.cc

Index: chrome/browser/protector/homepage_change.cc
diff --git a/chrome/browser/protector/homepage_change.cc b/chrome/browser/protector/homepage_change.cc
index 1da8f9a15e8b5ce4cf65be2e2bab168fa3b46c1d..84f1878d0df4b8673d5607607e48babf9ed27f74 100644
--- a/chrome/browser/protector/homepage_change.cc
+++ b/chrome/browser/protector/homepage_change.cc
@@ -49,7 +49,6 @@ class HomepageChange : public BasePrefsChange {
   virtual string16 GetDiscardButtonText() const OVERRIDE;
   virtual DisplayName GetApplyDisplayName() const OVERRIDE;
   virtual GURL GetNewSettingURL() const OVERRIDE;
-  virtual bool IsUserVisible() const OVERRIDE;
 
  private:
   virtual ~HomepageChange();
@@ -102,14 +101,10 @@ bool HomepageChange::Init(Profile* profile) {
 }
 
 void HomepageChange::Apply(Browser* browser) {
-  if (IsUserVisible()) {
-    // Don't report statistics if this change was applied as part of a composite
-    // change and is not user-visible.
-    UMA_HISTOGRAM_ENUMERATION(
-        kProtectorHistogramHomepageApplied,
-        new_homepage_type_,
-        HOMEPAGE_TYPE_COUNT);
-  }
+  UMA_HISTOGRAM_ENUMERATION(
+      kProtectorHistogramHomepageApplied,
+      new_homepage_type_,
+      HOMEPAGE_TYPE_COUNT);
   IgnorePrefChanges();
   PrefService* prefs = profile()->GetPrefs();
   prefs->SetString(prefs::kHomePage, new_homepage_);
@@ -118,23 +113,19 @@ void HomepageChange::Apply(Browser* browser) {
 }
 
 void HomepageChange::Discard(Browser* browser) {
-  if (IsUserVisible()) {
-    UMA_HISTOGRAM_ENUMERATION(
-        kProtectorHistogramHomepageDiscarded,
-        new_homepage_type_,
-        HOMEPAGE_TYPE_COUNT);
-  }
+  UMA_HISTOGRAM_ENUMERATION(
+      kProtectorHistogramHomepageDiscarded,
+      new_homepage_type_,
+      HOMEPAGE_TYPE_COUNT);
   IgnorePrefChanges();
   // Nothing to do here since backup has already been made active by Init().
 }
 
 void HomepageChange::Timeout() {
-  if (IsUserVisible()) {
-    UMA_HISTOGRAM_ENUMERATION(
-        kProtectorHistogramHomepageTimeout,
-        new_homepage_type_,
-        HOMEPAGE_TYPE_COUNT);
-  }
+  UMA_HISTOGRAM_ENUMERATION(
+      kProtectorHistogramHomepageTimeout,
+      new_homepage_type_,
+      HOMEPAGE_TYPE_COUNT);
 }
 
 int HomepageChange::GetBadgeIconID() const {
@@ -159,36 +150,41 @@ string16 HomepageChange::GetBubbleMessage() const {
 }
 
 string16 HomepageChange::GetApplyButtonText() const {
-  return new_homepage_is_ntp_ ?
+  GURL homepage_url(GetNewSettingURL());
+  return homepage_url.is_empty() ?
       l10n_util::GetStringUTF16(IDS_CHANGE_HOMEPAGE_NTP) :
       l10n_util::GetStringFUTF16(IDS_CHANGE_HOMEPAGE,
-                                 UTF8ToUTF16(GURL(new_homepage_).host()));
+                                 UTF8ToUTF16(homepage_url.host()));
 }
 
 string16 HomepageChange::GetDiscardButtonText() const {
-  return backup_homepage_is_ntp_ ?
+  GURL new_homepage_url(GetNewSettingURL());
+  GURL backup_homepage_url;
+  if (!backup_homepage_is_ntp_)
+    backup_homepage_url = GURL(backup_homepage_);
+  if (backup_homepage_url.host() == new_homepage_url.host()) {
+    // Display a generic string if old setting looks the same as the backup (for

[whywhat, 2012/04/29 16:24:15]

did you mean new setting by old setting?
won't it still be [ Keep settings ] [ Change to Google ] in the case of false
positive and still confusing to users?

[Ivan Korotkov, 2012/05/02 11:17:26]

Yep, typo.
It's not a false positive, just the case when not the homepage URL but something
other (the 'show button' checkbox) has changed and we decided to show the bubble
anyway in that case.

[whywhat, 2012/05/02 11:23:48]

Anyway, sounds confusing to me. Imagine that I had Google as my homepage, then
something changed my settings and I'm asked if I want to keep settings (which
was Google) or change it to Google. I'm asking myself what was it if not Google.
Not to mention, that I think "Keep setting" could be read as "Keep [new]
setting" so I'd click on Change to Google.

[Ivan Korotkov, 2012/05/02 11:29:40]

The string is "Keep my settings" to make it clearer. I agree  with you it's not
informative but, as, you know, we want to simplify these bubbles as much as
possible...
It's a rare event anyway that something external would change the 'show homepage
button' but not the homepage itself :)

+    // example, when homepage hasn't changed but 'show homepage button' has).
+    return l10n_util::GetStringUTF16(IDS_KEEP_SETTING);
+  }
+  return backup_homepage_url.is_empty() ?
       l10n_util::GetStringUTF16(IDS_KEEP_HOMEPAGE_NTP) :
       l10n_util::GetStringFUTF16(IDS_KEEP_HOMEPAGE,
-                                 UTF8ToUTF16(GURL(backup_homepage_).host()));
+                                 UTF8ToUTF16(backup_homepage_url.host()));
 }
 
 BaseSettingChange::DisplayName HomepageChange::GetApplyDisplayName() const {
-  return new_homepage_is_ntp_ ?
+  GURL homepage_url(GetNewSettingURL());
+  return homepage_url.is_empty() ?
       DisplayName(kDefaultNamePriority, string16()) :
       DisplayName(kHomepageChangeNamePriority,
-                  UTF8ToUTF16(GURL(new_homepage_).host()));
+                  UTF8ToUTF16(homepage_url.host()));
 }
 
 GURL HomepageChange::GetNewSettingURL() const {
   return new_homepage_is_ntp_ ? GURL() : GURL(new_homepage_);
 }
 
-bool HomepageChange::IsUserVisible() const {
-  // Should not be presented to user unless the homepage button was previously
-  // visible or has been made visible by this change.
-  return new_show_homepage_ || backup_show_homepage_;
-}
-
 BaseSettingChange* CreateHomepageChange(
     const std::string& actual_homepage,
     bool actual_homepage_is_ntp,