[Offline pages] Wrap page deletion notification parameters into one s…

…truct. Also add request_origin to this struct. Bug: 734753 Change-Id: I210bf0fa177f1379693b6b432c12f48b89d8cd28 Reviewed-on: https://chromium-review.googlesource.com/558564 Reviewed-by: Ted Choc <tedchoc@chromium.org> Reviewed-by: Filip Gorski <fgorski@chromium.org> Reviewed-by: vitaliii <vitaliii@chromium.org> Reviewed-by: Justin DeWitt <dewittj@chromium.org> Commit-Queue: Cathy Li <chili@chromium.org> Cr-Commit-Position: refs/heads/master@{#486160}
FairyWorld · Jul 12, 2017 · 631445b · 631445b
1 parent b26585d
commit 631445b
Show file tree

Hide file tree

Showing 24 changed files with 224 additions and 74 deletions.
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/DeletedPageInfo.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/DeletedPageInfo.java
@@ -0,0 +1,40 @@
+// Copyright 2017 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.
+
+package org.chromium.chrome.browser.offlinepages;
+
+/**
+ * Simple object representing important information of a deleted offline page.
+ */
+public class DeletedPageInfo {
+    private final long mOfflineId;
+    private final ClientId mClientId;
+    private final String mRequestOrigin;
+
+    public DeletedPageInfo(
+            long offlineId, String clientNamespace, String clientId, String requestOrigin) {
+        this(offlineId, new ClientId(clientNamespace, clientId), requestOrigin);
+    }
+
+    public DeletedPageInfo(long offlineId, ClientId clientId, String requestOrigin) {
+        mOfflineId = offlineId;
+        mClientId = clientId;
+        mRequestOrigin = requestOrigin;
+    }
+
+    /** @return offline id for the deleted page */
+    public long getOfflineId() {
+        return mOfflineId;
+    }
+
+    /** @return Client Id for the deleted page */
+    public ClientId getClientId() {
+        return mClientId;
+    }
+
+    /** @return request origin of the deleted page */
+    public String getRequestOrigin() {
+        return mRequestOrigin;
+    }
+}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
@@ -94,7 +94,7 @@ public void offlinePageAdded(OfflinePageItem addedPage) {}
          * @param offlineId The offline ID of the deleted offline page.
          * @param clientId The client supplied ID of the deleted offline page.
          */
