[protector] Homepage UI polishing.

chrome/browser/protector/homepage_change.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/basictypes.h"
 #include "base/metrics/histogram.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/protector/base_prefs_change.h"
 #include "chrome/browser/protector/histograms.h"
@@ skipping 31 matching lines @@
   virtual void Timeout() OVERRIDE;
   virtual int GetBadgeIconID() const OVERRIDE;
   virtual int GetMenuItemIconID() const OVERRIDE;
   virtual int GetBubbleIconID() const OVERRIDE;
   virtual string16 GetBubbleTitle() const OVERRIDE;
   virtual string16 GetBubbleMessage() const OVERRIDE;
   virtual string16 GetApplyButtonText() const OVERRIDE;
   virtual string16 GetDiscardButtonText() const OVERRIDE;
   virtual DisplayName GetApplyDisplayName() const OVERRIDE;
   virtual GURL GetNewSettingURL() const OVERRIDE;
-  virtual bool IsUserVisible() const OVERRIDE;
 
  private:
   virtual ~HomepageChange();
 
   const std::string new_homepage_;
   const std::string backup_homepage_;
   const bool new_homepage_is_ntp_;
   const bool backup_homepage_is_ntp_;
   const bool new_show_homepage_;
   const bool backup_show_homepage_;
@@ skipping 32 matching lines @@
   prefs->SetString(prefs::kHomePage, backup_homepage_);
   prefs->SetBoolean(prefs::kHomePageIsNewTabPage, backup_homepage_is_ntp_);
   prefs->SetBoolean(prefs::kShowHomeButton, backup_show_homepage_);
   DismissOnPrefChange(prefs::kHomePage);
   DismissOnPrefChange(prefs::kHomePageIsNewTabPage);
   DismissOnPrefChange(prefs::kShowHomeButton);
   return true;
 }
 
 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_);
   prefs->SetBoolean(prefs::kHomePageIsNewTabPage, new_homepage_is_ntp_);
   prefs->SetBoolean(prefs::kShowHomeButton, new_show_homepage_);
 }
 
 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 {
   // Icons are the same for homepage and startup settings.
   return IDR_HOMEPAGE_CHANGE_BADGE;
 }
 
 int HomepageChange::GetMenuItemIconID() const {
   return IDR_HOMEPAGE_CHANGE_MENU;
 }
 
 int HomepageChange::GetBubbleIconID() const {
   return IDR_HOMEPAGE_CHANGE_ALERT;
 }
 
 string16 HomepageChange::GetBubbleTitle() const {
   return l10n_util::GetStringUTF16(IDS_HOMEPAGE_CHANGE_TITLE);
 }
 
 string16 HomepageChange::GetBubbleMessage() const {
   return l10n_util::GetStringUTF16(IDS_HOMEPAGE_CHANGE_BUBBLE_MESSAGE);
 }
 
 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,
     bool actual_show_homepage,
     const std::string& backup_homepage,
     bool backup_homepage_is_ntp,
     bool backup_show_homepage) {
   return new HomepageChange(
       actual_homepage, actual_homepage_is_ntp, actual_show_homepage,
       backup_homepage, backup_homepage_is_ntp, backup_show_homepage);
 }
 
 }  // namespace protector