Implement important sites dialog for desktop.

chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc

Index: chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
diff --git a/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc b/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
index dd8d078d7892d8d5eb4838d738fec4e5e7f584a2..1f9ac6e14a59a20b6adaee6fb8c5c35222b8b821 100644
--- a/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
+++ b/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
@@ -5,18 +5,24 @@
 #include "chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h"
 
 #include <stddef.h>
+#include <vector>
 
+#include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/values.h"
 #include "chrome/browser/browsing_data/browsing_data_counter_factory.h"
 #include "chrome/browser/browsing_data/browsing_data_counter_utils.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
+#include "chrome/browser/browsing_data/browsing_data_important_sites_util.h"
 #include "chrome/browser/browsing_data/browsing_data_remover_factory.h"
+#include "chrome/browser/engagement/important_sites_util.h"
 #include "chrome/browser/history/web_history_service_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/common/channel_info.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
 #include "components/browsing_data/core/history_notice_utils.h"
 #include "components/browsing_data/core/pref_names.h"
@@ -27,6 +33,8 @@ namespace {
 
 const int kMaxTimesHistoryNoticeShown = 1;
 
+const int kMaxImportantSites = 10;
+
 // TODO(msramek): Get the list of deletion preferences from the JS side.
 const char* kCounterPrefs[] = {
   browsing_data::prefs::kDeleteBrowsingHistory,
@@ -38,6 +46,15 @@ const char* kCounterPrefs[] = {
   browsing_data::prefs::kDeletePasswords,
 };
 
+const char* REGISTERABLE_DOMAINS_FIELD = "registerableDomain";
+const char* REASON_BITFIELD_FIELD = "reasonBitfield";
+const char* EXAMPLE_ORIGIN_FIELD = "exampleOrigin";
+const char* DIALOG_DISABLED_FIELD = "dialogDisabled";
+const char* IMPORTANT_SITES_FIELD = "importantSites";
+const char* IS_CHECKED_FIELD = "isChecked";
+const char* STORAGE_SIZE_FIELD = "storageSize";
+const char* HAS_NOTIFICATIONS_FIELD = "hasNotifications";
+
 } // namespace
 
 namespace settings {
@@ -47,7 +64,9 @@ namespace settings {
 class ClearBrowsingDataHandler::TaskObserver
     : public BrowsingDataRemover::Observer {
  public:
-  TaskObserver(BrowsingDataRemover* remover, const base::Closure& callback);
+  TaskObserver(BrowsingDataRemover* remover,
+               const base::Closure& callback,
+               int task_count);
   ~TaskObserver() override;
 
   void OnBrowsingDataRemoverDone() override;
@@ -56,20 +75,25 @@ class ClearBrowsingDataHandler::TaskObserver
   base::Closure callback_;
   ScopedObserver<BrowsingDataRemover, BrowsingDataRemover::Observer>
       remover_observer_;
+  int task_count_;
 
   DISALLOW_COPY_AND_ASSIGN(TaskObserver);
 };
 
 ClearBrowsingDataHandler::TaskObserver::TaskObserver(
-    BrowsingDataRemover* remover, const base::Closure& callback)
-    : callback_(callback),
-      remover_observer_(this) {
+    BrowsingDataRemover* remover,
+    const base::Closure& callback,
+    int task_count)
+    : callback_(callback), remover_observer_(this), task_count_(task_count) {
   remover_observer_.Add(remover);
 }
 
 ClearBrowsingDataHandler::TaskObserver::~TaskObserver() {}
 
 void ClearBrowsingDataHandler::TaskObserver::OnBrowsingDataRemoverDone() {
+  DCHECK(task_count_);
+  if (--task_count_)
+    return;
   remover_observer_.RemoveAll();
   callback_.Run();
 }
@@ -94,6 +118,11 @@ void ClearBrowsingDataHandler::RegisterMessages() {
                  base::Unretained(this)));
 
   web_ui()->RegisterMessageCallback(
+      "fetchImportantSites",
+      base::Bind(&ClearBrowsingDataHandler::HandleFetchImportantSites,
+                 base::Unretained(this)));
+
+  web_ui()->RegisterMessageCallback(
       "initializeClearBrowsingData",
       base::Bind(&ClearBrowsingDataHandler::HandleInitialize,
                  base::Unretained(this)));
@@ -197,22 +226,52 @@ void ClearBrowsingDataHandler::HandleClearBrowsingData(
       prefs->GetInteger(browsing_data::prefs::kDeleteTimePeriod);
 
   std::string webui_callback_id;
-  CHECK_EQ(1U, args->GetSize());
+  CHECK_EQ(2U, args->GetSize());
   CHECK(args->GetString(0, &webui_callback_id));
 
+  std::vector<std::string> excluding_domains;
+  std::vector<int32_t> excluding_domain_reasons;
+  std::vector<std::string> ignoring_domains;
+  std::vector<int32_t> ignoring_domain_reasons;
+  const base::ListValue* important_sites = nullptr;
+  CHECK(args->GetList(1, &important_sites));
+  for (const auto& item : *important_sites) {
+    const base::DictionaryValue* site = nullptr;
+    CHECK(item->GetAsDictionary(&site));
+    bool is_checked = false;
+    CHECK(site->GetBoolean(IS_CHECKED_FIELD, &is_checked));
+    std::string domain;
+    int domain_reason = -1;
+    CHECK(site->GetString(REGISTERABLE_DOMAINS_FIELD, &domain));
+    CHECK(site->GetInteger(REASON_BITFIELD_FIELD, &domain_reason));
+    if (is_checked) {  // Selected important sites should be deleted.
+      ignoring_domains.push_back(domain);

[dmurph, 2017/03/06 21:16:48]

Sorry, language here is bad. Excluding means excluding from the clearing -
ignoring means we aren't excluding, but we record stats on the items we don't
choose - (but in the UI they are 'chosen'). So it should be opposite.

Feel free to change the argument names / documentation to avoid this confusion
in the future.

[dullweber, 2017/03/07 12:24:35]

Yes the naming is a bit confusing but I think the code is already doing the
right thing?
When a checkbox is checked we want to delete the data for this site?
All domains where the checkbox is not checked are put into excluding_domains and
this vector is then passed to the filter as a blacklist. 

[dmurph, 2017/03/07 21:20:13]

Ah, yeah, you're right, I messed myself up. Checked means we are deleting.

+      ignoring_domain_reasons.push_back(domain_reason);
+    } else {  // Unselected sites should be kept.
+      excluding_domains.push_back(domain);
+      excluding_domain_reasons.push_back(domain_reason);
+    }
+  }
+
+  if (!excluding_domains.empty() || !ignoring_domains.empty()) {
+    ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites(
+        profile_->GetOriginalProfile(), excluding_domains,
+        excluding_domain_reasons, ignoring_domains, ignoring_domain_reasons);
+  }
+
   BrowsingDataRemover* remover =
       BrowsingDataRemoverFactory::GetForBrowserContext(profile_);
   task_observer_ = base::MakeUnique<TaskObserver>(
       remover,
       base::Bind(&ClearBrowsingDataHandler::OnClearingTaskFinished,
-                 base::Unretained(this), webui_callback_id));
+                 base::Unretained(this), webui_callback_id),
+      /*task_count=*/2);
   browsing_data::TimePeriod time_period =
       static_cast<browsing_data::TimePeriod>(period_selected);
-  browsing_data::RecordDeletionForPeriod(time_period);
-  remover->RemoveAndReply(
-      browsing_data::CalculateBeginDeleteTime(time_period),
-      browsing_data::CalculateEndDeleteTime(time_period),
-      remove_mask, origin_mask, task_observer_.get());
+
+  browsing_data_important_sites_util::Remove(remove_mask, origin_mask,
+                                             time_period, excluding_domains,
+                                             remover, task_observer_.get());
 }
 
 void ClearBrowsingDataHandler::OnClearingTaskFinished(
@@ -246,6 +305,43 @@ void ClearBrowsingDataHandler::OnClearingTaskFinished(
   task_observer_.reset();
 }
 
+void ClearBrowsingDataHandler::HandleFetchImportantSites(
+    const base::ListValue* args) {
+  AllowJavascript();
+  const base::Value* callback_id;
+  CHECK(args->Get(0, &callback_id));
+
+  Profile* profile = profile_->GetOriginalProfile();
+  bool dialog_disabled =
+      ImportantSitesUtil::IsDialogDisabled(profile) ||
+      !base::FeatureList::IsEnabled(features::kImportantSitesInCBD);
+  auto important_sites_list = base::MakeUnique<base::ListValue>();
+
+  if (!dialog_disabled) {
+    std::vector<ImportantSitesUtil::ImportantDomainInfo> important_sites =
+        ImportantSitesUtil::GetImportantRegisterableDomains(profile,
+                                                            kMaxImportantSites);
+    for (const auto& info : important_sites) {
+      auto entry = base::MakeUnique<base::DictionaryValue>();
+      entry->SetString(REGISTERABLE_DOMAINS_FIELD, info.registerable_domain);
+      entry->SetInteger(REASON_BITFIELD_FIELD, info.reason_bitfield);

[dmurph, 2017/03/06 21:16:48]

You can extract reasons from the reason bitfield to know exactly why it was
chosen. If you do, please move that enum (in the important_sites_util.cc file)
to the .h file so we know to maintain those reasons.

[dullweber, 2017/03/07 12:24:35]

Moving the enum would change it from ImportantReason to
ImportantSitesUtil::ImportantReason and the functions in the anonymous namespace
are not happy about this. I added a static function to check if a
ImportantDomainInfo has notifications.

[dmurph, 2017/03/07 21:20:13]

Really? Even if you change the declarations or do a 'using ImportantReason =
ImportantSitesUtil::ImportantReason;' at the top of the cc file? You can declare
all of the enum values in the .h file and stuff (probably enum class it). My
brain thinks this should work but I may be wrong.

[dullweber, 2017/03/13 14:53:24]

That works, thanks! I don't think enum class is a good idea because it is used
for bitwise operations all the time.

+      entry->SetString(EXAMPLE_ORIGIN_FIELD, info.example_origin.spec());
+      // Initially all sites are selected for deletion.
+      entry->SetBoolean(IS_CHECKED_FIELD, true);
+      // TODO(dullweber): Get size and notification status.
+      entry->SetInteger(STORAGE_SIZE_FIELD, 0);
+      entry->SetBoolean(HAS_NOTIFICATIONS_FIELD, true);
+      important_sites_list->Append(std::move(entry));
+    }
+  }
+
+  std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
+  result->SetBoolean(DIALOG_DISABLED_FIELD, dialog_disabled);
+  result->Set(IMPORTANT_SITES_FIELD, std::move(important_sites_list));
+
+  ResolveJavascriptCallback(*callback_id, *result);
+}
+
 void ClearBrowsingDataHandler::HandleInitialize(const base::ListValue* args) {
   AllowJavascript();
   const base::Value* callback_id;