Clarify offline pages policy for tab restriction

Offline Pages namespaces can be configured to only allow its pages to be
presented in their original tabs. This is enforced using an undeclared
convention that pages for these namespaces will store the tab id value
in the ClientId::id field for their pages. This change makes this
convention more explicit.

Bug: 879801
Change-Id: I3ab3d871abb41d6de9112c55d8b3d8b6491c7386
Reviewed-on: https://chromium-review.googlesource.com/1200389
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589369}

chrome/browser/offline_pages/offline_page_utils.cc

@@ -59,15 +59,17 @@ class OfflinePageComparer {
 void OnGetPagesByURLDone(
     const GURL& url,
     int tab_id,
-    const std::vector<std::string>& namespaces_to_show_in_original_tab,
+    const std::vector<std::string>& namespaces_restricted_to_tab_from_client_id,
     base::OnceCallback<void(const std::vector<OfflinePageItem>&)> callback,
     const MultipleOfflinePageItemResult& pages) {
   std::vector<OfflinePageItem> selected_pages;
   std::string tab_id_str = base::IntToString(tab_id);
 
   // Exclude pages whose tab id does not match.
+  // Note: For this restriction to work offline pages saved to tab-bound
+  // namespaces must have the assigned tab id set to their ClientId::id field.
   for (const auto& page : pages) {
-    if (base::ContainsValue(namespaces_to_show_in_original_tab,
+    if (base::ContainsValue(namespaces_restricted_to_tab_from_client_id,
                             page.client_id.name_space) &&
         page.client_id.id != tab_id_str) {
       continue;
@@ -215,7 +217,7 @@ void OfflinePageUtils::SelectPagesForURL(
   offline_page_model->GetPagesByURL(
       url, base::BindOnce(&OnGetPagesByURLDone, url, tab_id,
                           offline_page_model->GetPolicyController()
-                              ->GetNamespacesRestrictedToOriginalTab(),
+                              ->GetNamespacesRestrictedToTabFromClientId(),
                           std::move(callback)));
 }
 

components/offline_pages/core/client_policy_controller.cc

@@ -27,7 +27,7 @@ ClientPolicyController::ClientPolicyController() {
                                      kUnlimitedPages, kUnlimitedPages)
           .SetExpirePeriod(base::TimeDelta::FromDays(30))
           .SetIsSupportedByRecentTabs(true)
-          .SetIsOnlyShownInOriginalTab(true)
+          .SetIsRestrictedToTabFromClientId(true)
           .Build()));
   policies_.insert(std::make_pair(
       kAsyncNamespace,
@@ -89,7 +89,7 @@ ClientPolicyController::ClientPolicyController() {
                                                     kUnlimitedPages, 1)
                          .SetIsRemovedOnCacheReset(true)
                          .SetExpirePeriod(base::TimeDelta::FromHours(1))
-                         .SetIsOnlyShownInOriginalTab(true)
+                         .SetIsRestrictedToTabFromClientId(true)
                          .Build()));
 
   // Fallback policy.
@@ -204,23 +204,25 @@ ClientPolicyController::GetNamespacesShownAsRecentlyVisitedSite() const {
   return *recent_tab_namespace_cache_;
 }
 
-bool ClientPolicyController::IsRestrictedToOriginalTab(
+bool ClientPolicyController::IsRestrictedToTabFromClientId(
     const std::string& name_space) const {
-  return GetPolicy(name_space).feature_policy.only_shown_in_original_tab;
+  return GetPolicy(name_space)
+      .feature_policy.is_restricted_to_tab_from_client_id;
 }
 
 const std::vector<std::string>&
-ClientPolicyController::GetNamespacesRestrictedToOriginalTab() const {
-  if (show_in_original_tab_cache_)
-    return *show_in_original_tab_cache_;
+ClientPolicyController::GetNamespacesRestrictedToTabFromClientId() const {
+  if (restricted_to_tab_from_client_id_cache_)
+    return *restricted_to_tab_from_client_id_cache_;
 
-  show_in_original_tab_cache_ = std::make_unique<std::vector<std::string>>();
+  restricted_to_tab_from_client_id_cache_ =
+      std::make_unique<std::vector<std::string>>();
   for (const auto& policy_item : policies_) {
-    if (policy_item.second.feature_policy.only_shown_in_original_tab)
-      show_in_original_tab_cache_->emplace_back(policy_item.first);
+    if (policy_item.second.feature_policy.is_restricted_to_tab_from_client_id)
+      restricted_to_tab_from_client_id_cache_->emplace_back(policy_item.first);
   }
 
-  return *show_in_original_tab_cache_;
+  return *restricted_to_tab_from_client_id_cache_;
 }
 
 bool ClientPolicyController::IsDisabledWhenPrefetchDisabled(

components/offline_pages/core/client_policy_controller.h

@@ -57,10 +57,13 @@ class ClientPolicyController {
   const std::vector<std::string>& GetNamespacesShownAsRecentlyVisitedSite()
       const;
 
-  // Returns whether pages for |name_space| should never be shown outside the
-  // tab they were generated in.
-  bool IsRestrictedToOriginalTab(const std::string& name_space) const;
-  const std::vector<std::string>& GetNamespacesRestrictedToOriginalTab() const;
+  // Returns whether pages for |name_space| should only be opened in a
+  // specifically assigned tab.
+  // Note: For this restriction to work offline pages saved to this namespace
+  // must have the respective tab id set to their ClientId::id field.
+  bool IsRestrictedToTabFromClientId(const std::string& name_space) const;
+  const std::vector<std::string>& GetNamespacesRestrictedToTabFromClientId()
+      const;
 
   bool IsDisabledWhenPrefetchDisabled(const std::string& name_space) const;
   const std::vector<std::string>& GetNamespacesDisabledWhenPrefetchDisabled()
@@ -89,7 +92,8 @@ class ClientPolicyController {
   mutable std::unique_ptr<std::vector<std::string>>
       user_requested_download_namespace_cache_;
   mutable std::unique_ptr<std::vector<std::string>> recent_tab_namespace_cache_;
-  mutable std::unique_ptr<std::vector<std::string>> show_in_original_tab_cache_;
+  mutable std::unique_ptr<std::vector<std::string>>
+      restricted_to_tab_from_client_id_cache_;
   mutable std::unique_ptr<std::vector<std::string>>
       disabled_when_prefetch_disabled_cache_;
 

components/offline_pages/core/client_policy_controller_unittest.cc

@@ -35,7 +35,8 @@ class ClientPolicyControllerTest : public testing::Test {
   void ExpectUserRequestedDownloadSupport(std::string name_space,
                                           bool expectation);
   void ExpectRecentTab(std::string name_space, bool expectation);
-  void ExpectOnlyOriginalTab(std::string name_space, bool expectation);
+  void ExpectRestrictedToTabFromClientId(std::string name_space,
+                                         bool expectation);
   void ExpectDisabledWhenPrefetchDisabled(std::string name_space,
                                           bool expectation);
 
@@ -106,19 +107,21 @@ void ClientPolicyControllerTest::ExpectRecentTab(std::string name_space,
          " a recently visited site.";
 }
 
-void ClientPolicyControllerTest::ExpectOnlyOriginalTab(std::string name_space,
-                                                       bool expectation) {
+void ClientPolicyControllerTest::ExpectRestrictedToTabFromClientId(
+    std::string name_space,
+    bool expectation) {
   std::vector<std::string> cache =
-      controller()->GetNamespacesRestrictedToOriginalTab();
+      controller()->GetNamespacesRestrictedToTabFromClientId();
   auto result = std::find(cache.begin(), cache.end(), name_space);
   EXPECT_EQ(expectation, result != cache.end())
       << "Namespace " << name_space
       << " had incorrect restriction when getting namespaces restricted to"
-         " the original tab";
-  EXPECT_EQ(expectation, controller()->IsRestrictedToOriginalTab(name_space))
+         " the tab from the client id field";
+  EXPECT_EQ(expectation,
+            controller()->IsRestrictedToTabFromClientId(name_space))
       << "Namespace " << name_space
       << " had incorrect restriction when directly checking if the namespace"
-         " is restricted to the original tab";
+         " is restricted to the tab from the client id field";
 }
 
 void ClientPolicyControllerTest::ExpectDisabledWhenPrefetchDisabled(
@@ -147,7 +150,7 @@ TEST_F(ClientPolicyControllerTest, FallbackTest) {
   ExpectDownloadSupport(kUndefinedNamespace, false);
   ExpectUserRequestedDownloadSupport(kUndefinedNamespace, false);
   ExpectRecentTab(kUndefinedNamespace, false);
-  ExpectOnlyOriginalTab(kUndefinedNamespace, false);
+  ExpectRestrictedToTabFromClientId(kUndefinedNamespace, false);
   ExpectDisabledWhenPrefetchDisabled(kUndefinedNamespace, false);
 }
 
@@ -160,7 +163,7 @@ TEST_F(ClientPolicyControllerTest, CheckBookmarkDefined) {
   ExpectDownloadSupport(kBookmarkNamespace, false);
   ExpectUserRequestedDownloadSupport(kBookmarkNamespace, false);
   ExpectRecentTab(kBookmarkNamespace, false);
-  ExpectOnlyOriginalTab(kBookmarkNamespace, false);
+  ExpectRestrictedToTabFromClientId(kBookmarkNamespace, false);
   ExpectDisabledWhenPrefetchDisabled(kBookmarkNamespace, false);
 }
 
@@ -173,7 +176,7 @@ TEST_F(ClientPolicyControllerTest, CheckLastNDefined) {
   ExpectDownloadSupport(kLastNNamespace, false);
   ExpectUserRequestedDownloadSupport(kLastNNamespace, false);
   ExpectRecentTab(kLastNNamespace, true);
-  ExpectOnlyOriginalTab(kLastNNamespace, true);
+  ExpectRestrictedToTabFromClientId(kLastNNamespace, true);
   ExpectDisabledWhenPrefetchDisabled(kLastNNamespace, false);
 }
 
@@ -186,7 +189,7 @@ TEST_F(ClientPolicyControllerTest, CheckAsyncDefined) {
   ExpectDownloadSupport(kAsyncNamespace, true);
   ExpectUserRequestedDownloadSupport(kAsyncNamespace, true);
   ExpectRecentTab(kAsyncNamespace, false);
-  ExpectOnlyOriginalTab(kAsyncNamespace, false);
+  ExpectRestrictedToTabFromClientId(kAsyncNamespace, false);
   ExpectDisabledWhenPrefetchDisabled(kAsyncNamespace, false);
 }
 
@@ -199,7 +202,7 @@ TEST_F(ClientPolicyControllerTest, CheckCCTDefined) {
   ExpectDownloadSupport(kCCTNamespace, false);
   ExpectUserRequestedDownloadSupport(kCCTNamespace, false);
   ExpectRecentTab(kCCTNamespace, false);
-  ExpectOnlyOriginalTab(kCCTNamespace, false);
+  ExpectRestrictedToTabFromClientId(kCCTNamespace, false);
   ExpectDisabledWhenPrefetchDisabled(kCCTNamespace, true);
 }
 
@@ -212,7 +215,7 @@ TEST_F(ClientPolicyControllerTest, CheckDownloadDefined) {
   ExpectDownloadSupport(kDownloadNamespace, true);
   ExpectUserRequestedDownloadSupport(kDownloadNamespace, true);
   ExpectRecentTab(kDownloadNamespace, false);
-  ExpectOnlyOriginalTab(kDownloadNamespace, false);
+  ExpectRestrictedToTabFromClientId(kDownloadNamespace, false);
   ExpectDisabledWhenPrefetchDisabled(kDownloadNamespace, false);
 }
 
@@ -226,7 +229,7 @@ TEST_F(ClientPolicyControllerTest, CheckNTPSuggestionsDefined) {
   ExpectDownloadSupport(kNTPSuggestionsNamespace, true);
   ExpectUserRequestedDownloadSupport(kNTPSuggestionsNamespace, true);
   ExpectRecentTab(kNTPSuggestionsNamespace, false);
-  ExpectOnlyOriginalTab(kNTPSuggestionsNamespace, false);
+  ExpectRestrictedToTabFromClientId(kNTPSuggestionsNamespace, false);
   ExpectDisabledWhenPrefetchDisabled(kNTPSuggestionsNamespace, false);
 }
 
@@ -240,7 +243,7 @@ TEST_F(ClientPolicyControllerTest, CheckSuggestedArticlesDefined) {
   ExpectDownloadSupport(kSuggestedArticlesNamespace, false);
   ExpectUserRequestedDownloadSupport(kSuggestedArticlesNamespace, false);
   ExpectRecentTab(kSuggestedArticlesNamespace, false);
-  ExpectOnlyOriginalTab(kSuggestedArticlesNamespace, false);
+  ExpectRestrictedToTabFromClientId(kSuggestedArticlesNamespace, false);
   ExpectDisabledWhenPrefetchDisabled(kSuggestedArticlesNamespace, true);
 }
 
@@ -254,7 +257,7 @@ TEST_F(ClientPolicyControllerTest, CheckLivePageSharingDefined) {
   ExpectDownloadSupport(kLivePageSharingNamespace, false);
   ExpectUserRequestedDownloadSupport(kLivePageSharingNamespace, false);
   ExpectRecentTab(kLivePageSharingNamespace, false);
-  ExpectOnlyOriginalTab(kLivePageSharingNamespace, true);
+  ExpectRestrictedToTabFromClientId(kLivePageSharingNamespace, true);
   ExpectDisabledWhenPrefetchDisabled(kLivePageSharingNamespace, false);
 }
 

components/offline_pages/core/offline_page_client_policy.h

@@ -53,8 +53,10 @@ struct FeaturePolicy {
   bool is_user_requested_download;
   // Whether pages are shown in recent tabs ui.
   bool is_supported_by_recent_tabs;
-  // Whether pages should only be viewed in the tab they were generated in.
-  bool only_shown_in_original_tab;
+  // Whether pages can only be viewed in a specific tab. Pages controlled by
+  // this policy must have their ClientId::id field set to their assigned tab's
+  // id.
+  bool is_restricted_to_tab_from_client_id;
   // Whether pages are removed on user-initiated cache reset. Defaults to true.
   bool is_removed_on_cache_reset;
   // Whether the namespace should be disabled if prefetching-related preferences
@@ -69,7 +71,7 @@ struct FeaturePolicy {
       : is_supported_by_download(false),
         is_user_requested_download(false),
         is_supported_by_recent_tabs(false),
-        only_shown_in_original_tab(false),
+        is_restricted_to_tab_from_client_id(false),
         is_removed_on_cache_reset(true),
         disabled_when_prefetch_disabled(false),
         is_suggested(false),
@@ -157,10 +159,10 @@ class OfflinePageClientPolicyBuilder {
     return *this;
   }
 
-  OfflinePageClientPolicyBuilder& SetIsOnlyShownInOriginalTab(
-      const bool only_shown_in_original_tab) {
-    policy_.feature_policy.only_shown_in_original_tab =
-        only_shown_in_original_tab;
+  OfflinePageClientPolicyBuilder& SetIsRestrictedToTabFromClientId(
+      const bool is_restricted_to_tab_from_client_id) {
+    policy_.feature_policy.is_restricted_to_tab_from_client_id =
+        is_restricted_to_tab_from_client_id;
     return *this;
   }