Get the catalog from the network

Uses the ExploreSitesFetcher to get the latest catalog from the network,
and upload it into our database for use on next run of ExploreSites.

Bug: 889104
Change-Id: I2d414e6cea59271252fa735bd59191706dead377
Reviewed-on: https://chromium-review.googlesource.com/1243673
Commit-Queue: Peter Williamson <petewil@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594589}

chrome/android/java/src/org/chromium/chrome/browser/explore_sites/ExploreSitesBridge.java

@@ -38,9 +38,9 @@ public class ExploreSitesBridge {
     /**
      * Causes a network request for updating the catalog.
      */
-    public static void updateCatalogFromNetwork(Profile profile, Callback<Void> finishedCallback) {
-        // TODO(dewittj): Connect to the ExploreSitesService on the native side.
-        finishedCallback.onResult(null);
+    public static void updateCatalogFromNetwork(
+            Profile profile, Callback<Boolean> finishedCallback) {
+        nativeUpdateCatalogFromNetwork(profile, finishedCallback);
     }
 
     /**
@@ -57,4 +57,7 @@ public class ExploreSitesBridge {
 
     private static native void nativeGetIcon(
             Profile profile, int siteID, Callback<Bitmap> callback);
+
+    private static native void nativeUpdateCatalogFromNetwork(
+            Profile profile, Callback<Boolean> callback);
 }

chrome/browser/android/explore_sites/explore_sites_bridge.cc

@@ -65,6 +65,11 @@ void ImageReady(ScopedJavaGlobalRef<jobject>(j_callback_obj),
   base::android::RunObjectCallbackAndroid(j_callback_obj, j_bitmap);
 }
 
+void UpdateCatalogDone(ScopedJavaGlobalRef<jobject>(j_callback_obj),
+                       bool result) {
+  base::android::RunBooleanCallbackAndroid(j_callback_obj, result);
+}
+
 }  // namespace
 
 // static
@@ -111,6 +116,7 @@ void JNI_ExploreSitesBridge_GetIcon(
       ExploreSitesServiceFactory::GetForBrowserContext(profile);
   if (!service) {
     DLOG(ERROR) << "Unable to create the ExploreSitesService!";
+
     base::android::RunObjectCallbackAndroid(j_callback_obj, nullptr);
     return;
   }
@@ -120,4 +126,25 @@ void JNI_ExploreSitesBridge_GetIcon(
       site_id, base::BindOnce(&ImageReady,
                               ScopedJavaGlobalRef<jobject>(j_callback_obj)));
 }
+
+void JNI_ExploreSitesBridge_UpdateCatalogFromNetwork(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& j_caller,
+    const JavaParamRef<jobject>& j_profile,
+    const JavaParamRef<jobject>& j_callback_obj) {
+  Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
+  DCHECK(profile);
+
+  ExploreSitesService* service =
+      ExploreSitesServiceFactory::GetForBrowserContext(profile);
+  if (!service) {
+    DLOG(ERROR) << "Unable to create the ExploreSitesService!";
+    base::android::RunBooleanCallbackAndroid(j_callback_obj, false);
+    return;
+  }
+
+  service->UpdateCatalogFromNetwork(base::BindOnce(
+      &UpdateCatalogDone, ScopedJavaGlobalRef<jobject>(j_callback_obj)));
+}
+
 }  // namespace explore_sites

chrome/browser/android/explore_sites/explore_sites_service.h

@@ -28,6 +28,10 @@ class ExploreSitesService : public KeyedService {
   // Returns via callback the image for a site. This is typically the site
   // favicon. Returns |nullptr| if there was an error or no match for |site_id|.
   virtual void GetSiteImage(int site_id, BitmapCallback callback) = 0;
+
+  // Fetch the latest catalog from the network and stores it locally. Returns
+  // true in the callback for success.
+  virtual void UpdateCatalogFromNetwork(BooleanCallback callback) = 0;
 };
 
 }  // namespace explore_sites

chrome/browser/android/explore_sites/explore_sites_service_factory.cc

@@ -19,6 +19,7 @@
 #include "chrome/common/chrome_constants.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "content/public/browser/browser_context.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace explore_sites {
 const base::FilePath::CharType kExploreSitesStoreDirname[] =
@@ -50,8 +51,11 @@ KeyedService* ExploreSitesServiceFactory::BuildServiceInstanceFor(
       profile->GetPath().Append(kExploreSitesStoreDirname);
   auto explore_sites_store =
       std::make_unique<ExploreSitesStore>(background_task_runner, store_path);
+  scoped_refptr<network::SharedURLLoaderFactory> url_fetcher =
+      profile->GetURLLoaderFactory();
 
-  return new ExploreSitesServiceImpl(std::move(explore_sites_store));
+  return new ExploreSitesServiceImpl(std::move(explore_sites_store),
+                                     url_fetcher);
 }
 
 }  // namespace explore_sites

chrome/browser/android/explore_sites/explore_sites_service_impl.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/android/explore_sites/explore_sites_service_impl.h"
 
+#include "base/logging.h"
 #include "base/task/post_task.h"
 #include "chrome/browser/android/explore_sites/catalog.pb.h"
 #include "chrome/browser/android/explore_sites/explore_sites_feature.h"
@@ -15,6 +16,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/service_manager_connection.h"
 #include "services/data_decoder/public/cpp/decode_image.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/geometry/size.h"
@@ -28,8 +30,12 @@ const int kFaviconsPerCategoryImage = 4;
 }
 
 ExploreSitesServiceImpl::ExploreSitesServiceImpl(
-    std::unique_ptr<ExploreSitesStore> store)
-    : task_queue_(this), explore_sites_store_(std::move(store)) {}
+    std::unique_ptr<ExploreSitesStore> store,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
+    : task_queue_(this),
+      explore_sites_store_(std::move(store)),
+      url_loader_factory_(url_loader_factory),
+      weak_ptr_factory_(this) {}
 
 ExploreSitesServiceImpl::~ExploreSitesServiceImpl() {}
 
@@ -64,18 +70,67 @@ void ExploreSitesServiceImpl::GetSiteImage(int site_id,
                      std::move(callback))));
 }
 
+void ExploreSitesServiceImpl::UpdateCatalogFromNetwork(
+    BooleanCallback callback) {
+  if (!IsExploreSitesEnabled())
+    return;
+  // If we are already fetching, don't interrupt a fetch in progress.
+  if (explore_sites_fetcher_ != nullptr)
+    return;
+
+  // TODO(petewil): Eventually get the catalog version from DB.
+  std::string catalog_version = "";
+  // TODO(petewil): Eventually get the country code from somewhere.
+  std::string country_code = "KE";
+
+  // Create a fetcher and start fetching the protobuf (async).
+  explore_sites_fetcher_ = ExploreSitesFetcher::CreateForGetCatalog(
+      base::BindOnce(&ExploreSitesServiceImpl::OnCatalogFetched,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)),
+      catalog_version, country_code, url_loader_factory_);
+}
+
+void ExploreSitesServiceImpl::OnCatalogFetched(
+    BooleanCallback callback,
+    ExploreSitesRequestStatus status,
+    std::unique_ptr<std::string> serialized_protobuf) {
+  explore_sites_fetcher_.reset(nullptr);
+
+  if (serialized_protobuf == nullptr) {
+    DVLOG(1) << "Empty catalog response received from network.";
+    std::move(callback).Run(false);
+    return;
+  }
+
+  // Convert the protobuf into a catalog object.
+  std::unique_ptr<explore_sites::GetCatalogResponse> catalog_response =
+      std::make_unique<explore_sites::GetCatalogResponse>();
+  if (!catalog_response->ParseFromString(*serialized_protobuf.get())) {
+    DVLOG(1) << "Failed to parse catalog";
+    std::move(callback).Run(false);
+    return;
+  }
+  std::string catalog_version = catalog_response->version_token();
+  std::unique_ptr<Catalog> catalog(catalog_response->release_catalog());
+
+  // Add the catalog to our internal database using AddUpdatedCatalog
+  AddUpdatedCatalog(catalog_version, std::move(catalog), std::move(callback));
+}
+
 void ExploreSitesServiceImpl::Shutdown() {}
 
 void ExploreSitesServiceImpl::OnTaskQueueIsIdle() {}
 
 void ExploreSitesServiceImpl::AddUpdatedCatalog(
     std::string version_token,
-    std::unique_ptr<Catalog> catalog_proto) {
+    std::unique_ptr<Catalog> catalog_proto,
+    BooleanCallback callback) {
   if (!IsExploreSitesEnabled())
     return;
 
   task_queue_.AddTask(std::make_unique<ImportCatalogTask>(
-      explore_sites_store_.get(), version_token, std::move(catalog_proto)));
+      explore_sites_store_.get(), version_token, std::move(catalog_proto),
+      std::move(callback)));
 }
 
 // static

chrome/browser/android/explore_sites/explore_sites_service_impl.h

@@ -8,10 +8,12 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "chrome/browser/android/explore_sites/explore_sites_fetcher.h"
 #include "chrome/browser/android/explore_sites/explore_sites_service.h"
 #include "chrome/browser/android/explore_sites/explore_sites_store.h"
 #include "chrome/browser/android/explore_sites/explore_sites_types.h"
 #include "components/offline_pages/task/task_queue.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 using offline_pages::TaskQueue;
 
@@ -21,7 +23,9 @@ class Catalog;
 class ExploreSitesServiceImpl : public ExploreSitesService,
                                 public TaskQueue::Delegate {
  public:
-  explicit ExploreSitesServiceImpl(std::unique_ptr<ExploreSitesStore> store);
+  ExploreSitesServiceImpl(
+      std::unique_ptr<ExploreSitesStore> store,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
   ~ExploreSitesServiceImpl() override;
 
   bool IsExploreSitesEnabled();
@@ -30,6 +34,7 @@ class ExploreSitesServiceImpl : public ExploreSitesService,
   void GetCatalog(CatalogCallback callback) override;
   void GetCategoryImage(int category_id, BitmapCallback callback) override;
   void GetSiteImage(int site_id, BitmapCallback callback) override;
+  void UpdateCatalogFromNetwork(BooleanCallback callback) override;
 
  private:
   // KeyedService implementation:
@@ -39,13 +44,20 @@ class ExploreSitesServiceImpl : public ExploreSitesService,
   void OnTaskQueueIsIdle() override;
 
   void AddUpdatedCatalog(std::string version_token,
-                         std::unique_ptr<Catalog> catalog_proto);
+                         std::unique_ptr<Catalog> catalog_proto,
+                         BooleanCallback callback);
 
   static void OnDecodeDone(BitmapCallback callback,
                            const SkBitmap& decoded_image);
   static void DecodeImageBytes(BitmapCallback callback,
                                EncodedImageList images);
 
+  // Callback returning from the UpdateCatalogFromNetwork operation.  It
+  // passes along the call back to the bridge and eventually back to Java land.
+  void OnCatalogFetched(BooleanCallback callback,
+                        ExploreSitesRequestStatus status,
+                        std::unique_ptr<std::string> serialized_protobuf);
+
   // True when Chrome starts up, this is reset after the catalog is requested
   // the first time in Chrome. This prevents the ESP from changing out from
   // under a viewer.
@@ -54,6 +66,9 @@ class ExploreSitesServiceImpl : public ExploreSitesService,
   // Used to control access to the ExploreSitesStore.
   TaskQueue task_queue_;
   std::unique_ptr<ExploreSitesStore> explore_sites_store_;
+  scoped_refptr<network ::SharedURLLoaderFactory> url_loader_factory_;
+  std::unique_ptr<ExploreSitesFetcher> explore_sites_fetcher_;
+  base::WeakPtrFactory<ExploreSitesServiceImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ExploreSitesServiceImpl);
 };

chrome/browser/android/explore_sites/explore_sites_service_impl_unittest.cc

+// Copyright 2018 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/android/explore_sites/explore_sites_service_impl.h"
+
+#include "base/feature_list.h"
+#include "base/message_loop/message_loop.h"
+#include "base/test/bind_test_util.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/test_mock_time_task_runner.h"
+#include "chrome/browser/android/chrome_feature_list.h"
+#include "chrome/browser/android/explore_sites/catalog.pb.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+const char kCategoryName[] = "Technology";
+const char kSite1Url[] = "https://example.com/";
+const char kSite2Url[] = "https://sample.com/";
+const char kSite1Name[] = "example";
+const char kSite2Name[] = "sample";
+}  // namespace
+
+namespace explore_sites {
+
+class ExploreSitesServiceImplTest : public testing::Test {
+ public:
+  ExploreSitesServiceImplTest();
+  ~ExploreSitesServiceImplTest() override = default;
+
+  void SetUp() override {
+    std::unique_ptr<ExploreSitesStore> store =
+        std::make_unique<ExploreSitesStore>(task_runner_);
+    service_ = std::make_unique<ExploreSitesServiceImpl>(
+        std::move(store), test_shared_url_loader_factory_);
+    success_ = false;
+    test_data_ = CreateTestDataProto();
+  }
+
+  void UpdateCatalogDoneCallback(bool success) { success_ = success; }
+  void CatalogCallback(
+      std::unique_ptr<std::vector<ExploreSitesCategory>> categories) {
+    if (categories != nullptr) {
+      database_categories_ = std::move(categories);
+    }
+  }
+
+  bool success() { return success_; }
+
+  std::vector<ExploreSitesCategory>* database_categories() {
+    return database_categories_.get();
+  }
+
+  ExploreSitesServiceImpl* service() { return service_.get(); }
+
+  std::string test_data() { return test_data_; }
+
+  void PumpLoop() { task_runner_->RunUntilIdle(); }
+
+  std::string CreateTestDataProto();
+
+  ExploreSitesRequestStatus SimulateFetcherData(
+      const std::string& response_data);
+
+ private:
+  network::TestURLLoaderFactory::PendingRequest* GetPendingRequest(
+      size_t index);
+
+  std::unique_ptr<explore_sites::ExploreSitesServiceImpl> service_;
+  bool success_;
+  std::unique_ptr<std::vector<ExploreSitesCategory>> database_categories_;
+  std::string test_data_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory>
+      test_shared_url_loader_factory_;
+  network::ResourceRequest last_resource_request_;
+  base::MessageLoopForIO message_loop_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExploreSitesServiceImplTest);
+};
+
+ExploreSitesServiceImplTest::ExploreSitesServiceImplTest()
+    : success_(false),
+      test_shared_url_loader_factory_(
+          base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+              &test_url_loader_factory_)),
+      task_runner_(new base::TestMockTimeTaskRunner) {
+  message_loop_.SetTaskRunner(task_runner_);
+}
+
+// Called by tests - response_data is the data we want to go back as the
+// response from the network.
+ExploreSitesRequestStatus ExploreSitesServiceImplTest::SimulateFetcherData(
+    const std::string& response_data) {
+  DCHECK(test_url_loader_factory_.pending_requests()->size() > 0);
+  test_url_loader_factory_.SimulateResponseForPendingRequest(
+      GetPendingRequest(0)->request.url.spec(), response_data);
+  return ExploreSitesRequestStatus::kSuccess;
+}
+
+// Helper to check the next request for the network.
+network::TestURLLoaderFactory::PendingRequest*
+ExploreSitesServiceImplTest::GetPendingRequest(size_t index) {
+  if (index >= test_url_loader_factory_.pending_requests()->size())
+    return nullptr;
+  network::TestURLLoaderFactory::PendingRequest* request =
+      &(*test_url_loader_factory_.pending_requests())[index];
+  DCHECK(request);
+  return request;
+}
+
+// This is a helper to generate testing data to use in tests.
+std::string ExploreSitesServiceImplTest::CreateTestDataProto() {
+  std::string serialized_protobuf;
+  explore_sites::GetCatalogResponse catalog_response;
+  explore_sites::Catalog* catalog = catalog_response.mutable_catalog();
+  explore_sites::Category* category = catalog->add_categories();
+  explore_sites::Site* site1 = category->add_sites();
+  explore_sites::Site* site2 = category->add_sites();
+
+  // Fill in fields we need to add to the EoS database.
+
+  // Create two sites.
+  site1->set_site_url(kSite1Url);
+  site1->set_title(kSite1Name);
+  site2->set_site_url(kSite2Url);
+  site2->set_title(kSite2Name);
+
+  // Create one category, technology.
+  category->set_type(Category_CategoryType_TECHNOLOGY);
+  category->set_localized_title(kCategoryName);
+
+  // Serialize it into a string.
+  catalog_response.SerializeToString(&serialized_protobuf);
+
+  // Print out the string
+  DVLOG(1) << "test data proto '" << serialized_protobuf << "'";
+
+  return serialized_protobuf;
+}
+
+TEST_F(ExploreSitesServiceImplTest, UpdateCatalogFromNetwork) {
+  std::string output_data;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(chrome::android::kExploreSites);
+
+  service()->UpdateCatalogFromNetwork(
+      base::BindOnce(&ExploreSitesServiceImplTest::UpdateCatalogDoneCallback,
+                     base::Unretained(this)));
+
+  // Simulate fetching using the test loader factory and test data.
+  SimulateFetcherData(test_data());
+
+  // Wait for callback to get called.
+  PumpLoop();
+
+  EXPECT_TRUE(success());
+
+  // Get the catalog and verify the contents.
+
+  // First call is to get update_catalog out of the way.  If GetCatalog has
+  // never been called before in this session, it won't return anything, it will
+  // just start the update process.  For our test, we've already put data into
+  // the catalog, but GetCatalog doesn't know that.
+  // TODO(petewil): Fix get catalog so it always returns data if it has some.
+  service()->GetCatalog(base::BindOnce(
+      &ExploreSitesServiceImplTest::CatalogCallback, base::Unretained(this)));
+  // Second call is to get the actual catalog sata into the update callback.
+  service()->GetCatalog(base::BindOnce(
+      &ExploreSitesServiceImplTest::CatalogCallback, base::Unretained(this)));
+  PumpLoop();
+
+  EXPECT_NE(nullptr, database_categories());
+  EXPECT_EQ(1U, database_categories()->size());
+
+  const ExploreSitesCategory& category = database_categories()->at(0);
+  EXPECT_EQ(Category_CategoryType_TECHNOLOGY, category.category_type);
+  EXPECT_EQ(std::string(kCategoryName), category.label);
+  EXPECT_EQ(2U, category.sites.size());
+
+  // Since the site name and url might come back in a different order than we
+  // started with, accept either order as long as one name and url match.
+  std::string site1Url = category.sites[0].url.spec();
+  std::string site2Url = category.sites[1].url.spec();
+  std::string site1Name = category.sites[0].title;
+  std::string site2Name = category.sites[1].title;
+  EXPECT_TRUE(site1Url == kSite1Url || site1Url == kSite2Url);
+  EXPECT_TRUE(site2Url == kSite1Url || site2Url == kSite2Url);
+  EXPECT_TRUE(site1Name == kSite1Name || site1Name == kSite2Name);
+  EXPECT_TRUE(site2Name == kSite1Name || site2Name == kSite2Name);
+}
+
+}  // namespace explore_sites

chrome/browser/android/explore_sites/explore_sites_types.h

@@ -56,6 +56,7 @@ struct ExploreSitesCategory {
 
 using CatalogCallback = base::OnceCallback<void(
     std::unique_ptr<std::vector<ExploreSitesCategory>>)>;
+using BooleanCallback = base::OnceCallback<void(bool)>;
 using EncodedImageBytes = std::vector<uint8_t>;
 using EncodedImageList = std::vector<std::unique_ptr<EncodedImageBytes>>;
 using EncodedImageListCallback = base::OnceCallback<void(EncodedImageList)>;

chrome/browser/android/explore_sites/import_catalog_task.cc

@@ -107,10 +107,12 @@ bool ImportCatalogSync(std::string version_token,
 
 ImportCatalogTask::ImportCatalogTask(ExploreSitesStore* store,
                                      std::string version_token,
-                                     std::unique_ptr<Catalog> catalog_proto)
+                                     std::unique_ptr<Catalog> catalog_proto,
+                                     BooleanCallback callback)
     : store_(store),
       version_token_(version_token),
       catalog_proto_(std::move(catalog_proto)),
+      callback_(std::move(callback)),
       weak_ptr_factory_(this) {}
 
 ImportCatalogTask::~ImportCatalogTask() = default;
@@ -128,6 +130,7 @@ void ImportCatalogTask::FinishedExecuting(bool result) {
   result_ = result;
   TaskComplete();
   DVLOG(1) << "Finished importing the catalog, result: " << result;
+  std::move(callback_).Run(result);
 }
 
 }  // namespace explore_sites

chrome/browser/android/explore_sites/import_catalog_task.h

@@ -7,6 +7,7 @@
 
 #include "chrome/browser/android/explore_sites/catalog.pb.h"
 #include "chrome/browser/android/explore_sites/explore_sites_store.h"
+#include "chrome/browser/android/explore_sites/explore_sites_types.h"
 #include "components/offline_pages/task/task.h"
 
 using offline_pages::Task;
@@ -24,7 +25,8 @@ class ImportCatalogTask : public Task {
  public:
   ImportCatalogTask(ExploreSitesStore* store,
                     std::string version_token,
-                    std::unique_ptr<Catalog> catalog_proto);
+                    std::unique_ptr<Catalog> catalog_proto,
+                    BooleanCallback callback);
   ~ImportCatalogTask() override;
 
   bool complete() const { return complete_; }
@@ -42,6 +44,7 @@ class ImportCatalogTask : public Task {
 
   bool complete_ = false;
   bool result_ = false;
+  BooleanCallback callback_;
 
   base::WeakPtrFactory<ImportCatalogTask> weak_ptr_factory_;
 };

chrome/browser/android/explore_sites/import_catalog_task_unittest.cc

@@ -40,6 +40,8 @@ class ExploreSitesImportCatalogTaskTest : public TaskTestBase {
 
   void SetUp() override {
     store_ = std::make_unique<ExploreSitesStore>(task_runner());
+    success_ = false;
+    callback_called_ = false;
   }
 
   ExploreSitesStore* store() { return store_.get(); }
@@ -51,15 +53,29 @@ class ExploreSitesImportCatalogTaskTest : public TaskTestBase {
     RunUntilIdle();
   }
 
+  void OnImportTaskDone(bool success) {
+    success_ = success;
+    callback_called_ = true;
+  }
+
+  bool success() { return success_; }
+
+  bool callback_called() { return callback_called_; }
+
  private:
   std::unique_ptr<ExploreSitesStore> store_;
+  bool success_;
+  bool callback_called_;
 
   DISALLOW_COPY_AND_ASSIGN(ExploreSitesImportCatalogTaskTest);
 };
 
 TEST_F(ExploreSitesImportCatalogTaskTest, StoreFailure) {
   store()->SetInitializationStatusForTest(InitializationStatus::FAILURE);
-  ImportCatalogTask task(store(), kVersionToken, std::make_unique<Catalog>());
+  ImportCatalogTask task(
+      store(), kVersionToken, std::make_unique<Catalog>(),
+      base::BindOnce(&ExploreSitesImportCatalogTaskTest::OnImportTaskDone,
+                     base::Unretained(this)));
   RunTask(&task);
 
   // A null catalog should be completed but return with an error.
@@ -68,7 +84,10 @@ TEST_F(ExploreSitesImportCatalogTaskTest, StoreFailure) {
 }
 
 TEST_F(ExploreSitesImportCatalogTaskTest, EmptyTask) {
-  ImportCatalogTask task(store(), kVersionToken, std::unique_ptr<Catalog>());
+  ImportCatalogTask task(
+      store(), kVersionToken, std::unique_ptr<Catalog>(),
+      base::BindOnce(&ExploreSitesImportCatalogTaskTest::OnImportTaskDone,
+                     base::Unretained(this)));
   RunTask(&task);
 
   // A null catalog should be completed but return with an error.
@@ -81,13 +100,19 @@ TEST_F(ExploreSitesImportCatalogTaskTest, EmptyTask) {
 // where it is the "current" catalog, and where it is the "downloading" catalog.
 TEST_F(ExploreSitesImportCatalogTaskTest, CatalogAlreadyInUse) {
   // Successfully import a catalog with "version_token".
-  ImportCatalogTask task(store(), kVersionToken, std::make_unique<Catalog>());
+  ImportCatalogTask task(
+      store(), kVersionToken, std::make_unique<Catalog>(),
+      base::BindOnce(&ExploreSitesImportCatalogTaskTest::OnImportTaskDone,
+                     base::Unretained(this)));
   RunTask(&task);
   ASSERT_TRUE(task.result());
 
   // Importing the same catalog again should cause a successful import,
   // since the catalog was not "current".
-  ImportCatalogTask task2(store(), kVersionToken, std::make_unique<Catalog>());
+  ImportCatalogTask task2(
+      store(), kVersionToken, std::make_unique<Catalog>(),
+      base::BindOnce(&ExploreSitesImportCatalogTaskTest::OnImportTaskDone,
+                     base::Unretained(this)));
   RunTask(&task2);
   EXPECT_TRUE(task2.result());
 
@@ -101,7 +126,10 @@ TEST_F(ExploreSitesImportCatalogTaskTest, CatalogAlreadyInUse) {
   }));
 
   // Now it should fail to import another copy of the same catalog.
-  ImportCatalogTask task3(store(), kVersionToken, std::make_unique<Catalog>());
+  ImportCatalogTask task3(
+      store(), kVersionToken, std::make_unique<Catalog>(),
+      base::BindOnce(&ExploreSitesImportCatalogTaskTest::OnImportTaskDone,
+                     base::Unretained(this)));
   RunTask(&task3);
   EXPECT_TRUE(task3.complete());
   EXPECT_FALSE(task3.result());
@@ -125,11 +153,16 @@ TEST_F(ExploreSitesImportCatalogTaskTest, BasicCatalog) {
   gmail->set_site_url(kGmailUrl);
   gmail->set_icon(kIcon);
 
-  ImportCatalogTask task(store(), kVersionToken, std::move(catalog));
+  ImportCatalogTask task(
+      store(), kVersionToken, std::move(catalog),
+      base::BindOnce(&ExploreSitesImportCatalogTaskTest::OnImportTaskDone,
+                     base::Unretained(this)));
   RunTask(&task);
 
   EXPECT_TRUE(task.complete());
   EXPECT_TRUE(task.result());
+  EXPECT_TRUE(success());
+  EXPECT_TRUE(callback_called());
 
   ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) {
     sql::Statement cat_count_s(

chrome/test/BUILD.gn

@@ -2320,6 +2320,7 @@ test("unit_tests") {
     "../browser/android/explore_sites/explore_sites_feature_unittest.cc",
     "../browser/android/explore_sites/explore_sites_fetcher_unittest.cc",
     "../browser/android/explore_sites/explore_sites_schema_unittest.cc",
+    "../browser/android/explore_sites/explore_sites_service_impl_unittest.cc",
     "../browser/android/explore_sites/explore_sites_store_unittest.cc",
     "../browser/android/explore_sites/get_catalog_task_unittest.cc",
     "../browser/android/explore_sites/get_images_task_unittest.cc",