[Offline pages]: Add information to broadcasted intent to help mitigate need...

[Offline pages]: Add information to broadcasted intent to help mitigate need for re-fetching the data

Bug: 812022
Change-Id: I58b99dd7b841c1312016217117b2ecd62c65188e
Reviewed-on: https://chromium-review.googlesource.com/991456
Commit-Queue: Cathy Li <chili@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560969}

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/CctOfflinePageModelObserver.java

@@ -7,9 +7,11 @@ package org.chromium.chrome.browser.offlinepages;
 import android.app.PendingIntent;
 import android.content.Context;
 import android.content.Intent;
+import android.os.Bundle;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.chrome.browser.AppHooks;
 
@@ -21,20 +23,38 @@ import org.chromium.chrome.browser.AppHooks;
  */
 public class CctOfflinePageModelObserver {
     private static final String TAG = "CctModelObserver";
+    // Key for pending intent used by receivers to verify origin.
     private static final String ORIGIN_VERIFICATION_KEY =
             "org.chromium.chrome.extra.CHROME_NAME_PENDING_INTENT";
-    private static final String ACTION_OFFLINE_PAGES_UPDATED =
+
+    // Key for bundle which stores information about the page changed.
+    @VisibleForTesting
+    static final String PAGE_INFO_KEY = "org.chromium.chrome.extra.OFFLINE_PAGE_INFO";
+    // Key within page info bundle for whether the page was added (true) or removed (false).
+    @VisibleForTesting
+    static final String IS_NEW_KEY = "is_new";
+    // Key within page info bundle for online url.
+    @VisibleForTesting
+    static final String URL_KEY = "url";
+    // Broadcast action.
+    @VisibleForTesting
+    static final String ACTION_OFFLINE_PAGES_UPDATED =
             "org.chromium.chrome.browser.offlinepages.OFFLINE_PAGES_CHANGED";
 
     @CalledByNative
-    private static void onPageChanged(String originString) {
+    @VisibleForTesting
+    static void onPageChanged(String originString, boolean isNew, String url) {
         OfflinePageOrigin origin = new OfflinePageOrigin(originString);
-        if (!origin.isChrome()) compareSignaturesAndFireIntent(origin);
+        if (origin.isChrome()) return;
+        Bundle bundle = new Bundle();
+        bundle.putBoolean(IS_NEW_KEY, isNew);
+        bundle.putString(URL_KEY, url);
+        compareSignaturesAndFireIntent(origin, bundle);
     }
 
-    private static void compareSignaturesAndFireIntent(OfflinePageOrigin origin) {
+    private static void compareSignaturesAndFireIntent(OfflinePageOrigin origin, Bundle pageInfo) {
         if (!isInWhitelist(origin.getAppName())) {
-            Log.w(TAG, "Non-whitelisted app");
+            Log.w(TAG, "Non-whitelisted app: " + origin.getAppName());
             return;
         }
         Context context = ContextUtils.getApplicationContext();
@@ -54,6 +74,7 @@ public class CctOfflinePageModelObserver {
         originVerification.cancel();
 
         intent.putExtra(ORIGIN_VERIFICATION_KEY, originVerification);
+        intent.putExtra(PAGE_INFO_KEY, pageInfo);
         context.sendBroadcast(intent);
     }
 

chrome/android/java_sources.gni

@@ -2081,6 +2081,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSectionTest.java",
   "junit/src/org/chromium/chrome/browser/ntp/cards/SectionListTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java",
+  "junit/src/org/chromium/chrome/browser/offlinepages/CctOfflinePageModelObserverTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/ClientIdTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java",
   "junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeUnitTest.java",

chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/CctOfflinePageModelObserverTest.java

+// 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.
+
+package org.chromium.chrome.browser.offlinepages;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.AdditionalMatchers.not;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import android.content.Context;
+import android.content.Intent;
+import android.content.pm.PackageInfo;
+import android.content.pm.PackageManager;
+import android.content.pm.Signature;
+import android.os.Bundle;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.ContextUtils;
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.browser.AppHooks;
+import org.chromium.chrome.browser.AppHooksImpl;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Test class for {@link CctOfflinePageModelObserver}.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class CctOfflinePageModelObserverTest {
+    private static final String APP_NAME = "abc.xyz";
+    private static final String ORIGIN_STRING = "[abc.xyz, []]";
+    private static final String URL = "http://www.google.com";
+
+    @Mock
+    private Context mContext;
+    @Mock
+    private PackageManager mPackageManager;
+    @Mock
+    private AppHooksImpl mAppHooks;
+
+    private PackageInfo mInfo;
+
+    @Before
+    public void setUp() throws PackageManager.NameNotFoundException {
+        MockitoAnnotations.initMocks(this);
+
+        mInfo = new PackageInfo();
+        mInfo.signatures = new Signature[0];
+
+        when(mPackageManager.getPackageInfo(eq(APP_NAME), anyInt())).thenReturn(mInfo);
+        when(mPackageManager.getPackageInfo(not(eq(APP_NAME)), anyInt()))
+                .thenThrow(new PackageManager.NameNotFoundException());
+
+        when(mContext.getPackageManager()).thenReturn(mPackageManager);
+
+        List<String> whitelist = new ArrayList<>();
+        whitelist.add(APP_NAME);
+        when(mAppHooks.getOfflinePagesCctWhitelist()).thenReturn(whitelist);
+
+        ContextUtils.initApplicationContextForTests(mContext);
+        AppHooks.setInstanceForTesting(mAppHooks);
+    }
+
+    @Test
+    public void testSendBroadcastForAppNameInWhitelist_addedPage() {
+        ArgumentCaptor<Intent> intentCaptor = ArgumentCaptor.forClass(Intent.class);
+        CctOfflinePageModelObserver.onPageChanged(ORIGIN_STRING, true, URL);
+
+        verify(mContext).sendBroadcast(intentCaptor.capture());
+
+        Intent intent = intentCaptor.getValue();
+        assertEquals(APP_NAME, intent.getPackage());
+        assertEquals(CctOfflinePageModelObserver.ACTION_OFFLINE_PAGES_UPDATED, intent.getAction());
+        Bundle pageInfo = intent.getParcelableExtra(CctOfflinePageModelObserver.PAGE_INFO_KEY);
+        assertNotNull(pageInfo);
+
+        assertTrue(pageInfo.getBoolean(CctOfflinePageModelObserver.IS_NEW_KEY));
+        assertEquals(URL, pageInfo.getString(CctOfflinePageModelObserver.URL_KEY));
+    }
+
+    @Test
+    public void testSendBroadcastForAppNameInWhitelist_deletedPage() {
+        ArgumentCaptor<Intent> intentCaptor = ArgumentCaptor.forClass(Intent.class);
+        CctOfflinePageModelObserver.onPageChanged(ORIGIN_STRING, false, URL);
+
+        verify(mContext).sendBroadcast(intentCaptor.capture());
+
+        Intent intent = intentCaptor.getValue();
+        assertEquals(APP_NAME, intent.getPackage());
+        assertEquals(CctOfflinePageModelObserver.ACTION_OFFLINE_PAGES_UPDATED, intent.getAction());
+        Bundle pageInfo = intent.getParcelableExtra(CctOfflinePageModelObserver.PAGE_INFO_KEY);
+        assertNotNull(pageInfo);
+
+        assertFalse(pageInfo.getBoolean(CctOfflinePageModelObserver.IS_NEW_KEY));
+        assertEquals(URL, pageInfo.getString(CctOfflinePageModelObserver.URL_KEY));
+    }
+
+    @Test
+    public void testDoesNotSendBroadcastForAppNameNotInWhitelist() {
+        String originString = "[xyz.abc,[]]";
+        CctOfflinePageModelObserver.onPageChanged(originString, true, URL);
+
+        ArgumentCaptor<Intent> intentCaptor = ArgumentCaptor.forClass(Intent.class);
+        verify(mContext, never()).sendBroadcast(intentCaptor.capture());
+    }
+
+    @Test
+    public void testDoesNotSendBroadcastForChrome() {
+        String originString = "";
+        CctOfflinePageModelObserver.onPageChanged(originString, true, URL);
+
+        ArgumentCaptor<Intent> intentCaptor = ArgumentCaptor.forClass(Intent.class);
+        verify(mContext, never()).sendBroadcast(intentCaptor.capture());
+    }
+}

chrome/browser/ntp_snippets/download_suggestions_provider_unittest.cc

@@ -323,7 +323,7 @@ class DownloadSuggestionsProviderTest : public testing::Test {
     DCHECK(provider_);
     offline_pages::OfflinePageModel::DeletedPageInfo info(
         item.offline_id, item.system_download_id, item.client_id,
-        item.request_origin);
+        item.request_origin, item.url);
     provider_->OfflinePageDeleted(info);
   }
 

chrome/browser/offline_pages/android/cct_origin_observer.cc

@@ -36,14 +36,16 @@ void CctOriginObserver::OfflinePageAdded(OfflinePageModel* model,
                                          const OfflinePageItem& added_page) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_CctOfflinePageModelObserver_onPageChanged(
-      env, ConvertUTF8ToJavaString(env, added_page.request_origin));
+      env, ConvertUTF8ToJavaString(env, added_page.request_origin), true,
+      ConvertUTF8ToJavaString(env, added_page.url.spec()));
 }
 
 void CctOriginObserver::OfflinePageDeleted(
     const OfflinePageModel::DeletedPageInfo& page_info) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_CctOfflinePageModelObserver_onPageChanged(
-      env, ConvertUTF8ToJavaString(env, page_info.request_origin));
+      env, ConvertUTF8ToJavaString(env, page_info.request_origin), false,
+      ConvertUTF8ToJavaString(env, page_info.url.spec()));
 }
 
 }  // namespace offline_pages

