Polish the result communication and display of the browsing data counters.

chrome/browser/ui/webui/options/clear_browser_data_handler.cc

Index: chrome/browser/ui/webui/options/clear_browser_data_handler.cc
diff --git a/chrome/browser/ui/webui/options/clear_browser_data_handler.cc b/chrome/browser/ui/webui/options/clear_browser_data_handler.cc
index 2c2dac59712734c3804d3209018e84901bb6efc4..f22f33c4cbc85b3e0243de2e06adf4f014a69bf9 100644
--- a/chrome/browser/ui/webui/options/clear_browser_data_handler.cc
+++ b/chrome/browser/ui/webui/options/clear_browser_data_handler.cc
@@ -12,6 +12,7 @@
 #include "base/metrics/sparse_histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
@@ -34,6 +35,7 @@
 #include "content/public/browser/web_ui.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/text/bytes_formatting.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 
 namespace {
@@ -74,11 +76,9 @@ void ClearBrowserDataHandler::InitializeHandler() {
                  base::Unretained(this)));
 
   if (AreCountersEnabled()) {
-    AddCounter(make_scoped_ptr(new PasswordsCounter()),
-               IDS_DEL_PASSWORDS_COUNTER);
-    AddCounter(make_scoped_ptr(new HistoryCounter()),
-               IDS_DEL_BROWSING_HISTORY_COUNTER);
-    AddCounter(make_scoped_ptr(new CacheCounter()), IDS_DEL_CACHE_COUNTER);
+    AddCounter(make_scoped_ptr(new PasswordsCounter()));
+    AddCounter(make_scoped_ptr(new HistoryCounter()));
+    AddCounter(make_scoped_ptr(new CacheCounter()));
 
     sync_service_ =
         ProfileSyncServiceFactory::GetForProfile(Profile::FromWebUI(web_ui()));
@@ -308,27 +308,77 @@ void ClearBrowserDataHandler::OnBrowsingHistoryPrefChanged() {
 }
 
 void ClearBrowserDataHandler::AddCounter(
-    scoped_ptr<BrowsingDataCounter> counter, int text_grd_id) {
+    scoped_ptr<BrowsingDataCounter> counter) {
   DCHECK(AreCountersEnabled());
 
   counter->Init(
       Profile::FromWebUI(web_ui()),
       base::Bind(&ClearBrowserDataHandler::UpdateCounterText,
-                 base::Unretained(this),
-                 counter->GetPrefName(),
-                 text_grd_id));
+                 base::Unretained(this)));
   counters_.push_back(counter.Pass());
 }
 
 void ClearBrowserDataHandler::UpdateCounterText(
-    const std::string& pref_name,
-    int text_grd_id,
-    bool finished,
-    BrowsingDataCounter::ResultInt count) {
+    scoped_ptr<BrowsingDataCounter::Result> result) {
   DCHECK(AreCountersEnabled());
-  base::string16 text = finished
-      ? l10n_util::GetPluralStringFUTF16(text_grd_id, count)
-      : l10n_util::GetStringUTF16(IDS_CLEAR_BROWSING_DATA_CALCULATING);
+  base::string16 text;
+  std::string pref_name = result->source()->GetPrefName();
+
+  if (!result->finished()) {
+    // The counter is still counting.
+    text = l10n_util::GetStringUTF16(IDS_CLEAR_BROWSING_DATA_CALCULATING);
+
+  } else if (pref_name == prefs::kDeletePasswords) {
+    // Passwords counter.
+    BrowsingDataCounter::ResultInt value = result->value();
+    text = value > 0
+        ? l10n_util::GetPluralStringFUTF16(IDS_DEL_PASSWORDS_COUNTER, value)
+        : l10n_util::GetStringUTF16(IDS_DEL_PASSWORDS_COUNTER_EMPTY);
+
+  } else if (pref_name == prefs::kDeleteCache) {
+    // Cache counter.
+    BrowsingDataCounter::ResultInt value = result->value();

[Bernhard Bauer, 2015/11/02 16:15:27]

Name this variable so that it reflects what the value means, i.e. something like
|cacheSizeBytes|.

[msramek, 2015/11/02 20:54:06]

Done. Here and elsewhere.

+
+    PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs();
+    BrowsingDataRemover::TimePeriod time_period =
+        static_cast<BrowsingDataRemover::TimePeriod>(
+            prefs->GetInteger(prefs::kDeleteTimePeriod));
+
+    // Three cases: Nonzero result for the entire cache, nonzero result for
+    // a subset of cache (i.e. a finite time interval), and almost zero (< 1MB).
+    static const int kBInMB = 1024 * 1024;

[Bernhard Bauer, 2015/11/02 16:15:27]

The name of this variable isn't really right. This is one megabyte in bytes, no?
Then, in keeping with the rule of naming quantities prefixed with their unit, it
would be kMegabyteBytes.

[msramek, 2015/11/02 20:54:06]

Hmm, haven't heard of that. In fact,
https://google.github.io/styleguide/cppguide.html#Constant_Names states
kDaysInAWeek, so I would keep kBytesInMegabyte, perhaps just expanding the units
to words.

[Bernhard Bauer, 2015/11/03 10:11:19]

Hm, I can't find any explicit mention in the style guide, but we definitely do
that (for all variables, not just constants). But writing things out is
definitely descriptive enough too. (Technically it should be kBytesInAMegabyte
though, no? 😃)

[msramek, 2015/11/03 10:55:45]

Yes :-) I wanted to be concise (although it's just one letter), but looking at
other constants throughout Chrome, they're actually quite verbose. So let's make
it at least sound properly and add "a".

+    if (value >= kBInMB) {
+      base::string16 formatted_size = ui::FormatBytes(value);
+      text = time_period == BrowsingDataRemover::EVERYTHING
+          ? formatted_size
+          : l10n_util::GetStringFUTF16(IDS_DEL_CACHE_COUNTER_UPPER_ESTIMATE,
+                                       formatted_size);
+    } else {
+      text = l10n_util::GetStringUTF16(IDS_DEL_CACHE_COUNTER_ALMOST_EMPTY);
+    }
+
+  } else if (pref_name == prefs::kDeleteBrowsingHistory) {
+    // History counter.
+    HistoryCounter::HistoryResult* history_result =
+        static_cast<HistoryCounter::HistoryResult*>(result.get());
+    BrowsingDataCounter::ResultInt value = history_result->value();
+    bool has_synced_visits = history_result->has_synced_visits();
+
+    // Four cases: [zero|nonzero] [local|web] result.
+    if (value > 0) {
+      text = has_synced_visits
+          ? l10n_util::GetPluralStringFUTF16(
+                IDS_DEL_BROWSING_HISTORY_COUNTER_SYNCED, value)
+          : l10n_util::GetPluralStringFUTF16(
+                IDS_DEL_BROWSING_HISTORY_COUNTER, value);
+    } else {
+      text = has_synced_visits
+          ? l10n_util::GetStringUTF16(
+                IDS_DEL_BROWSING_HISTORY_COUNTER_SYNCED_ONLY)
+          : l10n_util::GetStringUTF16(
+                IDS_DEL_BROWSING_HISTORY_COUNTER_EMPTY);
+    }
+  }
 
   web_ui()->CallJavascriptFunction(
       "ClearBrowserDataOverlay.updateCounter",