Move counters for passwords, history and autofill to components

chrome/browser/browsing_data/history_counter_browsertest.cc

Index: chrome/browser/browsing_data/history_counter_browsertest.cc
diff --git a/chrome/browser/browsing_data/history_counter_browsertest.cc b/chrome/browser/browsing_data/history_counter_browsertest.cc
index 1366efa53c8c271f3a1a7c5ae0b66273d79cd372..beb5519bbeb98b51f1ce03d9f3513510578cd461 100644
--- a/chrome/browser/browsing_data/history_counter_browsertest.cc
+++ b/chrome/browser/browsing_data/history_counter_browsertest.cc
@@ -2,13 +2,16 @@
 // 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 "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/run_loop.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/web_history_service_factory.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
 #include "chrome/browser/ui/browser.h"
 #include "components/browser_sync/browser/profile_sync_service.h"
@@ -38,14 +41,19 @@ class HistoryCounterTest : public SyncTest {
 
   void SetUpOnMainThread() override {
     time_ = base::Time::Now();
-    service_ = HistoryServiceFactory::GetForProfileWithoutCreating(
+    history_service_ = HistoryServiceFactory::GetForProfileWithoutCreating(
         browser()->profile());
+    fake_web_history_service_.reset(new history::FakeWebHistoryService(
+        ProfileOAuth2TokenServiceFactory::GetForProfile(browser()->profile()),
+        SigninManagerFactory::GetForProfile(browser()->profile()),
+        browser()->profile()->GetRequestContext()));
+
     SetHistoryDeletionPref(true);
     SetDeletionPeriodPref(browsing_data::EVERYTHING);
   }
 
   void AddVisit(const std::string url) {
-    service_->AddPage(GURL(url), time_, history::SOURCE_BROWSED);
+    history_service_->AddPage(GURL(url), time_, history::SOURCE_BROWSED);
   }
 
   const base::Time& GetCurrentTime() {
@@ -86,8 +94,9 @@ class HistoryCounterTest : public SyncTest {
     finished_ = result->Finished();
 
     if (finished_) {
-      HistoryCounter::HistoryResult* history_result =
-          static_cast<HistoryCounter::HistoryResult*>(result.get());
+      browsing_data::HistoryCounter::HistoryResult* history_result =
+          static_cast<browsing_data::HistoryCounter::HistoryResult*>(
+              result.get());
 
       local_result_ = history_result->Value();
       has_synced_visits_ = history_result->has_synced_visits();
@@ -111,9 +120,20 @@ class HistoryCounterTest : public SyncTest {
     CountingFinishedSinceLastAsked();
   }
 
+  history::WebHistoryService* GetFakeWebHistoryService() {
+    return fake_web_history_service_.get();
+  }
+
+  history::WebHistoryService* GetRealWebHistoryService(Profile* profile) {
+    return WebHistoryServiceFactory::GetForProfile(profile);
+  }
+
+  history::HistoryService* GetHistoryService() { return history_service_; }
+
  private:
   std::unique_ptr<base::RunLoop> run_loop_;
-  history::HistoryService* service_;
+  history::HistoryService* history_service_;
+  std::unique_ptr<history::FakeWebHistoryService> fake_web_history_service_;
   base::Time time_;
 
   bool finished_;
@@ -147,7 +167,12 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, DuplicateVisits) {
 
   Profile* profile = browser()->profile();
 
-  HistoryCounter counter(profile);
+  browsing_data::HistoryCounter counter(
+      ProfileSyncServiceFactory::GetForProfile(profile),
+      base::Bind(&HistoryCounterTest::GetRealWebHistoryService,
+                 base::Unretained(this), base::Unretained(profile)),
+      GetHistoryService());
+
   counter.Init(profile->GetPrefs(), base::Bind(&HistoryCounterTest::Callback,
                                                base::Unretained(this)));
   counter.Restart();
@@ -165,7 +190,12 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, PrefChanged) {
 
   Profile* profile = browser()->profile();
 
-  HistoryCounter counter(profile);
+  browsing_data::HistoryCounter counter(
+      ProfileSyncServiceFactory::GetForProfile(profile),
+      base::Bind(&HistoryCounterTest::GetRealWebHistoryService,
+                 base::Unretained(this), base::Unretained(profile)),
+      GetHistoryService());
+
   counter.Init(profile->GetPrefs(), base::Bind(&HistoryCounterTest::Callback,
                                                base::Unretained(this)));
   SetHistoryDeletionPref(true);
@@ -182,7 +212,12 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, PrefIsFalse) {
 
   Profile* profile = browser()->profile();
 
-  HistoryCounter counter(profile);
+  browsing_data::HistoryCounter counter(
+      ProfileSyncServiceFactory::GetForProfile(profile),
+      base::Bind(&HistoryCounterTest::GetRealWebHistoryService,
+                 base::Unretained(this), base::Unretained(profile)),
+      GetHistoryService());
+
   counter.Init(profile->GetPrefs(), base::Bind(&HistoryCounterTest::Callback,
                                                base::Unretained(this)));
   counter.Restart();
@@ -216,7 +251,12 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, PeriodChanged) {
 
   Profile* profile = browser()->profile();
 
-  HistoryCounter counter(profile);
+  browsing_data::HistoryCounter counter(
+      ProfileSyncServiceFactory::GetForProfile(profile),
+      base::Bind(&HistoryCounterTest::GetRealWebHistoryService,
+                 base::Unretained(this), base::Unretained(profile)),
+      GetHistoryService());
+
   counter.Init(profile->GetPrefs(), base::Bind(&HistoryCounterTest::Callback,
                                                base::Unretained(this)));
 
@@ -247,19 +287,20 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, Synced) {
   // for testing.
   Profile* profile = browser()->profile();
 
-  std::unique_ptr<history::FakeWebHistoryService> service(
-      new history::FakeWebHistoryService(
-          ProfileOAuth2TokenServiceFactory::GetForProfile(profile),
-          SigninManagerFactory::GetForProfile(profile),
-          profile->GetRequestContext()));
+  browsing_data::HistoryCounter counter(
+      ProfileSyncServiceFactory::GetForProfile(profile),
+      base::Bind(&HistoryCounterTest::GetFakeWebHistoryService,
+                 base::Unretained(this)),
+      GetHistoryService());
 
-  HistoryCounter counter(profile);
-  counter.SetWebHistoryServiceForTesting(service.get());
   counter.Init(profile->GetPrefs(), base::Bind(&HistoryCounterTest::Callback,
                                                base::Unretained(this)));
 
+  history::FakeWebHistoryService* fake_web_history_service =
+      static_cast<history::FakeWebHistoryService*>(GetFakeWebHistoryService());
+
   // No entries locally and no entries in Sync.
-  service->SetupFakeResponse(true /* success */, net::HTTP_OK);
+  fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);

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

This renaming seems very unnecessary.

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

Is web_history_service okay?

[msramek, 2016/07/20 18:26:12]

Hm. This seems to be a misunderstanding.

What I meant is that renaming this local variable (|service|) is kinda unrelated
to this CL and increases noise. This CL didn't change the meaning of the
variable, so we didn't have to change the name. And there was a recent ToTT
article about not adding unnecessary context to variable names (in this case,
it's clear from the context what kind of service this is). But nevermind - this
is more of a nitpicking now.

More importantly - I liked that you took advantage of the callback function to
pass TestingWebHistoryService in tests. These testing functions are always very
hacky, and that got rid of it in a natural way. I assume that you reverted that
part because of this comment, but that's not at all what I meant - can you
please change that back?

[ioanap, 2016/07/21 11:47:36]

Changed it back to |service|.

The reason I took out the callback was because checking if the
WebHistoryServiceFactory returns null, instead of "manually" checking if history
sync is enabled led to some weirdness in the tests. The RestartOnSyncChange
test(disabled) operates with the FakeWebHistoryService but relies on the check
within the actual factory, while the Synced test adds synced entries to the fake
service, but was built to bypass the check within the factory. 

I changed the GetFakeWebHistoryService callback to take those considerations
into account. 

[msramek, 2016/07/21 12:17:11]

I see it now. Yes, that bypass is a bit weird. One thing that could be done is
to simply initialize the Sync service in this test instead of setting
check_sync_status = false. However, I'm pretty sure that the other test is
disabled because something was flaky in the Sync interaction, so let's rather
not. I like this solution where we hide the details about Sync inside the
callback.

On a related note - can you please check if the last disabled test still passes
locally at your machine? It's really only disabled because of some flakiness,
but it should, in principle, work.

   counter.Restart();
   WaitForCounting();
   EXPECT_EQ(0u, GetLocalResult());
@@ -268,19 +309,19 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, Synced) {
   // No entries locally. There are some entries in Sync, but they are out of the
   // time range.
   SetDeletionPeriodPref(browsing_data::LAST_HOUR);
-  service->AddSyncedVisit(
+  fake_web_history_service->AddSyncedVisit(
       "www.google.com", GetCurrentTime() - base::TimeDelta::FromHours(2));
-  service->AddSyncedVisit(
+  fake_web_history_service->AddSyncedVisit(
       "www.chrome.com", GetCurrentTime() - base::TimeDelta::FromHours(2));
-  service->SetupFakeResponse(true /* success */, net::HTTP_OK);
+  fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
   counter.Restart();
   WaitForCounting();
   EXPECT_EQ(0u, GetLocalResult());
   EXPECT_FALSE(HasSyncedVisits());
 
   // No entries locally, but some entries in Sync.
-  service->AddSyncedVisit("www.google.com", GetCurrentTime());
-  service->SetupFakeResponse(true /* success */, net::HTTP_OK);
+  fake_web_history_service->AddSyncedVisit("www.google.com", GetCurrentTime());
+  fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
   counter.Restart();
   WaitForCounting();
   EXPECT_EQ(0u, GetLocalResult());
@@ -288,16 +329,16 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, Synced) {
 
   // To err on the safe side, if the server request fails, we assume that there
   // might be some items on the server.
-  service->SetupFakeResponse(true /* success */,
-                             net::HTTP_INTERNAL_SERVER_ERROR);
+  fake_web_history_service->SetupFakeResponse(true /* success */,
+                                              net::HTTP_INTERNAL_SERVER_ERROR);
   counter.Restart();
   WaitForCounting();
   EXPECT_EQ(0u, GetLocalResult());
   EXPECT_TRUE(HasSyncedVisits());
 
   // Same when the entire query fails.
-  service->SetupFakeResponse(false /* success */,
-                             net::HTTP_INTERNAL_SERVER_ERROR);
+  fake_web_history_service->SetupFakeResponse(false /* success */,
+                                              net::HTTP_INTERNAL_SERVER_ERROR);
   counter.Restart();
   WaitForCounting();
   EXPECT_EQ(0u, GetLocalResult());
@@ -306,15 +347,15 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, Synced) {
   // Nonzero local count, nonempty sync.
   AddVisit("https://www.google.com");
   AddVisit("https://www.chrome.com");
-  service->SetupFakeResponse(true /* success */, net::HTTP_OK);
+  fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
   counter.Restart();
   WaitForCounting();
   EXPECT_EQ(2u, GetLocalResult());
   EXPECT_TRUE(HasSyncedVisits());
 
   // Nonzero local count, empty sync.
-  service->ClearSyncedVisits();
-  service->SetupFakeResponse(true /* success */, net::HTTP_OK);
+  fake_web_history_service->ClearSyncedVisits();
+  fake_web_history_service->SetupFakeResponse(true /* success */, net::HTTP_OK);
   counter.Restart();
   WaitForCounting();
   EXPECT_EQ(2u, GetLocalResult());
@@ -332,13 +373,12 @@ IN_PROC_BROWSER_TEST_F(HistoryCounterTest, DISABLED_RestartOnSyncChange) {
   Profile* profile = GetProfile(kFirstProfileIndex);
 
   // Set up the fake web history service and the counter.
-  std::unique_ptr<history::FakeWebHistoryService> web_history_service(
-      new history::FakeWebHistoryService(
-          ProfileOAuth2TokenServiceFactory::GetForProfile(browser()->profile()),
-          SigninManagerFactory::GetForProfile(browser()->profile()),
-          browser()->profile()->GetRequestContext()));
-  HistoryCounter counter(profile);
-  counter.SetWebHistoryServiceForTesting(web_history_service.get());
+
+  browsing_data::HistoryCounter counter(
+      sync_service, base::Bind(&HistoryCounterTest::GetFakeWebHistoryService,
+                               base::Unretained(this)),
+      GetHistoryService());
+
   counter.Init(profile->GetPrefs(), base::Bind(&HistoryCounterTest::Callback,
                                                base::Unretained(this)));