[Password Manager] Change API of PasswordScriptsFetcher

This CL changes the API of PasswordScriptsFetcher to fit the current
design of Password Check on Android.

Bug: 1086114, 1092444

Change-Id: I4f617ec239fea9ef778874c94a2cdb442c242eb3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343080
Commit-Queue: Maxim Kolosovskiy  <kolos@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795966}

components/password_manager/core/browser/password_scripts_fetcher.h

@@ -21,18 +21,27 @@ class PasswordScriptsFetcher : public KeyedService {
   // Triggers pre-fetching the list of scripts. Should be called from UI
   // preceding Bulk Check.
   virtual void PrewarmCache() = 0;
-  // Reports metrics about the cache readiness. Should be called right before
-  // the first call of |GetPasswordScriptAvailability| within a given bulk
-  // check.
-  virtual void ReportCacheReadinessMetric() const = 0;
+  // Triggers fetching if the cache was never set or is stale (but doesn't
+  // trigger a duplicate request if another request is in-flight). Otherwise, it
+  // runs the callback immediately (the expected reaction as a call of
+  // |PrewarmCache| was supposed to fetch the data in advance). In case of
+  // several calls of the method, the callbacks will be called one after
+  // another.
+  virtual void Fetch(base::OnceClosure fetch_finished_callback) = 0;
   // Returns whether there is a password change script for |origin| via
-  // |callback|. If the cache was never set or is stale, it triggers a new
-  // network request (but doesn't trigger a duplicate request if another request
-  // is in-flight) and enqueues |callback|. Otherwise, it runs the callback
-  // immediately. In case of an network error, the verdict will default to no
-  // script being available.
-  virtual void GetPasswordScriptAvailability(const url::Origin& origin,
-                                             ResponseCallback callback) = 0;
+  // |callback|. If the cache was never set or is stale, it triggers a re-fetch.
+  // In case of a network error, the verdict will default to no script being
+  // available.
+  virtual void FetchScriptAvailability(const url::Origin& origin,
+                                       ResponseCallback callback) = 0;
+  // Immediately returns whether there is a password change script for |origin|.
+  // The method does NOT trigger any network requests if the cache is not ready
+  // or stale but reads the current state of the cache. In case of a network
+  // error while fetching the scripts, the result will always be false.
+  // TODO(crbug.com/1086114): It is better to deprecate this method and always
+  // use |FetchScriptAvailability| instead because |IsScriptAvailable| may
+  // return stale data.
+  virtual bool IsScriptAvailable(const url::Origin& origin) const = 0;
 };
 
 }  // namespace password_manager

components/password_manager/core/browser/password_scripts_fetcher_impl.cc

@@ -36,19 +36,35 @@ void PasswordScriptsFetcherImpl::PrewarmCache() {
     StartFetch();
 }
 
