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

chrome/browser/ui/webui/options/clear_browser_data_handler.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 "chrome/browser/ui/webui/options/clear_browser_data_handler.h"
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram_macros.h"
 #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"
 #include "chrome/browser/browsing_data/browsing_data_counter.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 #include "chrome/browser/browsing_data/cache_counter.h"
 #include "chrome/browser/browsing_data/history_counter.h"
 #include "chrome/browser/browsing_data/passwords_counter.h"
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/accelerator_utils.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/locale_settings.h"
 #include "components/google/core/browser/google_util.h"
 #include "content/public/browser/notification_details.h"
 #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 {
 
 const char kClearBrowsingDataLearnMoreUrl[] =
     "https://support.google.com/chrome/?p=settings_clear_browsing_data";
 
 bool AreCountersEnabled() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableClearBrowsingDataCounters);
@@ skipping 20 matching lines @@
   clear_plugin_lso_data_enabled_.Init(prefs::kClearPluginLSODataEnabled, prefs);
   pepper_flash_settings_enabled_.Init(prefs::kPepperFlashSettingsEnabled,
                                       prefs);
   allow_deleting_browser_history_.Init(
       prefs::kAllowDeletingBrowserHistory,
       prefs,
       base::Bind(&ClearBrowserDataHandler::OnBrowsingHistoryPrefChanged,
                  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()));
     if (sync_service_)
       sync_service_->AddObserver(this);
   }
 }
 
 void ClearBrowserDataHandler::InitializePage() {
   web_ui()->CallJavascriptFunction(
@@ skipping 209 matching lines @@
   web_ui()->CallJavascriptFunction("ClearBrowserDataOverlay.doneClearing");
 }
 
 void ClearBrowserDataHandler::OnBrowsingHistoryPrefChanged() {
   web_ui()->CallJavascriptFunction(
       "ClearBrowserDataOverlay.setAllowDeletingHistory",
       base::FundamentalValue(*allow_deleting_browser_history_));
 }
 
 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",
       base::StringValue(pref_name),
       base::StringValue(text));
 }
 
 void ClearBrowserDataHandler::OnStateChanged() {
   web_ui()->CallJavascriptFunction(
       "ClearBrowserDataOverlay.updateSyncWarning",
       base::FundamentalValue(sync_service_ && sync_service_->IsSyncActive()));
 }
 
 }  // namespace options