Make BrowsingDataHandler observe WebHistoryService deletions

BrowsingDataHandler observes local history deletions, but not synced
history deletions. In cases where there are no local history entries
to delete, it can happen that the history page does not reload and it
looks as if the deletion failed.

See https://docs.google.com/document/d/1Fd6CdBf6UMbYbkwSjEKyFOxew0Xid5IaT-QwnFchjig/
for background.

This CL
1. Adds an Observer subclass to the WebHistoryService.
2. Registers BrowsingHistoryHandler as a WebHistoryService::Observer;
   and since WebHistoryService's existence is based on whether history
   sync is enabled, we also register as a SyncServiceObserver.
3. Adds a test to browsing_history_handler_unittest.cc.

Also tested manually on Android - seems to solve the problem described
in the above mentioned document.

BUG=604114,630164

Review-Url: https://codereview.chromium.org/2263613002
Cr-Commit-Position: refs/heads/master@{#414679}

chrome/browser/ui/webui/browsing_history_handler.cc

@@ -50,6 +50,7 @@
 #include "components/query_parser/snippet.h"
 #include "components/sync/device_info/device_info.h"
 #include "components/sync/device_info/device_info_tracker.h"
+#include "components/sync/driver/sync_service_observer.h"
 #include "components/sync/protocol/history_delete_directive_specifics.pb.h"
 #include "components/sync/protocol/sync_enums.pb.h"
 #include "components/url_formatter/url_formatter.h"
@@ -316,10 +317,11 @@ bool BrowsingHistoryHandler::HistoryEntry::SortByTimeDescending(
 BrowsingHistoryHandler::BrowsingHistoryHandler()
     : has_pending_delete_request_(false),
       history_service_observer_(this),
+      web_history_service_observer_(this),
+      sync_service_observer_(this),
       has_synced_results_(false),
       has_other_forms_of_browsing_history_(false),
-      weak_factory_(this) {
-}
+      weak_factory_(this) {}
 
 BrowsingHistoryHandler::~BrowsingHistoryHandler() {
   query_task_tracker_.TryCancelAll();
@@ -343,10 +345,25 @@ void BrowsingHistoryHandler::RegisterMessages() {
 #endif
 
   // Get notifications when history is cleared.
-  history::HistoryService* hs = HistoryServiceFactory::GetForProfile(
+  history::HistoryService* local_history = HistoryServiceFactory::GetForProfile(
       profile, ServiceAccessType::EXPLICIT_ACCESS);
-  if (hs)
-    history_service_observer_.Add(hs);
+  if (local_history)
+    history_service_observer_.Add(local_history);
+
+  // Get notifications when web history is deleted.
+  history::WebHistoryService* web_history =
+      WebHistoryServiceFactory::GetForProfile(profile);
+  if (web_history) {
+    web_history_service_observer_.Add(web_history);
+  } else {
+    // If |web_history| is not available, it means that the history sync is
+    // disabled. Observe |sync_service| so that we can attach the listener
+    // in case it gets enabled later.
+    ProfileSyncService* sync_service =
+        ProfileSyncServiceFactory::GetForProfile(profile);
+    if (sync_service)
+      sync_service_observer_.Add(sync_service);
+  }
 
   web_ui()->RegisterMessageCallback("queryHistory",
       base::Bind(&BrowsingHistoryHandler::HandleQueryHistory,
@@ -375,6 +392,18 @@ bool BrowsingHistoryHandler::ExtractIntegerValueAtIndex(
   return false;
 }
 
+void BrowsingHistoryHandler::OnStateChanged() {
+  // If the history sync was enabled, start observing WebHistoryService.
+  // This method should not be called after we already added the observer.
+  history::WebHistoryService* web_history =
+      WebHistoryServiceFactory::GetForProfile(Profile::FromWebUI(web_ui()));
+  if (web_history) {
+    DCHECK(!web_history_service_observer_.IsObserving(web_history));
+    web_history_service_observer_.Add(web_history);
+    sync_service_observer_.RemoveAll();
+  }
+}
+
 void BrowsingHistoryHandler::WebHistoryTimeout() {
   // TODO(dubroy): Communicate the failure to the front end.
   if (!query_task_tracker_.HasTrackedTasks())
@@ -960,3 +989,7 @@ void BrowsingHistoryHandler::OnURLsDeleted(
   if (all_history || DeletionsDiffer(deleted_rows, urls_to_be_deleted_))
     web_ui()->CallJavascriptFunctionUnsafe("historyDeleted");
 }
+
+void BrowsingHistoryHandler::OnWebHistoryDeleted() {
+  web_ui()->CallJavascriptFunctionUnsafe("historyDeleted");
+}

chrome/browser/ui/webui/browsing_history_handler.h

@@ -20,6 +20,8 @@
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/history/core/browser/url_row.h"
 #include "components/history/core/browser/web_history_service.h"
+#include "components/history/core/browser/web_history_service_observer.h"
+#include "components/sync/driver/sync_service_observer.h"
 #include "content/public/browser/web_ui_message_handler.h"
 #include "url/gurl.h"
 
@@ -36,9 +38,15 @@ struct QueryOptions;
 class QueryResults;
 }
 
+namespace sync_driver {
+class SyncServiceObserver;
+}
+
 // The handler for Javascript messages related to the "history" view.
 class BrowsingHistoryHandler : public content::WebUIMessageHandler,
-                               public history::HistoryServiceObserver {
+                               public history::HistoryServiceObserver,
+                               public history::WebHistoryServiceObserver,
+                               public sync_driver::SyncServiceObserver {
  public:
   // Represents a history entry to be shown to the user, representing either
   // a local or remote visit. A single entry can represent multiple visits,
@@ -106,6 +114,9 @@ class BrowsingHistoryHandler : public content::WebUIMessageHandler,
   // WebUIMessageHandler implementation.
   void RegisterMessages() override;
 
+  // SyncServiceObserver implementation.
+  void OnStateChanged() override;
+
   // Handler for the "queryHistory" message.
   void HandleQueryHistory(const base::ListValue* args);
 
@@ -189,6 +200,9 @@ class BrowsingHistoryHandler : public content::WebUIMessageHandler,
                      const history::URLRows& deleted_rows,
                      const std::set<GURL>& favicon_urls) override;
 
+  // history::WebHistoryServiceObserver:
+  void OnWebHistoryDeleted() override;
+
   // Tracker for search requests to the history service.
   base::CancelableTaskTracker query_task_tracker_;
 
@@ -217,9 +231,18 @@ class BrowsingHistoryHandler : public content::WebUIMessageHandler,
   // Timer used to implement a timeout on a Web History response.
   base::OneShotTimer web_history_timer_;
 
+  // HistoryService (local history) observer.
   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
       history_service_observer_;
 
+  // WebHistoryService (synced history) observer.
+  ScopedObserver<history::WebHistoryService, history::WebHistoryServiceObserver>
+      web_history_service_observer_;
+
+  // ProfileSyncService observer listens to late initialization of history sync.
+  ScopedObserver<ProfileSyncService, sync_driver::SyncServiceObserver>
+      sync_service_observer_;
+
   // Whether the last call to Web History returned synced results.
   bool has_synced_results_;
 

chrome/browser/ui/webui/browsing_history_handler_unittest.cc

@@ -5,10 +5,32 @@
 #include "chrome/browser/ui/webui/browsing_history_handler.h"
 
 #include <stdint.h>
+#include <memory>
+#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/browser_sync/browser/test_profile_sync_service.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/sync/base/model_type.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 "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
 
 namespace {
 
@@ -48,8 +70,105 @@ bool ResultEquals(
   return result.time == correct_time && result.url == GURL(correct_result.url);
 }
 
+void IgnoreBoolAndDoNothing(bool ignored_argument) {}
+
+class TestSyncService : public TestProfileSyncService {
+ public:
+  explicit TestSyncService(Profile* profile)
+      : TestProfileSyncService(CreateProfileSyncServiceParamsForTest(profile)),
+        sync_active_(true) {}
+
+  bool IsSyncActive() const override { return sync_active_; }
+
+  syncer::ModelTypeSet GetActiveDataTypes() const override {
+    return syncer::ModelTypeSet::All();
+  }
+
+  void SetSyncActive(bool active) {
+    sync_active_ = active;
+    NotifyObservers();
+  }
+
+ private:
+  bool sync_active_;
+};
+
+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_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);
+
+    std::unique_ptr<history::FakeWebHistoryService> service =
+        base::MakeUnique<history::FakeWebHistoryService>(
+            ProfileOAuth2TokenServiceFactory::GetForProfile(profile),
+            SigninManagerFactory::GetForProfile(profile),
+            profile->GetRequestContext());
+    service->SetupFakeResponse(true /* success */, net::HTTP_OK);
+    return std::move(service);
+  }
+
+  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 +177,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 +256,51 @@ 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()->SetSyncActive(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()->SetSyncActive(false);
+    BrowsingHistoryHandlerWithWebUIForTesting handler(web_ui());
+    handler.RegisterMessages();
+    sync_service()->SetSyncActive(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());
+  }
+
+  // When history sync is not active, we don't listen to WebHistoryService
+  // deletions. The WebHistoryService object still exists (because it's a
+  // BrowserContextKeyedService), but is not visible to BrowsingHistoryHandler.
+  {
+    sync_service()->SetSyncActive(false);
+    BrowsingHistoryHandlerWithWebUIForTesting handler(web_ui());
+    handler.RegisterMessages();
+
+    web_history_service()->ExpireHistoryBetween(std::set<GURL>(), base::Time(),
+                                                base::Time::Max(), callback);
+
+    // No additional WebUI calls were made.
+    EXPECT_EQ(2U, web_ui()->call_data().size());
+  }
+}

components/history/core/browser/BUILD.gn

@@ -82,6 +82,7 @@ static_library("browser") {
     "visitsegment_database.h",
     "web_history_service.cc",
     "web_history_service.h",
+    "web_history_service_observer.h",
   ]
 
   public_deps = [

components/history/core/browser/web_history_service.cc

@@ -16,6 +16,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "components/history/core/browser/history_service_observer.h"
+#include "components/history/core/browser/web_history_service_observer.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/sync/driver/sync_util.h"
 #include "components/sync/protocol/history_status.pb.h"
@@ -340,6 +342,14 @@ WebHistoryService::~WebHistoryService() {
   base::STLDeleteElements(&pending_other_forms_of_browsing_history_requests_);
 }
 
+void WebHistoryService::AddObserver(WebHistoryServiceObserver* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void WebHistoryService::RemoveObserver(WebHistoryServiceObserver* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
 WebHistoryService::Request* WebHistoryService::CreateRequest(
     const GURL& url,
     const CompletionCallback& callback) {
@@ -419,8 +429,9 @@ void WebHistoryService::ExpireHistory(
 
   std::unique_ptr<Request> request(CreateRequest(url, completion_callback));
   request->SetPostData(post_data);
-  request->Start();
+  Request* request_ptr = request.get();
   pending_expire_requests_.insert(request.release());
+  request_ptr->Start();
 }
 
 void WebHistoryService::ExpireHistoryBetween(
@@ -553,6 +564,12 @@ void WebHistoryService::ExpireHistoryCompletionCallback(
       response_value->GetString("version_info", &server_version_info_);
   }
   callback.Run(response_value.get() && success);
+
+  // Inform the observers about the history deletion.
+  if (response_value.get() && success) {
+    FOR_EACH_OBSERVER(WebHistoryServiceObserver, observer_list_,
+                      OnWebHistoryDeleted());
+  }
 }
 
 void WebHistoryService::AudioHistoryCompletionCallback(

components/history/core/browser/web_history_service.h

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/keyed_service/core/keyed_service.h"
 
@@ -34,6 +35,8 @@ class SigninManagerBase;
 
 namespace history {
 
+class WebHistoryServiceObserver;
+
 // Provides an API for querying Google servers for a signed-in user's
 // synced history visits. It is roughly analogous to HistoryService, and
 // supports a similar API.
@@ -95,6 +98,9 @@ class WebHistoryService : public KeyedService {
       const scoped_refptr<net::URLRequestContextGetter>& request_context);
   ~WebHistoryService() override;
 
+  void AddObserver(WebHistoryServiceObserver* observer);
+  void RemoveObserver(WebHistoryServiceObserver* observer);
+
   // Searches synced history for visits matching |text_query|. The timeframe to
   // search, along with other options, is specified in |options|. If
   // |text_query| is empty, all visits in the timeframe will be returned.
@@ -219,6 +225,9 @@ class WebHistoryService : public KeyedService {
   // complete by profile shutdown.
   std::set<Request*> pending_other_forms_of_browsing_history_requests_;
 
+  // Observers.
+  base::ObserverList<WebHistoryServiceObserver, true> observer_list_;
+
   base::WeakPtrFactory<WebHistoryService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(WebHistoryService);

components/history/core/browser/web_history_service_observer.h

+// Copyright 2016 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.
+
+#ifndef COMPONENTS_HISTORY_CORE_BROWSER_WEB_HISTORY_SERVICE_OBSERVER_H_
+#define COMPONENTS_HISTORY_CORE_BROWSER_WEB_HISTORY_SERVICE_OBSERVER_H_
+
+namespace history {
+
+// Observes WebHistoryService actions. Currently only history expiration
+// is supported.
+// TODO(msramek): Consider defining ObservableHistoryService as a common
+// abstract ancestor for HistoryService and WebHistoryService. Then, we could
+// change the signatures of HistoryServiceObserver to use it for both classes.
+class WebHistoryServiceObserver {
+ public:
+  // Called after a successful WebHistoryService deletion request. This could
+  // be partial or complete history deletion.
+  virtual void OnWebHistoryDeleted() = 0;
+
+ protected:
+  virtual ~WebHistoryServiceObserver() {}
+};
+
+}  // history
+
+#endif  // COMPONENTS_HISTORY_CORE_BROWSER_WEB_HISTORY_SERVICE_OBSERVER_H_

components/history/core/test/fake_web_history_service.cc

@@ -24,8 +24,9 @@ namespace {
 // TODO(msramek): Find a way to keep these URLs in sync with what is used
 // in WebHistoryService.
 
-const char kLookupUrl[] =
-    "https://history.google.com/history/api/lookup";
+const char kLookupUrl[] = "https://history.google.com/history/api/lookup";
+
+const char kDeleteUrl[] = "https://history.google.com/history/api/delete";
 
 const char kChromeClient[] = "chrome";
 
@@ -105,8 +106,8 @@ const std::string& FakeRequest::GetResponseBody() {
   remove_query.ClearQuery();
   GURL base_url = url_.ReplaceComponents(remove_query);
 
-  // History query.
   if (base_url == GURL(kLookupUrl) && client == kChromeClient) {
+    // History query.
     int count = service_->GetNumberOfVisitsBetween(begin_, end_);
     if (max_count_ && max_count_ < count)
       count = max_count_;
@@ -115,17 +116,19 @@ const std::string& FakeRequest::GetResponseBody() {
     for (int i = 0; i < count; ++i)
       response_body_ += i ? ", {}" : "{}";
     response_body_ += "] }";
-  }
 
-  // Web and app activity query.
-  if (base_url == GURL(kLookupUrl) && client == kWebAndAppClient) {
+  } else if (base_url == GURL(kDeleteUrl) && client == kChromeClient) {
+    // Deletion query.
+    response_body_ = "{ \"just needs to be\" : \"a valid JSON.\" }";
+
+  } else if (base_url == GURL(kLookupUrl) && client == kWebAndAppClient) {
+    // Web and app activity query.
     response_body_ = base::StringPrintf(
         "{ \"history_recording_enabled\": %s }",
         service_->IsWebAndAppActivityEnabled() ? "true" : "false");
-  }
 
-  // Other forms of browsing history query.
-  if (url_.host() == kSyncServerHost) {
+  } else if (url_.host() == kSyncServerHost) {
+    // Other forms of browsing history query.
     std::unique_ptr<sync_pb::HistoryStatusResponse> history_status(
         new sync_pb::HistoryStatusResponse());
     history_status->set_has_derived_data(