[Offline pages] Updating the UpdateCallback in OPMStoreSQL

components/offline_pages/offline_page_metadata_store_sql.cc

Index: components/offline_pages/offline_page_metadata_store_sql.cc
diff --git a/components/offline_pages/offline_page_metadata_store_sql.cc b/components/offline_pages/offline_page_metadata_store_sql.cc
index 3d6cea505f28cdc55a22d79049fa71bdbd81db4f..31ab0e3677971998f902e135a1b7d306e6124535 100644
--- a/components/offline_pages/offline_page_metadata_store_sql.cc
+++ b/components/offline_pages/offline_page_metadata_store_sql.cc
@@ -21,6 +21,7 @@
 namespace offline_pages {
 
 using StoreState = OfflinePageMetadataStore::StoreState;
+using ItemActionStatus = OfflinePageMetadataStore::ItemActionStatus;
 
 namespace {
 
@@ -208,28 +209,27 @@ bool Insert(sql::Connection* db, const OfflinePageItem& item) {
   return statement.Run() && db->GetLastChangeCount() > 0;
 }
 
-bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
+bool Update(sql::Connection* db, const OfflinePageItem& item) {
   const char kSql[] =
-      "INSERT OR REPLACE INTO " OFFLINE_PAGES_TABLE_NAME
-      " (offline_id, online_url, client_namespace, client_id, file_path, "
-      "file_size, creation_time, last_access_time, access_count, "
-      "expiration_time, title)"
-      " VALUES "
-      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+      "UPDATE OR IGNORE " OFFLINE_PAGES_TABLE_NAME
+      " SET online_url = ?, client_namespace = ?, client_id = ?, file_path = ?,"
+      " file_size = ?, creation_time = ?, last_access_time = ?,"
+      " access_count = ?, expiration_time = ?, title = ?"
+      " WHERE offline_id = ?";
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
-  statement.BindInt64(0, item.offline_id);
-  statement.BindString(1, item.url.spec());
-  statement.BindString(2, item.client_id.name_space);
-  statement.BindString(3, item.client_id.id);
-  statement.BindString(4, GetUTF8StringFromPath(item.file_path));
-  statement.BindInt64(5, item.file_size);
-  statement.BindInt64(6, item.creation_time.ToInternalValue());
-  statement.BindInt64(7, item.last_access_time.ToInternalValue());
-  statement.BindInt(8, item.access_count);
-  statement.BindInt64(9, item.expiration_time.ToInternalValue());
-  statement.BindString16(10, item.title);
-  return statement.Run();
+  statement.BindString(0, item.url.spec());
+  statement.BindString(1, item.client_id.name_space);
+  statement.BindString(2, item.client_id.id);
+  statement.BindString(3, GetUTF8StringFromPath(item.file_path));
+  statement.BindInt64(4, item.file_size);
+  statement.BindInt64(5, item.creation_time.ToInternalValue());
+  statement.BindInt64(6, item.last_access_time.ToInternalValue());
+  statement.BindInt(7, item.access_count);
+  statement.BindInt64(8, item.expiration_time.ToInternalValue());
+  statement.BindString16(9, item.title);
+  statement.BindInt64(10, item.offline_id);
+  return statement.Run() && db->GetLastChangeCount() > 0;
 }
 
 bool InitDatabase(sql::Connection* db, base::FilePath path) {
@@ -279,6 +279,23 @@ void OpenConnectionSync(sql::Connection* db,
   runner->PostTask(FROM_HERE, base::Bind(callback, state));
 }
 
+bool GetPageByOfflineIdSync(sql::Connection* db,
+                            int64_t offline_id,
+                            OfflinePageItem* item) {
+  const char kSql[] =
+      "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id = ?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, offline_id);
+
+  bool found = false;
+  while (statement.Step()) {

[Pete Williamson, 2016/09/15 18:05:43]

Nit - why use "while" here if offline_id is unique?  Would "if" be better?

[fgorski, 2016/09/19 23:24:30]

Done.

+    *item = MakeOfflinePageItem(&statement);
+    found = true;
+  }
+
+  return found;
+}
+
 void GetOfflinePagesSync(
     sql::Connection* db,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
@@ -305,12 +322,33 @@ void AddOfflinePageSync(sql::Connection* db,
                         scoped_refptr<base::SingleThreadTaskRunner> runner,
                         const OfflinePageItem& offline_page,
                         const OfflinePageMetadataStore::AddCallback& callback) {
-  // TODO(fgorski): Only insert should happen here.
   bool success = Insert(db, offline_page);
   runner->PostTask(
       FROM_HERE,
-      base::Bind(callback, success ? OfflinePageMetadataStore::SUCCESS
-                                   : OfflinePageMetadataStore::ALREADY_EXISTS));
+      base::Bind(callback, success ? ItemActionStatus::SUCCESS
+                                   : ItemActionStatus::ALREADY_EXISTS));
+}
+
+void PostStoreErrorForAllPages(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const std::vector<OfflinePageItem>& pages,
+    const OfflinePageMetadataStore::UpdateCallback& callback) {
+  std::map<int64_t, ItemActionStatus> failed_results;
+  for (const auto& page : pages)
+    failed_results[page.offline_id] = ItemActionStatus::STORE_ERROR;
+  runner->PostTask(FROM_HERE, base::Bind(callback, failed_results,
+                                         std::vector<OfflinePageItem>()));
+}
+
+void PostStoreErrorForAllIds(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const std::vector<int64_t>& offline_ids,
+    const OfflinePageMetadataStore::UpdateCallback& callback) {
+  std::map<int64_t, ItemActionStatus> failed_results;
+  for (const auto& offline_id : offline_ids)
+    failed_results[offline_id] = ItemActionStatus::STORE_ERROR;
+  runner->PostTask(FROM_HERE, base::Bind(callback, failed_results,
+                                         std::vector<OfflinePageItem>()));
 }
 
 void UpdateOfflinePagesSync(
@@ -320,18 +358,29 @@ void UpdateOfflinePagesSync(
     const OfflinePageMetadataStore::UpdateCallback& callback) {
   // TODO(fgorski): Update the callback to provide information about all items
   // and not just a high level boolean.
+  std::map<int64_t, ItemActionStatus> failed_results;
+  std::vector<OfflinePageItem> updated_items;
+
   sql::Transaction transaction(db);
   if (!transaction.Begin()) {
-    runner->PostTask(FROM_HERE, base::Bind(callback, false));
+    PostStoreErrorForAllPages(runner, pages, callback);
     return;
   }
 
-  // TODO(fgorski): Switch to only accept updates and not create new items.
-  for (auto& page : pages)
-    InsertOrReplace(db, page);
+  for (auto& page : pages) {

[Pete Williamson, 2016/09/15 18:05:43]

Should this be const auto& ?

[fgorski, 2016/09/19 23:24:30]

Done.

+    bool updated = Update(db, page);
+    if (updated)
+      updated_items.push_back(page);
+    else
+      failed_results[page.offline_id] = ItemActionStatus::DOESNT_EXIST;
+  }
 
-  bool result = transaction.Commit();
-  runner->PostTask(FROM_HERE, base::Bind(callback, result));
+  if (!transaction.Commit()) {
+    PostStoreErrorForAllPages(runner, pages, callback);
+    return;
+  }
+  runner->PostTask(FROM_HERE,
+                   base::Bind(callback, failed_results, updated_items));
 }
 
 void RemoveOfflinePagesSync(
@@ -339,25 +388,42 @@ void RemoveOfflinePagesSync(
     sql::Connection* db,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     const OfflinePageMetadataStore::UpdateCallback& callback) {
-  // TODO(bburns): add UMA metrics here (and for levelDB).
+  // TODO(fgorski): Perhaps add metrics here.
+  std::map<int64_t, ItemActionStatus> failed_results;
+  std::vector<OfflinePageItem> removed_items;
 
   // If you create a transaction but don't Commit() it is automatically
   // rolled back by its destructor when it falls out of scope.
   sql::Transaction transaction(db);
   if (!transaction.Begin()) {
-    runner->PostTask(FROM_HERE, base::Bind(callback, false));
+    PostStoreErrorForAllIds(runner, offline_ids, callback);
     return;
   }
 
   for (auto offline_id : offline_ids) {

[Pete Williamson, 2016/09/15 18:05:43]

const?

[fgorski, 2016/09/19 23:24:30]

Switched to int64_t instead.

-    if (!DeleteByOfflineId(db, offline_id)) {
-      runner->PostTask(FROM_HERE, base::Bind(callback, false));
-      return;
+    OfflinePageItem page;
+    if (!GetPageByOfflineIdSync(db, offline_id, &page)) {

[Pete Williamson, 2016/09/15 18:05:43]

Do we need this SQL operation when we can just check the return value of
DeleteByOfflineId()?

[fgorski, 2016/09/19 23:24:30]

Yes, because we want to return the deleted items for observers.

+      failed_results[offline_id] = ItemActionStatus::DOESNT_EXIST;
+      continue;
     }
+
+    bool removed = DeleteByOfflineId(db, offline_id);
+    DCHECK(removed);  // No reason to fail at this point.
+    // TODO(fgorski): Perhaps provide separate status for delete failure that is
+    // not STORE_ERROR or DOESNT_EXIST.
+    if (removed)
+      removed_items.push_back(page);
+    else
+      failed_results[offline_id] = ItemActionStatus::DOESNT_EXIST;
+  }
+
+  if (!transaction.Commit()) {
+    PostStoreErrorForAllIds(runner, offline_ids, callback);
+    return;
   }
 
-  bool success = transaction.Commit();
-  runner->PostTask(FROM_HERE, base::Bind(callback, success));
+  runner->PostTask(FROM_HERE,
+                   base::Bind(callback, failed_results, removed_items));
 }
 
 void ResetSync(sql::Connection* db,
@@ -419,8 +485,11 @@ void OfflinePageMetadataStoreSQL::AddOfflinePage(
 void OfflinePageMetadataStoreSQL::UpdateOfflinePages(
     const std::vector<OfflinePageItem>& pages,
     const UpdateCallback& callback) {
-  if (!CheckDb(base::Bind(callback, false)))
+  if (!db_.get()) {
+    PostStoreErrorForAllPages(base::ThreadTaskRunnerHandle::Get(), pages,
+                              callback);
     return;
+  }
 
   background_task_runner_->PostTask(
       FROM_HERE,
@@ -436,8 +505,10 @@ void OfflinePageMetadataStoreSQL::RemoveOfflinePages(
   if (offline_ids.empty()) {
     // Nothing to do, but post a callback instead of calling directly
     // to preserve the async style behavior to prevent bugs.
-    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
-                                                  base::Bind(callback, true));
+    // TODO(fgorski): Refactor to accept item action status.
+    // It works not because the list of offline IDs is empty.
+    PostStoreErrorForAllIds(base::ThreadTaskRunnerHandle::Get(), offline_ids,

[Pete Williamson, 2016/09/15 18:05:43]

Is "STORE_ERROR" the right error for an empty list of things to remove?  That
seems like it should be a different kind of error code to me.

[fgorski, 2016/09/19 23:24:30]

Done.

+                            callback);
     return;
   }