[Offline pages] OPMStoreSQL implementation of Add disabling updates

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 6cf2f21c14e5a8b212c42fc347fc52c8c38b86e3..3d6cea505f28cdc55a22d79049fa71bdbd81db4f 100644
--- a/components/offline_pages/offline_page_metadata_store_sql.cc
+++ b/components/offline_pages/offline_page_metadata_store_sql.cc
@@ -182,6 +182,32 @@ OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
   return item;
 }
 
+bool Insert(sql::Connection* db, const OfflinePageItem& item) {
+  // Using 'INSERT OR FAIL' or 'INSERT OR ABORT' in the query below causes debug
+  // builds to DLOG.
+  const char kSql[] =
+      "INSERT OR IGNORE INTO " OFFLINE_PAGES_TABLE_NAME
+      " (offline_id, online_url, client_namespace, client_id, file_path, "

[dewittj, 2016/09/15 14:58:26]

Can we parametrize this  so that there's no chance that InsertOrReplace will get
out of sync with Insert?

[fgorski, 2016/09/15 16:53:47]

No, and you will see why in the patch that follows.

+      "file_size, creation_time, last_access_time, access_count, "
+      "expiration_time, title)"
+      " VALUES "
+      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+
+  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() && db->GetLastChangeCount() > 0;

[dewittj, 2016/09/15 14:58:26]

This function should return ItemActionStatus, so that statement.Run() == false
can be separated from LastChangeCount == 0.

[fgorski, 2016/09/15 16:53:47]

Done.

+}
+
 bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
   const char kSql[] =
       "INSERT OR REPLACE INTO " OFFLINE_PAGES_TABLE_NAME
@@ -280,9 +306,11 @@ void AddOfflinePageSync(sql::Connection* db,
                         const OfflinePageItem& offline_page,
                         const OfflinePageMetadataStore::AddCallback& callback) {
   // TODO(fgorski): Only insert should happen here.
-  InsertOrReplace(db, offline_page);
-  runner->PostTask(FROM_HERE,
-                   base::Bind(callback, OfflinePageMetadataStore::SUCCESS));
+  bool success = Insert(db, offline_page);
+  runner->PostTask(
+      FROM_HERE,
+      base::Bind(callback, success ? OfflinePageMetadataStore::SUCCESS
+                                   : OfflinePageMetadataStore::ALREADY_EXISTS));
 }
 
 void UpdateOfflinePagesSync(