Make BrowsingDataHandler observe WebHistoryService deletions

chrome/browser/ui/webui/browsing_history_handler_unittest.cc

Index: chrome/browser/ui/webui/browsing_history_handler_unittest.cc
diff --git a/chrome/browser/ui/webui/browsing_history_handler_unittest.cc b/chrome/browser/ui/webui/browsing_history_handler_unittest.cc
index f8f6a4b8331533cdfcfd3179575ad0f65efaf456..7e37df8e0f055673a537f31477eeca3fcada6ccd 100644
--- a/chrome/browser/ui/webui/browsing_history_handler_unittest.cc
+++ b/chrome/browser/ui/webui/browsing_history_handler_unittest.cc
@@ -5,10 +5,33 @@
 #include "chrome/browser/ui/webui/browsing_history_handler.h"
 
 #include <stdint.h>
+#include <set>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/history/web_history_service_factory.h"
+#include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
+#include "chrome/browser/signin/fake_signin_manager_builder.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/profile_sync_test_util.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/history/core/test/fake_web_history_service.h"
+#include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
+#include "components/signin/core/browser/fake_signin_manager.h"
+#include "components/signin/core/browser/test_signin_client.h"
+#include "components/sync/base/model_type.h"
+#include "components/sync/driver/fake_sync_service.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_web_ui.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
 
 namespace {
 
@@ -48,8 +71,100 @@ bool ResultEquals(
   return result.time == correct_time && result.url == GURL(correct_result.url);
 }
 
+void IgnoreBoolAndDoNothing(bool ignored_argument) {}
+
+class TestSyncService : public ProfileSyncService {
+ public:
+  explicit TestSyncService(Profile* profile)
+      : ProfileSyncService(CreateProfileSyncServiceParamsForTest(profile)),
+        sync_active_(false) {}
+
+  bool IsSyncActive() const override { return true; }
+
+  syncer::ModelTypeSet GetActiveDataTypes() const override {
+    return syncer::ModelTypeSet::All();
+  }
+
+  void set_sync_active(bool active) { sync_active_ = active; }
+
+ private:
+  bool sync_active_;

[tsergeant, 2016/08/24 08:47:04]

sync_active_ doesn't appear to be actually used anywhere? It seems as though the
second section of the test below passes because WebHistoryService is always
available, and not because of any changes to the Sync Service.

[msramek, 2016/08/24 12:16:07]

Thanks for spotting - I forgot the hardcoded "true" there.

I added |sync_active_| to IsSyncActive(), and also a NotifyObservers() call to
SetSyncActive(). I changed the class to inherit from TestProfileSyncService,
since NotifyObservers() is private in the regular ProfileSyncService.

I also added a third section to the test as a "control group" to prevent this
mistake from happening again.

Finally, I changed |sync_active_| to initialize to true, otherwise the
WebHistoryServiceFactory::GetForProfile() call in the test constructor would
return nullptr. This rigid relationship between ProfileSyncService and
WebHistoryService is actually the reason why we need this complicated test in
the first place.

+};
+
+class BrowsingHistoryHandlerWithWebUIForTesting
+    : public BrowsingHistoryHandler {
+ public:
+  explicit BrowsingHistoryHandlerWithWebUIForTesting(content::WebUI* web_ui) {
+    set_web_ui(web_ui);
+  }
+};
+
 }  // namespace
 
