[EoS] Send up the existing version_token (if any) when requesting the catalog.

This fixes a TODO where we weren't requesting that the server send us an
empty catalog when we have already downloaded the current version.

Bug: 898327
Change-Id: I39c0911a71947ac553ef224830d97fcdd39eed3d
Reviewed-on: https://chromium-review.googlesource.com/c/1297607Reviewed-by: Cathy Li <chili@chromium.org>
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602402}

chrome/browser/BUILD.gn

@@ -2177,6 +2177,8 @@ jumbo_split_static_library("browser") {
       "android/explore_sites/get_catalog_task.h",
       "android/explore_sites/get_images_task.cc",
       "android/explore_sites/get_images_task.h",
+      "android/explore_sites/get_version_task.cc",
+      "android/explore_sites/get_version_task.h",
       "android/explore_sites/history_statistics_reporter.cc",
       "android/explore_sites/history_statistics_reporter.h",
       "android/explore_sites/image_helper.cc",

chrome/browser/android/explore_sites/explore_sites_service_impl.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/android/explore_sites/explore_sites_store.h"
 #include "chrome/browser/android/explore_sites/get_catalog_task.h"
 #include "chrome/browser/android/explore_sites/get_images_task.h"
+#include "chrome/browser/android/explore_sites/get_version_task.h"
 #include "chrome/browser/android/explore_sites/image_helper.h"
 #include "chrome/browser/android/explore_sites/import_catalog_task.h"
 #include "components/offline_pages/task/task.h"
@@ -92,17 +93,13 @@ void ExploreSitesServiceImpl::UpdateCatalogFromNetwork(
       explore_sites_fetcher_->RestartAsImmediateFetchIfNotYet();
     return;
   }
-
-  // TODO(petewil): Eventually get the catalog version from DB.
-  std::string catalog_version = "";
-
-  // Create a fetcher and start fetching the protobuf (async).
-  explore_sites_fetcher_ = ExploreSitesFetcher::CreateForGetCatalog(
-      is_immediate_fetch, catalog_version, accept_languages,
-      url_loader_factory_,
-      base::BindOnce(&ExploreSitesServiceImpl::OnCatalogFetched,
-                     weak_ptr_factory_.GetWeakPtr()));
-  explore_sites_fetcher_->Start();
+  // We want to create the fetcher, but need to grab the current version from
+  // the DB first.
+  task_queue_.AddTask(std::make_unique<GetVersionTask>(
+      explore_sites_store_.get(),
+      base::BindOnce(&ExploreSitesServiceImpl::GotVersionToStartFetch,
+                     weak_ptr_factory_.GetWeakPtr(), is_immediate_fetch,
+                     accept_languages)));
 }
 
 void ExploreSitesServiceImpl::BlacklistSite(const std::string& url) {
@@ -126,6 +123,24 @@ void ValidateCatalog(Catalog* catalog) {
   }
 }
 
