Wipe out offline page data on clearing cookie and site data

components/offline_pages/offline_page_metadata_store_impl.cc

Index: components/offline_pages/offline_page_metadata_store_impl.cc
diff --git a/components/offline_pages/offline_page_metadata_store_impl.cc b/components/offline_pages/offline_page_metadata_store_impl.cc
index 974df27c1fc56a8d985ef3c4d9d7b3fd70b6e4cf..e11312ca32eb0e6f1da32924a05e34067ab4313a 100644
--- a/components/offline_pages/offline_page_metadata_store_impl.cc
+++ b/components/offline_pages/offline_page_metadata_store_impl.cc
@@ -16,7 +16,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/thread_task_runner_handle.h"
-#include "components/leveldb_proto/proto_database.h"
+#include "components/leveldb_proto/proto_database_impl.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/proto/offline_pages.pb.h"
 #include "third_party/leveldatabase/env_chromium.h"
@@ -36,8 +36,6 @@ const char kDatabaseUMAClientName[] = "OfflinePageMetadataStore";
 namespace offline_pages {
 namespace {
 
-typedef std::vector<OfflinePageEntry> OfflinePageEntryVector;
-
 void OfflinePageItemToEntry(const OfflinePageItem& item,
                             offline_pages::OfflinePageEntry* item_proto) {
   DCHECK(item_proto);
@@ -94,85 +92,73 @@ bool OfflinePageItemFromEntry(const offline_pages::OfflinePageEntry& item_proto,
   return true;
 }
 
-void OnLoadDone(const OfflinePageMetadataStore::LoadCallback& callback,
-                const base::Callback<void()>& failure_callback,
-                bool success,
-                scoped_ptr<OfflinePageEntryVector> entries) {
-  UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", success);
-  if (!success) {
-    DVLOG(1) << "Offline pages database load failed.";
-    failure_callback.Run();
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(callback, false, std::vector<OfflinePageItem>()));
-    return;
-  }
-
-  std::vector<OfflinePageItem> result;
-  for (OfflinePageEntryVector::iterator it = entries->begin();
-       it != entries->end(); ++it) {
-    OfflinePageItem item;
-    if (OfflinePageItemFromEntry(*it, &item))
-      result.push_back(item);
-    else
-      DVLOG(1) << "Failed to create offline page item from proto.";
-  }
-  UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", result.size());
-
-  base::MessageLoop::current()->PostTask(FROM_HERE,
-                                         base::Bind(callback, true, result));
-}
-
-void OnUpdateDone(const OfflinePageMetadataStore::UpdateCallback& callback,
-                  const base::Callback<void()>& failure_callback,
-                  bool success) {
-  if (!success) {
-    // TODO(fgorski): Add UMA for this case.
-    DVLOG(1) << "Offline pages database update failed.";
-    failure_callback.Run();
-  }
-
-  base::MessageLoop::current()->PostTask(FROM_HERE,
-                                         base::Bind(callback, success));
-}
-
 }  // namespace
 
 OfflinePageMetadataStoreImpl::OfflinePageMetadataStoreImpl(
-    scoped_ptr<ProtoDatabase<OfflinePageEntry>> database,
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
     const base::FilePath& database_dir)
-    : database_(database.Pass()), weak_ptr_factory_(this) {
-  database_->Init(kDatabaseUMAClientName, database_dir,
-                  base::Bind(&OfflinePageMetadataStoreImpl::OnInitDone,
-                             weak_ptr_factory_.GetWeakPtr()));
+    : background_task_runner_(background_task_runner),
+      database_dir_(database_dir),
+      weak_ptr_factory_(this) {
 }
 
 OfflinePageMetadataStoreImpl::~OfflinePageMetadataStoreImpl() {
 }
 
-void OfflinePageMetadataStoreImpl::OnInitDone(bool success) {
+void OfflinePageMetadataStoreImpl::Load(const LoadCallback& callback) {
+  // First initialize the database.
+  DCHECK(!database_);
+  database_.reset(new leveldb_proto::ProtoDatabaseImpl<OfflinePageEntry>(
+      background_task_runner_));
+  database_->Init(kDatabaseUMAClientName, database_dir_,
+                  base::Bind(&OfflinePageMetadataStoreImpl::LoadContinuation,
+                             weak_ptr_factory_.GetWeakPtr(),
+                             callback));
+}
+
+void OfflinePageMetadataStoreImpl::LoadContinuation(
+    const LoadCallback& callback,
+    bool success) {
   if (!success) {
-    // TODO(fgorski): Add UMA for this case.
-    DVLOG(1) << "Offline pages database init failed.";
-    ResetDB();
+    UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);

[fgorski, 2015/10/23 20:55:03]

does it make sense to database_.reset() here? to not have it hang in memory?

Otherwise, if we expect the caller to properly clean up by calling Reset, that
should be clearly documented.

[jianli, 2015/10/26 21:42:43]

Done.

+    callback.Run(false, std::vector<OfflinePageItem>());
     return;
   }
+
+  // After initialization, start to load the data.
+  database_->LoadEntries(
+      base::Bind(&OfflinePageMetadataStoreImpl::LoadCompleted,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback));
 }
 
-void OfflinePageMetadataStoreImpl::Load(const LoadCallback& callback) {
-  if (!database_.get()) {
-    // Failing fast here, because DB is not initialized, and there is nothing
-    // that can be done about it.
-    // Callback is invoked through message loop to avoid improper retry and
-    // simplify testing.
-    DVLOG(1) << "Offline pages database not available in Load.";
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE, base::Bind(callback, false, std::vector<OfflinePageItem>()));
+void OfflinePageMetadataStoreImpl::LoadCompleted(
+    const LoadCallback& callback,
+    bool success,
+    scoped_ptr<std::vector<OfflinePageEntry>> entries) {
+  DCHECK(entries);
+
+  if (!success) {
+    DVLOG(1) << "Offline pages database load failed.";
+    UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);

[fgorski, 2015/10/23 20:55:03]

It would likely make sense to distinguish this case from the previous one and
hence turn this UMA into an enum pointing to where it failed.

[jianli, 2015/10/26 21:42:43]

Done.

+    callback.Run(false, std::vector<OfflinePageItem>());
     return;
   }
 
-  database_->LoadEntries(base::Bind(
-      &OnLoadDone, callback, base::Bind(&OfflinePageMetadataStoreImpl::ResetDB,
-                                        weak_ptr_factory_.GetWeakPtr())));
+  std::vector<OfflinePageItem> result;
+  for (const auto& entry : *entries) {
+    OfflinePageItem item;
+    if (!OfflinePageItemFromEntry(entry, &item)) {

[fgorski, 2015/10/23 20:55:03]

Explain why you are changing this. This sounds too aggressive. This will make
the whole DB load fail if one of the entries is busted.

We could drop that entry instead of failing completely.

[jianli, 2015/10/26 21:42:43]

I don't think it is safe to allow partial load when certain data were corrupted
unless we add checksum to validate each entry. If some bad thing happens, we
should not trust the store data at all and we should rebuild it.

+      DVLOG(1) << "Failed to create offline page item from proto.";
+      UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);
+      callback.Run(false, std::vector<OfflinePageItem>());
+      return;
+    }
+    result.push_back(item);
+  }
+  UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", result.size());
+
+  callback.Run(true, result);
 }
 
 void OfflinePageMetadataStoreImpl::AddOrUpdateOfflinePage(
@@ -223,14 +209,35 @@ void OfflinePageMetadataStoreImpl::UpdateEntries(
 
   database_->UpdateEntries(
       entries_to_save.Pass(), keys_to_remove.Pass(),
-      base::Bind(&OnUpdateDone, callback,
-                 base::Bind(&OfflinePageMetadataStoreImpl::ResetDB,
-                            weak_ptr_factory_.GetWeakPtr())));
+      base::Bind(&OfflinePageMetadataStoreImpl::UpdateCompleted,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback));
+}
+
+void OfflinePageMetadataStoreImpl::UpdateCompleted(
+    const OfflinePageMetadataStore::UpdateCallback& callback,
+    bool success) {
+  if (!success) {
+    // TODO(fgorski): Add UMA for this case. Consider rebuilding the store.
+    DVLOG(1) << "Offline pages database update failed.";
+  }
+
+  callback.Run(success);
+}
+
+void OfflinePageMetadataStoreImpl::Reset(const ResetCallback& callback) {
+  database_->Destroy(
+      base::Bind(&OfflinePageMetadataStoreImpl::ResetCompleted,
+                weak_ptr_factory_.GetWeakPtr(),

[fgorski, 2015/10/23 20:55:03]

nit: git cl format is your friend.

[jianli, 2015/10/26 21:42:43]

Done.

+                callback));
 }
 
-void OfflinePageMetadataStoreImpl::ResetDB() {
+void OfflinePageMetadataStoreImpl::ResetCompleted(
+    const ResetCallback& callback,
+    bool success) {
   database_.reset();
   weak_ptr_factory_.InvalidateWeakPtrs();
+  callback.Run(success);
 }
 
 }  // namespace offline_pages