[Offline Pages] Removes two-step expiration related.

components/offline_pages/offline_page_metadata_store_sql.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/offline_pages/offline_page_metadata_store_sql.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
@@ skipping 15 matching lines @@
 // it can be used inline in other SQL statements below.
 #define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
 
 bool CreateOfflinePagesTable(sql::Connection* db) {
   const char kSql[] = "CREATE TABLE IF NOT EXISTS " OFFLINE_PAGES_TABLE_NAME
                       "(offline_id INTEGER PRIMARY KEY NOT NULL,"
                       " creation_time INTEGER NOT NULL,"
                       " file_size INTEGER NOT NULL,"
                       " last_access_time INTEGER NOT NULL,"
                       " access_count INTEGER NOT NULL,"
-                      " expiration_time INTEGER NOT NULL DEFAULT 0,"
                       " client_namespace VARCHAR NOT NULL,"
                       " client_id VARCHAR NOT NULL,"
                       " online_url VARCHAR NOT NULL,"
                       " file_path VARCHAR NOT NULL,"
                       " title VARCHAR NOT NULL DEFAULT '',"
                       " original_url VARCHAR NOT NULL DEFAULT ''"
                       ")";
   return db->Execute(kSql);
 }
 
@@ skipping 22 matching lines @@
       "access_count, client_namespace, client_id, "
       "online_url, file_path "
       "FROM temp_" OFFLINE_PAGES_TABLE_NAME;
   return UpgradeWithQuery(db, kSql);
 }
 
 bool UpgradeFrom53(sql::Connection* db) {
   const char kSql[] =
       "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
       " (offline_id, creation_time, file_size, last_access_time, "
-      "access_count, expiration_time, client_namespace, client_id, "

[fgorski, 2016/11/18 00:13:04]

this will revive expired pages.

Shouldn't we delete them as part of the upgrade?

Leaving it to metadata consistency check is an alternative strategy.

[romax, 2016/11/18 20:50:49]

I lean towards letting consistency check do the work. But let me know if you
have other concerns. The comment is added in offline_page_metadata_store_sql.h.
Let me know if we need a more visible place to have the statement.

+      "access_count, client_namespace, client_id, "
       "online_url, file_path) "
       "SELECT "
       "offline_id, creation_time, file_size, last_access_time, "
-      "access_count, expiration_time, client_namespace, client_id, "
+      "access_count, client_namespace, client_id, "
       "online_url, file_path "
       "FROM temp_" OFFLINE_PAGES_TABLE_NAME;
   return UpgradeWithQuery(db, kSql);
 }
 
 bool UpgradeFrom54(sql::Connection* db) {
   const char kSql[] =
       "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
       " (offline_id, creation_time, file_size, last_access_time, "
-      "access_count, expiration_time, client_namespace, client_id, "

[fgorski, 2016/11/18 00:13:04]

ditto

[romax, 2016/11/18 20:50:49]

Done.

+      "access_count, client_namespace, client_id, "
       "online_url, file_path, title) "
       "SELECT "
       "offline_id, creation_time, file_size, last_access_time, "
-      "access_count, expiration_time, client_namespace, client_id, "
+      "access_count, client_namespace, client_id, "
       "online_url, file_path, title "
       "FROM temp_" OFFLINE_PAGES_TABLE_NAME;
   return UpgradeWithQuery(db, kSql);
 }
 
 bool UpgradeFrom55(sql::Connection* db) {
   const char kSql[] =
       "INSERT INTO " OFFLINE_PAGES_TABLE_NAME
       " (offline_id, creation_time, file_size, last_access_time, "
-      "access_count, expiration_time, client_namespace, client_id, "
-      "online_url, file_path, title) "
+      "access_count, client_namespace, client_id, online_url, "
+      "file_path, title) "
       "SELECT "
       "offline_id, creation_time, file_size, last_access_time, "
-      "access_count, expiration_time, client_namespace, client_id, "
-      "online_url, file_path, title "
+      "access_count, client_namespace, client_id, online_url, "
+      "file_path, title "
       "FROM temp_" OFFLINE_PAGES_TABLE_NAME;
   return UpgradeWithQuery(db, kSql);
 }
 
 bool CreateSchema(sql::Connection* db) {
   // 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())
     return false;
 
   if (!db->DoesTableExist(OFFLINE_PAGES_TABLE_NAME)) {
     if (!CreateOfflinePagesTable(db))
       return false;
   }
 
   // Upgrade section. Details are described in the header file.
-  if (!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time")) {
+  if (!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time") &&

[jianli, 2016/11/18 00:15:48]

M56 table doesn't have expiration_time and title. How do we handle this?

[romax, 2016/11/18 20:50:49]

M56 should have title. It was added in M54

+      !db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "title")) {
     if (!UpgradeFrom52(db))
       return false;
   } else if (!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "title")) {
     if (!UpgradeFrom53(db))
       return false;
   } else if (db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "offline_url")) {
     if (!UpgradeFrom54(db))
       return false;
   } else if (!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "original_url")) {
     if (!UpgradeFrom55(db))
       return false;
   }