+void ExploreSitesServiceImpl::GotVersionToStartFetch(
+    bool is_immediate_fetch,
+    const std::string& accept_languages,
+    std::string catalog_version) {
+  if (explore_sites_fetcher_) {
+    // There was a race.
+    return;
+  }
+
+  // Create a fetcher and start fetching the protobuf (async).
+  explore_sites_fetcher_ = ExploreSitesFetcher::CreateForGetCatalog(
+      is_immediate_fetch, catalog_version, accept_languages,
+      url_loader_factory_,
+      base::BindOnce(&ExploreSitesServiceImpl::OnCatalogFetched,
+                     weak_ptr_factory_.GetWeakPtr()));
+  explore_sites_fetcher_->Start();
+}
+
 void ExploreSitesServiceImpl::OnCatalogFetched(
     ExploreSitesRequestStatus status,
     std::unique_ptr<std::string> serialized_protobuf) {
@@ -153,14 +168,18 @@ void ExploreSitesServiceImpl::OnCatalogFetched(
   std::unique_ptr<Catalog> catalog(catalog_response->release_catalog());
 
   // Check the catalog, canonicalizing any URLs in it.
-  ValidateCatalog(catalog.get());
-
-  // Add the catalog to our internal database.
-  task_queue_.AddTask(std::make_unique<ImportCatalogTask>(
-      explore_sites_store_.get(), catalog_version, std::move(catalog),
-      base::BindOnce(&ExploreSitesServiceImpl::NotifyCatalogUpdated,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     std::move(update_catalog_callbacks_))));
+  if (catalog) {
+    ValidateCatalog(catalog.get());
+
+    // Add the catalog to our internal database.
+    task_queue_.AddTask(std::make_unique<ImportCatalogTask>(
+        explore_sites_store_.get(), catalog_version, std::move(catalog),
+        base::BindOnce(&ExploreSitesServiceImpl::NotifyCatalogUpdated,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       std::move(update_catalog_callbacks_))));
+  } else {
+    NotifyCatalogUpdated(std::move(update_catalog_callbacks_), true);
+  }
   update_catalog_callbacks_.clear();
 }
 

chrome/browser/android/explore_sites/explore_sites_service_impl.h

@@ -58,6 +58,11 @@ class ExploreSitesServiceImpl : public ExploreSitesService,
   // TaskQueue::Delegate implementation:
   void OnTaskQueueIsIdle() override;
 
+  // Callback that is run when we have the catalog version.
+  void GotVersionToStartFetch(bool is_immediate_fetch,
+                              const std::string& accept_languages,
+                              std::string catalog_version);
+
   // Callback returning from the UpdateCatalogFromNetwork operation.  It
   // passes along the call back to the bridge and eventually back to Java land.
   void OnCatalogFetched(ExploreSitesRequestStatus status,

chrome/browser/android/explore_sites/explore_sites_service_impl_unittest.cc

@@ -14,6 +14,7 @@
 #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/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -28,6 +29,9 @@ const char kAcceptLanguages[] = "en-US,en;q=0.5";
 
 namespace explore_sites {
 
+using testing::HasSubstr;
+using testing::Not;
+
 class ExploreSitesServiceImplTest : public testing::Test {
  public:
   ExploreSitesServiceImplTest();
@@ -77,9 +81,11 @@ class ExploreSitesServiceImplTest : public testing::Test {
 
   void SimulateFetcherData(const std::string& response_data);
 
- private:
   network::TestURLLoaderFactory::PendingRequest* GetLastPendingRequest();
 
+  void ValidateTestCatalog();
+
+ private:
   std::unique_ptr<explore_sites::ExploreSitesServiceImpl> service_;
   bool success_;
   int callback_count_;
@@ -110,6 +116,8 @@ ExploreSitesServiceImplTest::ExploreSitesServiceImplTest()
 // response from the network.
 void ExploreSitesServiceImplTest::SimulateFetcherData(
     const std::string& response_data) {
+  PumpLoop();
+
   DCHECK(test_url_loader_factory_.pending_requests()->size() > 0);
   test_url_loader_factory_.SimulateResponseForPendingRequest(
       GetLastPendingRequest()->request.url.spec(), response_data, net::HTTP_OK,
@@ -124,6 +132,30 @@ ExploreSitesServiceImplTest::GetLastPendingRequest() {
   return request;
 }
 
+void ExploreSitesServiceImplTest::ValidateTestCatalog() {
+  EXPECT_EQ(GetCatalogStatus::kSuccess, database_status());
+  EXPECT_NE(nullptr, database_categories());
+  EXPECT_EQ(1U, database_categories()->size());
+
+  const ExploreSitesCategory& database_category = database_categories()->at(0);
+  EXPECT_EQ(Category_CategoryType_TECHNOLOGY, database_category.category_type);
+  EXPECT_EQ(std::string(kCategoryName), database_category.label);
+  EXPECT_EQ(2U, database_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.
+  EXPECT_NE(database_category.sites[0].site_id,
+            database_category.sites[1].site_id);
+  std::string site1Url = database_category.sites[0].url.spec();
+  std::string site2Url = database_category.sites[1].url.spec();
+  std::string site1Name = database_category.sites[0].title;
+  std::string site2Name = database_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);
+}
+
 // This is a helper to generate testing data to use in tests.
 std::string ExploreSitesServiceImplTest::CreateTestDataProto() {
   std::string serialized_protobuf;
@@ -184,25 +216,7 @@ TEST_F(ExploreSitesServiceImplTest, UpdateCatalogFromNetwork) {
       &ExploreSitesServiceImplTest::CatalogCallback, base::Unretained(this)));
   PumpLoop();
 
-  EXPECT_EQ(GetCatalogStatus::kSuccess, database_status());
-  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);
+  ValidateTestCatalog();
 }
 
 TEST_F(ExploreSitesServiceImplTest, MultipleUpdateCatalogFromNetwork) {
@@ -234,4 +248,50 @@ TEST_F(ExploreSitesServiceImplTest, MultipleUpdateCatalogFromNetwork) {
   EXPECT_EQ(3, callback_count());
 }
 
+TEST_F(ExploreSitesServiceImplTest, GetCachedCatalogFromNetwork) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(chrome::android::kExploreSites);
+
+  service()->UpdateCatalogFromNetwork(
+      false /*is_immediate_fetch*/, kAcceptLanguages,
+      base::BindOnce(&ExploreSitesServiceImplTest::UpdateCatalogDoneCallback,
+                     base::Unretained(this)));
+  PumpLoop();
+  EXPECT_THAT(GetLastPendingRequest()->request.url.query(),
+              Not(HasSubstr("version_token=abcd")));
+
+  SimulateFetcherData(test_data());
+  PumpLoop();
+  EXPECT_TRUE(success());
+
+  service()->UpdateCatalogFromNetwork(
+      false /*is_immediate_fetch*/, kAcceptLanguages,
+      base::BindOnce(&ExploreSitesServiceImplTest::UpdateCatalogDoneCallback,
+                     base::Unretained(this)));
+  PumpLoop();
+  EXPECT_THAT(GetLastPendingRequest()->request.url.query(),
+              HasSubstr("version_token=abcd"));
+
+  explore_sites::GetCatalogResponse catalog_response;
+  catalog_response.set_version_token("abcd");
+  std::string serialized_response;
+  catalog_response.SerializeToString(&serialized_response);
+  SimulateFetcherData(serialized_response);
+
+  PumpLoop();
+
+  // 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)));
+  PumpLoop();
+
+  ValidateTestCatalog();
+}
+
 }  // namespace explore_sites

chrome/browser/android/explore_sites/get_version_task.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/get_version_task.h"
+
+#include <string>
+
+#include "chrome/browser/android/explore_sites/explore_sites_schema.h"
+#include "sql/database.h"
+#include "sql/meta_table.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace explore_sites {
+
+std::string GetVersionSync(sql::Database* db) {
+  if (!db)
+    return "";
+
+  sql::MetaTable meta_table;
+  if (!ExploreSitesSchema::InitMetaTable(db, &meta_table))
+    return "";
+
+  std::string catalog_version_token;
+  if (meta_table.GetValue("downloading_catalog", &catalog_version_token) &&
+      !catalog_version_token.empty()) {
+    return catalog_version_token;
+  }
+
+  catalog_version_token = "";
+  if (meta_table.GetValue("current_catalog", &catalog_version_token) &&
+      !catalog_version_token.empty()) {
+    return catalog_version_token;
+  }
+
+  return "";
+}
+
+GetVersionTask::GetVersionTask(ExploreSitesStore* store,
+                               base::OnceCallback<void(std::string)> callback)
+    : store_(store), callback_(std::move(callback)), weak_ptr_factory_(this) {}
+
+GetVersionTask::~GetVersionTask() = default;
+
+void GetVersionTask::Run() {
+  store_->Execute(base::BindOnce(&GetVersionSync),
+                  base::BindOnce(&GetVersionTask::FinishedExecuting,
+                                 weak_ptr_factory_.GetWeakPtr()),
+                  std::string());
+}
+
+void GetVersionTask::FinishedExecuting(std::string catalog_version_token) {
+  TaskComplete();
+  std::move(callback_).Run(catalog_version_token);
+}
+
+}  // namespace explore_sites

chrome/browser/android/explore_sites/get_version_task.h

+// 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.
+
+#ifndef CHROME_BROWSER_ANDROID_EXPLORE_SITES_GET_VERSION_TASK_H_
+#define CHROME_BROWSER_ANDROID_EXPLORE_SITES_GET_VERSION_TASK_H_
+
+#include "base/callback.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;
+
+namespace explore_sites {
+
+// Fetches the version of the catalog that we have currently. If we are in
+// the state where the catalog is already downloaded but not yet in use, we
+// return the "downloading" catalog version.
+class GetVersionTask : public Task {
+ public:
+  GetVersionTask(ExploreSitesStore* store,
+                 base::OnceCallback<void(std::string)> callback);
+  ~GetVersionTask() override;
+
+ private:
+  // Task implementation:
+  void Run() override;
+
+  void FinishedExecuting(std::string result);
+
+  ExploreSitesStore* store_;  // outlives this class.
+  base::OnceCallback<void(std::string)> callback_;
+  base::WeakPtrFactory<GetVersionTask> weak_ptr_factory_;
+};
+
+}  // namespace explore_sites
+
+#endif  // CHROME_BROWSER_ANDROID_EXPLORE_SITES_GET_VERSION_TASK_H_

chrome/browser/android/explore_sites/get_version_task_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/get_version_task.h"
+
+#include <memory>
+
+#include "base/logging.h"
+#include "base/test/bind_test_util.h"
+#include "base/test/mock_callback.h"
+#include "chrome/browser/android/explore_sites/explore_sites_schema.h"
+#include "components/offline_pages/task/task.h"
+#include "components/offline_pages/task/task_test_base.h"
+#include "sql/database.h"
+#include "sql/meta_table.h"
+#include "sql/statement.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using offline_pages::TaskTestBase;
+
+namespace explore_sites {
+
+class ExploreSitesGetVersionTaskTest : public TaskTestBase {
+ public:
+  ExploreSitesGetVersionTaskTest() = default;
+  ~ExploreSitesGetVersionTaskTest() override = default;
+
+  void SetUp() override {
+    store_ = std::make_unique<ExploreSitesStore>(task_runner());
+    success_ = false;
+    callback_called_ = false;
+  }
+
+  ExploreSitesStore* store() { return store_.get(); }
+
+  void ExecuteSync(base::RepeatingCallback<bool(sql::Database*)> query) {
+    store()->Execute(base::OnceCallback<bool(sql::Database*)>(query),
+                     base::BindOnce([](bool result) { ASSERT_TRUE(result); }),
+                     false);
+    RunUntilIdle();
+  }
+
+  void SetCurrentVersion(std::string version) {
+    SetVersion("current_catalog", version);
+  }
+
+  void SetDownloadingVersion(std::string version) {
+    SetVersion("downloading_catalog", version);
+  }
+
+  void SetVersion(const char* key, std::string version) {
+    ExecuteSync(base::BindLambdaForTesting([&](sql::Database* db) {
+      sql::MetaTable meta_table;
+      if (!ExploreSitesSchema::InitMetaTable(db, &meta_table))
+        return false;
+
+      return meta_table.SetValue(key, version);
+    }));
+  }
+
+ private:
+  std::unique_ptr<ExploreSitesStore> store_;
+  bool success_;
+  bool callback_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExploreSitesGetVersionTaskTest);
+};
+
+TEST_F(ExploreSitesGetVersionTaskTest, StoreFailure) {
+  store()->SetInitializationStatusForTest(InitializationStatus::FAILURE);
+  GetVersionTask task(store(),
+                      base::BindLambdaForTesting(
+                          [&](std::string result) { EXPECT_EQ("", result); }));
+  RunTask(&task);
+}
+
+TEST_F(ExploreSitesGetVersionTaskTest, NoVersionsAvailable) {
+  std::string version = "unexpected";
+  GetVersionTask task(store(),
+                      base::BindLambdaForTesting(
+                          [&](std::string result) { version = result; }));
+  RunTask(&task);
+  EXPECT_EQ("", version);
+}
+
+TEST_F(ExploreSitesGetVersionTaskTest, CurrentCatalogNoDownloadingCatalog) {
+  std::string current_version = "1234x";
+  SetCurrentVersion(current_version);
+  std::string version;
+  GetVersionTask task(store(),
+                      base::BindLambdaForTesting(
+                          [&](std::string result) { version = result; }));
+  RunTask(&task);
+
+  EXPECT_EQ(current_version, version);
+}
+
+TEST_F(ExploreSitesGetVersionTaskTest, CurrentCatalogAndDownloadingCatalog) {
+  std::string current_version = "1234x";
+  SetCurrentVersion(current_version);
+  std::string downloading_version = "5678x";
+  SetDownloadingVersion(downloading_version);
+  std::string version;
+  GetVersionTask task(store(),
+                      base::BindLambdaForTesting(
+                          [&](std::string result) { version = result; }));
+  RunTask(&task);
+
+  EXPECT_EQ(downloading_version, version);
+}
+
+}  // namespace explore_sites

chrome/test/BUILD.gn

@@ -2319,6 +2319,7 @@ test("unit_tests") {
     "../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",
+    "../browser/android/explore_sites/get_version_task_unittest.cc",
     "../browser/android/explore_sites/history_statistics_reporter_unittest.cc",
     "../browser/android/explore_sites/image_helper_unittest.cc",
     "../browser/android/explore_sites/import_catalog_task_unittest.cc",