-void PasswordScriptsFetcherImpl::ReportCacheReadinessMetric() const {
+void PasswordScriptsFetcherImpl::Fetch(
+    base::OnceClosure fetch_finished_callback) {
   CacheState state = IsCacheStale()
                          ? (url_loader_ ? CacheState::kWaiting
                                         : (last_fetch_timestamp_.is_null()
                                                ? CacheState::kNeverSet
                                                : CacheState::kStale))
                          : CacheState::kReady;
-
   base::UmaHistogramEnumeration(
       "PasswordManager.PasswordScriptsFetcher.CacheState", state);
+
+  if (state == CacheState::kReady)
+    std::move(fetch_finished_callback).Run();
+  else
+    fetch_finished_callbacks_.emplace_back(std::move(fetch_finished_callback));
+
+  switch (state) {
+    case CacheState::kReady:
+    case CacheState::kWaiting:
+      // No new fetching.
+      break;
+    case CacheState::kNeverSet:
+    case CacheState::kStale:
+      StartFetch();
+      break;
+  }
 }
 
-void PasswordScriptsFetcherImpl::GetPasswordScriptAvailability(
+void PasswordScriptsFetcherImpl::FetchScriptAvailability(
     const url::Origin& origin,
     ResponseCallback callback) {
   if (IsCacheStale()) {
@@ -61,6 +77,12 @@ void PasswordScriptsFetcherImpl::GetPasswordScriptAvailability(
   RunResponseCallback(origin, std::move(callback));
 }
 
+bool PasswordScriptsFetcherImpl::IsScriptAvailable(
+    const url::Origin& origin) const {
+  return password_change_domains_.find(origin) !=
+         password_change_domains_.end();
+}
+
 void PasswordScriptsFetcherImpl::StartFetch() {
   static const base::NoDestructor<base::TimeDelta> kFetchTimeout(
       base::TimeDelta::FromMinutes(kFetchTimeoutInSeconds));
@@ -121,6 +143,8 @@ void PasswordScriptsFetcherImpl::OnFetchComplete(
   base::UmaHistogramEnumeration(
       "PasswordManager.PasswordScriptsFetcher.ParsingResult", parsing_result);
 
+  for (auto& callback : std::exchange(fetch_finished_callbacks_, {}))
+    std::move(callback).Run();
   for (auto& callback : std::exchange(pending_callbacks_, {}))
     RunResponseCallback(std::move(callback.first), std::move(callback.second));
 }

components/password_manager/core/browser/password_scripts_fetcher_impl.h

@@ -64,9 +64,10 @@ class PasswordScriptsFetcherImpl
 
   // PasswordScriptsFetcher:
   void PrewarmCache() override;
-  void ReportCacheReadinessMetric() const override;
-  void GetPasswordScriptAvailability(const url::Origin& origin,
-                                     ResponseCallback callback) override;
+  void Fetch(base::OnceClosure fetch_finished_callback) override;
+  void FetchScriptAvailability(const url::Origin& origin,
+                               ResponseCallback callback) override;
+  bool IsScriptAvailable(const url::Origin& origin) const override;
 
 #if defined(UNIT_TEST)
   void make_cache_stale_for_testing() {
@@ -95,6 +96,8 @@ class PasswordScriptsFetcherImpl
   // Timestamp of the last finished request.
   base::TimeTicks last_fetch_timestamp_;
   // Stores the callbacks that are waiting for the request to finish.
+  std::vector<base::OnceClosure> fetch_finished_callbacks_;
+  // Stores the per-origin callbacks that are waiting for the request to finish.
   std::vector<std::pair<url::Origin, ResponseCallback>> pending_callbacks_;
   // URL loader object for the gstatic request. If |url_loader_| is not null, a
   // request is currently in flight.

components/password_manager/core/browser/password_scripts_fetcher_impl_unittests.cc

@@ -51,7 +51,10 @@ class PasswordScriptsFetcherImplTest : public ::testing::Test {
         test_shared_loader_factory_);
   }
 
-  void TearDown() override { EXPECT_EQ(0, GetNumberOfPendingRequests()); }
+  void TearDown() override {
+    EXPECT_EQ(0, GetNumberOfPendingRequests());
+    EXPECT_EQ(0u, pending_fetch_finished_callbacks_);
+  }
 
   void SimulateResponse() { SimulateResponseWithContent(kTestResponseContent); }
 
@@ -67,7 +70,10 @@ class PasswordScriptsFetcherImplTest : public ::testing::Test {
   }
 
   void StartBulkCheck() {
-    fetcher()->ReportCacheReadinessMetric();
+    pending_fetch_finished_callbacks_++;
+    fetcher()->Fetch(
+        base::BindOnce(&PasswordScriptsFetcherImplTest::RecordFetchFinished,
+                       base::Unretained(this)));
     RequestSingleScriptAvailability(GetOriginWithScript1());
     RequestSingleScriptAvailability(GetOriginWithScript2());
     RequestSingleScriptAvailability(GetOriginWithoutScript());
@@ -85,8 +91,7 @@ class PasswordScriptsFetcherImplTest : public ::testing::Test {
 
  private:
   void RequestSingleScriptAvailability(const url::Origin& origin) {
-    fetcher_->GetPasswordScriptAvailability(origin,
-                                            GenerateResponseCallback(origin));
+    fetcher_->FetchScriptAvailability(origin, GenerateResponseCallback(origin));
   }
 
   void RecordResponse(url::Origin origin, bool has_script) {
@@ -99,6 +104,8 @@ class PasswordScriptsFetcherImplTest : public ::testing::Test {
     }
   }
 
+  void RecordFetchFinished() { pending_fetch_finished_callbacks_--; }
+
   PasswordScriptsFetcher::ResponseCallback GenerateResponseCallback(
       url::Origin origin) {
     return base::BindOnce(&PasswordScriptsFetcherImplTest::RecordResponse,
@@ -107,6 +114,7 @@ class PasswordScriptsFetcherImplTest : public ::testing::Test {
 
   base::test::SingleThreadTaskEnvironment task_environment_;
   base::flat_map<url::Origin, bool> recorded_responses_;
+  size_t pending_fetch_finished_callbacks_ = 0;
   std::unique_ptr<PasswordScriptsFetcherImpl> fetcher_;
   std::unique_ptr<network::TestURLLoaderFactory> test_url_loader_factory_;
   scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
@@ -246,4 +254,33 @@ TEST_F(PasswordScriptsFetcherImplTest, ServerError) {
       net::HttpStatusCode::HTTP_BAD_REQUEST, 1u);
 }
 
+TEST_F(PasswordScriptsFetcherImplTest, IsScriptAvailable) {
+  // |IsScriptAvailable| does not trigger any network requests and returns the
+  // default value (false).
+  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithScript1()));
+  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithScript2()));
+  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithoutScript()));
+  EXPECT_EQ(0, GetNumberOfPendingRequests());
+
+  StartBulkCheck();
+  EXPECT_EQ(1, GetNumberOfPendingRequests());
+  // Bulk check restart doesn't trigger new network request if the cache's state
+  // is |kWaiting|.
+  StartBulkCheck();
+  EXPECT_EQ(1, GetNumberOfPendingRequests());
+  SimulateResponse();
+  base::RunLoop().RunUntilIdle();
+  // Cache is ready.
+  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript1()));
+  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript2()));
+  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithoutScript()));
+
+  // |IsScriptAvailable| does not trigger refetching and returns stale values.
+  fetcher()->make_cache_stale_for_testing();
+  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript1()));
+  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript2()));
+  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithoutScript()));
+  EXPECT_EQ(0, GetNumberOfPendingRequests());
+}
+
 }  // namespace password_manager