auto-fetch: store tab ID in ClientId

This changes how the ClientId is generated for this feature. The tabID
is not used yet, but will be
needed to implement the system notification correctly.

I manually verified that FindTab() returns the correct ID.

Bug: 883486
Change-Id: I466fc5d3eb1b6f373d969a3d8eb88de7e96854ce
Reviewed-on: https://chromium-review.googlesource.com/c/1327409
Commit-Queue: Dan H <harringtond@google.com>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608057}

chrome/browser/offline_pages/offline_page_auto_fetcher.cc

@@ -6,14 +6,37 @@
 
 #include <utility>
 
+#include "chrome/browser/android/tab_android.h"
 #include "chrome/browser/offline_pages/offline_page_auto_fetcher_service.h"
 #include "chrome/browser/offline_pages/offline_page_auto_fetcher_service_factory.h"
+#include "chrome/browser/ui/android/tab_model/tab_model.h"
+#include "chrome/browser/ui/android/tab_model/tab_model_list.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "url/gurl.h"
 
 namespace offline_pages {
+namespace {
+
+TabAndroid* FindTab(content::RenderFrameHost* render_frame_host) {
+  content::WebContents* web_contents =
+      content::WebContents::FromRenderFrameHost(render_frame_host);
+  if (!web_contents)
+    return nullptr;
+  TabModel* tab_model = TabModelList::GetTabModelForWebContents(web_contents);
+  if (!tab_model)
+    return nullptr;
+  // For this use-case, it's OK to fail if the active tab doesn't match
+  // web_contents.
+  if (tab_model->GetActiveWebContents() != web_contents)
+    return nullptr;
+
+  return tab_model->GetTabAt(tab_model->GetActiveIndex());
+}
+
+}  // namespace
 
 OfflinePageAutoFetcher::OfflinePageAutoFetcher(
     content::RenderFrameHost* render_frame_host)
@@ -23,9 +46,14 @@ OfflinePageAutoFetcher::~OfflinePageAutoFetcher() = default;
 
 void OfflinePageAutoFetcher::TrySchedule(bool user_requested,
                                          TryScheduleCallback callback) {
+  TabAndroid* tab = FindTab(render_frame_host_);
+  if (!tab) {
+    std::move(callback).Run(OfflinePageAutoFetcherScheduleResult::kOtherError);
+    return;
+  }
   GetService()->TrySchedule(user_requested,
                             render_frame_host_->GetLastCommittedURL(),
-                            std::move(callback));
+                            tab->GetAndroidId(), std::move(callback));
 }
 
 void OfflinePageAutoFetcher::CancelSchedule() {

chrome/browser/offline_pages/offline_page_auto_fetcher_service.cc

@@ -4,10 +4,15 @@
 
 #include "chrome/browser/offline_pages/offline_page_auto_fetcher_service.h"
 
+#include <string>
 #include <utility>
 
+#include "base/optional.h"
+#include "base/strings/strcat.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/offline_pages/request_coordinator_factory.h"
+#include "components/offline_pages/core/auto_fetch.h"
 #include "components/offline_pages/core/background/request_coordinator.h"
 #include "components/offline_pages/core/background/save_page_request.h"
 #include "components/offline_pages/core/client_id.h"
@@ -21,17 +26,24 @@ namespace offline_pages {
 namespace {
 constexpr int kMaximumInFlight = 3;
 
-ClientId URLToClientId(const GURL& url) {
-  // By using namespace.URL as the client ID, we ensure that only a single
-  // request for a page can be in flight.
-
+bool URLMatches(const GURL& a, const GURL& b) {
   // We strip the fragment, because when loading an offline page, only the most
   // recent page saved with the URL (fragment stripped) can be loaded.
   GURL::Replacements remove_fragment;
   remove_fragment.ClearRef();
-  GURL url_to_match = url.ReplaceComponents(remove_fragment);
+  return a.ReplaceComponents(remove_fragment) ==
+         b.ReplaceComponents(remove_fragment);
+}
 
-  return ClientId(kAutoAsyncNamespace, url_to_match.spec());
+SavePageRequest* FindRequest(
+    const std::vector<std::unique_ptr<SavePageRequest>>& all_requests,
+    const GURL& url) {
+  for (const auto& request : all_requests) {
+    if (request->client_id().name_space == kAutoAsyncNamespace &&
+        URLMatches(request->url(), url))
+      return request.get();
+  }
+  return nullptr;
 }
 }  // namespace
 
@@ -64,10 +76,11 @@ OfflinePageAutoFetcherService::~OfflinePageAutoFetcherService() {}
 
 void OfflinePageAutoFetcherService::TrySchedule(bool user_requested,
                                                 const GURL& url,
+                                                int android_tab_id,
                                                 TryScheduleCallback callback) {
-  StartOrEnqueue(
-      base::BindOnce(&OfflinePageAutoFetcherService::TryScheduleStep1,
-                     GetWeakPtr(), user_requested, url, std::move(callback)));
+  StartOrEnqueue(base::BindOnce(
+      &OfflinePageAutoFetcherService::TryScheduleStep1, GetWeakPtr(),
+      user_requested, url, android_tab_id, std::move(callback)));
 }
 
 void OfflinePageAutoFetcherService::CancelSchedule(const GURL& url) {
@@ -78,6 +91,7 @@ void OfflinePageAutoFetcherService::CancelSchedule(const GURL& url) {
 void OfflinePageAutoFetcherService::TryScheduleStep1(
     bool user_requested,
     const GURL& url,
+    int android_tab_id,
     TryScheduleCallback callback,
     TaskToken token) {
   // Return an early failure if the URL is not suitable.
@@ -89,9 +103,10 @@ void OfflinePageAutoFetcherService::TryScheduleStep1(
 
   // We need to do some checks on in-flight requests before scheduling the
   // fetch. So first, get the list of all requests, and proceed to step 2.
-  request_coordinator_->GetAllRequests(base::BindOnce(
-      &OfflinePageAutoFetcherService::TryScheduleStep2, GetWeakPtr(),
-      std::move(token), user_requested, url, std::move(callback),
+  request_coordinator_->GetAllRequests(
+      base::BindOnce(&OfflinePageAutoFetcherService::TryScheduleStep2,
+                     GetWeakPtr(), std::move(token), user_requested, url,
+                     android_tab_id, std::move(callback),
                      // Unretained is OK because coordinator is calling us back.
                      base::Unretained(request_coordinator_)));
 }
@@ -100,20 +115,18 @@ void OfflinePageAutoFetcherService::TryScheduleStep2(
     TaskToken token,
     bool user_requested,
     const GURL& url,
+    int android_tab_id,
     TryScheduleCallback callback,
     RequestCoordinator* coordinator,
     std::vector<std::unique_ptr<SavePageRequest>> all_requests) {
   // If a request for this page is already scheduled, report scheduling as
   // successful without doing anything.
-  const ClientId url_client_id = URLToClientId(url);
-  for (const auto& request : all_requests) {
-    if (url_client_id == request->client_id()) {
+  if (FindRequest(all_requests, url)) {
     std::move(callback).Run(
         OfflinePageAutoFetcherScheduleResult::kAlreadyScheduled);
     TaskComplete(std::move(token));
     return;
   }
-  }
 
   // Respect kMaximumInFlight.
   if (!user_requested) {
@@ -134,7 +147,8 @@ void OfflinePageAutoFetcherService::TryScheduleStep2(
   // Finally, schedule a new request, and proceed to step 3.
   RequestCoordinator::SavePageLaterParams params;
   params.url = url;
-  params.client_id = url_client_id;
+  auto_fetch::ClientIdMetadata metadata(android_tab_id);
+  params.client_id = auto_fetch::MakeClientId(metadata);
   params.user_requested = false;
   params.availability =
       RequestCoordinator::RequestAvailability::ENABLED_FOR_OFFLINER;
@@ -170,16 +184,14 @@ void OfflinePageAutoFetcherService::CancelScheduleStep2(
     RequestCoordinator* coordinator,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   // Cancel the request if it's found in the list of all requests.
-  const ClientId url_client_id = URLToClientId(url);
-  for (const auto& request : requests) {
-    if (url_client_id == request->client_id()) {
+  SavePageRequest* matching_request = FindRequest(requests, url);
+  if (matching_request) {
     coordinator->RemoveRequests(
-          {request->request_id()},
+        {matching_request->request_id()},
         base::BindOnce(&OfflinePageAutoFetcherService::CancelScheduleStep3,
                        GetWeakPtr(), std::move(token)));
     return;
   }
-  }
   TaskComplete(std::move(token));
 }
 

chrome/browser/offline_pages/offline_page_auto_fetcher_service.h

@@ -59,6 +59,7 @@ class OfflinePageAutoFetcherService : public KeyedService {
 
   void TrySchedule(bool user_requested,
                    const GURL& url,
+                   int android_tab_id,
                    TryScheduleCallback callback);
   void CancelSchedule(const GURL& url);
 
@@ -90,12 +91,14 @@ class OfflinePageAutoFetcherService : public KeyedService {
 
   void TryScheduleStep1(bool user_requested,
                         const GURL& url,
+                        int android_tab_id,
                         TryScheduleCallback callback,
                         TaskToken token);
   void TryScheduleStep2(
       TaskToken token,
       bool user_requested,
       const GURL& url,
+      int android_tab_id,
       TryScheduleCallback callback,
       RequestCoordinator* coordinator,
       std::vector<std::unique_ptr<SavePageRequest>> all_requests);

chrome/browser/offline_pages/offline_page_auto_fetcher_service_unittest.cc

@@ -13,6 +13,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/offline_pages/core/background/request_coordinator.h"
 #include "components/offline_pages/core/background/test_request_queue_store.h"
+#include "components/offline_pages/core/client_namespace_constants.h"
 #include "content/public/test/navigation_simulator.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -20,6 +21,7 @@
 
 namespace offline_pages {
 namespace {
+const int kTabId = 1;
 using OfflinePageAutoFetcherScheduleResult =
     chrome::mojom::OfflinePageAutoFetcherScheduleResult;
 
@@ -71,7 +73,8 @@ TEST_F(OfflinePageAutoFetcherServiceTest, TryScheduleSuccess) {
       result_callback;
   EXPECT_CALL(result_callback,
               Run(OfflinePageAutoFetcherScheduleResult::kScheduled));
-  service_->TrySchedule(false, GURL("http://foo.com"), result_callback.Get());
+  service_->TrySchedule(false, GURL("http://foo.com"), kTabId,
+                        result_callback.Get());
   browser_thread_bundle_.RunUntilIdle();
   EXPECT_EQ(1ul, GetRequestsSync().size());
 }
@@ -82,7 +85,8 @@ TEST_F(OfflinePageAutoFetcherServiceTest, AttemptInvalidURL) {
       result_callback;
   EXPECT_CALL(result_callback,
               Run(OfflinePageAutoFetcherScheduleResult::kOtherError));
-  service_->TrySchedule(false, GURL("ftp://foo.com"), result_callback.Get());
+  service_->TrySchedule(false, GURL("ftp://foo.com"), kTabId,
+                        result_callback.Get());
   browser_thread_bundle_.RunUntilIdle();
   EXPECT_EQ(0ul, GetRequestsSync().size());
 }
@@ -98,8 +102,10 @@ TEST_F(OfflinePageAutoFetcherServiceTest, TryScheduleDuplicate) {
               Run(OfflinePageAutoFetcherScheduleResult::kAlreadyScheduled))
       .Times(1);
   // The page should only be saved once, because the fragment is ignored.
-  service_->TrySchedule(false, GURL("http://foo.com#A"), result_callback.Get());
-  service_->TrySchedule(false, GURL("http://foo.com#Z"), result_callback.Get());
+  service_->TrySchedule(false, GURL("http://foo.com#A"), kTabId,
+                        result_callback.Get());
+  service_->TrySchedule(false, GURL("http://foo.com#Z"), kTabId,
+                        result_callback.Get());
   browser_thread_bundle_.RunUntilIdle();
   EXPECT_EQ(1ul, GetRequestsSync().size());
 }
@@ -120,15 +126,20 @@ TEST_F(OfflinePageAutoFetcherServiceTest, AttemptAutoScheduleMoreThanMaximum) {
       .Times(1);
 
   // Three requests within quota.
-  service_->TrySchedule(false, GURL("http://foo.com/1"), result_callback.Get());
-  service_->TrySchedule(false, GURL("http://foo.com/2"), result_callback.Get());
-  service_->TrySchedule(false, GURL("http://foo.com/3"), result_callback.Get());
+  service_->TrySchedule(false, GURL("http://foo.com/1"), kTabId,
+                        result_callback.Get());
+  service_->TrySchedule(false, GURL("http://foo.com/2"), kTabId,
+                        result_callback.Get());
+  service_->TrySchedule(false, GURL("http://foo.com/3"), kTabId,
+                        result_callback.Get());
 
   // Quota is exhausted.
-  service_->TrySchedule(false, GURL("http://foo.com/4"), result_callback.Get());
+  service_->TrySchedule(false, GURL("http://foo.com/4"), kTabId,
+                        result_callback.Get());
 
   // User-requested, quota is not enforced.
-  service_->TrySchedule(true, GURL("http://foo.com/5"), result_callback.Get());
+  service_->TrySchedule(true, GURL("http://foo.com/5"), kTabId,
+                        result_callback.Get());
 
   browser_thread_bundle_.RunUntilIdle();
 }
@@ -141,15 +152,20 @@ TEST_F(OfflinePageAutoFetcherServiceTest,
   EXPECT_CALL(result_callback,
               Run(OfflinePageAutoFetcherScheduleResult::kScheduled))
       .Times(4);
-  service_->TrySchedule(true, GURL("http://foo.com/1"), result_callback.Get());
-  service_->TrySchedule(true, GURL("http://foo.com/2"), result_callback.Get());
-  service_->TrySchedule(true, GURL("http://foo.com/3"), result_callback.Get());
-  service_->TrySchedule(true, GURL("http://foo.com/4"), result_callback.Get());
+  service_->TrySchedule(true, GURL("http://foo.com/1"), kTabId,
+                        result_callback.Get());
+  service_->TrySchedule(true, GURL("http://foo.com/2"), kTabId,
+                        result_callback.Get());
+  service_->TrySchedule(true, GURL("http://foo.com/3"), kTabId,
+                        result_callback.Get());
+  service_->TrySchedule(true, GURL("http://foo.com/4"), kTabId,
+                        result_callback.Get());
   browser_thread_bundle_.RunUntilIdle();
 }
 
 TEST_F(OfflinePageAutoFetcherServiceTest, CancelSuccess) {
-  service_->TrySchedule(false, GURL("http://foo.com"), base::DoNothing());
+  service_->TrySchedule(false, GURL("http://foo.com"), kTabId,
+                        base::DoNothing());
   browser_thread_bundle_.RunUntilIdle();
   service_->CancelSchedule(GURL("http://foo.com"));
   browser_thread_bundle_.RunUntilIdle();
@@ -157,7 +173,8 @@ TEST_F(OfflinePageAutoFetcherServiceTest, CancelSuccess) {
 }
 
 TEST_F(OfflinePageAutoFetcherServiceTest, CancelNotExist) {
-  service_->TrySchedule(false, GURL("http://foo.com"), base::DoNothing());
+  service_->TrySchedule(false, GURL("http://foo.com"), kTabId,
+                        base::DoNothing());
   browser_thread_bundle_.RunUntilIdle();
   service_->CancelSchedule(GURL("http://NOT-FOO.com"));
   browser_thread_bundle_.RunUntilIdle();

components/offline_pages/core/BUILD.gn

@@ -12,6 +12,8 @@ static_library("core") {
     "archive_manager.h",
     "archive_validator.cc",
     "archive_validator.h",
+    "auto_fetch.cc",
+    "auto_fetch.h",
     "background_snapshot_controller.cc",
     "background_snapshot_controller.h",
     "client_id.cc",
@@ -152,6 +154,7 @@ source_set("unit_tests") {
   sources = [
     "archive_manager_unittest.cc",
     "archive_validator_unittest.cc",
+    "auto_fetch_unittest.cc",
     "background_snapshot_controller_unittest.cc",
     "client_policy_controller_unittest.cc",
     "model/add_page_task_unittest.cc",

components/offline_pages/core/auto_fetch.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 "components/offline_pages/core/auto_fetch.h"
+
+#include "base/strings/strcat.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
+#include "components/offline_pages/core/client_namespace_constants.h"
+
+namespace offline_pages {
+namespace auto_fetch {
+ClientIdMetadata::ClientIdMetadata() = default;
+ClientIdMetadata::ClientIdMetadata(const ClientIdMetadata&) = default;
+
+ClientId MakeClientId(const ClientIdMetadata& metadata) {
+  // Here, the 'A' prefix is used so that future versions can easily change the
+  // format if necessary.
+  return ClientId(kAutoAsyncNamespace,
+                  base::StrCat({"A", std::to_string(metadata.android_tab_id)}));
+}
+
+base::Optional<ClientIdMetadata> ExtractMetadata(const ClientId& id) {
+  if (id.name_space != kAutoAsyncNamespace)
+    return base::nullopt;
+  if (id.id.empty() || id.id[0] != 'A')
+    return base::nullopt;
+  ClientIdMetadata metadata;
+  if (!base::StringToInt(base::StringPiece(id.id).substr(1),
+                         &metadata.android_tab_id))
+    return base::nullopt;
+  return metadata;
+}
+
+}  // namespace auto_fetch
+}  // namespace offline_pages

components/offline_pages/core/auto_fetch.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 COMPONENTS_OFFLINE_PAGES_CORE_AUTO_FETCH_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_AUTO_FETCH_H_
+
+#include "base/optional.h"
+#include "components/offline_pages/core/client_id.h"
+
+// Most auto-fetch code is in browser/offline_pages. This file contains code
+// that needs to be accessed within components/offline_pages.
+
+namespace offline_pages {
+namespace auto_fetch {
+
+// This metadata is stored in the |ClientId|'s |id| field.
+struct ClientIdMetadata {
+  ClientIdMetadata();
+  ClientIdMetadata(const ClientIdMetadata&);
+  explicit ClientIdMetadata(int android_tab_id)
+      : android_tab_id(android_tab_id) {}
+  // ID of the Android tab that initiated the request.
+  int android_tab_id;
+};
+
+ClientId MakeClientId(const ClientIdMetadata& metadata);
+// Extract metadata from a |ClientId| that was created with |MakeClientId|.
+base::Optional<ClientIdMetadata> ExtractMetadata(const ClientId& id);
+
+}  // namespace auto_fetch
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_AUTO_FETCH_H_

components/offline_pages/core/auto_fetch_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 "components/offline_pages/core/auto_fetch.h"
+
+#include "components/offline_pages/core/client_namespace_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace offline_pages {
+namespace auto_fetch {
+namespace {
+
+TEST(AutoFetch, MakeClientId) {
+  EXPECT_EQ(ClientId(kAutoAsyncNamespace, "A123"),
+            auto_fetch::MakeClientId(auto_fetch::ClientIdMetadata(123)));
+}
+
+TEST(AutoFetch, ExtractMetadataSuccess) {
+  base::Optional<auto_fetch::ClientIdMetadata> metadata =
+      auto_fetch::ExtractMetadata(
+          auto_fetch::MakeClientId(auto_fetch::ClientIdMetadata(123)));
+  ASSERT_TRUE(metadata);
+  EXPECT_EQ(123, metadata.value().android_tab_id);
+}
+
+TEST(AutoFetch, ExtractMetadataFail) {
+  EXPECT_FALSE(
+      auto_fetch::ExtractMetadata(ClientId(kAutoAsyncNamespace, "123")));
+  EXPECT_FALSE(auto_fetch::ExtractMetadata(ClientId(kAutoAsyncNamespace, "")));
+  EXPECT_FALSE(auto_fetch::ExtractMetadata(ClientId("other", "A123")));
+}
+
+}  // namespace
+}  // namespace auto_fetch
+}  // namespace offline_pages