Move counters for passwords, history and autofill to components

components/browsing_data/core/counters/history_counter.cc

Index: components/browsing_data/core/counters/history_counter.cc
diff --git a/chrome/browser/browsing_data/history_counter.cc b/components/browsing_data/core/counters/history_counter.cc
similarity index 65%
rename from chrome/browser/browsing_data/history_counter.cc
rename to components/browsing_data/core/counters/history_counter.cc
index 1a6388f2215b7f44802ef673792ffca72126f693..87529d29f0a44fb3cf26108860959f96bbe766da 100644
--- a/chrome/browser/browsing_data/history_counter.cc
+++ b/components/browsing_data/core/counters/history_counter.cc
@@ -2,57 +2,59 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/browsing_data/history_counter.h"
+#include "components/browsing_data/core/counters/history_counter.h"
 
 #include <limits.h>
 #include <stdint.h>
 
 #include "base/memory/ptr_util.h"
 #include "base/timer/timer.h"
-#include "chrome/browser/history/history_service_factory.h"
-#include "chrome/browser/history/web_history_service_factory.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/browser_sync/browser/profile_sync_service.h"
 #include "components/browsing_data/core/pref_names.h"
-#include "components/history/core/browser/history_service.h"
-#include "components/history/core/browser/web_history_service.h"
-#include "content/public/browser/browser_thread.h"
 
 namespace {
 static const int64_t kWebHistoryTimeoutSeconds = 10;
 }
 
