Upgrade OfflinePages on demand, part 2

In this part, we add the code to actually move the offline page,
update ADM, update the OfflinePageModel, and call back to the
java side code.

There is also a small refactoring to use the same symbol everywhere
for our database table name, it has moved into offline_store_utils.h.

Bug: 758690
Change-Id: Ia540cfdd9f94ac93a1211eb8b7184507b3906080
Reviewed-on: https://chromium-review.googlesource.com/985208
Commit-Queue: Peter Williamson <petewil@chromium.org>
Reviewed-by: Dmitry Titov <dimich@chromium.org>
Reviewed-by: Jian Li <jianli@chromium.org>
Reviewed-by: Yafei Duan <romax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547801}

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

@@ -568,10 +568,11 @@ public class OfflinePageBridge {
      * @param url Url of the offline page.
      * @param filePath Path to the file for the offline page.
      * @param size Length of the offline page file.
-     * @param publishedCallback Function to call when publishing is done.
+     * @param publishedCallback Function to call when publishing is done.  This will be called
+     *        with the new path of the file.
      */
     public void publishInternalPage(Profile profile, long offlineId, String title, String url,
-            String filePath, long size, Callback<OfflinePageItem> publishedCallback) {
+            String filePath, long size, Callback<String> publishedCallback) {
         nativePublishInternalPage(mNativeOfflinePageBridge, profile, offlineId, title, url,
                 filePath, size, publishedCallback);
     }
@@ -858,7 +859,7 @@ public class OfflinePageBridge {
     @VisibleForTesting
     private native void nativePublishInternalPage(long nativeOfflinePageBridge, Profile profile,
             long offlineId, String title, String url, String filePath, long size,
-            Callback<OfflinePageItem> publishedCallback);
+            Callback<String> publishedCallback);
 
     private native void nativeSelectPageForOnlineUrl(
             long nativeOfflinePageBridge, String onlineUrl, int tabId,

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

@@ -430,9 +430,6 @@ public class OfflinePageUtils {
             return false;
         }
 
-        // If the page is not in a public location, we cannot share it.
-        if (offlinePageBridge.isInPrivateDirectory(offlinePath)) return false;
-
         return true;
     }
 
@@ -453,8 +450,8 @@ public class OfflinePageUtils {
     public static void publishThenShareInternalPage(final Activity activity, Profile profile,
             OfflinePageBridge offlinePageBridge, OfflinePageItem offlinePage,
             final Callback<ShareParams> shareCallback) {
-        Callback<OfflinePageItem> publishPageCallback =
-                new PublishPageCallback(activity, shareCallback);
+        Callback<String> publishPageCallback =
+                new PublishPageCallback(activity, offlinePage, shareCallback);
         offlinePageBridge.publishInternalPage(profile, offlinePage.getOfflineId(),
                 offlinePage.getTitle(), offlinePage.getUrl(), offlinePage.getFilePath(),
                 offlinePage.getFileSize(), publishPageCallback);

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

@@ -14,21 +14,37 @@ import org.chromium.chrome.browser.share.ShareParams;
  * This callback will save the state we need when the JNI call is done, and start the next stage of
  * processing for sharing.
  */
-public class PublishPageCallback implements Callback<OfflinePageItem> {
+public class PublishPageCallback implements Callback<String> {
     private Callback<ShareParams> mShareCallback;
+    OfflinePageItem mPage;
     private Activity mActivity;
     private static final String TAG = "PublishPageCallback";
 
     /** Create a callback for use when page publishing is completed. */
-    public PublishPageCallback(Activity activity, Callback<ShareParams> shareCallback) {
+    public PublishPageCallback(
+            Activity activity, OfflinePageItem page, Callback<ShareParams> shareCallback) {
         mActivity = activity;
+        mPage = page;
         mShareCallback = shareCallback;
     }
 
     @Override
     @CalledByNative
     /** Report results of publishing. */
-    public void onResult(OfflinePageItem page) {
+    public void onResult(String newFilePath) {
+        OfflinePageItem page = null;
+        // If the sharing failed, the file path will be empty.  We'll call the share callback
+        // with a null page to indicate failure.
+        if (!newFilePath.isEmpty()) {
+            // Make a new OfflinePageItem with the new path.
+            page = new OfflinePageItem(mPage.getUrl(), mPage.getOfflineId(),
+                    mPage.getClientId().getNamespace(), mPage.getClientId().getId(),
+                    mPage.getTitle(), newFilePath, mPage.getFileSize(), mPage.getCreationTimeMs(),
+                    mPage.getAccessCount(), mPage.getLastAccessTimeMs(), mPage.getRequestOrigin());
+        }
+
+        // TODO(petewil): Sharing seems out of place here. Move the call to sharing
+        // back to OfflinePageUtils.
         OfflinePageUtils.sharePublishedPage(page, mActivity, mShareCallback);
     }
 }

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

@@ -320,10 +320,18 @@ public class OfflinePageUtilsTest {
                 boolean shared =
                         OfflinePageUtils.maybeShareOfflinePage(mActivityTestRule.getActivity(),
                                 mActivityTestRule.getActivity().getActivityTab(), shareCallback);
-                // The attempt to share a page from our private internal directory should fail.
-                Assert.assertFalse(shared);
+                // The attempt to share a page from our private internal directory should succeed.
+                Assert.assertTrue(shared);
             }
         });
+
+        // Wait for share callback to get called.
+        Assert.assertTrue(semaphore.tryAcquire(TIMEOUT_MS, TimeUnit.MILLISECONDS));
+        // Assert that URI is what we expected.
+        String foundUri = shareCallback.getSharedUri();
+        Uri uri = Uri.parse(foundUri);
+        String uriPath = uri.getPath();
+        Assert.assertEquals(TEST_PAGE, uriPath);
     }
 
     // Checks on the UI thread if an offline path corresponds to a sharable file.
@@ -353,9 +361,10 @@ public class OfflinePageUtilsTest {
         final String privatePath = activity().getApplicationContext().getCacheDir().getPath();
         final String publicPath = Environment.getExternalStorageDirectory().getPath();
 
-        // Check that an offline page item in the private directory is not sharable.
+        // Check that an offline page item in the private directory is sharable, since we can
+        // upgrade it.
         final String fullPrivatePath = privatePath + CACHE_SUBDIR + NEW_FILE;
-        checkIfOfflinePageIsSharable(fullPrivatePath, SHARED_URI, false);
+        checkIfOfflinePageIsSharable(fullPrivatePath, SHARED_URI, true);
 
         // Check that an offline page item with no file path is not sharable.
         checkIfOfflinePageIsSharable(EMPTY_PATH, SHARED_URI, false);

chrome/browser/offline_pages/android/offline_page_bridge.cc

@@ -294,22 +294,18 @@ void SavePageLaterCallback(const ScopedJavaGlobalRef<jobject>& j_callback_obj,
 
 void PublishPageDone(
     const ScopedJavaGlobalRef<jobject>& j_published_callback_obj,
-    const OfflinePageItem& offline_page) {
+    const base::FilePath& file_path,
+    bool success) {
   // Create a java side OfflinePageItem for this offline_page.
   JNIEnv* env = base::android::AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> j_page =
-      Java_OfflinePageBridge_createOfflinePageItem(
-          env, ConvertUTF8ToJavaString(env, offline_page.url.spec()),
-          offline_page.offline_id,
-          ConvertUTF8ToJavaString(env, offline_page.client_id.name_space),
-          ConvertUTF8ToJavaString(env, offline_page.client_id.id),
-          ConvertUTF16ToJavaString(env, offline_page.title),
-          ConvertUTF8ToJavaString(env, offline_page.file_path.value()),
-          offline_page.file_size, offline_page.creation_time.ToJavaTime(),
-          offline_page.access_count, offline_page.last_access_time.ToJavaTime(),
-          ConvertUTF8ToJavaString(env, offline_page.request_origin));
-
-  base::android::RunCallbackAndroid(j_published_callback_obj, j_page);
+
+  base::FilePath file_path_or_empty;
+  if (success)
+    file_path_or_empty = file_path;
+
+  base::android::RunCallbackAndroid(
+      j_published_callback_obj,
+      ConvertUTF8ToJavaString(env, file_path.value()));
 }
 
 }  // namespace

components/offline_pages/core/BUILD.gn

@@ -50,6 +50,8 @@ static_library("core") {
     "model/startup_maintenance_task.h",
     "model/store_thumbnail_task.cc",
     "model/store_thumbnail_task.h",
+    "model/update_file_path_task.cc",
+    "model/update_file_path_task.h",
     "offline_event_logger.cc",
     "offline_event_logger.h",
     "offline_page_archiver.cc",

components/offline_pages/core/model/offline_page_model_taskified.cc

@@ -27,6 +27,7 @@
 #include "components/offline_pages/core/model/mark_page_accessed_task.h"
 #include "components/offline_pages/core/model/offline_page_model_utils.h"
 #include "components/offline_pages/core/model/startup_maintenance_task.h"
+#include "components/offline_pages/core/model/update_file_path_task.h"
 #include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/offline_page_metadata_store_sql.h"
 #include "components/offline_pages/core/offline_page_model.h"
@@ -542,21 +543,21 @@ void OfflinePageModelTaskified::PublishInternalArchiveDone(
     PublishPageCallback publish_done_callback,
     const OfflinePageItem& offline_page,
     PublishArchiveResult* publish_results) {
-  // Return an empty OfflinePageItem if we were unable to move the page.  The
-  // offline_id will be 0, which marks it as invalid.
+  // Call the callback with success == false if we failed to move the page.
   if (publish_results->move_result != SavePageResult::SUCCESS) {
-    OfflinePageItem empty_offline_page;
-    std::move(publish_done_callback).Run(empty_offline_page);
+    std::move(publish_done_callback).Run(publish_results->new_file_path, false);
     return;
   }
 
-  // TODO(petewil): 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.
+  // 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, publish_results->new_file_path,
+      base::BindOnce(std::move(publish_done_callback),
+                     publish_results->new_file_path));
 
-  // Return to the OfflinePageBridge callback passed in.
-  std::move(publish_done_callback).Run(offline_page);
+  task_queue_.AddTask(std::move(task));
 }
 
 void OfflinePageModelTaskified::OnAddPageForSavePageDone(

components/offline_pages/core/model/offline_page_model_taskified.h

@@ -216,7 +216,7 @@ class OfflinePageModelTaskified : public OfflinePageModel,
   // Callback for when publishing an internal archive has completed.
   void PublishInternalArchiveDone(PublishPageCallback publish_done_callback,
                                   const OfflinePageItem& offline_page,
-                                  PublishArchiveResult* move_results);
+                                  PublishArchiveResult* publish_results);
 
   // Method for unpublishing the page from the system download manager.
   static void RemoveFromDownloadManager(

components/offline_pages/core/model/offline_page_model_taskified_unittest.cc

@@ -29,6 +29,7 @@
 #include "components/offline_pages/core/offline_page_model.h"
 #include "components/offline_pages/core/offline_page_test_archiver.h"
 #include "components/offline_pages/core/offline_page_types.h"
+#include "components/offline_pages/core/offline_store_utils.h"
 #include "components/offline_pages/core/system_download_manager_stub.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -67,6 +68,34 @@ const std::string kEmptyRequestOrigin("");
 const std::string kTestDigest("test digest");
 const int64_t kDownloadId = 42LL;
 
+// Class to receive the callback for page publish completion.
+// TODO(romax): Convert this to a mock callback like the other tests use.
+class PublishPageTestCallback {
+ public:
+  PublishPageTestCallback()
+      : callback_called_(false), weak_ptr_factory_(this) {}
+
+  void Run(const base::FilePath& file_path, bool success) {
+    callback_called_ = true;
+    success_ = false;
+    file_path_ = file_path;
+    success_ = success;
+  }
+
+  bool callback_called() const { return callback_called_; }
+  bool success() const { return success_; };
+  const base::FilePath file_path() const { return file_path_; };
+  base::WeakPtr<PublishPageTestCallback> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+ private:
+  bool callback_called_;
+  bool success_;
+  base::FilePath file_path_;
+  base::WeakPtrFactory<PublishPageTestCallback> weak_ptr_factory_;
+};
+
 }  // namespace
 
 class OfflinePageModelTaskifiedTest : public testing::Test,
@@ -1221,9 +1250,6 @@ TEST_F(OfflinePageModelTaskifiedTest,
 #endif
 TEST_F(OfflinePageModelTaskifiedTest,
        MAYBE_CheckPagesSavedInSeparateDirsPublic) {
-  auto feature_list = std::make_unique<base::test::ScopedFeatureList>();
-  feature_list->InitAndEnableFeature(
-      offline_pages::kOfflinePagesSharingFeature);
   // Save a temporary page.
   auto archiver = BuildArchiver(kTestUrl, ArchiverResult::SUCCESSFULLY_CREATED);
   int64_t temporary_id = SavePageWithExpectedResult(
@@ -1254,6 +1280,51 @@ TEST_F(OfflinePageModelTaskifiedTest,
   EXPECT_NE(temporary_page_path.DirName(), persistent_page_path.DirName());
 }
 
+// This test is affected by https://crbug.com/725685, which only affects windows
+// platform.
+#if defined(OS_WIN)
+#define MAYBE_CheckPublishInternalArchive DISABLED_CheckPublishInternalArchive
+#else
+#define MAYBE_CheckPublishInternalArchive CheckPublishInternalArchive
+#endif
+TEST_F(OfflinePageModelTaskifiedTest, MAYBE_CheckPublishInternalArchive) {
+  // Save a persistent page into our internal directory that will not be
+  // published. We use a "browser actions" page for this purpose.
+  std::unique_ptr<OfflinePageTestArchiver> test_archiver =
+      BuildArchiver(kTestUrl2, ArchiverResult::SUCCESSFULLY_CREATED);
+  int64_t persistent_id = SavePageWithExpectedResult(
+      kTestUrl2, kTestBrowserActionsClientId, GURL(), kEmptyRequestOrigin,
+      std::move(test_archiver), SavePageResult::SUCCESS);
+
+  std::unique_ptr<OfflinePageItem> persistent_page =
+      store_test_util()->GetPageByOfflineId(persistent_id);
+
+  ASSERT_TRUE(persistent_page);
+
+  base::FilePath persistent_page_path = persistent_page->file_path;
+
+  // For a page in the browser actions namespace, it gets moved to the
+  // a private internal directory inside chromium.
+  EXPECT_TRUE(private_archive_dir_path().IsParent(persistent_page_path));
+
+  // Make another archiver, since SavePageWithExpectedResult deleted the first
+  // one.
+  test_archiver =
+      BuildArchiver(kTestUrl2, ArchiverResult::SUCCESSFULLY_CREATED);
+
+  // Publish the page from our internal store.
+  PublishPageTestCallback test_callback;
+  PublishPageCallback publish_done_callback =
+      base::BindOnce(&PublishPageTestCallback::Run, test_callback.GetWeakPtr());
+
+  model()->PublishInternalArchive(*persistent_page, std::move(test_archiver),
+                                  std::move(publish_done_callback));
+  PumpLoop();
+
+  // Check that the page was published as expected.
+  ASSERT_TRUE(test_callback.callback_called());
+}
+
 // This test is disabled since it's lacking the ability of mocking store failure
 // in store_test_utils. https://crbug.com/781023
 // TODO(romax): reenable the test once the above issue is resolved.

components/offline_pages/core/model/update_file_path_task.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/model/update_file_path_task.h"
+
+#include "base/bind.h"
+#include "components/offline_pages/core/client_namespace_constants.h"
+#include "components/offline_pages/core/model/offline_page_model_utils.h"
+#include "components/offline_pages/core/offline_page_metadata_store_sql.h"
+#include "components/offline_pages/core/offline_store_utils.h"
+#include "sql/connection.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace offline_pages {
+
+namespace {
+
+bool UpdateFilePathSync(const base::FilePath& new_file_path,
+                        int64_t offline_id,
+                        sql::Connection* db) {
+  if (!db)
+    return false;
+
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return false;
+
+  // Update the file_path to point to the new path.
+  const char kSqlUpdate[] =
+      "UPDATE OR IGNORE offlinepages_v1"
+      " SET file_path = ?"
+      " 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);
+
+  if (!update_statement.Run())
+    return false;
+
+  if (!transaction.Commit())
+    return false;
+
+  return true;
+}
+
+}  // namespace
+
+UpdateFilePathTask::UpdateFilePathTask(OfflinePageMetadataStoreSQL* store,
+                                       int64_t offline_id,
+                                       const base::FilePath& file_path,
+                                       UpdateFilePathDoneCallback callback)
+    : store_(store),
+      offline_id_(offline_id),
+      file_path_(file_path),
+      callback_(std::move(callback)),
+      weak_ptr_factory_(this) {
+  DCHECK(store_);
+}
+
+UpdateFilePathTask::~UpdateFilePathTask(){};
+
+void UpdateFilePathTask::Run() {
+  store_->Execute(base::BindOnce(&UpdateFilePathSync, file_path_, offline_id_),
+                  base::BindOnce(&UpdateFilePathTask::OnUpdateFilePathDone,
+                                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void UpdateFilePathTask::OnUpdateFilePathDone(bool result) {
+  // Forward the updated offline page to the callback
+  std::move(callback_).Run(result);
+  TaskComplete();
+}
+
+}  // namespace offline_pages

components/offline_pages/core/model/update_file_path_task.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_FILE_PATH_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_FILE_PATH_TASK_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_model.h"
+#include "components/offline_pages/core/task.h"
+
+namespace offline_pages {
+
+using ReadResult = GetPagesTask::ReadResult;
+
+class OfflinePageMetadataStoreSQL;
+
+// 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 {
+ public:
+  UpdateFilePathTask(OfflinePageMetadataStoreSQL* store,
+                     int64_t offline_id,
+                     const base::FilePath& file_path,
+                     UpdateFilePathDoneCallback callback);
+  ~UpdateFilePathTask() override;
+
+  // Task implementation.
+  void Run() override;
+
+ private:
+  void OnUpdateFilePathDone(bool result);
+
+  // The metadata store used to update the page. Not owned.
+  OfflinePageMetadataStoreSQL* store_;
+
+  int64_t offline_id_;
+  base::FilePath file_path_;
+  UpdateFilePathDoneCallback callback_;
+
+  base::WeakPtrFactory<UpdateFilePathTask> weak_ptr_factory_;
+  DISALLOW_COPY_AND_ASSIGN(UpdateFilePathTask);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_MODEL_UPDATE_FILE_PATH_TASK_H_

components/offline_pages/core/offline_page_test_archiver.cc

@@ -25,13 +25,14 @@ OfflinePageTestArchiver::OfflinePageTestArchiver(
       result_(result),
       size_to_report_(size_to_report),
       create_archive_called_(false),
+      publish_archive_called_(false),
       delayed_(false),
       result_title_(result_title),
       digest_to_report_(digest_to_report),
       task_runner_(task_runner) {}
 
 OfflinePageTestArchiver::~OfflinePageTestArchiver() {
-  EXPECT_TRUE(create_archive_called_);
+  EXPECT_TRUE(create_archive_called_ || publish_archive_called_);
 }
 
 void OfflinePageTestArchiver::CreateArchive(
@@ -53,6 +54,7 @@ void OfflinePageTestArchiver::PublishArchive(
     const base::FilePath& new_file_path,
     SystemDownloadManager* download_manager,
     PublishArchiveDoneCallback publish_done_callback) {
+  publish_archive_called_ = true;
   publish_archive_result_.move_result = SavePageResult::SUCCESS;
   publish_archive_result_.new_file_path = offline_page.file_path;
   publish_archive_result_.download_id = 0;

components/offline_pages/core/offline_page_test_archiver.h

@@ -85,6 +85,7 @@ class OfflinePageTestArchiver : public OfflinePageArchiver {
   ArchiverResult result_;
   int64_t size_to_report_;
   bool create_archive_called_;
+  bool publish_archive_called_;
   bool delayed_;
   base::string16 result_title_;
   std::string digest_to_report_;

components/offline_pages/core/offline_page_types.h

@@ -108,8 +108,10 @@ typedef base::OnceCallback<void(std::unique_ptr<OfflinePageThumbnail>)>
     GetThumbnailCallback;
 typedef base::OnceCallback<void(bool)> CleanupThumbnailsCallback;
 
-// Callback used for publishing an offline page.
-using PublishPageCallback = base::OnceCallback<void(const OfflinePageItem&)>;
+// Callbacks used for publishing an offline page.
+using PublishPageCallback =
+    base::OnceCallback<void(const base::FilePath&, bool)>;
+using UpdateFilePathDoneCallback = base::OnceCallback<void(bool)>;
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_CORE_OFFLINE_PAGE_TYPES_H_