Support saving offline pages to downloads collection

On Android Q+, publish offline pages to the downloads collection
rather than DownloadManager.

Bug: 966236
Change-Id: Ibef10c8b35fb62dd0e088580ed1cd3f9a2614723
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1641664
Commit-Queue: Ian Wells <iwells@chromium.org>
Reviewed-by: Dan H <harringtond@chromium.org>
Cr-Commit-Position: refs/heads/master@{#678316}

chrome/android/chrome_test_java_sources.gni

@@ -251,6 +251,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java",
   "javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java",
   "javatests/src/org/chromium/chrome/browser/offlinepages/MHTMLPageTest.java",
+  "javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageArchivePublisherBridgeTest.java",
   "javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageAutoFetchTest.java",
   "javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java",
   "javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java",

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

@@ -6,15 +6,28 @@ package org.chromium.chrome.browser.offlinepages;
 
 import android.annotation.TargetApi;
 import android.app.DownloadManager;
+import android.content.ContentResolver;
+import android.content.ContentValues;
 import android.content.Context;
 import android.net.Uri;
 import android.os.Build;
+import android.os.Environment;
+import android.provider.MediaStore.MediaColumns;
+import android.text.format.DateUtils;
 
+import org.chromium.base.ContentUriUtils;
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 
+import java.io.FileInputStream;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+
 /**
  * Since the {@link AndroidDownloadManager} can only be accessed from Java, this bridge will
  * transfer all C++ calls over to Java land for making the call to ADM.  This is a one-way bridge,
@@ -34,7 +47,8 @@ public class OfflinePageArchivePublisherBridge {
 
     /** Returns true if DownloadManager is installed on the phone. */
     @CalledByNative
-    private static boolean isAndroidDownloadManagerInstalled() {
+    @VisibleForTesting
+    public static boolean isAndroidDownloadManagerInstalled() {
         DownloadManager downloadManager = getDownloadManager();
         return (downloadManager != null);
     }
@@ -50,7 +64,8 @@ public class OfflinePageArchivePublisherBridge {
      * @return the download ID of this item as assigned by the download manager.
      */
     @CalledByNative
-    private static long addCompletedDownload(String title, String description, String path,
+    @VisibleForTesting
+    public static long addCompletedDownload(String title, String description, String path,
             long length, String uri, String referer) {
         try {
             // Call the proper version of the pass through based on the supported API level.
@@ -93,7 +108,8 @@ public class OfflinePageArchivePublisherBridge {
      * @return the number of IDs that were removed.
      */
     @CalledByNative
-    private static int remove(long[] ids) {
+    @VisibleForTesting
+    public static int remove(long[] ids) {
         DownloadManager downloadManager = getDownloadManager();
         try {
             if (downloadManager == null) return 0;
@@ -109,4 +125,84 @@ public class OfflinePageArchivePublisherBridge {
         return (DownloadManager) ContextUtils.getApplicationContext().getSystemService(
                 Context.DOWNLOAD_SERVICE);
     }
+
+    /**
+     * Adds an archive to the downloads collection on Android Q+. Preferred alternative to
+     * addCompletedDownload for Android Q and later.
+     *
+     * TODO(iwells): Remove reflection once API level 29 is supported.
+     *
+     * @param page Offline page to be published.
+     * @return Content URI referring to the published page.
+     */
+    @CalledByNative
+    @VisibleForTesting
+    public static String publishArchiveToDownloadsCollection(OfflinePageItem page) {
+        assert !org.chromium.base.BuildInfo.isAtLeastQ();
+
+        final String isPending = "is_pending"; // MediaStore.IS_PENDING
+
+        // Collect all fields for creating intermediate URI.
+        final long now = System.currentTimeMillis() / 1000;
+        ContentValues pendingValues = new ContentValues();
+        pendingValues.put(MediaColumns.DATE_ADDED, now);
+        pendingValues.put(MediaColumns.DATE_MODIFIED, now);
+        pendingValues.put(isPending, 1);
+        pendingValues.put("download_uri", page.getUrl()); // MediaStore.DownloadColumns.DOWNLOAD_URI
+
+        Uri externalDownloadUri;
+        try {
+            // Class android.provider.MediaStore.Downloads added in API level 29.
+            Class<?> downloadsClazz = Class.forName("android.provider.MediaStore$Downloads");
+            Field externalUriField = downloadsClazz.getDeclaredField("EXTERNAL_CONTENT_URI");
+            externalDownloadUri = (Uri) externalUriField.get(null);
+            Field primaryDirectoryField = MediaColumns.class.getDeclaredField("PRIMARY_DIRECTORY");
+            pendingValues.put(
+                    (String) primaryDirectoryField.get(null), Environment.DIRECTORY_DOWNLOADS);
+        } catch (Exception e) {
+            Log.d(TAG, "Unable to set pending download fields.", e);
+            return "";
+        }
+
+        // Pending URI will expire after 3 days.
+        long newExpirationTime = (System.currentTimeMillis() + 3 * DateUtils.DAY_IN_MILLIS) / 1000;
+        pendingValues.put("date_expires", newExpirationTime);
+
+        // Create intermediate URI.
+        ContentResolver contentResolver = ContextUtils.getApplicationContext().getContentResolver();
+        Uri intermediateUri = contentResolver.insert(externalDownloadUri, pendingValues);
+        if (intermediateUri == null || !ContentUriUtils.isContentUri(intermediateUri.toString())) {
+            Log.d(TAG, "Failed to create intermediate URI.");
+            return "";
+        }
+
+        // Copy archive to intermediate URI.
+        try {
+            // Class android.os.FileUtils added in API level 29.
+            Class<?> fileUtilsClazz = Class.forName("android.os.FileUtils");
+            Method copyMethod =
+                    fileUtilsClazz.getMethod("copy", InputStream.class, OutputStream.class);
+
+            OutputStream out = contentResolver.openOutputStream(intermediateUri);
+            InputStream in = new FileInputStream(page.getFilePath());
+            copyMethod.invoke(null, in, out);
+            in.close();
+            out.close();
+        } catch (Exception e) {
+            Log.d(TAG, "Unable to copy archive to pending URI.", e);
+            return "";
+        }
+
+        // Set display name, MIME type, and pending -> false.
+        final ContentValues publishValues = new ContentValues();
+        publishValues.put(isPending, 0);
+        publishValues.putNull("date_expires");
+        publishValues.put(MediaColumns.DISPLAY_NAME, page.getTitle());
+        publishValues.put(MediaColumns.MIME_TYPE, "multipart/related");
+        if (contentResolver.update(intermediateUri, publishValues, null, null) != 1) {
+            Log.d(TAG, "Failed to finish publishing archive.");
+        }
+
+        return intermediateUri.toString();
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageArchivePublisherBridgeTest.java

+// Copyright 2019 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 android.support.test.InstrumentationRegistry;
+import android.support.test.filters.SmallTest;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.ContentUriUtils;
+import org.chromium.base.task.PostTask;
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.DisableIf;
+import org.chromium.base.test.util.MinAndroidSdkLevel;
+import org.chromium.chrome.browser.ChromeActivity;
+import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.offlinepages.OfflinePageBridge.OfflinePageModelObserver;
+import org.chromium.chrome.browser.offlinepages.OfflinePageBridge.SavePageCallback;
+import org.chromium.chrome.browser.profiles.Profile;
+import org.chromium.chrome.test.ChromeActivityTestRule;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.content_public.browser.UiThreadTaskTraits;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
+import org.chromium.net.NetworkChangeNotifier;
+import org.chromium.net.test.EmbeddedTestServer;
+
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+/** Unit tests for {@link OfflinePageArchivePublisherBridge}. */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+public class OfflinePageArchivePublisherBridgeTest {
+    @Rule
+    public ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
+            new ChromeActivityTestRule<>(ChromeActivity.class);
+
+    private static final String TEST_PAGE = "/chrome/test/data/android/about.html";
+    private static final int TIMEOUT_MS = 5000;
+    private static final ClientId TEST_CLIENT_ID =
+            new ClientId(OfflinePageBridge.DOWNLOAD_NAMESPACE, "1234");
+
+    private OfflinePageBridge mOfflinePageBridge;
+    private EmbeddedTestServer mTestServer;
+    private String mTestPage;
+
+    private void initializeBridgeForProfile(final boolean incognitoProfile)
+            throws InterruptedException {
+        final Semaphore semaphore = new Semaphore(0);
+        PostTask.runOrPostTask(UiThreadTaskTraits.DEFAULT, () -> {
+            Profile profile = Profile.getLastUsedProfile();
+            if (incognitoProfile) {
+                profile = profile.getOffTheRecordProfile();
+            }
+            // Ensure we start in an offline state.
+            mOfflinePageBridge = OfflinePageBridge.getForProfile(profile);
+            if (mOfflinePageBridge == null || mOfflinePageBridge.isOfflinePageModelLoaded()) {
+                semaphore.release();
+                return;
+            }
+            mOfflinePageBridge.addObserver(new OfflinePageModelObserver() {
+                @Override
+                public void offlinePageModelLoaded() {
+                    semaphore.release();
+                    mOfflinePageBridge.removeObserver(this);
+                }
+            });
+        });
+        Assert.assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS));
+        if (!incognitoProfile) Assert.assertNotNull(mOfflinePageBridge);
+    }
+
+    @Before
+    public void setUp() throws Exception {
+        mActivityTestRule.startMainActivityOnBlankPage();
+
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            // Ensure we start in an offline state.
+            NetworkChangeNotifier.forceConnectivityState(false);
+            if (!NetworkChangeNotifier.isInitialized()) {
+                NetworkChangeNotifier.init();
+            }
+        });
+
+        initializeBridgeForProfile(false);
+
+        mTestServer = EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
+        mTestPage = mTestServer.getURL(TEST_PAGE);
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        mTestServer.stopAndDestroyServer();
+    }
+
+    // TODO(iwells): Change "29" to "Build.VERSION_CODES.Q" when it's available.
+    @Test
+    @SmallTest
+    @DisableIf.Build(sdk_is_greater_than = 28)
+    public void testAddCompletedDownload() throws InterruptedException, TimeoutException {
+        Assert.assertTrue(OfflinePageArchivePublisherBridge.isAndroidDownloadManagerInstalled());
+
+        mActivityTestRule.loadUrl(mTestPage);
+        savePage(TEST_CLIENT_ID);
+        OfflinePageItem page = OfflineTestUtil.getAllPages().get(0);
+
+        long downloadId = OfflinePageArchivePublisherBridge.addCompletedDownload(page.getTitle(),
+                "description", page.getFilePath(), page.getFileSize(), page.getUrl(), "");
+
+        Assert.assertNotEquals(0L, downloadId);
+    }
+
+    @Test
+    @SmallTest
+    @DisableIf.Build(sdk_is_greater_than = 28)
+    public void testRemove() throws InterruptedException, TimeoutException {
+        Assert.assertTrue(OfflinePageArchivePublisherBridge.isAndroidDownloadManagerInstalled());
+
+        mActivityTestRule.loadUrl(mTestPage);
+        savePage(TEST_CLIENT_ID);
+        OfflinePageItem page = OfflineTestUtil.getAllPages().get(0);
+
+        long downloadId = OfflinePageArchivePublisherBridge.addCompletedDownload(page.getTitle(),
+                "description", page.getFilePath(), page.getFileSize(), page.getUrl(), "");
+
+        Assert.assertNotEquals(0L, downloadId);
+
+        long[] ids = new long[] {downloadId};
+        Assert.assertEquals(1, OfflinePageArchivePublisherBridge.remove(ids));
+    }
+
+    @Test
+    @SmallTest
+    @MinAndroidSdkLevel(29)
+    public void testPublishArchiveToDownloadsCollection()
+            throws InterruptedException, TimeoutException {
+        // Save a page and publish.
+        mActivityTestRule.loadUrl(mTestPage);
+        savePage(TEST_CLIENT_ID);
+        OfflinePageItem page = OfflineTestUtil.getAllPages().get(0);
+
+        String publishedUri =
+                OfflinePageArchivePublisherBridge.publishArchiveToDownloadsCollection(page);
+        Assert.assertFalse(publishedUri.isEmpty());
+        Assert.assertTrue(ContentUriUtils.isContentUri(publishedUri));
+        Assert.assertTrue(ContentUriUtils.contentUriExists(publishedUri));
+    }
+
+    // Returns offline ID.
+    private void savePage(final ClientId clientId) throws InterruptedException {
+        final Semaphore semaphore = new Semaphore(0);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            mOfflinePageBridge.savePage(
+                    mActivityTestRule.getWebContents(), clientId, new SavePageCallback() {
+                        @Override
+                        public void onSavePageDone(int savePageResult, String url, long offlineId) {
+                            semaphore.release();
+                        }
+                    });
+        });
+        Assert.assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS));
+    }
+}

chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java

@@ -69,7 +69,7 @@ public class OfflinePageRequestTest {
         Tab tab = mActivityTestRule.getActivity().getActivityTab();
 
         // Load and save an offline page.
-        savePage(testUrl);
+        savePage(testUrl, CLIENT_ID);
         Assert.assertFalse(isErrorPage(tab));
         Assert.assertFalse(isOfflinePage(tab));
 
@@ -101,7 +101,7 @@ public class OfflinePageRequestTest {
         Tab tab = mActivityTestRule.getActivity().getActivityTab();
 
         // Load and save an offline page for the url with a fragment.
-        savePage(testUrlWithFragment);
+        savePage(testUrlWithFragment, CLIENT_ID);
         Assert.assertFalse(isErrorPage(tab));
         Assert.assertFalse(isOfflinePage(tab));
 
@@ -116,13 +116,47 @@ public class OfflinePageRequestTest {
         Assert.assertTrue(isOfflinePage(tab));
     }
 
-    private void savePage(String url) throws InterruptedException {
+    @Test
+    @SmallTest
+    @DisabledTest(message = "crbug.com/786233")
+    public void testLoadOfflinePageFromDownloadsOnDisconnectedNetwork() throws Exception {
+        // Specifically tests saving to and loading from Downloads.
+        EmbeddedTestServer testServer =
+                EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
+        String testUrl = testServer.getURL(TEST_PAGE);
+        String aboutUrl = testServer.getURL(ABOUT_PAGE);
+
+        Tab tab = mActivityTestRule.getActivity().getActivityTab();
+
+        // Load and save a persistent offline page using a persistent namespace so that the archive
+        // will be published.
+        savePage(testUrl, new ClientId(OfflinePageBridge.DOWNLOAD_NAMESPACE, "1234"));
+        Assert.assertFalse(isErrorPage(tab));
+        Assert.assertFalse(isOfflinePage(tab));
+
+        // Load another page.
+        mActivityTestRule.loadUrl(aboutUrl);
+        Assert.assertFalse(isErrorPage(tab));
+        Assert.assertFalse(isOfflinePage(tab));
+
+        // Stop the server and also disconnect the network.
+        testServer.stopAndDestroyServer();
+        TestThreadUtils.runOnUiThreadBlocking(
+                () -> { NetworkChangeNotifier.forceConnectivityState(false); });
+
+        // Load the page that has an offline copy. The offline page should be shown.
+        mActivityTestRule.loadUrl(testUrl);
+        Assert.assertFalse(isErrorPage(tab));
+        Assert.assertTrue(isOfflinePage(tab));
+    }
+
+    private void savePage(String url, ClientId clientId) throws InterruptedException {
         mActivityTestRule.loadUrl(url);
 
         final Semaphore semaphore = new Semaphore(0);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             mOfflinePageBridge.savePage(
-                    mActivityTestRule.getWebContents(), CLIENT_ID, new SavePageCallback() {
+                    mActivityTestRule.getWebContents(), clientId, new SavePageCallback() {
                         @Override
                         public void onSavePageDone(int savePageResult, String url, long offlineId) {
                             Assert.assertEquals(

chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.cc

@@ -7,6 +7,7 @@
 #include <errno.h>
 #include <utility>
 
+#include "base/android/build_info.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
@@ -17,16 +18,16 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_runner_util.h"
 #include "chrome/android/chrome_jni_headers/OfflinePageArchivePublisherBridge_jni.h"
+#include "chrome/browser/offline_pages/android/offline_page_bridge.h"
 #include "components/offline_pages/core/archive_manager.h"
 #include "components/offline_pages/core/model/offline_page_model_utils.h"
 #include "components/offline_pages/core/offline_store_utils.h"
 
-using base::android::ScopedJavaLocalRef;
-
 namespace offline_pages {
 
 namespace {
 
+using base::android::ScopedJavaLocalRef;
 using offline_pages::SavePageResult;
 
 // Creates a singleton Delegate.
@@ -35,15 +36,25 @@ OfflinePageArchivePublisherImpl::Delegate* GetDefaultDelegate() {
   return &delegate;
 }
 
+bool ShouldUseDownloadsCollection() {
+  return base::android::BuildInfo::GetInstance()->is_at_least_q();
+}
+
 // Helper function to do the move and register synchronously. Make sure this is
 // called from a background thread.
 PublishArchiveResult MoveAndRegisterArchive(
     const offline_pages::OfflinePageItem& offline_page,
     const base::FilePath& publish_directory,
     OfflinePageArchivePublisherImpl::Delegate* delegate) {
-  PublishArchiveResult archive_result;
+  // For Android Q+, use the downloads collection rather than DownloadManager.
+  if (ShouldUseDownloadsCollection()) {
+    return delegate->AddCompletedDownload(offline_page);
+  }
+
+  OfflinePageItem published_page(offline_page);
+
   // Calculate the new file name.
-  base::FilePath new_file_path =
+  published_page.file_path =
       offline_pages::model_utils::GenerateUniqueFilenameForOfflinePage(
           offline_page.title, offline_page.url, publish_directory);
 
@@ -54,9 +65,8 @@ PublishArchiveResult MoveAndRegisterArchive(
   }
 
   // Move the file.
-  bool moved = base::Move(offline_page.file_path, new_file_path);
+  bool moved = base::Move(offline_page.file_path, published_page.file_path);
   if (!moved) {
-    archive_result.move_result = SavePageResult::FILE_MOVE_FAILED;
     DVPLOG(0) << "OfflinePage publishing file move failure " << __func__;
 
     if (!base::PathExists(offline_page.file_path)) {
@@ -67,38 +77,26 @@ PublishArchiveResult MoveAndRegisterArchive(
       DVLOG(0) << "Target directory does not exist, " << publish_directory
                << " " << __func__;
     }
-    return archive_result;
+    return PublishArchiveResult::Failure(SavePageResult::FILE_MOVE_FAILED);
   }
 
   // Tell the download manager about our file, get back an id.
   if (!delegate->IsDownloadManagerInstalled()) {
-    archive_result.move_result = SavePageResult::ADD_TO_DOWNLOAD_MANAGER_FAILED;
-    return archive_result;
+    return PublishArchiveResult::Failure(
+        SavePageResult::ADD_TO_DOWNLOAD_MANAGER_FAILED);
   }
 
-  // TODO(petewil): Handle empty page title.
-  std::string page_title = base::UTF16ToUTF8(offline_page.title);
-  // We use the title for a description, since the add to the download manager
-  // fails without a description, and we don't have anything better to use.
-  int64_t download_id = delegate->AddCompletedDownload(
-      page_title, page_title,
-      offline_pages::store_utils::ToDatabaseFilePath(new_file_path),
-      offline_page.file_size, offline_page.url.spec(), std::string());
-  if (download_id == 0LL) {
-    archive_result.move_result = SavePageResult::ADD_TO_DOWNLOAD_MANAGER_FAILED;
-    return archive_result;
-  }
-
-  // Put results into the result object.
-  archive_result.move_result = SavePageResult::SUCCESS;
-  archive_result.new_file_path = new_file_path;
-  archive_result.download_id = download_id;
-
-  return archive_result;
+  return delegate->AddCompletedDownload(published_page);
 }
 
 }  // namespace
 
+// static
+PublishArchiveResult PublishArchiveResult::Failure(
+    SavePageResult save_page_result) {
+  return {save_page_result, PublishedArchiveId()};
+}
+
 OfflinePageArchivePublisherImpl::OfflinePageArchivePublisherImpl(
     ArchiveManager* archive_manager)
     : archive_manager_(archive_manager), delegate_(GetDefaultDelegate()) {}
@@ -122,7 +120,18 @@ void OfflinePageArchivePublisherImpl::PublishArchive(
 }
 
 void OfflinePageArchivePublisherImpl::UnpublishArchives(
-    const std::vector<int64_t>& download_manager_ids) const {
+    const std::vector<PublishedArchiveId>& publish_ids) const {
+  std::vector<int64_t> download_manager_ids;
+
+  for (auto& id : publish_ids) {
+    if (id.download_id == kArchivePublishedWithoutDownloadId) {
+      DCHECK(id.new_file_path.IsContentUri());
+      base::DeleteFile(id.new_file_path, false);
+    } else if (id.download_id != kArchiveNotPublished) {
+      download_manager_ids.push_back(id.download_id);
+    }
+  }
+
   delegate_->Remove(download_manager_ids);
 }
 
@@ -136,28 +145,51 @@ bool OfflinePageArchivePublisherImpl::Delegate::IsDownloadManagerInstalled() {
   return is_installed;
 }
 
-int64_t OfflinePageArchivePublisherImpl::Delegate::AddCompletedDownload(
-    const std::string& title,
-    const std::string& description,
-    const std::string& path,
-    int64_t length,
-    const std::string& uri,
-    const std::string& referer) {
+PublishArchiveResult
+OfflinePageArchivePublisherImpl::Delegate::AddCompletedDownload(
+    const OfflinePageItem& page) {
   JNIEnv* env = base::android::AttachCurrentThread();
+
+  if (ShouldUseDownloadsCollection()) {
+    base::FilePath new_file_path = base::FilePath(ConvertJavaStringToUTF8(
+        Java_OfflinePageArchivePublisherBridge_publishArchiveToDownloadsCollection(
+            env,
+            android::OfflinePageBridge::ConvertToJavaOfflinePage(env, page))));
+
+    if (new_file_path.empty())
+      return PublishArchiveResult::Failure(SavePageResult::FILE_MOVE_FAILED);
+
+    return {SavePageResult::SUCCESS,
+            {kArchivePublishedWithoutDownloadId, new_file_path}};
+  }
+
+  // TODO(petewil): Handle empty page title.
+  std::string page_title = base::UTF16ToUTF8(page.title);
+
   // Convert strings to jstring references.
   ScopedJavaLocalRef<jstring> j_title =
-      base::android::ConvertUTF8ToJavaString(env, title);
+      base::android::ConvertUTF8ToJavaString(env, page_title);
+  // We use the title for a description, since the add to the download manager
+  // fails without a description, and we don't have anything better to use.
   ScopedJavaLocalRef<jstring> j_description =
-      base::android::ConvertUTF8ToJavaString(env, description);
-  ScopedJavaLocalRef<jstring> j_path =
-      base::android::ConvertUTF8ToJavaString(env, path);
+      base::android::ConvertUTF8ToJavaString(env, page_title);
+  ScopedJavaLocalRef<jstring> j_path = base::android::ConvertUTF8ToJavaString(
+      env, offline_pages::store_utils::ToDatabaseFilePath(page.file_path));
   ScopedJavaLocalRef<jstring> j_uri =
-      base::android::ConvertUTF8ToJavaString(env, uri);
+      base::android::ConvertUTF8ToJavaString(env, page.url.spec());
   ScopedJavaLocalRef<jstring> j_referer =
-      base::android::ConvertUTF8ToJavaString(env, referer);
-
-  return Java_OfflinePageArchivePublisherBridge_addCompletedDownload(
-      env, j_title, j_description, j_path, length, j_uri, j_referer);
+      base::android::ConvertUTF8ToJavaString(env, std::string());
+
+  int64_t download_id =
+      Java_OfflinePageArchivePublisherBridge_addCompletedDownload(
+          env, j_title, j_description, j_path, page.file_size, j_uri,
+          j_referer);
+  DCHECK_NE(download_id, kArchivePublishedWithoutDownloadId);
+  if (download_id == kArchiveNotPublished)
+    return PublishArchiveResult::Failure(
+        SavePageResult::ADD_TO_DOWNLOAD_MANAGER_FAILED);
+
+  return {SavePageResult::SUCCESS, {download_id, page.file_path}};
 }
 
 int OfflinePageArchivePublisherImpl::Delegate::Remove(

chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.h

@@ -31,18 +31,11 @@ class OfflinePageArchivePublisherImpl : public OfflinePageArchivePublisher {
     // Returns true if a system download manager is available on this platform.
     virtual bool IsDownloadManagerInstalled();
 
-    // Returns the download manager ID of the download, which we will place in
-    // the offline pages database as part of the offline page item.
-    // TODO(petewil): it might make sense to move all these params into a
-    // struct.
-    virtual int64_t AddCompletedDownload(const std::string& title,
-                                         const std::string& description,
-                                         const std::string& path,
-                                         int64_t length,
-                                         const std::string& uri,
-                                         const std::string& referer);
-
-    // Returns the number of pages removed.
+    // Adds the archive to downloads.
+    virtual PublishArchiveResult AddCompletedDownload(
+        const OfflinePageItem& page);
+
+    // Removes pages from downloads, returning the number of pages removed.
     virtual int Remove(
         const std::vector<int64_t>& android_download_manager_ids);
 
@@ -63,7 +56,7 @@ class OfflinePageArchivePublisherImpl : public OfflinePageArchivePublisher {
       PublishArchiveDoneCallback publish_done_callback) const override;
 
   void UnpublishArchives(
-      const std::vector<int64_t>& download_manager_ids) const override;
+      const std::vector<PublishedArchiveId>& publish_ids) const override;
 
  private:
   ArchiveManager* archive_manager_;

chrome/browser/offline_pages/android/offline_page_archive_publisher_impl_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/offline_pages/android/offline_page_archive_publisher_impl.h"
 
+#include "base/android/build_info.h"
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ -86,14 +87,11 @@ class TestArchivePublisherDelegate
       : download_id_(id_to_use), last_removed_id_(0), installed_(installed) {}
 
   bool IsDownloadManagerInstalled() override { return installed_; }
-  int64_t AddCompletedDownload(const std::string& title,
-                               const std::string& description,
-                               const std::string& path,
-                               int64_t length,
-                               const std::string& uri,
-                               const std::string& referer) override {
-    return download_id_;
+  PublishArchiveResult AddCompletedDownload(
+      const OfflinePageItem& page) override {
+    return {SavePageResult::SUCCESS, {download_id_, page.file_path}};
   }
+
   int Remove(
       const std::vector<int64_t>& android_download_manager_ids) override {
     int count = static_cast<int>(android_download_manager_ids.size());
@@ -144,16 +142,26 @@ TEST_F(OfflinePageArchivePublisherImplTest, PublishArchive) {
   PumpLoop();
 
   EXPECT_EQ(SavePageResult::SUCCESS, publish_archive_result().move_result);
-  EXPECT_EQ(kDownloadId, publish_archive_result().download_id);
-  // Check there is a file in the new location.
-  EXPECT_TRUE(public_archive_dir_path().IsParent(
-      publish_archive_result().new_file_path));
-  EXPECT_TRUE(base::PathExists(publish_archive_result().new_file_path));
-  // Check there is no longer a file in the old location.
-  EXPECT_FALSE(base::PathExists(old_file_path));
+  EXPECT_EQ(kDownloadId, publish_archive_result().id.download_id);
+
+  // The file move should not happen on Android Q and later.
+  if (!base::android::BuildInfo::GetInstance()->is_at_least_q()) {
+    // Check there is a file in the new location.
+    EXPECT_TRUE(public_archive_dir_path().IsParent(
+        publish_archive_result().id.new_file_path));
+    EXPECT_TRUE(base::PathExists(publish_archive_result().id.new_file_path));
+    // Check there is no longer a file in the old location.
+    EXPECT_FALSE(base::PathExists(old_file_path));
+  } else {
+    EXPECT_FALSE(public_archive_dir_path().IsParent(
+        publish_archive_result().id.new_file_path));
+    // new_file_path should be the same as the page's file path.
+    EXPECT_TRUE(base::PathExists(publish_archive_result().id.new_file_path));
+    EXPECT_TRUE(base::PathExists(old_file_path));
+  }
 }
 
-TEST_F(OfflinePageArchivePublisherImplTest, UnpublishArchive) {
+TEST_F(OfflinePageArchivePublisherImplTest, UnpublishArchives) {
   ArchiveManager archive_manager(temporary_dir_path(),
                                  private_archive_dir_path(),
                                  public_archive_dir_path(), task_runner());
@@ -162,7 +170,14 @@ TEST_F(OfflinePageArchivePublisherImplTest, UnpublishArchive) {
   OfflinePageArchivePublisherImpl publisher(&archive_manager);
   publisher.SetDelegateForTesting(&delegate);
 
-  std::vector<int64_t> ids_to_remove = {kDownloadId};
+  // This needs to be very close to a real content URI or DeleteContentUri will
+  // throw an exception.
+  base::FilePath test_content_uri =
+      base::FilePath("content://downloads/download/43");
+
+  std::vector<PublishedArchiveId> ids_to_remove{
+      {kDownloadId, base::FilePath()},
+      {kArchivePublishedWithoutDownloadId, test_content_uri}};
   publisher.UnpublishArchives(std::move(ids_to_remove));
 
   EXPECT_EQ(kDownloadId, delegate.last_removed_id());

components/offline_pages/core/BUILD.gn

@@ -48,8 +48,8 @@ static_library("core") {
     "model/startup_maintenance_task.h",
     "model/store_visuals_task.cc",
     "model/store_visuals_task.h",
-    "model/update_file_path_task.cc",
-    "model/update_file_path_task.h",
+    "model/update_publish_id_task.cc",
+    "model/update_publish_id_task.h",
     "model/visuals_availability_task.cc",
     "model/visuals_availability_task.h",
     "offline_clock.cc",
@@ -170,7 +170,7 @@ source_set("unit_tests") {
     "model/persistent_page_consistency_check_task_unittest.cc",
     "model/startup_maintenance_task_unittest.cc",
     "model/store_visuals_task_unittest.cc",
-    "model/update_file_path_task_unittest.cc",
+    "model/update_publish_id_task_unittest.cc",
     "model/visuals_availability_task_unittest.cc",
     "offline_event_logger_unittest.cc",
     "offline_page_feature_unittest.cc",

components/offline_pages/core/model/offline_page_item_generator.cc

@@ -33,9 +33,11 @@ OfflinePageItem OfflinePageItemGenerator::CreateItem() {
   item.access_count = access_count_;
   item.digest = digest_;
   item.file_missing_time = file_missing_time_;
-  if (use_offline_id_as_system_download_id_) {
+  if (use_offline_id_as_system_download_id_)
     item.system_download_id = item.offline_id;
-  }
+  else
+    item.system_download_id = system_download_id_;
+
   return item;
 }
 
@@ -108,4 +110,8 @@ void OfflinePageItemGenerator::SetUseOfflineIdAsSystemDownloadId(bool enable) {
   use_offline_id_as_system_download_id_ = enable;
 }
 
+void OfflinePageItemGenerator::SetSystemDownloadId(int64_t system_download_id) {
+  system_download_id_ = system_download_id;
+}
+
 }  // namespace offline_pages

components/offline_pages/core/model/offline_page_item_generator.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "components/offline_pages/core/client_namespace_constants.h"
+#include "components/offline_pages/core/offline_page_archive_publisher.h"
 #include "components/offline_pages/core/offline_page_item.h"
 
 class GURL;
@@ -38,6 +39,7 @@ class OfflinePageItemGenerator {
   void SetDigest(const std::string& digest);
   void SetFileMissingTime(base::Time file_missing_time);
   void SetUseOfflineIdAsSystemDownloadId(bool enable);
+  void SetSystemDownloadId(int64_t system_download_id);
 
  private:
   std::string namespace_ = kDefaultNamespace;
@@ -52,6 +54,7 @@ class OfflinePageItemGenerator {
   base::FilePath archive_dir_;
   std::string digest_;
   base::Time file_missing_time_;
+  int64_t system_download_id_ = kArchiveNotPublished;
 
   bool use_offline_id_as_system_download_id_ = false;
 };

components/offline_pages/core/model/offline_page_model_taskified.cc

@@ -28,7 +28,7 @@
 #include "components/offline_pages/core/model/persistent_page_consistency_check_task.h"
 #include "components/offline_pages/core/model/startup_maintenance_task.h"
 #include "components/offline_pages/core/model/store_visuals_task.h"
-#include "components/offline_pages/core/model/update_file_path_task.h"
+#include "components/offline_pages/core/model/update_publish_id_task.h"
 #include "components/offline_pages/core/model/visuals_availability_task.h"
 #include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/offline_page_feature.h"
@@ -478,8 +478,8 @@ void OfflinePageModelTaskified::PublishArchiveDone(
                           add_page_start_time - publish_start_time);
 
   OfflinePageItem page = offline_page;
-  page.file_path = publish_results.new_file_path;
-  page.system_download_id = publish_results.download_id;
+  page.file_path = publish_results.id.new_file_path;
+  page.system_download_id = publish_results.id.download_id;
 
   AddPage(page, base::BindOnce(
                     &OfflinePageModelTaskified::OnAddPageForSavePageDone,
@@ -501,21 +501,22 @@ void OfflinePageModelTaskified::PublishInternalArchiveDone(
     PublishPageCallback publish_done_callback,
     const OfflinePageItem& offline_page,
     PublishArchiveResult publish_results) {
-  base::FilePath file_path = publish_results.new_file_path;
   SavePageResult result = publish_results.move_result;
   // Call the callback with success == false if we failed to move the page.
   if (result != SavePageResult::SUCCESS) {
-    std::move(publish_done_callback).Run(file_path, result);
+    std::move(publish_done_callback)
+        .Run(publish_results.id.new_file_path, result);
     return;
   }
 
   // Update the OfflinePageModel with the new location for the page, which is
-  // found in move_results.new_file_path, and with the download ID found at
-  // move_results.download_id.  Return the updated offline_page to the callback.
-  auto task = std::make_unique<UpdateFilePathTask>(
-      store_.get(), offline_page.offline_id, file_path,
+  // found in move_results.id.new_file_path, and with the download ID found at
+  // move_results.id.download_id.  Return the updated offline_page to the
+  // callback.
+  auto task = std::make_unique<UpdatePublishIdTask>(
+      store_.get(), offline_page.offline_id, publish_results.id,
       base::BindOnce(&OnUpdateFilePathDone, std::move(publish_done_callback),
-                     file_path, result));
+                     publish_results.id.new_file_path, result));
   task_queue_.AddTask(std::move(task));
 }
 
@@ -566,7 +567,7 @@ void OfflinePageModelTaskified::OnDeleteDone(
     DeletePageResult result,
     const std::vector<OfflinePageItem>& deleted_items) {
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.DeletePageResult", result);
-  std::vector<int64_t> system_download_ids;
+  std::vector<PublishedArchiveId> publish_ids;
 
   // Notify observers and run callback.
   for (const auto& item : deleted_items) {
@@ -576,15 +577,15 @@ void OfflinePageModelTaskified::OnDeleteDone(
     offline_event_logger_.RecordPageDeleted(item.offline_id);
     for (Observer& observer : observers_)
       observer.OfflinePageDeleted(item);
-    if (item.system_download_id != 0)
-      system_download_ids.push_back(item.system_download_id);
+
+    publish_ids.emplace_back(item.system_download_id, item.file_path);
   }
 
   // Remove the page from the system download manager. We don't need to wait for
   // completion before calling the delete page callback.
-  task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&OfflinePageModelTaskified::Unpublish,
-                                archive_publisher_.get(), system_download_ids));
+  task_runner_->PostTask(FROM_HERE,
+                         base::BindOnce(&OfflinePageModelTaskified::Unpublish,
+                                        archive_publisher_.get(), publish_ids));
 
   if (!callback.is_null())
     std::move(callback).Run(result);
@@ -610,9 +611,9 @@ void OfflinePageModelTaskified::OnStoreFaviconDone(int64_t offline_id,
 
 void OfflinePageModelTaskified::Unpublish(
     OfflinePageArchivePublisher* publisher,
-    const std::vector<int64_t>& system_download_ids) {
-  if (system_download_ids.size() > 0)
-    publisher->UnpublishArchives(system_download_ids);
+    const std::vector<PublishedArchiveId>& publish_ids) {
+  if (!publish_ids.empty())
+    publisher->UnpublishArchives(publish_ids);
 }
 
 void OfflinePageModelTaskified::ScheduleMaintenanceTasks() {
@@ -662,10 +663,8 @@ void OfflinePageModelTaskified::RunMaintenanceTasks(base::Time now,
 
 void OfflinePageModelTaskified::OnPersistentPageConsistencyCheckDone(
     bool success,
-    const std::vector<int64_t>& pages_deleted) {
-  // TODO(https://crbug.com/834909): Use the temporary hidden bit in
-  // DownloadUIAdapter instead of removing directly.
-  Unpublish(archive_publisher_.get(), pages_deleted);
+    const std::vector<PublishedArchiveId>& ids_of_deleted_pages) {
+  Unpublish(archive_publisher_.get(), ids_of_deleted_pages);
 }
 
 void OfflinePageModelTaskified::OnClearCachedPagesDone(

components/offline_pages/core/model/offline_page_model_taskified.h

@@ -170,7 +170,7 @@ class OfflinePageModelTaskified : public OfflinePageModel,
                               ClearStorageTask::ClearStorageResult result);
   void OnPersistentPageConsistencyCheckDone(
       bool success,
-      const std::vector<int64_t>& pages_deleted);
+      const std::vector<PublishedArchiveId>& ids_of_deleted_pages);
 
   // Callback for when PublishArchive has completd.
   void PublishArchiveDone(SavePageCallback save_page_callback,
@@ -185,7 +185,7 @@ class OfflinePageModelTaskified : public OfflinePageModel,
 
   // Method for unpublishing the page from downloads.
   static void Unpublish(OfflinePageArchivePublisher* publisher,
-                        const std::vector<int64_t>& system_download_ids);
+                        const std::vector<PublishedArchiveId>& publish_ids);
 
   // Other utility methods.
   void RemovePagesMatchingUrlAndNamespace(const OfflinePageItem& page);

components/offline_pages/core/model/offline_page_model_taskified_unittest.cc

@@ -882,7 +882,8 @@ TEST_F(OfflinePageModelTaskifiedTest, DeletePagesWithCriteria) {
   EXPECT_EQ(last_deleted_page().offline_id, page1.offline_id);
   EXPECT_EQ(1UL, test_utils::GetFileCountInDirectory(temporary_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(page1.system_download_id, publisher()->last_removed_id());
+  EXPECT_EQ(page1.system_download_id,
+            publisher()->last_removed_id().download_id);
   histogram_tester()->ExpectUniqueSample(
       "OfflinePages.DeletePageCount",
       static_cast<int>(
@@ -1493,7 +1494,8 @@ TEST_F(OfflinePageModelTaskifiedTest, PersistentPageConsistencyCheckExecuted) {
   EXPECT_EQ(0UL,
             test_utils::GetFileCountInDirectory(public_archive_dir_path()));
   EXPECT_EQ(1LL, store_test_util()->GetPageCount());
-  EXPECT_EQ(page.system_download_id, publisher()->last_removed_id());
+  EXPECT_EQ(page.system_download_id,
+            publisher()->last_removed_id().download_id);
   histogram_tester()->ExpectTotalCount(
       "OfflinePages.ConsistencyCheck.Persistent.Result", 3);
 }

components/offline_pages/core/model/offline_page_test_utils.cc

@@ -10,6 +10,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "components/offline_pages/core/offline_page_archive_publisher.h"
 #include "components/offline_pages/core/offline_page_item.h"
 #include "components/offline_pages/core/offline_page_visuals.h"
 #include "components/offline_pages/core/offline_store_utils.h"
@@ -69,7 +70,7 @@ std::ostream& operator<<(std::ostream& out, const OfflinePageItem& item) {
   if (!item.request_origin.empty()) {
     value.SetKey("request_origin", Value(item.request_origin));
   }
-  if (item.system_download_id != 0) {
+  if (item.system_download_id != kArchiveNotPublished) {
     value.SetKey("system_download_id",
                  Value(std::to_string(item.system_download_id)));
   }

components/offline_pages/core/model/persistent_page_consistency_check_task.cc

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
@@ -80,12 +81,12 @@ PersistentPageConsistencyCheckSync(
     const ClientPolicyController* policy_controller,
     base::Time check_time,
     sql::Database* db) {
-  std::vector<int64_t> download_ids_of_deleted_pages;
+  std::vector<PublishedArchiveId> publish_ids_of_deleted_pages;
 
   sql::Transaction transaction(db);
   if (!transaction.Begin())
     return {SyncOperationResult::TRANSACTION_BEGIN_ERROR,
-            download_ids_of_deleted_pages};
+            publish_ids_of_deleted_pages};
 
   std::vector<OfflinePageItem> persistent_page_infos =
       GetPersistentPages(policy_controller, db);
@@ -103,7 +104,9 @@ PersistentPageConsistencyCheckSync(
       } else {
         if (check_time - item.file_missing_time > kExpireThreshold) {
           page_ids_to_delete.push_back(item.offline_id);
-          download_ids_of_deleted_pages.push_back(item.system_download_id);
+
+          publish_ids_of_deleted_pages.emplace_back(item.system_download_id,
+                                                    item.file_path);
         }
       }
     }
@@ -113,7 +116,7 @@ PersistentPageConsistencyCheckSync(
       !MarkPagesAsMissing(pages_found_missing, check_time, db) ||
       !MarkPagesAsReappeared(pages_reappeared, db)) {
     return {SyncOperationResult::DB_OPERATION_ERROR,
-            download_ids_of_deleted_pages};
+            publish_ids_of_deleted_pages};
   }
 
   if (page_ids_to_delete.size() > 0) {
@@ -134,9 +137,9 @@ PersistentPageConsistencyCheckSync(
 
   if (!transaction.Commit())
     return {SyncOperationResult::TRANSACTION_COMMIT_ERROR,
-            download_ids_of_deleted_pages};
+            publish_ids_of_deleted_pages};
 
-  return {SyncOperationResult::SUCCESS, download_ids_of_deleted_pages};
+  return {SyncOperationResult::SUCCESS, publish_ids_of_deleted_pages};
 }
 
 }  // namespace
@@ -145,8 +148,8 @@ PersistentPageConsistencyCheckTask::CheckResult::CheckResult() = default;
 
 PersistentPageConsistencyCheckTask::CheckResult::CheckResult(
     SyncOperationResult result,
-    const std::vector<int64_t>& system_download_ids)
-    : result(result), download_ids_of_deleted_pages(system_download_ids) {}
+    const std::vector<PublishedArchiveId>& ids_of_deleted_pages)
+    : result(result), ids_of_deleted_pages(ids_of_deleted_pages) {}
 
 PersistentPageConsistencyCheckTask::CheckResult::CheckResult(
     const CheckResult& other) = default;
@@ -196,7 +199,7 @@ void PersistentPageConsistencyCheckTask::OnPersistentPageConsistencyCheckDone(
   if (check_result.result != SyncOperationResult::SUCCESS) {
     std::move(callback_).Run(false, {});
   } else {
-    std::move(callback_).Run(true, check_result.download_ids_of_deleted_pages);
+    std::move(callback_).Run(true, check_result.ids_of_deleted_pages);
   }
   TaskComplete();
 }

components/offline_pages/core/model/persistent_page_consistency_check_task.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "components/offline_pages/core/offline_page_archive_publisher.h"
 #include "components/offline_pages/core/offline_store_types.h"
 #include "components/offline_pages/task/task.h"
 
@@ -23,20 +24,20 @@ class OfflinePageMetadataStore;
 // missing entries back to normal.
 class PersistentPageConsistencyCheckTask : public Task {
  public:
-  using PersistentPageConsistencyCheckCallback =
-      base::OnceCallback<void(bool success,
-                              const std::vector<int64_t>& pages_deleted)>;
+  using PersistentPageConsistencyCheckCallback = base::OnceCallback<void(
+      bool success,
+      const std::vector<PublishedArchiveId>& ids_of_deleted_pages)>;
 
   struct CheckResult {
     CheckResult();
     CheckResult(SyncOperationResult result,
-                const std::vector<int64_t>& system_download_ids);
+                const std::vector<PublishedArchiveId>& ids_of_deleted_pages);
     CheckResult(const CheckResult& other);
     CheckResult& operator=(const CheckResult& other);
     ~CheckResult();
 
     SyncOperationResult result;
-    std::vector<int64_t> download_ids_of_deleted_pages;
+    std::vector<PublishedArchiveId> ids_of_deleted_pages;
   };
 
   PersistentPageConsistencyCheckTask(

components/offline_pages/core/model/persistent_page_consistency_check_task_unittest.cc

@@ -15,10 +15,12 @@
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/model/model_task_test_base.h"
 #include "components/offline_pages/core/model/offline_page_test_utils.h"
+#include "components/offline_pages/core/offline_page_archive_publisher.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::A;
 using testing::Eq;
+using testing::IsEmpty;
 using testing::UnorderedElementsAre;
 
 namespace offline_pages {
@@ -82,14 +84,16 @@ TEST_F(PersistentPageConsistencyCheckTaskTest,
   EXPECT_EQ(1UL, test_utils::GetFileCountInDirectory(PublicDir()));
 
   base::MockCallback<PersistentPageConsistencyCheckCallback> callback;
-  EXPECT_CALL(callback,
-              Run(Eq(true), UnorderedElementsAre(page2.system_download_id,
-                                                 page5.system_download_id)));
-
-  auto task = std::make_unique<PersistentPageConsistencyCheckTask>(
+  EXPECT_CALL(
+      callback,
+      Run(Eq(true),
+          UnorderedElementsAre(
+              PublishedArchiveId{page2.system_download_id, page2.file_path},
+              PublishedArchiveId{page5.system_download_id, page5.file_path})));
+
+  RunTask(std::make_unique<PersistentPageConsistencyCheckTask>(
       store(), archive_manager(), policy_controller(), base::Time::Now(),
-      callback.Get());
-  RunTask(std::move(task));
+      callback.Get()));
 
   EXPECT_EQ(4LL, store_test_util()->GetPageCount());
   EXPECT_EQ(1UL, test_utils::GetFileCountInDirectory(PrivateDir()));
@@ -116,4 +120,41 @@ TEST_F(PersistentPageConsistencyCheckTaskTest,
       static_cast<int>(SyncOperationResult::SUCCESS), 1);
 }
 
+#if defined(OS_WIN)
+#define MAYBE_ClearExpiredPersistentPagesByFilePath \
+  DISABLED_ClearExpiredPersistentPagesByFilePath
+#else
+#define MAYBE_ClearExpiredPersistentPagesByFilePath \
+  ClearExpiredPersistentPagesByFilePath
+#endif
+TEST_F(PersistentPageConsistencyCheckTaskTest,
+       MAYBE_ClearExpiredPersistentPagesByFilePath) {
+  base::Time expire_time = base::Time::Now() - base::TimeDelta::FromDays(400);
+  // |page| will be deleted from DB, since it's been expired for longer than
+  // threshold.
+  generator()->SetSystemDownloadId(kArchivePublishedWithoutDownloadId);
+  generator()->SetNamespace(kDownloadNamespace);
+  generator()->SetArchiveDirectory(PublicDir());
+  generator()->SetFileMissingTime(expire_time);
+  OfflinePageItem page = AddPageWithoutFile();
+
+  EXPECT_EQ(1LL, store_test_util()->GetPageCount());
+
+  base::MockCallback<PersistentPageConsistencyCheckCallback> callback;
+  EXPECT_CALL(callback,
+              Run(Eq(true), UnorderedElementsAre(PublishedArchiveId{
+                                page.system_download_id, page.file_path})));
+
+  RunTask(std::make_unique<PersistentPageConsistencyCheckTask>(
+      store(), archive_manager(), policy_controller(), base::Time::Now(),
+      callback.Get()));
+
+  EXPECT_FALSE(store_test_util()->GetPageByOfflineId(page.offline_id));
+  histogram_tester()->ExpectUniqueSample(
+      "OfflinePages.ConsistencyCheck.Persistent.ExpiredEntryCount", 1, 1);
+  histogram_tester()->ExpectUniqueSample(
+      "OfflinePages.ConsistencyCheck.Persistent.Result",
+      static_cast<int>(SyncOperationResult::SUCCESS), 1);
+}
+
 }  // namespace offline_pages

components/offline_pages/core/model/update_file_path_task.cc → components/offline_pages/core/model/update_publish_id_task.cc

@@ -2,7 +2,7 @@
 // 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/model/update_file_path_task.h"
+#include "components/offline_pages/core/model/update_publish_id_task.h"
 
 #include "base/bind.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
@@ -17,9 +17,9 @@ namespace offline_pages {
 
 namespace {
 
-bool UpdateFilePathSync(const base::FilePath& new_file_path,
-                        int64_t offline_id,
-                        sql::Database* db) {
+bool UpdatePublishIdSync(const PublishedArchiveId& publish_id,
+                         int64_t offline_id,
+                         sql::Database* db) {
   sql::Transaction transaction(db);
   if (!transaction.Begin())
     return false;
@@ -27,13 +27,14 @@ bool UpdateFilePathSync(const base::FilePath& new_file_path,
   // Update the file_path to point to the new path.
   static const char kSqlUpdate[] =
       "UPDATE OR IGNORE offlinepages_v1"
-      " SET file_path = ?"
-      " WHERE offline_id = ?";
+      " SET file_path=?,system_download_id=?"
+      " WHERE offline_id=?";
   sql::Statement update_statement(
       db->GetCachedStatement(SQL_FROM_HERE, kSqlUpdate));
-  update_statement.BindString(
-      0, offline_pages::store_utils::ToDatabaseFilePath(new_file_path));
-  update_statement.BindInt64(1, offline_id);
+  update_statement.BindString(0, offline_pages::store_utils::ToDatabaseFilePath(
+                                     publish_id.new_file_path));
+  update_statement.BindInt64(1, publish_id.download_id);
+  update_statement.BindInt64(2, offline_id);
 
   if (!update_statement.Run())
     return false;
@@ -46,27 +47,29 @@ bool UpdateFilePathSync(const base::FilePath& new_file_path,
 
 }  // namespace
 
-UpdateFilePathTask::UpdateFilePathTask(OfflinePageMetadataStore* store,
-                                       int64_t offline_id,
-                                       const base::FilePath& file_path,
-                                       UpdateFilePathDoneCallback callback)
+UpdatePublishIdTask::UpdatePublishIdTask(
+    OfflinePageMetadataStore* store,
+    int64_t offline_id,
+    const PublishedArchiveId& publish_id,
+    base::OnceCallback<void(bool)> callback)
     : store_(store),
       offline_id_(offline_id),
-      file_path_(file_path),
+      publish_id_(publish_id),
       callback_(std::move(callback)) {
   DCHECK(store_);
 }
 
-UpdateFilePathTask::~UpdateFilePathTask() {}
+UpdatePublishIdTask::~UpdatePublishIdTask() {}
 
-void UpdateFilePathTask::Run() {
-  store_->Execute(base::BindOnce(&UpdateFilePathSync, file_path_, offline_id_),
-                  base::BindOnce(&UpdateFilePathTask::OnUpdateFilePathDone,
-                                 weak_ptr_factory_.GetWeakPtr()),
-                  false);
+void UpdatePublishIdTask::Run() {
+  store_->Execute(
+      base::BindOnce(&UpdatePublishIdSync, publish_id_, offline_id_),
+      base::BindOnce(&UpdatePublishIdTask::OnUpdatePublishIdDone,
+                     weak_ptr_factory_.GetWeakPtr()),
+      false);
 }
 
-void UpdateFilePathTask::OnUpdateFilePathDone(bool result) {
+void UpdatePublishIdTask::OnUpdatePublishIdDone(bool result) {
   // Forward the updated offline page to the callback
   std::move(callback_).Run(result);
   TaskComplete();

components/offline_pages/core/model/update_file_path_task.h → components/offline_pages/core/model/update_publish_id_task.h

@@ -2,14 +2,17 @@
 // 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_MODEL_UPDATE_FILE_PATH_TASK_H_
-#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_FILE_PATH_TASK_H_
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_PUBLISH_ID_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_PUBLISH_ID_TASK_H_
 
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "components/offline_pages/core/model/get_pages_task.h"
+#include "components/offline_pages/core/offline_page_archive_publisher.h"
 #include "components/offline_pages/core/offline_page_model.h"
+#include "components/offline_pages/core/offline_page_types.h"
 #include "components/offline_pages/task/task.h"
 
 namespace offline_pages {
@@ -20,31 +23,31 @@ class OfflinePageMetadataStore;
 
 // Task that updates the file path in the metadata store. It takes the offline
 // ID of the page accessed, the new file path, and the completion callback.
-class UpdateFilePathTask : public Task {
+class UpdatePublishIdTask : public Task {
  public:
-  UpdateFilePathTask(OfflinePageMetadataStore* store,
-                     int64_t offline_id,
-                     const base::FilePath& file_path,
-                     UpdateFilePathDoneCallback callback);
-  ~UpdateFilePathTask() override;
+  UpdatePublishIdTask(OfflinePageMetadataStore* store,
+                      int64_t offline_id,
+                      const PublishedArchiveId& publish_id,
+                      base::OnceCallback<void(bool)> callback);
+  ~UpdatePublishIdTask() override;
 
   // Task implementation.
   void Run() override;
 
  private:
-  void OnUpdateFilePathDone(bool result);
+  void OnUpdatePublishIdDone(bool result);
 
   // The metadata store used to update the page. Not owned.
   OfflinePageMetadataStore* store_;
 
   int64_t offline_id_;
-  base::FilePath file_path_;
-  UpdateFilePathDoneCallback callback_;
+  PublishedArchiveId publish_id_;
+  base::OnceCallback<void(bool)> callback_;
 
-  base::WeakPtrFactory<UpdateFilePathTask> weak_ptr_factory_{this};
-  DISALLOW_COPY_AND_ASSIGN(UpdateFilePathTask);
+  base::WeakPtrFactory<UpdatePublishIdTask> weak_ptr_factory_{this};
+  DISALLOW_COPY_AND_ASSIGN(UpdatePublishIdTask);
 };
 
 }  // namespace offline_pages
 
-#endif  // COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_FILE_PATH_TASK_H_
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_PUBLISH_ID_TASK_H_

components/offline_pages/core/model/update_file_path_task_unittest.cc → components/offline_pages/core/model/update_publish_id_task_unittest.cc

@@ -2,65 +2,42 @@
 // 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/model/update_file_path_task.h"
+#include "components/offline_pages/core/model/update_publish_id_task.h"
 
 #include <memory>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
+#include "base/test/mock_callback.h"
 #include "components/offline_pages/core/model/model_task_test_base.h"
 #include "components/offline_pages/core/model/offline_page_item_generator.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
-namespace {
+class UpdatePublishIdTaskTest : public ModelTaskTestBase {};
 
-// Callback to check results of task.
-class UpdateFilePathTaskTestCallback {
- public:
-  UpdateFilePathTaskTestCallback() : called_(false), success_(false) {}
-
-  bool called() const { return called_; }
-
-  bool success() const { return success_; }
-
-  void Run(bool success) {
-    called_ = true;
-    success_ = success;
-  }
-
-  base::WeakPtr<UpdateFilePathTaskTestCallback> GetWeakPtr() {
-    return weak_ptr_factory_.GetWeakPtr();
-  }
-
- private:
-  bool called_;
-  bool success_;
-  base::WeakPtrFactory<UpdateFilePathTaskTestCallback> weak_ptr_factory_{this};
-};
-}  // namespace
-
-class UpdateFilePathTaskTest : public ModelTaskTestBase {};
-
-TEST_F(UpdateFilePathTaskTest, UpdateFilePath) {
+TEST_F(UpdatePublishIdTaskTest, UpdatePublishId) {
   OfflinePageItem page = generator()->CreateItem();
   store_test_util()->InsertItem(page);
   base::FilePath new_file_path(FILE_PATH_LITERAL("/new/path/to/file"));
-  UpdateFilePathTaskTestCallback done_callback;
+  int64_t new_system_download_id = 42LL;
+
+  base::MockCallback<base::OnceCallback<void(bool)>> callback;
+
+  EXPECT_CALL(callback, Run(true));
 
   // Build and run a task to change the file path in the offline page model.
-  auto task = std::make_unique<UpdateFilePathTask>(
-      store(), page.offline_id, new_file_path,
-      base::BindOnce(&UpdateFilePathTaskTestCallback::Run,
-                     done_callback.GetWeakPtr()));
+  auto task = std::make_unique<UpdatePublishIdTask>(
+      store(), page.offline_id,
+      PublishedArchiveId(new_system_download_id, new_file_path),
+      callback.Get());
   RunTask(std::move(task));
 
   auto offline_page = store_test_util()->GetPageByOfflineId(page.offline_id);
 
   EXPECT_EQ(new_file_path, offline_page->file_path);
-  EXPECT_TRUE(done_callback.called());
-  EXPECT_TRUE(done_callback.success());
+  EXPECT_EQ(new_system_download_id, offline_page->system_download_id);
 }
 
 }  // namespace offline_pages

components/offline_pages/core/offline_page_archive_publisher.h

@@ -19,11 +19,38 @@ class SequencedTaskRunner;
 
 namespace offline_pages {
 
+// These constants are used to set offline_page_item.download_id when no
+// download ID is available.
+const int64_t kArchiveNotPublished = 0LL;
+const int64_t kArchivePublishedWithoutDownloadId = -1LL;
+
+// Identifies one published archive. Before Android Q, a published archive is
+// assigned a download ID; on Q and later, a published archive is assigned a
+// content URI.
+struct PublishedArchiveId {
+  PublishedArchiveId() = default;
+  PublishedArchiveId(int64_t download_id, const base::FilePath& new_file_path)
+      : download_id(download_id), new_file_path(new_file_path) {}
+  bool operator==(const PublishedArchiveId& other) const {
+    return download_id == other.download_id &&
+           new_file_path == other.new_file_path;
+  }
+
+  // Identifier returned by Android DownloadManager when present, or
+  // kArchivePublishedWithoutDownloadManager otherwise. Set to
+  // kArchiveNotPublished if publishing failed.
+  int64_t download_id = kArchiveNotPublished;
+
+  // The published archive's path or content URI; empty if publishing failed.
+  base::FilePath new_file_path;
+};
+
 // The result of publishing an offline page to Downloads.
 struct PublishArchiveResult {
   SavePageResult move_result;
-  base::FilePath new_file_path;
-  int64_t download_id;
+  PublishedArchiveId id;
+
+  static PublishArchiveResult Failure(SavePageResult save_page_result);
 };
 
 // Interface of a class responsible for publishing offline page archives to
@@ -43,9 +70,9 @@ class OfflinePageArchivePublisher {
       const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
       PublishArchiveDoneCallback publish_done_callback) const = 0;
 
-  // Removes the archives identified by |download_manager_ids| from downloads.
+  // Removes  archives from downloads.
   virtual void UnpublishArchives(
-      const std::vector<int64_t>& download_manager_ids) const = 0;
+      const std::vector<PublishedArchiveId>& archive_ids) const = 0;
 };
 
 }  // namespace offline_pages

components/offline_pages/core/offline_page_test_archive_publisher.cc

@@ -25,7 +25,6 @@ OfflinePageTestArchivePublisher::OfflinePageTestArchivePublisher(
       archive_attempt_failure_(false),
       use_verbatim_archive_path_(false),
       download_id_(download_id_to_use),
-      last_removed_id_(0LL),
       archive_manager_(archive_manager) {}
 
 OfflinePageTestArchivePublisher::~OfflinePageTestArchivePublisher() {
@@ -42,7 +41,6 @@ void OfflinePageTestArchivePublisher::PublishArchive(
 
   if (archive_attempt_failure_) {
     publish_archive_result.move_result = SavePageResult::FILE_MOVE_FAILED;
-    publish_archive_result.download_id = 0LL;
   } else {
     publish_archive_result.move_result = SavePageResult::SUCCESS;
 
@@ -51,13 +49,13 @@ void OfflinePageTestArchivePublisher::PublishArchive(
     // published archive path is built using publish_directory. The tests should
     // be fixed and this special case should be removed.
     if (use_verbatim_archive_path_) {
-      publish_archive_result.new_file_path = offline_page.file_path;
+      publish_archive_result.id.new_file_path = offline_page.file_path;
     } else {
-      publish_archive_result.new_file_path =
+      publish_archive_result.id.new_file_path =
           archive_manager_->GetPublicArchivesDir().Append(
               offline_page.file_path.BaseName());
     }
-    publish_archive_result.download_id = download_id_;
+    publish_archive_result.id.download_id = download_id_;
   }
 
   background_task_runner->PostTask(
@@ -66,9 +64,9 @@ void OfflinePageTestArchivePublisher::PublishArchive(
 }
 
 void OfflinePageTestArchivePublisher::UnpublishArchives(
-    const std::vector<int64_t>& download_manager_ids) const {
-  if (!download_manager_ids.empty())
-    last_removed_id_ = download_manager_ids.back();
+    const std::vector<PublishedArchiveId>& archive_ids) const {
+  if (!archive_ids.empty())
+    last_removed_id_ = archive_ids.back();
 }
 
 }  // namespace offline_pages

components/offline_pages/core/offline_page_test_archive_publisher.h

@@ -35,7 +35,7 @@ class OfflinePageTestArchivePublisher : public OfflinePageArchivePublisher {
       PublishArchiveDoneCallback publish_done_callback) const override;
 
   void UnpublishArchives(
-      const std::vector<int64_t>& download_manager_ids) const override;
+      const std::vector<PublishedArchiveId>& archive_ids) const override;
 
   void set_archive_attempt_failure(bool fail) {
     archive_attempt_failure_ = fail;
@@ -47,7 +47,7 @@ class OfflinePageTestArchivePublisher : public OfflinePageArchivePublisher {
 
   void use_verbatim_archive_path(bool use) { use_verbatim_archive_path_ = use; }
 
-  int64_t last_removed_id() const { return last_removed_id_; }
+  PublishedArchiveId last_removed_id() const { return last_removed_id_; }
 
  private:
   bool expect_publish_archive_called_;
@@ -55,7 +55,7 @@ class OfflinePageTestArchivePublisher : public OfflinePageArchivePublisher {
   bool archive_attempt_failure_;
   bool use_verbatim_archive_path_;
   int64_t download_id_;
-  mutable int64_t last_removed_id_;
+  mutable PublishedArchiveId last_removed_id_;
 
   ArchiveManager* archive_manager_;
 };

components/offline_pages/core/offline_page_types.h

@@ -110,7 +110,6 @@ typedef base::OnceCallback<void(bool)> CleanupVisualsCallback;
 // Callbacks used for publishing an offline page.
 using PublishPageCallback =
     base::OnceCallback<void(const base::FilePath&, SavePageResult)>;
-using UpdateFilePathDoneCallback = base::OnceCallback<void(bool)>;
 
 }  // namespace offline_pages