-        public void offlinePageDeleted(long offlineId, ClientId clientId) {}
+        public void offlinePageDeleted(DeletedPageInfo deletedPage) {}
     }
 
     /**
@@ -568,9 +568,9 @@ protected void offlinePageBridgeDestroyed() {
     }
 
     @CalledByNative
-    void offlinePageDeleted(long offlineId, ClientId clientId) {
+    void offlinePageDeleted(DeletedPageInfo deletedPage) {
         for (OfflinePageModelObserver observer : mObservers) {
-            observer.offlinePageDeleted(offlineId, clientId);
+            observer.offlinePageDeleted(deletedPage);
         }
     }
 
@@ -595,6 +595,12 @@ private static ClientId createClientId(String clientNamespace, String id) {
         return new ClientId(clientNamespace, id);
     }
 
+    @CalledByNative
+    private static DeletedPageInfo createDeletedPageInfo(
+            long offlineId, String clientNamespace, String clientId, String requestOrigin) {
+        return new DeletedPageInfo(offlineId, clientNamespace, clientId, requestOrigin);
+    }
+
     private static native boolean nativeIsOfflineBookmarksEnabled();
     private static native boolean nativeIsPageSharingEnabled();
     private static native boolean nativeCanSavePage(String url);

diff --git a/...oid/java/src/org/chromium/chrome/browser/suggestions/SuggestionsOfflineModelObserver.java b/...oid/java/src/org/chromium/chrome/browser/suggestions/SuggestionsOfflineModelObserver.java
@@ -7,7 +7,7 @@
 import android.support.annotation.Nullable;
 
 import org.chromium.base.Callback;
-import org.chromium.chrome.browser.offlinepages.ClientId;
+import org.chromium.chrome.browser.offlinepages.DeletedPageInfo;
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
 import org.chromium.chrome.browser.offlinepages.OfflinePageItem;
 
@@ -46,13 +46,13 @@ public void offlinePageAdded(OfflinePageItem addedPage) {
     }
 
     @Override
-    public void offlinePageDeleted(long offlineId, ClientId clientId) {
+    public void offlinePageDeleted(DeletedPageInfo deletedPage) {
         for (T suggestion : getOfflinableSuggestions()) {
             if (suggestion.requiresExactOfflinePage()) continue;
 
             Long suggestionOfflineId = suggestion.getOfflinePageOfflineId();
             if (suggestionOfflineId == null) continue;
-            if (suggestionOfflineId != offlineId) continue;
+            if (suggestionOfflineId != deletedPage.getOfflineId()) continue;
 
             // The old value cannot be simply removed without a request to the
             // model, because there may be an older offline page for the same

diff --git a/chrome/android/java_sources.gni b/chrome/android/java_sources.gni
@@ -682,6 +682,7 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java",
   "java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerProcessor.java",
   "java/src/org/chromium/chrome/browser/offlinepages/ClientId.java",
+  "java/src/org/chromium/chrome/browser/offlinepages/DeletedPageInfo.java",
   "java/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTask.java",
   "java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java",
   "java/src/org/chromium/chrome/browser/offlinepages/OfflinePageItem.java",

diff --git a/...android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeUnitTest.java b/...android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeUnitTest.java
@@ -82,9 +82,9 @@ public class MockOfflinePageModelObserver extends OfflinePageModelObserver {
         public long lastDeletedOfflineId;
         public ClientId lastDeletedClientId;
 
-        public void offlinePageDeleted(long offlineId, ClientId clientId) {
-            lastDeletedOfflineId = offlineId;
-            lastDeletedClientId = clientId;
+        public void offlinePageDeleted(DeletedPageInfo deletedPage) {
+            lastDeletedOfflineId = deletedPage.getOfflineId();
+            lastDeletedClientId = deletedPage.getClientId();
         }
     }
 
@@ -110,7 +110,7 @@ public void testRemovePageByClientId() {
 
         ClientId testClientId = new ClientId(TEST_NAMESPACE, TEST_ID);
         long testOfflineId = 123;
-        mBridge.offlinePageDeleted(testOfflineId, testClientId);
+        mBridge.offlinePageDeleted(new DeletedPageInfo(testOfflineId, testClientId, ""));
         assertEquals(testOfflineId, observer1.lastDeletedOfflineId);
         assertEquals(testClientId, observer1.lastDeletedClientId);
         assertEquals(testOfflineId, observer2.lastDeletedOfflineId);

diff --git a/chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc b/chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc
@@ -267,8 +267,7 @@ void OfflinePageEvaluationBridge::OfflinePageAdded(
     const OfflinePageItem& added_page) {}
 
 void OfflinePageEvaluationBridge::OfflinePageDeleted(
-    int64_t offline_id,
-    const ClientId& client_id) {}
+    const DeletedPageInfo& page_info) {}
 
 // Implement RequestCoordinator::Observer
 void OfflinePageEvaluationBridge::OnAdded(const SavePageRequest& request) {

diff --git a/chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h b/chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.h
@@ -44,8 +44,7 @@ class OfflinePageEvaluationBridge : public OfflinePageModel::Observer,
   void OfflinePageModelLoaded(OfflinePageModel* model) override;
   void OfflinePageAdded(OfflinePageModel* model,
                         const OfflinePageItem& added_page) override;
-  void OfflinePageDeleted(int64_t offline_id,
-                          const ClientId& client_id) override;
+  void OfflinePageDeleted(const DeletedPageInfo& page_info) override;
 
   // RequestCoordinator::Observer implementation.
   void OnAdded(const SavePageRequest& request) override;

diff --git a/chrome/browser/android/offline_pages/offline_page_bridge.cc b/chrome/browser/android/offline_pages/offline_page_bridge.cc
@@ -80,6 +80,16 @@ ScopedJavaLocalRef<jobject> ToJavaOfflinePageItem(
       offline_page.access_count, offline_page.last_access_time.ToJavaTime());
 }
 
+ScopedJavaLocalRef<jobject> ToJavaDeletedPageInfo(
+    JNIEnv* env,
+    const OfflinePageModel::DeletedPageInfo& deleted_page) {
+  return Java_OfflinePageBridge_createDeletedPageInfo(
+      env, deleted_page.offline_id,
+      ConvertUTF8ToJavaString(env, deleted_page.client_id.name_space),
+      ConvertUTF8ToJavaString(env, deleted_page.client_id.id),
+      ConvertUTF8ToJavaString(env, deleted_page.request_origin));
+}
+
 void CheckPagesExistOfflineCallback(
     const ScopedJavaGlobalRef<jobject>& j_callback_obj,
     const OfflinePageModel::CheckPagesExistOfflineResult& offline_pages) {
@@ -297,11 +307,11 @@ void OfflinePageBridge::OfflinePageAdded(OfflinePageModel* model,
       env, java_ref_, ToJavaOfflinePageItem(env, added_page));
 }
 
-void OfflinePageBridge::OfflinePageDeleted(int64_t offline_id,
-                                           const ClientId& client_id) {
+void OfflinePageBridge::OfflinePageDeleted(
+    const OfflinePageModel::DeletedPageInfo& page_info) {
   JNIEnv* env = base::android::AttachCurrentThread();
-  Java_OfflinePageBridge_offlinePageDeleted(env, java_ref_, offline_id,
-                                            CreateClientId(env, client_id));
+  Java_OfflinePageBridge_offlinePageDeleted(
+      env, java_ref_, ToJavaDeletedPageInfo(env, page_info));
 }
 
 void OfflinePageBridge::CheckPagesExistOffline(

diff --git a/chrome/browser/android/offline_pages/offline_page_bridge.h b/chrome/browser/android/offline_pages/offline_page_bridge.h
@@ -42,8 +42,8 @@ class OfflinePageBridge : public OfflinePageModel::Observer,
   void OfflinePageModelLoaded(OfflinePageModel* model) override;
   void OfflinePageAdded(OfflinePageModel* model,
                         const OfflinePageItem& added_page) override;
-  void OfflinePageDeleted(int64_t offline_id,
-                          const ClientId& client_id) override;
+  void OfflinePageDeleted(
+      const OfflinePageModel::DeletedPageInfo& page_info) override;
 
   void CheckPagesExistOffline(
       JNIEnv* env,

diff --git a/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc b/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
@@ -138,7 +138,8 @@ class RecentTabHelperTest
     page_added_count_++;
     all_pages_needs_updating_ = true;
   }
-  void OfflinePageDeleted(int64_t, const offline_pages::ClientId&) override {
+  void OfflinePageDeleted(
+      const OfflinePageModel::DeletedPageInfo& pageInfo) override {
     model_removed_count_++;
     all_pages_needs_updating_ = true;
   }

diff --git a/chrome/browser/ntp_snippets/download_suggestions_provider.cc b/chrome/browser/ntp_snippets/download_suggestions_provider.cc
@@ -383,12 +383,11 @@ void DownloadSuggestionsProvider::OfflinePageAdded(
 }
 
 void DownloadSuggestionsProvider::OfflinePageDeleted(
-    int64_t offline_id,
-    const offline_pages::ClientId& client_id) {
+    const offline_pages::OfflinePageModel::DeletedPageInfo& page_info) {
   DCHECK(offline_page_model_);
   if (IsClientIdForOfflinePageDownload(
-          offline_page_model_->GetPolicyController(), client_id)) {
-    InvalidateSuggestion(GetOfflinePagePerCategoryID(offline_id));
+          offline_page_model_->GetPolicyController(), page_info.client_id)) {
+    InvalidateSuggestion(GetOfflinePagePerCategoryID(page_info.offline_id));
   }
 }
 

diff --git a/chrome/browser/ntp_snippets/download_suggestions_provider.h b/chrome/browser/ntp_snippets/download_suggestions_provider.h
@@ -92,8 +92,9 @@ class DownloadSuggestionsProvider
   void OfflinePageAdded(
       offline_pages::OfflinePageModel* model,
       const offline_pages::OfflinePageItem& added_page) override;
-  void OfflinePageDeleted(int64_t offline_id,
-                          const offline_pages::ClientId& client_id) override;
+  void OfflinePageDeleted(
+      const offline_pages::OfflinePageModel::DeletedPageInfo& page_info)
+      override;
 
   // content::DownloadManager::Observer implementation.
   void OnDownloadCreated(content::DownloadManager* manager,

diff --git a/chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc b/chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc
@@ -324,7 +324,9 @@ class DownloadSuggestionsProviderTest : public testing::Test {
 
   void FireOfflinePageDeleted(const OfflinePageItem& item) {
     DCHECK(provider_);
-    provider_->OfflinePageDeleted(item.offline_id, item.client_id);
+    offline_pages::OfflinePageModel::DeletedPageInfo info(
+        item.offline_id, item.client_id, item.request_origin);
+    provider_->OfflinePageDeleted(info);
   }
 
   void FireDownloadCreated(DownloadItem* item) {

diff --git a/components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc b/components/ntp_snippets/offline_pages/recent_tab_suggestions_provider_unittest.cc
@@ -116,7 +116,9 @@ class RecentTabSuggestionsProviderTestNoLoad : public testing::Test {
     int tab_id = offline_pages::RecentTabsUIAdapterDelegate::TabIdFromClientId(
         item.client_id);
     RemoveTab(tab_id);
-    ui_adapter_->OfflinePageDeleted(item.offline_id, item.client_id);
+    ui_adapter_->OfflinePageDeleted(
+        offline_pages::OfflinePageModel::DeletedPageInfo(
+            item.offline_id, item.client_id, "" /* request_origin */));
   }
 
   std::set<std::string> ReadDismissedIDsFromPrefs() {

diff --git a/components/ntp_snippets/remote/prefetched_pages_tracker_impl.cc b/components/ntp_snippets/remote/prefetched_pages_tracker_impl.cc
@@ -86,10 +86,9 @@ void PrefetchedPagesTrackerImpl::OfflinePageAdded(
 }
 
 void PrefetchedPagesTrackerImpl::OfflinePageDeleted(
-    int64_t offline_id,
-    const offline_pages::ClientId& client_id) {
+    const offline_pages::OfflinePageModel::DeletedPageInfo& page_info) {
   std::map<int64_t, GURL>::iterator it =
-      offline_id_to_url_mapping_.find(offline_id);
+      offline_id_to_url_mapping_.find(page_info.offline_id);
   if (it != offline_id_to_url_mapping_.end()) {
     DCHECK(prefetched_urls_.count(it->second));
     prefetched_urls_.erase(it->second);

diff --git a/components/ntp_snippets/remote/prefetched_pages_tracker_impl.h b/components/ntp_snippets/remote/prefetched_pages_tracker_impl.h
@@ -41,8 +41,9 @@ class PrefetchedPagesTrackerImpl
   void OfflinePageAdded(
       offline_pages::OfflinePageModel* model,
       const offline_pages::OfflinePageItem& added_page) override;
-  void OfflinePageDeleted(int64_t offline_id,
-                          const offline_pages::ClientId& client_id) override;
+  void OfflinePageDeleted(
+      const offline_pages::OfflinePageModel::DeletedPageInfo& page_info)
+      override;
 
  private:
   void Initialize(const std::vector<offline_pages::OfflinePageItem>&

diff --git a/components/ntp_snippets/remote/prefetched_pages_tracker_impl_unittest.cc b/components/ntp_snippets/remote/prefetched_pages_tracker_impl_unittest.cc
@@ -141,7 +141,8 @@ TEST_F(PrefetchedPagesTrackerImplTest, ShouldDeletePrefetchedURLWhenNotified) {
 
   ASSERT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
-  tracker.OfflinePageDeleted(item.offline_id, item.client_id);
+  tracker.OfflinePageDeleted(offline_pages::OfflinePageModel::DeletedPageInfo(
+      item.offline_id, item.client_id, "" /* request_origin */));
   EXPECT_FALSE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
 }
@@ -160,8 +161,9 @@ TEST_F(PrefetchedPagesTrackerImplTest,
 
   ASSERT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
-  tracker.OfflinePageDeleted(manually_downloaded_item.offline_id,
-                             manually_downloaded_item.client_id);
+  tracker.OfflinePageDeleted(offline_pages::OfflinePageModel::DeletedPageInfo(
+      manually_downloaded_item.offline_id, manually_downloaded_item.client_id,
+      "" /* request_origin */));
   EXPECT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
 }

diff --git a/components/offline_pages/core/downloads/download_ui_adapter.cc b/components/offline_pages/core/downloads/download_ui_adapter.cc
@@ -116,11 +116,11 @@ void DownloadUIAdapter::OfflinePageAdded(OfflinePageModel* model,
   AddItemHelper(base::MakeUnique<ItemInfo>(added_page, temporarily_hidden));
 }
 
-void DownloadUIAdapter::OfflinePageDeleted(int64_t offline_id,
-                                           const ClientId& client_id) {
-  if (!delegate_->IsVisibleInUI(client_id))
+void DownloadUIAdapter::OfflinePageDeleted(
+    const OfflinePageModel::DeletedPageInfo& page_info) {
+  if (!delegate_->IsVisibleInUI(page_info.client_id))
     return;
-  DeleteItemHelper(client_id.id);
+  DeleteItemHelper(page_info.client_id.id);
 }
 
 // RequestCoordinator::Observer

diff --git a/components/offline_pages/core/downloads/download_ui_adapter.h b/components/offline_pages/core/downloads/download_ui_adapter.h
@@ -109,8 +109,8 @@ class DownloadUIAdapter : public OfflinePageModel::Observer,
   void OfflinePageModelLoaded(OfflinePageModel* model) override;
   void OfflinePageAdded(OfflinePageModel* model,
                         const OfflinePageItem& added_page) override;
-  void OfflinePageDeleted(int64_t offline_id,
-                          const ClientId& client_id) override;
+  void OfflinePageDeleted(
+      const OfflinePageModel::DeletedPageInfo& page_info) override;
 
   // RequestCoordinator::Observer
   void OnAdded(const SavePageRequest& request) override;

diff --git a/components/offline_pages/core/downloads/download_ui_adapter_unittest.cc b/components/offline_pages/core/downloads/download_ui_adapter_unittest.cc
@@ -112,8 +112,9 @@ class MockOfflinePageModel : public StubOfflinePageModel {
   void DeletePageAndNotifyAdapter(const std::string& guid) {
     for (const auto& page : pages) {
       if (page.second.client_id.id == guid) {
-        observer_->OfflinePageDeleted(page.second.offline_id,
-                                      page.second.client_id);
+        DeletedPageInfo info(page.second.offline_id, page.second.client_id,
+                             page.second.request_origin);
+        observer_->OfflinePageDeleted(info);
         pages.erase(page.first);
         return;
       }

diff --git a/components/offline_pages/core/offline_page_model.cc b/components/offline_pages/core/offline_page_model.cc
@@ -25,6 +25,14 @@ OfflinePageModel::SavePageParams::SavePageParams(const SavePageParams& other) {
 
 OfflinePageModel::SavePageParams::~SavePageParams() {}
 
+OfflinePageModel::DeletedPageInfo::DeletedPageInfo(
+    int64_t offline_id,
+    const ClientId& client_id,
+    const std::string& request_origin)
+    : offline_id(offline_id),
+      client_id(client_id),
+      request_origin(request_origin) {}
+
 // static
 bool OfflinePageModel::CanSaveURL(const GURL& url) {
   return url.is_valid() && url.SchemeIsHTTPOrHTTPS();

diff --git a/components/offline_pages/core/offline_page_model.h b/components/offline_pages/core/offline_page_model.h
@@ -82,6 +82,19 @@ class OfflinePageModel : public base::SupportsUserData {
     std::string request_origin;
   };
 
+  // Information about a deleted page.
+  struct DeletedPageInfo {
+    DeletedPageInfo(int64_t offline_id,
+                    const ClientId& client_id,
+                    const std::string& request_origin);
+    // The ID of the deleted page.
+    int64_t offline_id;
+    // Client ID of the deleted page.
+    ClientId client_id;
+    // The origin that the page was saved on behalf of.
+    std::string request_origin;
+  };
+
   // Observer of the OfflinePageModel.
   class Observer {
    public:
@@ -93,8 +106,7 @@ class OfflinePageModel : public base::SupportsUserData {
                                   const OfflinePageItem& added_page) = 0;
 
     // Invoked when an offline copy related to |offline_id| was deleted.
-    virtual void OfflinePageDeleted(int64_t offline_id,
-                                    const ClientId& client_id) = 0;
+    virtual void OfflinePageDeleted(const DeletedPageInfo& page_info) = 0;
 
    protected:
     virtual ~Observer() = default;

diff --git a/components/offline_pages/core/offline_page_model_impl.cc b/components/offline_pages/core/offline_page_model_impl.cc
@@ -982,8 +982,9 @@ void OfflinePageModelImpl::OnRemoveOfflinePagesDone(
   }
 
   for (const auto& page : result->updated_items) {
+    DeletedPageInfo info(page.offline_id, page.client_id, page.request_origin);
     for (Observer& observer : observers_)
-      observer.OfflinePageDeleted(page.offline_id, page.client_id);
+      observer.OfflinePageDeleted(info);
   }
 
   DeletePageResult delete_result;