[fgorski, 2016/11/18 00:13:04]

your expiration_time check should go here...

it means that all of the above where checked and we must be in M56 or later.

[romax, 2016/11/18 20:50:49]

Done.

 
   // TODO(fgorski): Add indices here.
   return transaction.Commit();
 }
 
 bool DeleteByOfflineId(sql::Connection* db, int64_t offline_id) {
   static const char kSql[] =
       "DELETE FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id=?";
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, offline_id);
@@ skipping 23 matching lines @@
 // Create an offline page item from a SQL result.  Expects complete rows with
 // all columns present.
 OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
   int64_t id = statement->ColumnInt64(0);
   base::Time creation_time =
       base::Time::FromInternalValue(statement->ColumnInt64(1));
   int64_t file_size = statement->ColumnInt64(2);
   base::Time last_access_time =
       base::Time::FromInternalValue(statement->ColumnInt64(3));
   int access_count = statement->ColumnInt(4);
-  base::Time expiration_time =
-      base::Time::FromInternalValue(statement->ColumnInt64(5));
-  ClientId client_id(statement->ColumnString(6), statement->ColumnString(7));
-  GURL url(statement->ColumnString(8));
-  base::FilePath path(GetPathFromUTF8String(statement->ColumnString(9)));
-  base::string16 title = statement->ColumnString16(10);
-  GURL original_url(statement->ColumnString(11));
+  ClientId client_id(statement->ColumnString(5), statement->ColumnString(6));
+  GURL url(statement->ColumnString(7));
+  base::FilePath path(GetPathFromUTF8String(statement->ColumnString(8)));
+  base::string16 title = statement->ColumnString16(9);
+  GURL original_url(statement->ColumnString(10));
 
   OfflinePageItem item(url, id, client_id, path, file_size, creation_time);
   item.last_access_time = last_access_time;
   item.access_count = access_count;
-  item.expiration_time = expiration_time;
   item.title = title;
   item.original_url = original_url;
   return item;
 }
 
 ItemActionStatus 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, "
       "file_size, creation_time, last_access_time, access_count, "
-      "expiration_time, title, original_url)"
+      "title, original_url)"
       " 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);
-  statement.BindString(11, item.original_url.spec());
+  statement.BindString16(9, item.title);
+  statement.BindString(10, item.original_url.spec());
   if (!statement.Run())
     return ItemActionStatus::STORE_ERROR;
   if (db->GetLastChangeCount() == 0)
     return ItemActionStatus::ALREADY_EXISTS;
   return ItemActionStatus::SUCCESS;
 }
 
 bool Update(sql::Connection* db, const OfflinePageItem& item) {
   const char kSql[] =
       "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 = ?, original_url = ?"
+      " access_count = ?, title = ?, original_url = ?"
       " WHERE offline_id = ?";
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   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.BindString(10, item.original_url.spec());
-  statement.BindInt64(11, item.offline_id);
+  statement.BindString16(8, item.title);
+  statement.BindString(9, item.original_url.spec());
+  statement.BindInt64(10, item.offline_id);
   return statement.Run() && db->GetLastChangeCount() > 0;
 }
 
 bool InitDatabase(sql::Connection* db, base::FilePath path) {
   db->set_page_size(4096);
   db->set_cache_size(500);
   db->set_histogram_tag("OfflinePageMetadata");
   db->set_exclusive_locking();
 
   base::File::Error err;
@@ skipping 335 matching lines @@
 
 bool OfflinePageMetadataStoreSQL::CheckDb(const base::Closure& callback) {
   if (!db_.get() || state_ != StoreState::LOADED) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
     return false;
   }
   return true;
 }
 
 }  // namespace offline_pages