Save offline copy only for http/https pages

BUG=537018,545689
TEST=tests added
TBR=rkaplow@chromium.org

Review URL: https://codereview.chromium.org/1420633002

Cr-Commit-Position: refs/heads/master@{#355692}

3 files changed, 84 insertions, 33 deletions

diff --git a/components/offline_pages/offline_page_model.cc b/components/offline_pages/offline_page_model.cc
index d3a0e59..c007366 100644
--- a/components/offline_pages/offline_page_model.cc
+++ b/components/offline_pages/offline_page_model.cc
@@ -89,6 +89,11 @@ void FindPagesMissingArchiveFile(
 
 }  // namespace
 
+// static
+bool OfflinePageModel::CanSavePage(const GURL& url) {
+  return url.SchemeIsHTTPOrHTTPS();
+}
+
 OfflinePageModel::OfflinePageModel(
     scoped_ptr<OfflinePageMetadataStore> store,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner)
@@ -125,6 +130,14 @@ void OfflinePageModel::SavePage(const GURL& url,
                                 scoped_ptr<OfflinePageArchiver> archiver,
                                 const SavePageCallback& callback) {
   DCHECK(is_loaded_);
+
+  // Skip saving the page that is not intended to be saved, like local file
+  // page.
+  if (!CanSavePage(url)) {
+    InformSavePageDone(callback, SavePageResult::SKIPPED);
+    return;
+  }
+
   DCHECK(archiver.get());
   archiver->CreateArchive(base::Bind(&OfflinePageModel::OnCreateArchiveDone,
                                      weak_ptr_factory_.GetWeakPtr(), url,
diff --git a/components/offline_pages/offline_page_model.h b/components/offline_pages/offline_page_model.h
index 073d274..ab08104 100644
--- a/components/offline_pages/offline_page_model.h
+++ b/components/offline_pages/offline_page_model.h
@@ -65,6 +65,9 @@ class OfflinePageModel : public KeyedService,
     ARCHIVE_CREATION_FAILED,
     STORE_FAILURE,
     ALREADY_EXISTS,
+    // Certain pages, i.e. file URL or NTP, will not be saved because these
+    // are already locally accisible.
+    SKIPPED,
     // NOTE: always keep this entry at the end. Add new result types only
     // immediately above this line. Make sure to update the corresponding
     // histogram enum accordingly.
@@ -115,6 +118,9 @@ class OfflinePageModel : public KeyedService,
   typedef base::Callback<void(SavePageResult)> SavePageCallback;
   typedef base::Callback<void(DeletePageResult)> DeletePageCallback;
 
+  // Returns true if an offline copy can be saved for the given URL.
+  static bool CanSavePage(const GURL& url);
+
   // All blocking calls/disk access will happen on the provided |task_runner|.
   OfflinePageModel(
       scoped_ptr<OfflinePageMetadataStore> store,
diff --git a/components/offline_pages/offline_page_model_unittest.cc b/components/offline_pages/offline_page_model_unittest.cc
index 5e72c22..aab0a4a 100644
--- a/components/offline_pages/offline_page_model_unittest.cc
+++ b/components/offline_pages/offline_page_model_unittest.cc
@@ -29,7 +29,10 @@ namespace {
 const GURL kTestUrl("http://example.com");
 const int64 kTestPageBookmarkId1 = 1234LL;
 const GURL kTestUrl2("http://other.page.com");
+const GURL kTestUrl3("http://test.xyz");
+const GURL kFileUrl("file:///foo");
 const int64 kTestPageBookmarkId2 = 5678LL;
+const int64 kTestPageBookmarkId3 = 42LL;
 const int64 kTestFileSize = 876543LL;
 
 class OfflinePageTestStore : public OfflinePageMetadataStore {
@@ -52,6 +55,10 @@ class OfflinePageTestStore : public OfflinePageMetadataStore {
                               const UpdateCallback& callback) override;
   void RemoveOfflinePages(const std::vector<int64>& bookmark_ids,
                           const UpdateCallback& callback) override;
+
+  void UpdateLastAccessTime(int64 bookmark_id,
+                            const base::Time& last_access_time);
+
   const OfflinePageItem& last_saved_page() const { return last_saved_page_; }
 
   void set_test_scenario(TestScenario scenario) { scenario_ = scenario; };
@@ -124,6 +131,16 @@ void OfflinePageTestStore::RemoveOfflinePages(
   task_runner_->PostTask(FROM_HERE, base::Bind(callback, result));
 }
 
+void OfflinePageTestStore:: UpdateLastAccessTime(
+    int64 bookmark_id, const base::Time& last_access_time) {
+  for (auto& offline_page : offline_pages_) {
+    if (offline_page.bookmark_id == bookmark_id) {
+      offline_page.last_access_time = last_access_time;
+      return;
+    }
+  }
+}
+
 }  // namespace
 
 class OfflinePageModelTest;
@@ -221,7 +238,6 @@ class OfflinePageModelTest
 
  private:
   base::MessageLoop message_loop_;
-  scoped_ptr<base::RunLoop> run_loop_;
   base::ScopedTempDir temp_dir_;
 
   scoped_ptr<OfflinePageModel> model_;
@@ -280,7 +296,7 @@ void OfflinePageModelTest::SetUp() {
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   model_ = BuildModel(BuildStore().Pass()).Pass();
   model_->AddObserver(this);
-  PumpLoop();
+  base::RunLoop().RunUntilIdle();
 }
 
 void OfflinePageModelTest::TearDown() {
@@ -290,7 +306,6 @@ void OfflinePageModelTest::TearDown() {
 
 void OfflinePageModelTest::OfflinePageModelLoaded(OfflinePageModel* model) {
   ASSERT_EQ(model_.get(), model);
-  run_loop_->Quit();
 }
 
 void OfflinePageModelTest::OfflinePageModelChanged(OfflinePageModel* model) {
@@ -299,22 +314,18 @@ void OfflinePageModelTest::OfflinePageModelChanged(OfflinePageModel* model) {
 
 void OfflinePageModelTest::OfflinePageDeleted(int64 bookmark_id) {
   last_deleted_bookmark_id_ = bookmark_id;
-  run_loop_->Quit();
 }
 
 void OfflinePageModelTest::OnSavePageDone(
     OfflinePageModel::SavePageResult result) {
-  run_loop_->Quit();
   last_save_result_ = result;
 }
 
 void OfflinePageModelTest::OnDeletePageDone(DeletePageResult result) {
-  run_loop_->Quit();
   last_delete_result_ = result;
 }
 
 void OfflinePageModelTest::OnStoreUpdateDone(bool /* success - ignored */) {
-  run_loop_->Quit();
 }
 
 scoped_ptr<OfflinePageTestArchiver> OfflinePageModelTest::BuildArchiver(
@@ -346,8 +357,7 @@ void OfflinePageModelTest::ResetModel() {
 }
 
 void OfflinePageModelTest::PumpLoop() {
-  run_loop_.reset(new base::RunLoop());
-  run_loop_->Run();
+  base::RunLoop().RunUntilIdle();
 }
 
 void OfflinePageModelTest::ResetResults() {
@@ -468,6 +478,14 @@ TEST_F(OfflinePageModelTest, SavePageOfflineCreationStoreWriteFailure) {
   EXPECT_EQ(SavePageResult::STORE_FAILURE, last_save_result());
 }
 
+TEST_F(OfflinePageModelTest, SavePageLocalFileFailed) {
+  model()->SavePage(
+      kFileUrl, kTestPageBookmarkId1, scoped_ptr<OfflinePageTestArchiver>(),
+      base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
+  PumpLoop();
+  EXPECT_EQ(SavePageResult::SKIPPED, last_save_result());
+}
+
 TEST_F(OfflinePageModelTest, SavePageOfflineArchiverTwoPages) {
   scoped_ptr<OfflinePageTestArchiver> archiver(
       BuildArchiver(kTestUrl,
@@ -814,35 +832,40 @@ TEST_F(OfflinePageModelTest, GetPageByOfflineURL) {
 // clean up, hence the numbers in time delta.
 TEST_F(OfflinePageModelTest, GetPagesToCleanUp) {
   base::Time now = base::Time::Now();
-  base::Time forty_days_ago = now - base::TimeDelta::FromDays(40);
-  OfflinePageItem page_1(
-      GURL(kTestUrl), kTestPageBookmarkId1,
-      base::FilePath(FILE_PATH_LITERAL("/test/location/page1.mhtml")),
-      kTestFileSize, forty_days_ago);
-  GetStore()->AddOrUpdateOfflinePage(
-      page_1,
-      base::Bind(&OfflinePageModelTest::OnStoreUpdateDone, AsWeakPtr()));
+
+  scoped_ptr<OfflinePageTestArchiver> archiver(
+      BuildArchiver(kTestUrl,
+                    OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED)
+          .Pass());
+  model()->SavePage(
+      kTestUrl, kTestPageBookmarkId1, archiver.Pass(),
+      base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
   PumpLoop();
+  GetStore()->UpdateLastAccessTime(kTestPageBookmarkId1,
+                                   now - base::TimeDelta::FromDays(40));
 
-  OfflinePageItem page_2(
-      GURL(kTestUrl2), kTestPageBookmarkId2,
-      base::FilePath(FILE_PATH_LITERAL("/test/location/page2.mhtml")),
-      kTestFileSize, forty_days_ago);
-  page_2.last_access_time = now - base::TimeDelta::FromDays(31);
-  GetStore()->AddOrUpdateOfflinePage(
-      page_2,
-      base::Bind(&OfflinePageModelTest::OnStoreUpdateDone, AsWeakPtr()));
+  scoped_ptr<OfflinePageTestArchiver> archiver2(
+      BuildArchiver(kTestUrl2,
+                    OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED)
+          .Pass());
+  model()->SavePage(
+      kTestUrl2, kTestPageBookmarkId2, archiver2.Pass(),
+      base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
   PumpLoop();
+  GetStore()->UpdateLastAccessTime(kTestPageBookmarkId2,
+                                   now - base::TimeDelta::FromDays(31));
+
 
-  OfflinePageItem page_3(
-      GURL("http://test.xyz"), 42,
-      base::FilePath(FILE_PATH_LITERAL("/test/location/page3.mhtml")),
-      kTestFileSize, forty_days_ago);
-  page_3.last_access_time = now - base::TimeDelta::FromDays(29);
-  GetStore()->AddOrUpdateOfflinePage(
-      page_3,
-      base::Bind(&OfflinePageModelTest::OnStoreUpdateDone, AsWeakPtr()));
+  scoped_ptr<OfflinePageTestArchiver> archiver3(
+      BuildArchiver(kTestUrl3,
+                    OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED)
+          .Pass());
+  model()->SavePage(
+      kTestUrl3, kTestPageBookmarkId3, archiver3.Pass(),
+      base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
   PumpLoop();
+  GetStore()->UpdateLastAccessTime(kTestPageBookmarkId3,
+                                   now - base::TimeDelta::FromDays(29));
 
   ResetModel();
 
@@ -856,4 +879,13 @@ TEST_F(OfflinePageModelTest, GetPagesToCleanUp) {
   EXPECT_EQ(kTestPageBookmarkId2, pages_to_clean_up[1].bookmark_id);
 }
 
+TEST_F(OfflinePageModelTest, CanSavePage) {
+  EXPECT_TRUE(OfflinePageModel::CanSavePage(GURL("http://foo")));
+  EXPECT_TRUE(OfflinePageModel::CanSavePage(GURL("https://foo")));
+  EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("file:///foo")));
+  EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("data:image/png;base64,ab")));
+  EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("chrome://version")));
+  EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("chrome-native://newtab/")));
+}
+
 }  // namespace offline_pages