components/ntp_snippets/remote/prefetched_pages_tracker_impl_unittest.cc

@@ -140,7 +140,7 @@ TEST_F(PrefetchedPagesTrackerImplTest, ShouldDeletePrefetchedURLWhenNotified) {
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
   tracker.OfflinePageDeleted(offline_pages::OfflinePageModel::DeletedPageInfo(
       item.offline_id, kSystemDownloadId, item.client_id,
-      /*request_origin=*/""));
+      /*request_origin=*/"", item.original_url));
   EXPECT_FALSE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
 }
@@ -163,7 +163,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
   tracker.OfflinePageDeleted(offline_pages::OfflinePageModel::DeletedPageInfo(
       manually_downloaded_item.offline_id, kSystemDownloadId,
       manually_downloaded_item.client_id,
-      /*request_origin=*/""));
+      /*request_origin=*/"", manually_downloaded_item.original_url));
   EXPECT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
 }
@@ -274,7 +274,7 @@ TEST_F(PrefetchedPagesTrackerImplTest,
 
   tracker.OfflinePageDeleted(offline_pages::OfflinePageModel::DeletedPageInfo(
       first_item.offline_id, kSystemDownloadId, first_item.client_id,
-      /*request_origin=*/""));
+      /*request_origin=*/"", first_item.original_url));
 
   // Only one offline page (out of two) has been removed, the remaining one
   // should be reported here.
