[Offline Pages] Implementing create archive task.

Implementing CreateArchiveTask, which is a task for starting the archive
creation but will not wait for the completion of the process, in order
to support multiple archivers working in parallel. The creator of the
task has to provide a CreateArchiveTaskCallback, which will be
processing the arguments and decide whether the creation is successful,
so that it can create AddPageTask for next steps.
Also duplicating most of the tests related with archive creation from
the OfflinePageModelImpl unit tests.

Bug: 753595
Change-Id: I915daab56a260ccaa9c028c0ea95c2ff0646d461
Reviewed-on: https://chromium-review.googlesource.com/634084
Commit-Queue: Yafei Duan <romax@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501468}

components/offline_pages/core/BUILD.gn

@@ -18,6 +18,8 @@ static_library("core") {
     "client_policy_controller.h",
     "model/add_page_task.cc",
     "model/add_page_task.h",
+    "model/create_archive_task.cc",
+    "model/create_archive_task.h",
     "model/offline_store_utils.cc",
     "model/offline_store_utils.h",
     "offline_event_logger.cc",
@@ -107,6 +109,7 @@ source_set("unit_tests") {
   sources = [
     "archive_manager_unittest.cc",
     "client_policy_controller_unittest.cc",
+    "model/create_archive_task_unittest.cc",
     "offline_event_logger_unittest.cc",
     "offline_page_metadata_store_unittest.cc",
     "offline_page_model_event_logger_unittest.cc",

components/offline_pages/core/model/create_archive_task.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/model/create_archive_task.h"
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/time/default_clock.h"
+#include "components/offline_pages/core/model/offline_store_utils.h"
+#include "components/offline_pages/core/offline_page_model.h"
+
+namespace offline_pages {
+
+using ArchiverResult = OfflinePageArchiver::ArchiverResult;
+
+CreateArchiveTask::CreateArchiveTask(
+    const base::FilePath& archives_dir,
+    const OfflinePageModel::SavePageParams& save_page_params,
+    OfflinePageArchiver* archiver,
+    const CreateArchiveTaskCallback& callback)
+    : archives_dir_(archives_dir),
+      save_page_params_(save_page_params),
+      archiver_(archiver),
+      callback_(callback),
+      clock_(new base::DefaultClock()),
+      skip_clearing_original_url_for_testing_(false) {}
+
+CreateArchiveTask::~CreateArchiveTask() {}
+
+void CreateArchiveTask::Run() {
+  CreateArchive();
+}
+
+void CreateArchiveTask::CreateArchive() {
+  OfflinePageItem proposed_page;
+  // Skip saving the page that is not intended to be saved, like local file
+  // page.
+  if (!OfflinePageModel::CanSaveURL(save_page_params_.url)) {
+    InformCreateArchiveFailed(ArchiverResult::ERROR_SKIPPED);
+    return;
+  }
+
+  // The web contents is not available if archiver is not created and passed.
+  if (!archiver_) {
+    InformCreateArchiveFailed(ArchiverResult::ERROR_CONTENT_UNAVAILABLE);
+    return;
+  }
+
+  // If we already have an offline id, use it. If not, generate one.
+  int64_t offline_id = save_page_params_.proposed_offline_id;
+  if (offline_id == OfflinePageModel::kInvalidOfflineId)
+    offline_id = OfflineStoreUtils::GenerateOfflineId();
+
+  // Create a proposed OfflinePageItem to pass in callback, the page will be
+  // missing fields, which are going to be filled when the archive creation
+  // finishes.
+  proposed_page.url = save_page_params_.url;
+  proposed_page.offline_id = offline_id;
+  proposed_page.client_id = save_page_params_.client_id;
+  proposed_page.creation_time = clock_->Now();
+  proposed_page.request_origin = save_page_params_.request_origin;
+
+  // Don't record the original URL if it is identical to the final URL. This is
+  // because some websites might route the redirect finally back to itself upon
+  // the completion of certain action, i.e., authentication, in the middle.
+  if (skip_clearing_original_url_for_testing_ ||
+      save_page_params_.original_url != proposed_page.url) {
+    proposed_page.original_url = save_page_params_.original_url;
+  }
+
+  OfflinePageArchiver::CreateArchiveParams create_archive_params;
+  // If the page is being saved in the background, we should try to remove the
+  // popup overlay that obstructs viewing the normal content.
+  create_archive_params.remove_popup_overlay = save_page_params_.is_background;
+  create_archive_params.use_page_problem_detectors =
+      save_page_params_.use_page_problem_detectors;
+  archiver_->CreateArchive(archives_dir_, create_archive_params,
+                           base::Bind(callback_, proposed_page));
+
+  // The task will complete here, and the callback will be called once the
+  // |archiver_| is done with the archive creation. This enables multiple
+  // archive creation going on by not blocking the TaskQueue waiting for the
+  // completion.
+  TaskComplete();
+}
+
+void CreateArchiveTask::InformCreateArchiveFailed(ArchiverResult result) {
+  callback_.Run(OfflinePageItem(), archiver_, result, GURL(), base::FilePath(),
+                base::string16(), 0);
+  TaskComplete();
+}
+
+}  // namespace offline_pages

components/offline_pages/core/model/create_archive_task.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_CREATE_ARCHIVE_TASK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_CREATE_ARCHIVE_TASK_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/time/clock.h"
+#include "components/offline_pages/core/offline_page_archiver.h"
+#include "components/offline_pages/core/offline_page_model.h"
+#include "components/offline_pages/core/offline_page_types.h"
+#include "components/offline_pages/core/task.h"
+
+namespace base {
+class FilePath;
+}  // namespace base
+
+namespace offline_pages {
+
+// Task that start an archive creation using the OfflinePageArchiver passed in,
+// along with the SavePageParams.
+// The task will complete before the archive creation actually finishes, in
+// order to allow multiple archivers to work in parallel.
+// The lifetime of the archiver needs to be managed by the caller, this task
+// will not own the archiver. The ownership of the callback passed in will be
+// transferred to the archiver, so that it can still get executed after this
+// task gets destroyed.
+// TODO(romax): Verify that this task is actually needed. If we don't access the
+// database, making this a task seems to much, and it can be simplified as a
+// method.
+class CreateArchiveTask : public Task {
+ public:
+  // The arguments of the callback are (in order):
+  // - Proposed OfflinePageItem, filled with information from SavePageParams,
+  // - Pointer to the archiver.
+  // - ArchiverResult of the archive creation,
+  // - The saved url, which is acquired from the WebContent of the archiver,
+  // - The file path to the saved archive,
+  // - Title of the saved page,
+  // - Size of the saved file.
+  // TODO(romax): simplify the callback and related interface if possible. The
+  // url, path, title and file size can be filled into the OfflinePageItem
+  // during archive creation.
+  typedef base::Callback<void(OfflinePageItem,
+                              OfflinePageArchiver*,
+                              OfflinePageArchiver::ArchiverResult,
+                              const GURL&,
+                              const base::FilePath&,
+                              const base::string16&,
+                              int64_t)>
+      CreateArchiveTaskCallback;
+
+  CreateArchiveTask(const base::FilePath& archives_dir,
+                    const OfflinePageModel::SavePageParams& save_page_params,
+                    OfflinePageArchiver* archiver,
+                    const CreateArchiveTaskCallback& callback);
+  ~CreateArchiveTask() override;
+
+  // Task implementation.
+  void Run() override;
+
+  void set_clock_for_testing(std::unique_ptr<base::Clock> clock) {
+    clock_ = std::move(clock);
+  }
+
+  void set_skip_clearing_original_url_for_testing() {
+    skip_clearing_original_url_for_testing_ = true;
+  }
+
+ private:
+  void CreateArchive();
+  void InformCreateArchiveFailed(OfflinePageArchiver::ArchiverResult result);
+
+  // The directory to save the archive.
+  base::FilePath archives_dir_;
+  OfflinePageModel::SavePageParams save_page_params_;
+  // The archiver used in the task. Not owned.
+  OfflinePageArchiver* archiver_;
+  CreateArchiveTaskCallback callback_;
+  std::unique_ptr<base::Clock> clock_;
+
+  bool skip_clearing_original_url_for_testing_;
+
+  DISALLOW_COPY_AND_ASSIGN(CreateArchiveTask);
+};
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_MODEL_CREATE_ARCHIVE_TASK_H_

components/offline_pages/core/model/create_archive_task_unittest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/offline_pages/core/model/create_archive_task.h"
+
+#include "base/bind.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/weak_ptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/offline_pages/core/offline_page_item.h"
+#include "components/offline_pages/core/offline_page_test_archiver.h"
+#include "components/offline_pages/core/offline_page_test_store.h"
+#include "components/offline_pages/core/offline_page_types.h"
+#include "components/offline_pages/core/test_task.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace offline_pages {
+
+using ArchiverResult = OfflinePageArchiver::ArchiverResult;
+using SavePageParams = OfflinePageModel::SavePageParams;
+
+namespace {
+const char kTestClientNamespace[] = "default";
+const GURL kTestUrl("http://example.com");
+const GURL kTestUrl2("http://other.page.com");
+const GURL kFileUrl("file:///foo");
+const ClientId kTestClientId1(kTestClientNamespace, "1234");
+const int64_t kTestFileSize = 876543LL;
+const base::string16 kTestTitle = base::UTF8ToUTF16("a title");
+const std::string kRequestOrigin("abc.xyz");
+}  // namespace
+
+class CreateArchiveTaskTest
+    : public testing::Test,
+      public OfflinePageTestArchiver::Observer,
+      public base::SupportsWeakPtr<CreateArchiveTaskTest> {
+ public:
+  CreateArchiveTaskTest();
+  ~CreateArchiveTaskTest() override;
+
+  void SetUp() override;
+
+  // OfflinePageTestArchiver::Observer implementation.
+  void SetLastPathCreatedByArchiver(const base::FilePath& file_path) override;
+
+  void PumpLoop();
+  void ResetResults();
+  void OnCreateArchiveDone(OfflinePageItem offline_page,
+                           OfflinePageArchiver* archiver,
+                           ArchiverResult result,
+                           const GURL& saved_url,
+                           const base::FilePath& file_path,
+                           const base::string16& title,
+                           int64_t file_size);
+  std::unique_ptr<OfflinePageTestArchiver> BuildArchiver(const GURL& url,
+                                                         ArchiverResult result);
+  void CreateArchiveWithParams(const SavePageParams& save_page_params,
+                               OfflinePageArchiver* archiver);
+
+  void CreateArchiveWithArchiver(const GURL& gurl,
+                                 const ClientId& client_id,
+                                 const GURL& original_url,
+                                 const std::string& request_origin,
+                                 OfflinePageArchiver* archiver);
+
+  void CreateArchiveWithResult(const GURL& gurl,
+                               const ClientId& client_id,
+                               const GURL& original_url,
+                               const std::string& request_origin,
+                               ArchiverResult expected_result);
+
+  const base::FilePath& archives_dir() { return temp_dir_.GetPath(); }
+  const OfflinePageItem& last_page_of_archive() {
+    return last_page_of_archive_;
+  }
+  OfflinePageArchiver* last_saved_archiver() { return last_saved_archiver_; }
+  ArchiverResult last_create_archive_result() {
+    return last_create_archive_result_;
+  }
+  const base::FilePath& last_archiver_path() { return last_archiver_path_; }
+  const GURL& last_saved_url() { return last_saved_url_; }
+  const base::FilePath& last_saved_file_path() { return last_saved_file_path_; }
+  const base::string16& last_saved_title() { return last_saved_title_; }
+  int64_t last_saved_file_size() { return last_saved_file_size_; }
+
+ private:
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ThreadTaskRunnerHandle task_runner_handle_;
+  base::ScopedTempDir temp_dir_;
+
+  OfflinePageItem last_page_of_archive_;
+  OfflinePageArchiver* last_saved_archiver_;
+  ArchiverResult last_create_archive_result_;
+  base::FilePath last_archiver_path_;
+  GURL last_saved_url_;
+  base::FilePath last_saved_file_path_;
+  base::string16 last_saved_title_;
+  int64_t last_saved_file_size_;
+  // Owning a task to prevent it being destroyed in the heap when calling
+  // CreateArchiveWithParams, which will lead to a heap-use-after-free on
+  // trybots.
+  std::unique_ptr<CreateArchiveTask> task_;
+};
+
+CreateArchiveTaskTest::CreateArchiveTaskTest()
+    : task_runner_(new base::TestSimpleTaskRunner()),
+      task_runner_handle_(task_runner_) {}
+
+CreateArchiveTaskTest::~CreateArchiveTaskTest() {}
+
+void CreateArchiveTaskTest::SetUp() {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+}
+
+void CreateArchiveTaskTest::SetLastPathCreatedByArchiver(
+    const base::FilePath& file_path) {
+  last_archiver_path_ = file_path;
+}
+
+void CreateArchiveTaskTest::PumpLoop() {
+  task_runner_->RunUntilIdle();
+}
+
+void CreateArchiveTaskTest::ResetResults() {
+  last_create_archive_result_ = ArchiverResult::ERROR_CANCELED;
+  last_page_of_archive_ = OfflinePageItem();
+  last_saved_url_ = GURL();
+  last_saved_file_path_ = base::FilePath();
+  last_saved_title_ = base::string16();
+  last_saved_file_size_ = 0;
+  last_archiver_path_.clear();
+}
+void CreateArchiveTaskTest::OnCreateArchiveDone(OfflinePageItem offline_page,
+                                                OfflinePageArchiver* archiver,
+                                                ArchiverResult result,
+                                                const GURL& saved_url,
+                                                const base::FilePath& file_path,
+                                                const base::string16& title,
+                                                int64_t file_size) {
+  last_page_of_archive_ = offline_page;
+  last_saved_archiver_ = archiver;
+  last_create_archive_result_ = result;
+  last_saved_url_ = saved_url;
+  last_saved_file_path_ = file_path;
+  last_saved_title_ = title;
+  last_saved_file_size_ = file_size;
+}
+
+std::unique_ptr<OfflinePageTestArchiver> CreateArchiveTaskTest::BuildArchiver(
+    const GURL& url,
+    ArchiverResult result) {
+  return std::unique_ptr<OfflinePageTestArchiver>(
+      new OfflinePageTestArchiver(this, url, result, kTestTitle, kTestFileSize,
+                                  base::ThreadTaskRunnerHandle::Get()));
+}
+
+void CreateArchiveTaskTest::CreateArchiveWithParams(
+    const SavePageParams& save_page_params,
+    OfflinePageArchiver* archiver) {
+  task_ = base::MakeUnique<CreateArchiveTask>(
+      archives_dir(), save_page_params, archiver,
+      base::Bind(&CreateArchiveTaskTest::OnCreateArchiveDone, AsWeakPtr()));
+  task_->Run();
+  PumpLoop();
+  // Check if the archiver is the same with the one in the callback.
+  EXPECT_EQ(archiver, last_saved_archiver());
+}
+
+void CreateArchiveTaskTest::CreateArchiveWithArchiver(
+    const GURL& gurl,
+    const ClientId& client_id,
+    const GURL& original_url,
+    const std::string& request_origin,
+    OfflinePageArchiver* archiver) {
+  OfflinePageModel::SavePageParams save_page_params;
+  save_page_params.url = gurl;
+  save_page_params.client_id = client_id;
+  save_page_params.original_url = original_url;
+  save_page_params.is_background = false;
+  save_page_params.request_origin = request_origin;
+  CreateArchiveWithParams(save_page_params, archiver);
+}
+
+void CreateArchiveTaskTest::CreateArchiveWithResult(
+    const GURL& gurl,
+    const ClientId& client_id,
+    const GURL& original_url,
+    const std::string& request_origin,
+    ArchiverResult expected_result) {
+  std::unique_ptr<OfflinePageTestArchiver> archiver(
+      BuildArchiver(gurl, expected_result));
+  CreateArchiveWithArchiver(gurl, client_id, original_url, request_origin,
+                            archiver.get());
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveSuccessful) {
+  CreateArchiveWithResult(
+      kTestUrl, kTestClientId1, kTestUrl2, "",
+      OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
+
+  // Check the last result to be successful.
+  EXPECT_EQ(ArchiverResult::SUCCESSFULLY_CREATED, last_create_archive_result());
+  const OfflinePageItem& offline_page = last_page_of_archive();
+
+  // The values will be set during archive creation.
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ(kTestUrl2, offline_page.original_url);
+  EXPECT_EQ("", offline_page.request_origin);
+
+  // The values that will not be set during archive creation, but will be set to
+  // default value in the constructor of OfflinePageItem.
+  EXPECT_EQ(0, offline_page.access_count);
+  EXPECT_EQ(0, offline_page.flags);
+
+  // The values that will not be set during archive creation, but will be in the
+  // CreateArchiveTaskCallback.
+  EXPECT_EQ(last_archiver_path(), last_saved_file_path());
+  EXPECT_EQ(kTestFileSize, last_saved_file_size());
+  EXPECT_EQ(kTestTitle, last_saved_title());
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveSuccessfulWithSameOriginalURL) {
+  // Pass the original URL same as the final URL, the original will be empty in
+  // this case.
+  CreateArchiveWithResult(
+      kTestUrl, kTestClientId1, kTestUrl, "",
+      OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
+
+  // Check the last result to be successful.
+  EXPECT_EQ(ArchiverResult::SUCCESSFULLY_CREATED, last_create_archive_result());
+  const OfflinePageItem& offline_page = last_page_of_archive();
+
+  // The original URL should be empty.
+  EXPECT_TRUE(offline_page.original_url.is_empty());
+
+  // The values will be set during archive creation.
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ("", offline_page.request_origin);
+
+  // The values that will not be set during archive creation, but will be set to
+  // default value in the constructor of OfflinePageItem.
+  EXPECT_EQ(0, offline_page.access_count);
+  EXPECT_EQ(0, offline_page.flags);
+
+  // The values that will not be set during archive creation, but will be in the
+  // CreateArchiveTaskCallback.
+  EXPECT_EQ(last_archiver_path(), last_saved_file_path());
+  EXPECT_EQ(kTestFileSize, last_saved_file_size());
+  EXPECT_EQ(kTestTitle, last_saved_title());
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveSuccessfulWithRequestOrigin) {
+  CreateArchiveWithResult(
+      kTestUrl, kTestClientId1, kTestUrl2, kRequestOrigin,
+      OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
+
+  // Check the last result to be successful.
+  EXPECT_EQ(ArchiverResult::SUCCESSFULLY_CREATED, last_create_archive_result());
+  const OfflinePageItem& offline_page = last_page_of_archive();
+
+  // The values will be set during archive creation.
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ(kTestUrl2, offline_page.original_url);
+  EXPECT_EQ(kRequestOrigin, offline_page.request_origin);
+
+  // The values that will not be set during archive creation, but will be set to
+  // default value in the constructor of OfflinePageItem.
+  EXPECT_EQ(0, offline_page.access_count);
+  EXPECT_EQ(0, offline_page.flags);
+
+  // The values that will not be set during archive creation, but will be in the
+  // CreateArchiveTaskCallback.
+  EXPECT_EQ(last_archiver_path(), last_saved_file_path());
+  EXPECT_EQ(kTestFileSize, last_saved_file_size());
+  EXPECT_EQ(kTestTitle, last_saved_title());
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveWithArchiverCanceled) {
+  CreateArchiveWithResult(kTestUrl, kTestClientId1, GURL(), "",
+                          OfflinePageArchiver::ArchiverResult::ERROR_CANCELED);
+  EXPECT_EQ(ArchiverResult::ERROR_CANCELED, last_create_archive_result());
+
+  // The values will be set during archive creation.
+  const OfflinePageItem& offline_page = last_page_of_archive();
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ(GURL(), offline_page.original_url);
+  EXPECT_EQ("", offline_page.request_origin);
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveWithArchiverDeviceFull) {
+  CreateArchiveWithResult(
+      kTestUrl, kTestClientId1, GURL(), "",
+      OfflinePageArchiver::ArchiverResult::ERROR_DEVICE_FULL);
+  EXPECT_EQ(ArchiverResult::ERROR_DEVICE_FULL, last_create_archive_result());
+
+  // The values will be set during archive creation.
+  const OfflinePageItem& offline_page = last_page_of_archive();
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ(GURL(), offline_page.original_url);
+  EXPECT_EQ("", offline_page.request_origin);
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveWithArchiverContentUnavailable) {
+  CreateArchiveWithResult(
+      kTestUrl, kTestClientId1, GURL(), "",
+      OfflinePageArchiver::ArchiverResult::ERROR_CONTENT_UNAVAILABLE);
+  EXPECT_EQ(ArchiverResult::ERROR_CONTENT_UNAVAILABLE,
+            last_create_archive_result());
+
+  // The values will be set during archive creation.
+  const OfflinePageItem& offline_page = last_page_of_archive();
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ(GURL(), offline_page.original_url);
+  EXPECT_EQ("", offline_page.request_origin);
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveWithCreationFailed) {
+  CreateArchiveWithResult(
+      kTestUrl, kTestClientId1, GURL(), "",
+      OfflinePageArchiver::ArchiverResult::ERROR_ARCHIVE_CREATION_FAILED);
+  EXPECT_EQ(ArchiverResult::ERROR_ARCHIVE_CREATION_FAILED,
+            last_create_archive_result());
+
+  // The values will be set during archive creation.
+  const OfflinePageItem& offline_page = last_page_of_archive();
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ(GURL(), offline_page.original_url);
+  EXPECT_EQ("", offline_page.request_origin);
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveWithArchiverReturnedWrongUrl) {
+  GURL test_url("http://other.random.url.com");
+  std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
+      test_url, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
+  CreateArchiveWithArchiver(kTestUrl, kTestClientId1, GURL(), "",
+                            archiver.get());
+
+  // Since the task will not judge the result even if the |saved_url| in the
+  // callback is not the same with the SavePageParams.url, so the result will be
+  // SUCCESSFULLY_CREATED, but the |last_saved_url()| will be |test_url|. The
+  // creator of the task will be responsible to detect this failure case.
+  EXPECT_EQ(ArchiverResult::SUCCESSFULLY_CREATED, last_create_archive_result());
+  EXPECT_EQ(test_url, last_saved_url());
+
+  // The values will be set during archive creation.
+  const OfflinePageItem& offline_page = last_page_of_archive();
+  EXPECT_EQ(kTestUrl, offline_page.url);
+  EXPECT_NE(OfflinePageModel::kInvalidOfflineId, offline_page.offline_id);
+  EXPECT_EQ(kTestClientId1, offline_page.client_id);
+  EXPECT_EQ(GURL(), offline_page.original_url);
+  EXPECT_EQ("", offline_page.request_origin);
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveLocalFileFailed) {
+  // Don't create archiver since it will not be needed for pages that are not
+  // going to be saved.
+  CreateArchiveWithArchiver(kFileUrl, kTestClientId1, GURL(), "", nullptr);
+  EXPECT_EQ(ArchiverResult::ERROR_SKIPPED, last_create_archive_result());
+
+  // Since the return happens before setting the fields, the OfflinePageItem
+  // returned in the callback will be constructed by default ctor.
+  EXPECT_EQ(OfflinePageItem(), last_page_of_archive());
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveFailedWithNullptr) {
+  // Passing in a nullptr intentionally for test.
+  CreateArchiveWithArchiver(kTestUrl, kTestClientId1, GURL(), "", nullptr);
+  EXPECT_EQ(ArchiverResult::ERROR_CONTENT_UNAVAILABLE,
+            last_create_archive_result());
+
+  // Since the return happens before setting the fields, the OfflinePageItem
+  // returned in the callback will be constructed by default ctor.
+  EXPECT_EQ(OfflinePageItem(), last_page_of_archive());
+}
+
+TEST_F(CreateArchiveTaskTest, CreateArchiveInBackground) {
+  std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
+      kTestUrl, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
+  OfflinePageTestArchiver* archiver_ptr = archiver.get();
+
+  OfflinePageModel::SavePageParams save_page_params;
+  save_page_params.url = kTestUrl;
+  save_page_params.client_id = kTestClientId1;
+  save_page_params.is_background = true;
+  save_page_params.use_page_problem_detectors = false;
+
+  CreateArchiveWithParams(save_page_params, archiver_ptr);
+
+  EXPECT_TRUE(archiver_ptr->create_archive_called());
+  // |remove_popup_overlay| should be turned on on background mode.
+  EXPECT_TRUE(archiver_ptr->create_archive_params().remove_popup_overlay);
+}
+
+}  // namespace offline_pages

components/offline_pages/core/model/offline_store_utils.cc

@@ -4,7 +4,10 @@
 
 #include "components/offline_pages/core/model/offline_store_utils.h"
 
+#include <limits>
+
 #include "base/files/file_path.h"
+#include "base/rand_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 
@@ -35,4 +38,9 @@ AddPageResult OfflineStoreUtils::ItemActionStatusToAddPageResult(
   }
 }
 
+// static
+int64_t OfflineStoreUtils::GenerateOfflineId() {
+  return base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1;
+}
+
 }  // namespace offline_pages

components/offline_pages/core/model/offline_store_utils.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_OFFLINE_PAGES_CORE_MODEL_OFFLINE_STORE_UTILS_H_
 #define COMPONENTS_OFFLINE_PAGES_CORE_MODEL_OFFLINE_STORE_UTILS_H_
 
+#include <stdint.h>
 #include <string>
 
 #include "components/offline_pages/core/offline_page_types.h"
@@ -23,6 +24,9 @@ class OfflineStoreUtils {
 
   // Converts an ItemActionStatus to AddPageResult.
   static AddPageResult ItemActionStatusToAddPageResult(ItemActionStatus status);
+
+  // Generates a random offline id;
+  static int64_t GenerateOfflineId();
 };
 
 }  // namespace offline_pages

components/offline_pages/core/offline_page_archiver.h

@@ -36,7 +36,7 @@ namespace offline_pages {
 // does not happen.
 //
 // If the page is not completely loaded, it is up to the implementation of the
-// archiver whether to respond with ERROR_CONTENT_UNAVAILBLE, wait longer to
+// archiver whether to respond with ERROR_CONTENT_UNAVAILABLE, wait longer to
 // actually snapshot a complete page, or snapshot whatever is available at that
 // point in time (what the user sees).
 class OfflinePageArchiver {
@@ -52,6 +52,8 @@ class OfflinePageArchiver {
                                     // there was a security error.
     ERROR_ERROR_PAGE,               // We detected an error page.
     ERROR_INTERSTITIAL_PAGE,        // We detected an interstitial page.
+    ERROR_SKIPPED,                  // Page shouldn't be archived like NTP or
+                                    // file urls.
   };
 
   // Describes the parameters to control how to create an archive.

components/offline_pages/core/offline_page_item.cc

@@ -7,7 +7,8 @@
 namespace offline_pages {
 
 OfflinePageItem::OfflinePageItem()
-    : file_size(0),
+    : offline_id(0),
+      file_size(0),
       access_count(0),
       flags(NO_FLAG),
       system_download_id(0),