+class BrowsingHistoryHandlerTest : public ::testing::Test {
+ public:
+  void SetUp() override {
+    TestingProfile::Builder builder;
+    builder.AddTestingFactory(ProfileOAuth2TokenServiceFactory::GetInstance(),
+                              &BuildFakeProfileOAuth2TokenService);
+    builder.AddTestingFactory(SigninManagerFactory::GetInstance(),
+                              &BuildFakeSigninManagerBase);
+    builder.AddTestingFactory(ProfileSyncServiceFactory::GetInstance(),
+                              &BuildFakeSyncService);
+    builder.AddTestingFactory(WebHistoryServiceFactory::GetInstance(),
+                              &BuildFakeWebHistoryService);
+    profile_ = builder.Build();
+
+    sync_service_ = static_cast<TestSyncService*>(
+        ProfileSyncServiceFactory::GetForProfile(profile_.get()));
+    web_history_service_ = static_cast<history::FakeWebHistoryService*>(
+        WebHistoryServiceFactory::GetForProfile(profile_.get()));
+    web_history_service_->SetupFakeResponse(true /* success */, net::HTTP_OK);
+
+    web_contents_.reset(content::WebContents::Create(
+        content::WebContents::CreateParams(profile_.get())));
+    web_ui_.reset(new content::TestWebUI);
+    web_ui_->set_web_contents(web_contents_.get());
+  }
+
+  void TearDown() override {
+    web_contents_.reset();
+    web_ui_.reset();
+    profile_.reset();
+  }
+
+  Profile* profile() { return profile_.get(); }
+  TestSyncService* sync_service() { return sync_service_; }
+  history::WebHistoryService* web_history_service() {
+    return web_history_service_;
+  }
+  content::TestWebUI* web_ui() { return web_ui_.get(); }
+
+ private:
+  static std::unique_ptr<KeyedService> BuildFakeSyncService(
+      content::BrowserContext* context) {
+    return base::MakeUnique<TestSyncService>(
+        static_cast<TestingProfile*>(context));
+  }
+
+  static std::unique_ptr<KeyedService> BuildFakeWebHistoryService(
+      content::BrowserContext* context) {
+    Profile* profile = static_cast<TestingProfile*>(context);
+
+    return base::MakeUnique<history::FakeWebHistoryService>(
+        ProfileOAuth2TokenServiceFactory::GetForProfile(profile),
+        SigninManagerFactory::GetForProfile(profile),
+        profile->GetRequestContext());
+  }
+
+  content::TestBrowserThreadBundle thread_bundle_;
+  std::unique_ptr<TestingProfile> profile_;
+  TestSyncService* sync_service_;
+  history::FakeWebHistoryService* web_history_service_;
+  std::unique_ptr<content::TestWebUI> web_ui_;
+  std::unique_ptr<content::WebContents> web_contents_;
+};
+
 // Tests that the MergeDuplicateResults method correctly removes duplicate
 // visits to the same URL on the same day.
 // Fails on Android.  http://crbug.com/2345
@@ -58,7 +173,7 @@ bool ResultEquals(
 #else
 #define MAYBE_MergeDuplicateResults MergeDuplicateResults
 #endif
-TEST(BrowsingHistoryHandlerTest, MAYBE_MergeDuplicateResults) {
+TEST_F(BrowsingHistoryHandlerTest, MAYBE_MergeDuplicateResults) {
   {
     // Basic test that duplicates on the same day are removed.
     TestResult test_data[] = {
@@ -137,3 +252,36 @@ TEST(BrowsingHistoryHandlerTest, MAYBE_MergeDuplicateResults) {
     EXPECT_EQ(1u, results[1].all_timestamps.size());
   }
 }
+
+// Tests that BrowsingHistoryHandler observes WebHistoryService deletions.
+TEST_F(BrowsingHistoryHandlerTest, ObservingWebHistoryDeletions) {
+  base::Callback<void(bool)> callback = base::Bind(&IgnoreBoolAndDoNothing);
+
+  // BrowsingHistoryHandler listens to WebHistoryService history deletions.
+  {
+    sync_service()->set_sync_active(true);
+    BrowsingHistoryHandlerWithWebUIForTesting handler(web_ui());
+    handler.RegisterMessages();
+
+    web_history_service()->ExpireHistoryBetween(std::set<GURL>(), base::Time(),
+                                                base::Time::Max(), callback);
+
+    EXPECT_EQ(1U, web_ui()->call_data().size());
+    EXPECT_EQ("historyDeleted", web_ui()->call_data().back()->function_name());
+  }
+
+  // BrowsingHistoryHandler will listen to WebHistoryService deletions even if
+  // history sync is activated later.
+  {
+    sync_service()->set_sync_active(false);
+    BrowsingHistoryHandlerWithWebUIForTesting handler(web_ui());
+    handler.RegisterMessages();
+    sync_service()->set_sync_active(true);
+
+    web_history_service()->ExpireHistoryBetween(std::set<GURL>(), base::Time(),
+                                                base::Time::Max(), callback);
+
+    EXPECT_EQ(2U, web_ui()->call_data().size());
+    EXPECT_EQ("historyDeleted", web_ui()->call_data().back()->function_name());
+  }
+}