@@ -299,14 +299,14 @@ TEST_F(PrefetchedPagesTrackerImplTest,
 
   tracker.OfflinePageDeleted(offline_pages::OfflinePageModel::DeletedPageInfo(
       first_item.offline_id, kSystemDownloadId, first_item.client_id,
-      /*request_origin=*/""));
+      /*request_origin=*/"", first_item.original_url));
 
   ASSERT_TRUE(
       tracker.PrefetchedOfflinePageExists(GURL("http://prefetched.com")));
 
   tracker.OfflinePageDeleted(offline_pages::OfflinePageModel::DeletedPageInfo(
       second_item.offline_id, kSystemDownloadId, second_item.client_id,
-      /*request_origin=*/""));
+      /*request_origin=*/"", second_item.original_url));
 
   // All offline pages have been removed, their absence should be reported here.
   EXPECT_FALSE(

components/offline_pages/core/downloads/download_ui_adapter_unittest.cc

@@ -155,7 +155,8 @@ class MockOfflinePageModel : public StubOfflinePageModel {
     for (const auto& page : pages) {
       if (page.second.client_id.id == guid) {
         DeletedPageInfo info(page.second.offline_id, kSystemDownloadId,
-                             page.second.client_id, page.second.request_origin);
+                             page.second.client_id, page.second.request_origin,
+                             page.second.original_url);
         observer_->OfflinePageDeleted(info);
         pages.erase(page.first);
         return;

components/offline_pages/core/model/delete_page_task.cc

@@ -42,7 +42,7 @@ namespace {
 // please take a look at GetCachedDeletedPageInfoWrappersByUrlPredicateSync.
 #define INFO_WRAPPER_FIELDS                                                  \
   "offline_id, system_download_id, client_namespace, client_id, file_path, " \
-  "request_origin, access_count, creation_time"
+  "request_origin, access_count, creation_time, online_url"
 #define INFO_WRAPPER_FIELD_COUNT 8
 
 struct DeletedPageInfoWrapper {
@@ -56,6 +56,7 @@ struct DeletedPageInfoWrapper {
   // Used by metric collection only:
   int access_count;
   base::Time creation_time;
+  GURL url;
 };
 
 DeletedPageInfoWrapper CreateInfoWrapper(const sql::Statement& statement) {
@@ -70,6 +71,7 @@ DeletedPageInfoWrapper CreateInfoWrapper(const sql::Statement& statement) {
   info_wrapper.access_count = statement.ColumnInt(6);
   info_wrapper.creation_time =
       store_utils::FromDatabaseTime(statement.ColumnInt64(7));
+  info_wrapper.url = GURL(statement.ColumnString(8));
   return info_wrapper;
 }
 
@@ -134,7 +136,8 @@ DeletePageTaskResult DeletePagesByDeletedPageInfoWrappersSync(
       if (DeletePageEntryByOfflineIdSync(db, info_wrapper.offline_id)) {
         deleted_page_infos.emplace_back(
             info_wrapper.offline_id, info_wrapper.system_download_id,
-            info_wrapper.client_id, info_wrapper.request_origin);
+            info_wrapper.client_id, info_wrapper.request_origin,
+            info_wrapper.url);
       }
     }
   }

components/offline_pages/core/offline_page_model.cc

@@ -27,11 +27,13 @@ OfflinePageModel::DeletedPageInfo::DeletedPageInfo(
     int64_t offline_id,
     int64_t system_download_id,
     const ClientId& client_id,
-    const std::string& request_origin)
+    const std::string& request_origin,
+    const GURL& url)
     : offline_id(offline_id),
       system_download_id(system_download_id),
       client_id(client_id),
-      request_origin(request_origin) {}
+      request_origin(request_origin),
+      url(url) {}
 
 // static
 bool OfflinePageModel::CanSaveURL(const GURL& url) {

components/offline_pages/core/offline_page_model.h

@@ -82,7 +82,8 @@ class OfflinePageModel : public base::SupportsUserData, public KeyedService {
     DeletedPageInfo(int64_t offline_id,
                     int64_t system_download_id,
                     const ClientId& client_id,
-                    const std::string& request_origin);
+                    const std::string& request_origin,
+                    const GURL& url);
     // The ID of the deleted page.
     int64_t offline_id;
     // The system download manager id of the deleted page.  This will be 0 if
@@ -92,6 +93,8 @@ class OfflinePageModel : public base::SupportsUserData, public KeyedService {
     ClientId client_id;
     // The origin that the page was saved on behalf of.
     std::string request_origin;
+    // URL of the page that was deleted.
+    GURL url;
   };
 
   // Observer of the OfflinePageModel.