[Shopping service] Add API to wait for service to be ready

This patch adds an API that allows clients to wait until the shopping
services' dependencies are ready to be queried. In practice, we're
currently only waiting for sync to finish initialization. If not
ready, the callback is delayed until it is, otherwise it is executed
immediately. This will be a benefit to UI that shows close to startup
and is already using the async APIs in the shopping service. The
WebUI handler has been updated to use this.

Worth noting: we're not actually blocked on sync, we just need to
confirm the consent to use sync is available.

Bug: b:283829151
Change-Id: I370f976dda255a98baff53344bfd4e23bbb8f881
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4552801
Commit-Queue: Matthew Jones <mdjones@chromium.org>
Reviewed-by: Mei Liang <meiliang@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149768}

components/commerce/core/BUILD.gn

@@ -206,6 +206,7 @@ static_library("shopping_service") {
     "//components/session_proto_db:core",
     "//components/signin/public/identity_manager:identity_manager",
     "//components/sync/service",
+    "//components/unified_consent",
     "//services/network/public/cpp:cpp",
     "//ui/base",
     "//url:url",

components/commerce/core/mock_shopping_service.cc

@@ -26,6 +26,7 @@ MockShoppingService::MockShoppingService()
                                 nullptr,
                                 nullptr) {
   // Set up some defaults so tests don't have to explicitly set up each.
+  SetIsReady(true);
   SetResponseForGetProductInfoForUrl(absl::nullopt);
   SetResponsesForGetUpdatedProductInfoForBookmarks(
       std::map<int64_t, ProductInfo>());
@@ -145,6 +146,17 @@ void MockShoppingService::SetIsShoppingListEligible(bool eligible) {
       .WillByDefault(testing::Return(eligible));
 }
 
+void MockShoppingService::SetIsReady(bool ready) {
+  ON_CALL(*this, WaitForReady)
+      .WillByDefault(
+          [ready, this](base::OnceCallback<void(ShoppingService*)> callback) {
+            if (ready) {
+              base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+                  FROM_HERE, base::BindOnce(std::move(callback), this));
+            }
+          });
+}
+
 void MockShoppingService::SetIsClusterIdTrackedByUserResponse(bool is_tracked) {
   ON_CALL(*this, IsClusterIdTrackedByUser)
       .WillByDefault([is_tracked](uint64_t cluster_id,

components/commerce/core/mock_shopping_service.h

@@ -83,6 +83,10 @@ class MockShoppingService : public commerce::ShoppingService {
               (override));
   MOCK_METHOD(void, ScheduleSavedProductUpdate, (), (override));
   MOCK_METHOD(bool, IsShoppingListEligible, (), (override));
+  MOCK_METHOD(void,
+              WaitForReady,
+              (base::OnceCallback<void(ShoppingService*)>),
+              (override));
   MOCK_METHOD(void,
               IsClusterIdTrackedByUser,
               (uint64_t cluster_id, base::OnceCallback<void(bool)> callback),
@@ -101,6 +105,7 @@ class MockShoppingService : public commerce::ShoppingService {
   void SetGetAllSubscriptionsCallbackValue(
       std::vector<CommerceSubscription> subscriptions);
   void SetIsShoppingListEligible(bool enabled);
+  void SetIsReady(bool ready);
   void SetIsClusterIdTrackedByUserResponse(bool is_tracked);
   void SetIsMerchantViewerEnabled(bool is_enabled);
   void SetGetAllPriceTrackedBookmarksCallbackValue(

components/commerce/core/shopping_service.cc

@@ -47,6 +47,7 @@
 #include "components/search/ntp_features.h"
 #include "components/session_proto_db/session_proto_storage.h"
 #include "components/sync/service/sync_service.h"
+#include "components/unified_consent/url_keyed_data_collection_consent_helper.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "ui/base/resource/resource_bundle.h"
 
@@ -104,8 +105,13 @@ ShoppingService::ShoppingService(
       locale_on_startup_(locale_on_startup),
       opt_guide_(opt_guide),
       pref_service_(pref_service),
+      sync_service_(sync_service),
       bookmark_model_(bookmark_model),
       power_bookmark_service_(power_bookmark_service),
+      bookmark_consent_throttle_(
+          unified_consent::UrlKeyedDataCollectionConsentHelper::
+              NewPersonalizedBookmarksDataCollectionConsentHelper(
+                  sync_service)),
       weak_ptr_factory_(this) {
   // Register for the types of information we're allowed to receive from
   // optimization guide.
@@ -957,6 +963,24 @@ bool ShoppingService::IsShoppingListEligible() {
                                 country_on_startup_, locale_on_startup_);
 }
 
+void ShoppingService::WaitForReady(
+    base::OnceCallback<void(ShoppingService*)> callback) {
+  bookmark_consent_throttle_.EnqueueRequest(base::BindOnce(
+      [](base::WeakPtr<ShoppingService> service,
+         syncer::SyncService* sync_service,
+         base::OnceCallback<void(ShoppingService*)> callback,
+         bool has_consent) {
+        if (service.WasInvalidated() || !sync_service ||
+            sync_service->GetTransportState() !=
+                syncer::SyncService::TransportState::ACTIVE) {
+          std::move(callback).Run(nullptr);
+          return;
+        }
+        std::move(callback).Run(service.get());
+      },
+      AsWeakPtr(), sync_service_, std::move(callback)));
+}
+
 bool ShoppingService::IsShoppingListEligible(AccountChecker* account_checker,
                                              PrefService* prefs,
                                              const std::string& country_code,

components/commerce/core/shopping_service.h

@@ -25,6 +25,7 @@
 #include "components/commerce/core/subscriptions/commerce_subscription.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/optimization_guide/core/optimization_guide_decision.h"
+#include "components/unified_consent/consent_throttle.h"
 #include "services/data_decoder/public/cpp/data_decoder.h"
 
 class GURL;
@@ -51,13 +52,17 @@ class NewOptimizationGuideDecider;
 class OptimizationMetadata;
 }  // namespace optimization_guide
 
+namespace power_bookmarks {
+class PowerBookmarkService;
+}  // namespace power_bookmarks
+
 namespace signin {
 class IdentityManager;
 }  // namespace signin
 
-namespace power_bookmarks {
-class PowerBookmarkService;
-}  // namespace power_bookmarks
+namespace syncer {
+class SyncService;
+}  // namespace syncer
 
 namespace commerce {
 
@@ -309,6 +314,14 @@ class ShoppingService : public KeyedService, public base::SupportsUserData {
   // whether to create critical, feature-related infrastructure.
   virtual bool IsShoppingListEligible();
 
+  // Wait for the shopping service and all of its dependent components to be
+  // ready before attempting to access different features. This can be used for
+  // UI that is available shortly after startup. If the dependencies time out or
+  // the browser is being shut down, a null pointer to the shopping service will
+  // be passed to the callback.
+  virtual void WaitForReady(
+      base::OnceCallback<void(ShoppingService*)> callback);
+
   // Check whether a product (based on cluster ID) is explicitly price tracked
   // by the user.
   virtual void IsClusterIdTrackedByUser(
@@ -484,6 +497,8 @@ class ShoppingService : public KeyedService, public base::SupportsUserData {
 
   raw_ptr<PrefService> pref_service_;
 
+  raw_ptr<syncer::SyncService> sync_service_;
+
   raw_ptr<bookmarks::BookmarkModel> bookmark_model_;
 
   std::unique_ptr<AccountChecker> account_checker_;
@@ -512,6 +527,10 @@ class ShoppingService : public KeyedService, public base::SupportsUserData {
   std::unique_ptr<commerce::metrics::ScheduledMetricsManager>
       scheduled_metrics_manager_;
 
+  // A consent throttle that will hold callbacks until the specific consent is
+  // obtained.
+  unified_consent::ConsentThrottle bookmark_consent_throttle_;
+
   // Ensure certain functions are being executed on the same thread.
   SEQUENCE_CHECKER(sequence_checker_);
 

components/commerce/core/shopping_service_metrics_unittest.cc

@@ -46,6 +46,7 @@ class ShoppingServiceMetricsTest : public ShoppingServiceTestBase {
   ~ShoppingServiceMetricsTest() override = default;
 
   void SetUp() override {
+    ShoppingServiceTestBase::SetUp();
     histogram_tester_ = std::make_unique<base::HistogramTester>();
   }
 

components/commerce/core/shopping_service_test_base.cc

@@ -253,6 +253,11 @@ ShoppingServiceTestBase::ShoppingServiceTestBase()
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       optimization_guide::switches::kDisableCheckingUserPermissionsForTesting);
   RegisterPrefs(pref_service_->registry());
+}
+
+ShoppingServiceTestBase::~ShoppingServiceTestBase() = default;
+
+void ShoppingServiceTestBase::SetUp() {
   shopping_service_ = std::make_unique<ShoppingService>(
       "us", "en-us", bookmark_model_.get(), opt_guide_.get(),
       pref_service_.get(), identity_test_env_->identity_manager(),
@@ -262,8 +267,6 @@ ShoppingServiceTestBase::ShoppingServiceTestBase()
       nullptr, nullptr);
 }
 
-ShoppingServiceTestBase::~ShoppingServiceTestBase() = default;
-
 void ShoppingServiceTestBase::TestBody() {}
 
 void ShoppingServiceTestBase::TearDown() {

components/commerce/core/shopping_service_test_base.h

@@ -157,6 +157,8 @@ class ShoppingServiceTestBase : public testing::Test {
   ShoppingServiceTestBase operator=(const ShoppingServiceTestBase&) = delete;
   ~ShoppingServiceTestBase() override;
 
+  void SetUp() override;
+
   void TestBody() override;
 
   void TearDown() override;

components/commerce/core/shopping_service_unittest.cc

@@ -23,6 +23,7 @@
 #include "components/power_bookmarks/core/proto/shopping_specifics.pb.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/search/ntp_features.h"
+#include "components/sync/test/test_sync_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using optimization_guide::OptimizationGuideDecision;
@@ -779,4 +780,63 @@ TEST_F(ShoppingServiceTest,
   ASSERT_FALSE(IsShoppingListEligible(&checker, &prefs, "ZZ", "zz-zz"));
 }
 
+class ShoppingServiceReadyTest : public ShoppingServiceTest {
+ public:
+  ShoppingServiceReadyTest() = default;
+  ShoppingServiceReadyTest(const ShoppingServiceReadyTest&) = delete;
+  ShoppingServiceReadyTest operator=(const ShoppingServiceReadyTest&) = delete;
+  ~ShoppingServiceReadyTest() override = default;
+
+  void SetUp() override {
+    sync_service_->SetTransportState(
+        syncer::SyncService::TransportState::INITIALIZING);
+    ShoppingServiceTest::SetUp();
+  }
+};
+
+TEST_F(ShoppingServiceReadyTest, TestServiceReadyDelaysForSync) {
+  test_features_.InitWithFeatures({kShoppingList}, {});
+
+  bool service_ready = false;
+  shopping_service_->WaitForReady(
+      base::BindOnce([](bool* service_ready,
+                        ShoppingService* service) { *service_ready = true; },
+                     &service_ready));
+
+  base::RunLoop().RunUntilIdle();
+
+  // The ready check should not have run since sync is not ready.
+  ASSERT_FALSE(service_ready);
+
+  sync_service_->SetHasSyncConsent(true);
+  sync_service_->SetInitialSyncFeatureSetupComplete(true);
+  sync_service_->SetTransportState(syncer::SyncService::TransportState::ACTIVE);
+  sync_service_->FireStateChanged();
+
+  base::RunLoop().RunUntilIdle();
+
+  // The run loop should be finished now.
+  ASSERT_TRUE(service_ready);
+}
+
+TEST_F(ShoppingServiceReadyTest, TestServiceReadyDelaysForSync_SyncActive) {
+  test_features_.InitWithFeatures({kShoppingList}, {});
+
+  sync_service_->SetHasSyncConsent(true);
+  sync_service_->SetInitialSyncFeatureSetupComplete(true);
+  sync_service_->SetTransportState(syncer::SyncService::TransportState::ACTIVE);
+  sync_service_->FireStateChanged();
+
+  bool service_ready = false;
+  shopping_service_->WaitForReady(
+      base::BindOnce([](bool* service_ready,
+                        ShoppingService* service) { *service_ready = true; },
+                     &service_ready));
+
+  base::RunLoop().RunUntilIdle();
+
+  // The ready check should complete since sync was already active.
+  ASSERT_TRUE(service_ready);
+}
+
 }  // namespace commerce

components/commerce/core/webui/shopping_list_handler.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <vector>
 
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/sequenced_task_runner.h"
@@ -104,15 +105,23 @@ ShoppingListHandler::~ShoppingListHandler() = default;
 
 void ShoppingListHandler::GetAllPriceTrackedBookmarkProductInfo(
     GetAllPriceTrackedBookmarkProductInfoCallback callback) {
-  if (!shopping_service_->IsShoppingListEligible()) {
-    base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback),
-                                  std::vector<BookmarkProductInfoPtr>()));
-    return;
-  }
-  shopping_service_->GetAllPriceTrackedBookmarks(
-      base::BindOnce(&ShoppingListHandler::OnFetchPriceTrackedBookmarks,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  shopping_service_->WaitForReady(base::BindOnce(
+      [](base::WeakPtr<ShoppingListHandler> handler,
+         GetAllPriceTrackedBookmarkProductInfoCallback callback,
+         ShoppingService* service) {
+        if (!service || !service->IsShoppingListEligible() ||
+            handler.WasInvalidated()) {
+          base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+              FROM_HERE, base::BindOnce(std::move(callback),
+                                        std::vector<BookmarkProductInfoPtr>()));
+          return;
+        }
+
+        service->GetAllPriceTrackedBookmarks(
+            base::BindOnce(&ShoppingListHandler::OnFetchPriceTrackedBookmarks,
+                           handler, std::move(callback)));
+      },
+      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void ShoppingListHandler::OnFetchPriceTrackedBookmarks(
@@ -131,17 +140,25 @@ void ShoppingListHandler::OnFetchPriceTrackedBookmarks(
 
 void ShoppingListHandler::GetAllShoppingBookmarkProductInfo(
     GetAllShoppingBookmarkProductInfoCallback callback) {
-  if (!shopping_service_->IsShoppingListEligible()) {
-    std::move(callback).Run({});
-    return;
-  }
-  std::vector<const bookmarks::BookmarkNode*> bookmarks =
-      shopping_service_->GetAllShoppingBookmarks();
-
-  std::vector<BookmarkProductInfoPtr> info_list =
-      BookmarkListToMojoList(*bookmark_model_, bookmarks, locale_);
-
-  std::move(callback).Run(std::move(info_list));
+  shopping_service_->WaitForReady(base::BindOnce(
+      [](base::WeakPtr<ShoppingListHandler> handler,
+         GetAllShoppingBookmarkProductInfoCallback callback,
+         ShoppingService* service) {
+        if (!service || !service->IsShoppingListEligible() ||
+            handler.WasInvalidated()) {
+          std::move(callback).Run({});
+          return;
+        }
+
+        std::vector<const bookmarks::BookmarkNode*> bookmarks =
+            service->GetAllShoppingBookmarks();
+
+        std::vector<BookmarkProductInfoPtr> info_list = BookmarkListToMojoList(
+            *(handler->bookmark_model_), bookmarks, handler->locale_);
+
+        std::move(callback).Run(std::move(info_list));
+      },
+      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
 void ShoppingListHandler::TrackPriceForBookmark(int64_t bookmark_id) {