Update access info when an offline page is being visited

We need to update last access time and increase access count.

BUG=491352

Patch

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

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

10 files changed, 155 insertions, 22 deletions

diff --git a/components/offline_pages/offline_page_item.cc b/components/offline_pages/offline_page_item.cc
index 4e4d697..a017f7b 100644
--- a/components/offline_pages/offline_page_item.cc
+++ b/components/offline_pages/offline_page_item.cc
@@ -14,7 +14,8 @@ const int kCurrentVersion = 1;
 
 OfflinePageItem::OfflinePageItem()
     : version(kCurrentVersion),
-      file_size(0) {
+      file_size(0),
+      access_count(0) {
 }
 
 OfflinePageItem::OfflinePageItem(const GURL& url,
@@ -25,7 +26,8 @@ OfflinePageItem::OfflinePageItem(const GURL& url,
       bookmark_id(bookmark_id),
       version(kCurrentVersion),
       file_path(file_path),
-      file_size(file_size) {
+      file_size(file_size),
+      access_count(0) {
 }
 
 OfflinePageItem::OfflinePageItem(const GURL& url,
@@ -39,7 +41,8 @@ OfflinePageItem::OfflinePageItem(const GURL& url,
       file_path(file_path),
       file_size(file_size),
       creation_time(creation_time),
-      last_access_time(creation_time) {
+      last_access_time(creation_time),
+      access_count(0) {
 }
 
 OfflinePageItem::~OfflinePageItem() {
diff --git a/components/offline_pages/offline_page_item.h b/components/offline_pages/offline_page_item.h
index ac9bcb8..0ae4a8c 100644
--- a/components/offline_pages/offline_page_item.h
+++ b/components/offline_pages/offline_page_item.h
@@ -47,6 +47,8 @@ struct OfflinePageItem {
   base::Time creation_time;
   // The time when the offline archive was last accessed.
   base::Time last_access_time;
+  // Number of times that the offline archive has been accessed.
+  int access_count;
 };
 
 }  // namespace offline_pages
diff --git a/components/offline_pages/offline_page_metadata_store.h b/components/offline_pages/offline_page_metadata_store.h
index 18620bd..d50159f 100644
--- a/components/offline_pages/offline_page_metadata_store.h
+++ b/components/offline_pages/offline_page_metadata_store.h
@@ -32,10 +32,10 @@ class OfflinePageMetadataStore {
   // Get all of the offline pages from the store.
   virtual void Load(const LoadCallback& callback) = 0;
 
-  // Asynchronously adds offline page metadata to the store.
+  // Asynchronously adds or updates offline page metadata to the store.
   // Result of the update is passed in callback.
-  virtual void AddOfflinePage(const OfflinePageItem& offline_page,
-                              const UpdateCallback& callback) = 0;
+  virtual void AddOrUpdateOfflinePage(const OfflinePageItem& offline_page,
+                                      const UpdateCallback& callback) = 0;
 
   // Asynchronously removes offline page metadata from the store.
   // Result of the update is passed in callback.
diff --git a/components/offline_pages/offline_page_metadata_store_impl.cc b/components/offline_pages/offline_page_metadata_store_impl.cc
index 90b7b45..b9801f7 100644
--- a/components/offline_pages/offline_page_metadata_store_impl.cc
+++ b/components/offline_pages/offline_page_metadata_store_impl.cc
@@ -43,6 +43,7 @@ void OfflinePageItemToEntry(const OfflinePageItem& item,
   item_proto->set_file_size(item.file_size);
   item_proto->set_creation_time(item.creation_time.ToInternalValue());
   item_proto->set_last_access_time(item.last_access_time.ToInternalValue());
+  item_proto->set_access_count(item.access_count);
 }
 
 bool OfflinePageItemFromEntry(const offline_pages::OfflinePageEntry& item_proto,
@@ -71,6 +72,9 @@ bool OfflinePageItemFromEntry(const offline_pages::OfflinePageEntry& item_proto,
     item->last_access_time =
         base::Time::FromInternalValue(item_proto.last_access_time());
   }
+  if (item_proto.has_access_count()) {
+    item->access_count = item_proto.access_count();
+  }
   return true;
 }
 
@@ -155,7 +159,7 @@ void OfflinePageMetadataStoreImpl::Load(const LoadCallback& callback) {
                                         weak_ptr_factory_.GetWeakPtr())));
 }
 
-void OfflinePageMetadataStoreImpl::AddOfflinePage(
+void OfflinePageMetadataStoreImpl::AddOrUpdateOfflinePage(
     const OfflinePageItem& offline_page_item,
     const UpdateCallback& callback) {
   scoped_ptr<ProtoDatabase<OfflinePageEntry>::KeyEntryVector> entries_to_save(
diff --git a/components/offline_pages/offline_page_metadata_store_impl.h b/components/offline_pages/offline_page_metadata_store_impl.h
index 828f935..3bab989 100644
--- a/components/offline_pages/offline_page_metadata_store_impl.h
+++ b/components/offline_pages/offline_page_metadata_store_impl.h
@@ -35,8 +35,8 @@ class OfflinePageMetadataStoreImpl : public OfflinePageMetadataStore {
 
   // OfflinePageMetadataStore implementation:
   void Load(const LoadCallback& callback) override;
-  void AddOfflinePage(const OfflinePageItem& offline_page_record,
-                      const UpdateCallback& callback) override;
+  void AddOrUpdateOfflinePage(const OfflinePageItem& offline_page_record,
+                              const UpdateCallback& callback) override;
   void RemoveOfflinePages(const std::vector<int64>& bookmark_ids,
                           const UpdateCallback& callback) override;
 
diff --git a/components/offline_pages/offline_page_metadata_store_impl_unittest.cc b/components/offline_pages/offline_page_metadata_store_impl_unittest.cc
index 30ce39c..8ea31cf 100644
--- a/components/offline_pages/offline_page_metadata_store_impl_unittest.cc
+++ b/components/offline_pages/offline_page_metadata_store_impl_unittest.cc
@@ -122,7 +122,7 @@ TEST_F(OfflinePageMetadataStoreImplTest, AddOfflinePageThenLoad) {
 
   OfflinePageItem offline_page(GURL(kTestURL), kTestBookmarkId,
                                base::FilePath(kFilePath), kFileSize);
-  store->AddOfflinePage(
+  store->AddOrUpdateOfflinePage(
       offline_page,
       base::Bind(&OfflinePageMetadataStoreImplTest::UpdateCallback,
                  base::Unretained(this), ADD));
@@ -154,7 +154,7 @@ TEST_F(OfflinePageMetadataStoreImplTest, AddOfflinePageRestartLoad) {
 
   OfflinePageItem offline_page(GURL(kTestURL), kTestBookmarkId,
                                base::FilePath(kFilePath), kFileSize);
-  store->AddOfflinePage(
+  store->AddOrUpdateOfflinePage(
       offline_page,
       base::Bind(&OfflinePageMetadataStoreImplTest::UpdateCallback,
                  base::Unretained(this), ADD));
@@ -180,6 +180,7 @@ TEST_F(OfflinePageMetadataStoreImplTest, AddOfflinePageRestartLoad) {
   EXPECT_EQ(offline_page.file_size, offline_pages_[0].file_size);
   EXPECT_EQ(offline_page.creation_time, offline_pages_[0].creation_time);
   EXPECT_EQ(offline_page.last_access_time, offline_pages_[0].last_access_time);
+  EXPECT_EQ(offline_page.access_count, offline_pages_[0].access_count);
 }
 
 // Tests removing offline page metadata from the store, for which it first adds
@@ -189,7 +190,7 @@ TEST_F(OfflinePageMetadataStoreImplTest, RemoveOfflinePage) {
 
   OfflinePageItem offline_page(GURL(kTestURL), kTestBookmarkId,
                                base::FilePath(kFilePath), kFileSize);
-  store->AddOfflinePage(
+  store->AddOrUpdateOfflinePage(
       offline_page,
       base::Bind(&OfflinePageMetadataStoreImplTest::UpdateCallback,
                  base::Unretained(this), ADD));
@@ -252,7 +253,7 @@ TEST_F(OfflinePageMetadataStoreImplTest, AddRemoveMultipleOfflinePages) {
       base::FilePath(FILE_PATH_LITERAL("//other.page.com.mhtml"));
   OfflinePageItem offline_page_2(GURL("https://other.page.com"), 5678LL,
                                  file_path_2, 12345, base::Time::Now());
-  store->AddOfflinePage(
+  store->AddOrUpdateOfflinePage(
       offline_page_1,
       base::Bind(&OfflinePageMetadataStoreImplTest::UpdateCallback,
                  base::Unretained(this), ADD));
@@ -261,7 +262,7 @@ TEST_F(OfflinePageMetadataStoreImplTest, AddRemoveMultipleOfflinePages) {
   EXPECT_EQ(STATUS_TRUE, last_status_);
 
   ClearResults();
-  store->AddOfflinePage(
+  store->AddOrUpdateOfflinePage(
       offline_page_2,
       base::Bind(&OfflinePageMetadataStoreImplTest::UpdateCallback,
                  base::Unretained(this), ADD));
@@ -306,6 +307,68 @@ TEST_F(OfflinePageMetadataStoreImplTest, AddRemoveMultipleOfflinePages) {
   EXPECT_EQ(offline_page_2.creation_time, offline_pages_[0].creation_time);
   EXPECT_EQ(offline_page_2.last_access_time,
             offline_pages_[0].last_access_time);
+  EXPECT_EQ(offline_page_2.access_count, offline_pages_[0].access_count);
+}
+
+// Tests updating offline page metadata from the store.
+TEST_F(OfflinePageMetadataStoreImplTest, UpdateOfflinePage) {
+  scoped_ptr<OfflinePageMetadataStoreImpl> store(BuildStore());
+
+  // First, adds a fresh page.
+  OfflinePageItem offline_page(GURL(kTestURL), kTestBookmarkId,
+                               base::FilePath(kFilePath), kFileSize);
+  store->AddOrUpdateOfflinePage(
+      offline_page,
+      base::Bind(&OfflinePageMetadataStoreImplTest::UpdateCallback,
+                 base::Unretained(this), ADD));
+  PumpLoop();
+  EXPECT_EQ(ADD, last_called_callback_);
+  EXPECT_EQ(STATUS_TRUE, last_status_);
+
+  ClearResults();
+  store->Load(base::Bind(&OfflinePageMetadataStoreImplTest::LoadCallback,
+                         base::Unretained(this)));
+  PumpLoop();
+
+  EXPECT_EQ(LOAD, last_called_callback_);
+  EXPECT_EQ(STATUS_TRUE, last_status_);
+  EXPECT_EQ(1U, offline_pages_.size());
+  EXPECT_EQ(offline_page.url, offline_pages_[0].url);
+  EXPECT_EQ(offline_page.bookmark_id, offline_pages_[0].bookmark_id);
+  EXPECT_EQ(offline_page.version, offline_pages_[0].version);
+  EXPECT_EQ(offline_page.file_path, offline_pages_[0].file_path);
+  EXPECT_EQ(offline_page.file_size, offline_pages_[0].file_size);
+  EXPECT_EQ(offline_page.creation_time, offline_pages_[0].creation_time);
+  EXPECT_EQ(offline_page.last_access_time, offline_pages_[0].last_access_time);
+  EXPECT_EQ(offline_page.access_count, offline_pages_[0].access_count);
+
+  // Then updates some data.
+  offline_page.file_size = kFileSize + 1;
+  offline_page.access_count++;
+  store->AddOrUpdateOfflinePage(
+      offline_page,
+      base::Bind(&OfflinePageMetadataStoreImplTest::UpdateCallback,
+                 base::Unretained(this), ADD));
+  PumpLoop();
+  EXPECT_EQ(ADD, last_called_callback_);
+  EXPECT_EQ(STATUS_TRUE, last_status_);
+
+  ClearResults();
+  store->Load(base::Bind(&OfflinePageMetadataStoreImplTest::LoadCallback,
+                         base::Unretained(this)));
+  PumpLoop();
+
+  EXPECT_EQ(LOAD, last_called_callback_);
+  EXPECT_EQ(STATUS_TRUE, last_status_);
+  EXPECT_EQ(1U, offline_pages_.size());
+  EXPECT_EQ(offline_page.url, offline_pages_[0].url);
+  EXPECT_EQ(offline_page.bookmark_id, offline_pages_[0].bookmark_id);
+  EXPECT_EQ(offline_page.version, offline_pages_[0].version);
+  EXPECT_EQ(offline_page.file_path, offline_pages_[0].file_path);
+  EXPECT_EQ(offline_page.file_size, offline_pages_[0].file_size);
+  EXPECT_EQ(offline_page.creation_time, offline_pages_[0].creation_time);
+  EXPECT_EQ(offline_page.last_access_time, offline_pages_[0].last_access_time);
+  EXPECT_EQ(offline_page.access_count, offline_pages_[0].access_count);
 }
 
 }  // namespace
diff --git a/components/offline_pages/offline_page_model.cc b/components/offline_pages/offline_page_model.cc
index f4d7870..a3eac94 100644
--- a/components/offline_pages/offline_page_model.cc
+++ b/components/offline_pages/offline_page_model.cc
@@ -112,6 +112,23 @@ void OfflinePageModel::SavePage(const GURL& url,
   pending_archivers_.push_back(archiver.Pass());
 }
 
+void OfflinePageModel::MarkPageAccessed(int64 bookmark_id) {
+  DCHECK(is_loaded_);
+  auto iter = offline_pages_.find(bookmark_id);
+  if (iter == offline_pages_.end())
+    return;
+
+  // Make a copy of the cached item and update it. The cached item should only
+  // be updated upon the successful store operation.
+  OfflinePageItem offline_page_item = iter->second;
+  offline_page_item.last_access_time = base::Time::Now();
+  offline_page_item.access_count++;
+  store_->AddOrUpdateOfflinePage(
+      offline_page_item,
+      base::Bind(&OfflinePageModel::OnUpdateOfflinePageDone,
+                 weak_ptr_factory_.GetWeakPtr(), offline_page_item));
+}
+
 void OfflinePageModel::DeletePageByBookmarkId(
     int64 bookmark_id,
     const DeletePageCallback& callback) {
@@ -216,7 +233,7 @@ void OfflinePageModel::OnCreateArchiveDone(const GURL& requested_url,
 
   OfflinePageItem offline_page_item(url, bookmark_id, file_path, file_size,
                                     base::Time::Now());
-  store_->AddOfflinePage(
+  store_->AddOrUpdateOfflinePage(
       offline_page_item,
       base::Bind(&OfflinePageModel::OnAddOfflinePageDone,
                  weak_ptr_factory_.GetWeakPtr(), archiver, callback,
@@ -240,6 +257,13 @@ void OfflinePageModel::OnAddOfflinePageDone(OfflinePageArchiver* archiver,
   DeletePendingArchiver(archiver);
 }
 
+void OfflinePageModel::OnUpdateOfflinePageDone(
+    const OfflinePageItem& offline_page_item, bool success) {
+  // Update the item in the cache only upon success.
+  if (success)
+    offline_pages_[offline_page_item.bookmark_id] = offline_page_item;
+}
+
 void OfflinePageModel::BookmarkModelChanged() {
 }
 
diff --git a/components/offline_pages/offline_page_model.h b/components/offline_pages/offline_page_model.h
index dd55a7e..648a0f4 100644
--- a/components/offline_pages/offline_page_model.h
+++ b/components/offline_pages/offline_page_model.h
@@ -130,6 +130,11 @@ class OfflinePageModel : public KeyedService,
                 scoped_ptr<OfflinePageArchiver> archiver,
                 const SavePageCallback& callback);
 
+  // Marks that the offline page related to the passed |bookmark_id| has been
+  // accessed. Its access info, including last access time and access count,
+  // will be updated. Requires that the model is loaded.
+  void MarkPageAccessed(int64 bookmark_id);
+
   // Deletes an offline page related to the passed |bookmark_id|. Requires that
   // the model is loaded.
   void DeletePageByBookmarkId(int64 bookmark_id,
@@ -203,6 +208,9 @@ class OfflinePageModel : public KeyedService,
   void InformDeletePageDone(const DeletePageCallback& callback,
                             DeletePageResult result);
 
+  void OnUpdateOfflinePageDone(const OfflinePageItem& offline_page_item,
+                               bool success);
+
   // Persistent store for offline page metadata.
   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 f60e0f5..0aa80ab 100644
--- a/components/offline_pages/offline_page_model_unittest.cc
+++ b/components/offline_pages/offline_page_model_unittest.cc
@@ -48,8 +48,8 @@ class OfflinePageTestStore : public OfflinePageMetadataStore {
 
   // OfflinePageMetadataStore overrides:
   void Load(const LoadCallback& callback) override;
-  void AddOfflinePage(const OfflinePageItem& offline_page,
-                      const UpdateCallback& callback) override;
+  void AddOrUpdateOfflinePage(const OfflinePageItem& offline_page,
+                              const UpdateCallback& callback) override;
   void RemoveOfflinePages(const std::vector<int64>& bookmark_ids,
                           const UpdateCallback& callback) override;
   const OfflinePageItem& last_saved_page() const { return last_saved_page_; }
@@ -95,8 +95,8 @@ void OfflinePageTestStore::Load(const LoadCallback& callback) {
   }
 }
 
-void OfflinePageTestStore::AddOfflinePage(const OfflinePageItem& offline_page,
-                                          const UpdateCallback& callback) {
+void OfflinePageTestStore::AddOrUpdateOfflinePage(
+    const OfflinePageItem& offline_page, const UpdateCallback& callback) {
   last_saved_page_ = offline_page;
   bool result = scenario_ != TestScenario::WRITE_FAILED;
   if (result) {
@@ -370,6 +370,7 @@ TEST_F(OfflinePageModelTest, SavePageSuccessful) {
   EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].bookmark_id);
   EXPECT_EQ(archiver_path, offline_pages[0].file_path);
   EXPECT_EQ(kTestFileSize, offline_pages[0].file_size);
+  EXPECT_EQ(0, offline_pages[0].access_count);
 }
 
 TEST_F(OfflinePageModelTest, SavePageOfflineArchiverCancelled) {
@@ -503,10 +504,35 @@ TEST_F(OfflinePageModelTest, SavePageOfflineArchiverTwoPages) {
   EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].bookmark_id);
   EXPECT_EQ(archiver_path, offline_pages[0].file_path);
   EXPECT_EQ(kTestFileSize, offline_pages[0].file_size);
+  EXPECT_EQ(0, offline_pages[0].access_count);
   EXPECT_EQ(kTestUrl2, offline_pages[1].url);
   EXPECT_EQ(kTestPageBookmarkId2, offline_pages[1].bookmark_id);
   EXPECT_EQ(archiver_path2, offline_pages[1].file_path);
   EXPECT_EQ(kTestFileSize, offline_pages[1].file_size);
+  EXPECT_EQ(0, offline_pages[1].access_count);
+}
+
+TEST_F(OfflinePageModelTest, MarkPageAccessed) {
+  scoped_ptr<OfflinePageTestArchiver> archiver(
+      BuildArchiver(kTestUrl,
+                    OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED)
+          .Pass());
+  model()->SavePage(
+      kTestUrl, kTestPageBookmarkId1, archiver.Pass(),
+      base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
+  PumpLoop();
+
+  // This will increase access_count by one.
+  model()->MarkPageAccessed(kTestPageBookmarkId1);
+  base::RunLoop().RunUntilIdle();
+
+  const std::vector<OfflinePageItem>& offline_pages = model()->GetAllPages();
+
+  EXPECT_EQ(1UL, offline_pages.size());
+  EXPECT_EQ(kTestUrl, offline_pages[0].url);
+  EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].bookmark_id);
+  EXPECT_EQ(kTestFileSize, offline_pages[0].file_size);
+  EXPECT_EQ(1, offline_pages[0].access_count);
 }
 
 TEST_F(OfflinePageModelTest, GetAllPagesStoreEmpty) {
@@ -692,7 +718,7 @@ TEST_F(OfflinePageModelTest, GetPagesToCleanUp) {
       GURL(kTestUrl), kTestPageBookmarkId1,
       base::FilePath(FILE_PATH_LITERAL("/test/location/page1.mhtml")),
       kTestFileSize, now - base::TimeDelta::FromDays(40));
-  GetStore()->AddOfflinePage(
+  GetStore()->AddOrUpdateOfflinePage(
       page_1,
       base::Bind(&OfflinePageModelTest::OnStoreUpdateDone, AsWeakPtr()));
   PumpLoop();
@@ -701,7 +727,7 @@ TEST_F(OfflinePageModelTest, GetPagesToCleanUp) {
       GURL(kTestUrl2), kTestPageBookmarkId2,
       base::FilePath(FILE_PATH_LITERAL("/test/location/page2.mhtml")),
       kTestFileSize, now - base::TimeDelta::FromDays(31));
-  GetStore()->AddOfflinePage(
+  GetStore()->AddOrUpdateOfflinePage(
       page_2,
       base::Bind(&OfflinePageModelTest::OnStoreUpdateDone, AsWeakPtr()));
   PumpLoop();
@@ -710,7 +736,7 @@ TEST_F(OfflinePageModelTest, GetPagesToCleanUp) {
       GURL("http://test.xyz"), 42,
       base::FilePath(FILE_PATH_LITERAL("/test/location/page3.mhtml")),
       kTestFileSize, now - base::TimeDelta::FromDays(29));
-  GetStore()->AddOfflinePage(
+  GetStore()->AddOrUpdateOfflinePage(
       page_3,
       base::Bind(&OfflinePageModelTest::OnStoreUpdateDone, AsWeakPtr()));
   PumpLoop();
diff --git a/components/offline_pages/proto/offline_pages.proto b/components/offline_pages/proto/offline_pages.proto
index 07025a1..16d8b8f 100644
--- a/components/offline_pages/proto/offline_pages.proto
+++ b/components/offline_pages/proto/offline_pages.proto
@@ -33,4 +33,7 @@ message OfflinePageEntry {
 
   // Last access time of the offline archive.
   optional int64 last_access_time = 7;
+
+  // Number of times that the offline archive has been accessed.
+  optional int32 access_count = 8;
 }