-HistoryCounter::HistoryCounter(Profile* profile)
-    : BrowsingDataCounter(browsing_data::prefs::kDeleteBrowsingHistory),
-      profile_(profile),
+namespace browsing_data {
+
+HistoryCounter::HistoryCounter(
+    ProfileSyncService* profile_sync_service,

[msramek, 2016/07/20 13:41:16]

nit: The ordering here is strange. In the order of simplicity and relevance to
this counter, I would use:
1. HistoryService
2. WebHistoryService
3. ProfileSyncService

[ioanap, 2016/07/20 17:50:36]

Done.

+    const GetUpdatedWebHistoryServiceCallback& callback,
+    history::HistoryService* history_service)
+    : pref_name_(browsing_data::prefs::kDeleteBrowsingHistory),
+      profile_sync_service_(profile_sync_service),
+      web_history_service_callback_(callback),
+      history_service_(history_service),
       has_synced_visits_(false),
       local_counting_finished_(false),
       web_counting_finished_(false),
-      testing_web_history_service_(nullptr),
-      sync_service_(nullptr),
       history_sync_enabled_(false),
       weak_ptr_factory_(this) {}
 
 HistoryCounter::~HistoryCounter() {
-  if (sync_service_)
-    sync_service_->RemoveObserver(this);
+  if (profile_sync_service_)

[msramek, 2016/07/20 13:41:16]

This is an unnecessary change, since we don't need to distinguish between
profile- and other sync services here...

[ioanap, 2016/07/20 17:50:36]

Done.

+    profile_sync_service_->RemoveObserver(this);
 }
 
 void HistoryCounter::OnInitialized() {
-  sync_service_ = ProfileSyncServiceFactory::GetForProfile(profile_);
-  if (sync_service_)
-    sync_service_->AddObserver(this);
-  history_sync_enabled_ = !!WebHistoryServiceFactory::GetForProfile(profile_);
+  if (profile_sync_service_)
+    profile_sync_service_->AddObserver(this);
+  history_sync_enabled_ = IsHistorySyncEnabled();
 }
 
 bool HistoryCounter::HasTrackedTasks() {
   return cancelable_task_tracker_.HasTrackedTasks();
 }
 
-void HistoryCounter::SetWebHistoryServiceForTesting(
-    history::WebHistoryService* service) {
-  testing_web_history_service_ = service;
+const std::string& HistoryCounter::GetPrefName() const {
+  return pref_name_;
+}
+
+bool HistoryCounter::IsHistorySyncEnabled() {
+  return profile_sync_service_ && profile_sync_service_->IsSyncActive() &&

[msramek, 2016/07/20 13:41:16]

The semantics here are that history sync is enabled iff a web history service
exists. Of course, this has the same meaning, but it's a very loose coupling. If
the conditions in WebHistoryFactory changed, this might lead to unexpected
behavior.

I think that in the current setup, we should simply test whether or not the
callback returns null.

[ioanap, 2016/07/20 17:50:37]

I see... I added this before I had the callback in place. I changed it now to
check the return value of the callback.

Done.

+         profile_sync_service_->GetActiveDataTypes().Has(
+             syncer::HISTORY_DELETE_DIRECTIVES);
 }
 
 void HistoryCounter::Count() {
@@ -64,22 +66,14 @@ void HistoryCounter::Count() {
   // Count the locally stored items.
   local_counting_finished_ = false;
 
-  history::HistoryService* service = HistoryServiceFactory::GetForProfile(
-      profile_, ServiceAccessType::EXPLICIT_ACCESS);
-
-  service->GetHistoryCount(
-      GetPeriodStart(),
-      base::Time::Max(),
+  history_service_->GetHistoryCount(
+      GetPeriodStart(), base::Time::Max(),
       base::Bind(&HistoryCounter::OnGetLocalHistoryCount,
                  weak_ptr_factory_.GetWeakPtr()),
       &cancelable_task_tracker_);
 
   // If the history sync is enabled, test if there is at least one synced item.
-  // If the testing web history service is present, use that one instead.
-  history::WebHistoryService* web_history =
-      testing_web_history_service_
-          ? testing_web_history_service_
-          : WebHistoryServiceFactory::GetForProfile(profile_);
+  history::WebHistoryService* web_history = web_history_service_callback_.Run();
 
   if (!web_history) {
     web_counting_finished_ = true;
@@ -89,9 +83,7 @@ void HistoryCounter::Count() {
   web_counting_finished_ = false;
 
   web_history_timeout_.Start(
-      FROM_HERE,
-      base::TimeDelta::FromSeconds(kWebHistoryTimeoutSeconds),
-      this,
+      FROM_HERE, base::TimeDelta::FromSeconds(kWebHistoryTimeoutSeconds), this,
       &HistoryCounter::OnWebHistoryTimeout);
 
   history::QueryOptions options;
@@ -99,8 +91,7 @@ void HistoryCounter::Count() {
   options.begin_time = GetPeriodStart();
   options.end_time = base::Time::Max();
   web_history_request_ = web_history->QueryHistory(
-      base::string16(),
-      options,
+      base::string16(), options,
       base::Bind(&HistoryCounter::OnGetWebHistoryCount,
                  weak_ptr_factory_.GetWeakPtr()));
 
@@ -141,8 +132,7 @@ void HistoryCounter::OnGetWebHistoryCount(
   // entry in the "event" list.
   const base::ListValue* events;
   has_synced_visits_ =
-      !result ||
-      (result->GetList("event", &events) && !events->empty());
+      !result || (result->GetList("event", &events) && !events->empty());
   web_counting_finished_ = true;
   MergeResults();
 }
@@ -168,20 +158,15 @@ void HistoryCounter::MergeResults() {
       new HistoryResult(this, local_result_, has_synced_visits_)));
 }
 
-HistoryCounter::HistoryResult::HistoryResult(
-    const HistoryCounter* source,
-    ResultInt value,
-    bool has_synced_visits)
-    : FinishedResult(source, value),
-      has_synced_visits_(has_synced_visits) {
-}
+HistoryCounter::HistoryResult::HistoryResult(const HistoryCounter* source,
+                                             ResultInt value,
+                                             bool has_synced_visits)
+    : FinishedResult(source, value), has_synced_visits_(has_synced_visits) {}
 
-HistoryCounter::HistoryResult::~HistoryResult() {
-}
+HistoryCounter::HistoryResult::~HistoryResult() {}
 
 void HistoryCounter::OnStateChanged() {
-  bool history_sync_enabled_new_state =
-      !!WebHistoryServiceFactory::GetForProfile(profile_);
+  bool history_sync_enabled_new_state = IsHistorySyncEnabled();
 
   // If the history sync was just enabled or disabled, restart the counter
   // so that we update the result accordingly.
@@ -190,3 +175,5 @@ void HistoryCounter::OnStateChanged() {
     Restart();
   }
 }
+
+}  // namespace browsing_data