Move counters for passwords, history and autofill to components

chrome/browser/browsing_data/history_counter_browsertest.cc

 // Copyright (c) 2015 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/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"
 #include "components/browsing_data/core/browsing_data_utils.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 "components/history/core/test/fake_web_history_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "net/http/http_status_code.h"
 #include "url/gurl.h"
 
 namespace {
 
 class HistoryCounterTest : public SyncTest {
  public:
   HistoryCounterTest() : SyncTest(SINGLE_CLIENT) {
     // TODO(msramek): Only one of the test cases, RestartOnSyncChange, is a Sync
     // integration test. Extract it and move it to the rest of integration tests
     // in chrome/browser/sync/test/integration/. Change this class back to
     // InProcessBrowserTest.
   }
 
   ~HistoryCounterTest() override {};
 
   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() {
     return time_;
   }
 
   void RevertTimeInDays(int days) {
     time_ -= base::TimeDelta::FromDays(days);
   }
 
@@ skipping 20 matching lines @@
   bool HasSyncedVisits() {
     DCHECK(finished_);
     return has_synced_visits_;
   }
 
   void Callback(
       std::unique_ptr<browsing_data::BrowsingDataCounter::Result> result) {
     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();
     }
 
     if (run_loop_ && finished_)
       run_loop_->Quit();
   }
 
   bool CountingFinishedSinceLastAsked() {
     bool result = finished_;
     finished_ = false;
     return result;
   }
 
   void WaitForCountingOrConfirmFinished() {
     if (CountingFinishedSinceLastAsked())
       return;
 
     WaitForCounting();
     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_;
   browsing_data::BrowsingDataCounter::ResultInt local_result_;
   bool has_synced_visits_;
 };
 
 // Tests that the counter considers duplicate visits from the same day
 // to be a single item.
 IN_PROC_BROWSER_TEST_F(HistoryCounterTest, DuplicateVisits) {
@@ skipping 13 matching lines @@
   AddVisit("https://www.chrome.com");   // 6 items
   AddVisit("https://www.chrome.com");
   AddVisit("https://www.google.com");   // 7 items
   AddVisit("https://www.chrome.com");
   AddVisit("https://www.google.com");
   AddVisit("https://www.google.com");
   AddVisit("https://www.chrome.com");
 
   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();
 
   WaitForCounting();
   EXPECT_EQ(7u, GetLocalResult());
 }
 
 // Tests that the counter starts counting automatically when the deletion
 // pref changes to true.
 IN_PROC_BROWSER_TEST_F(HistoryCounterTest, PrefChanged) {
   SetHistoryDeletionPref(false);
   AddVisit("https://www.google.com");
   AddVisit("https://www.chrome.com");
 
   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);
 
   WaitForCounting();
   EXPECT_EQ(2u, GetLocalResult());
 }
 
 // Tests that the counter does not count history if the deletion
 // preference is false.
 IN_PROC_BROWSER_TEST_F(HistoryCounterTest, PrefIsFalse) {
   SetHistoryDeletionPref(false);
   AddVisit("https://www.google.com");
 
   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();
 
   EXPECT_FALSE(counter.HasTrackedTasks());
 }
 
 // Tests that changing the deletion period restarts the counting, and that
 // the result takes visit dates into account.
 IN_PROC_BROWSER_TEST_F(HistoryCounterTest, PeriodChanged) {
@@ skipping 13 matching lines @@
   AddVisit("https://www.chrome.com");
   AddVisit("https://www.example.com");
 
   RevertTimeInDays(10);
   AddVisit("https://www.example.com");
   AddVisit("https://www.example.com");
   AddVisit("https://www.example.com");
 
   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)));
 
   SetDeletionPeriodPref(browsing_data::LAST_HOUR);
   WaitForCounting();
   EXPECT_EQ(1u, GetLocalResult());
 
   SetDeletionPeriodPref(browsing_data::LAST_DAY);
   WaitForCounting();
   EXPECT_EQ(1u, GetLocalResult());
@@ skipping 10 matching lines @@
   WaitForCounting();
   EXPECT_EQ(9u, GetLocalResult());
 }
 
 // Test the behavior for a profile that syncs history.
 IN_PROC_BROWSER_TEST_F(HistoryCounterTest, Synced) {
   // WebHistoryService makes network requests, so we need to use a fake one
   // 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());
   EXPECT_FALSE(HasSyncedVisits());
 
   // 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());
   EXPECT_TRUE(HasSyncedVisits());
 
   // 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());
   EXPECT_TRUE(HasSyncedVisits());
 
   // 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());
   EXPECT_FALSE(HasSyncedVisits());
 }
 
 // Test that the counting restarts when history sync state changes.
 // TODO(crbug.com/553421): Enable this test and move it to the
 // sync/test/integration directory.
 IN_PROC_BROWSER_TEST_F(HistoryCounterTest, DISABLED_RestartOnSyncChange) {
   // Set up the Sync client.
   ASSERT_TRUE(SetupClients());
   static const int kFirstProfileIndex = 0;
   ProfileSyncService* sync_service = GetSyncService(kFirstProfileIndex);
   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)));
 
   // Note that some Sync operations notify observers immediately (and thus there
   // is no need to call |WaitForCounting()|; in fact, it would block the test),
   // while other operations only post the task on UI thread's message loop
   // (which requires calling |WaitForCounting()| for them to run). Therefore,
   // this test always checks if the callback has already run and only waits
   // if it has not.
 
@@ skipping 42 matching lines @@
   // history deletion did not change. However, in reality we can get two
   // notifications, one that history sync has stopped and another that it is
   // active again.
 
   // Stopping the Sync service triggers a restart.
   sync_service->RequestStop(sync_driver::SyncService::CLEAR_DATA);
   WaitForCountingOrConfirmFinished();
 }
 
 }  // namespace