The implementation of AndroidCacheDatabase.

chrome/browser/history/android/android_cache_database.cc

Index: chrome/browser/history/android/android_cache_database.cc
diff --git a/chrome/browser/history/android/android_cache_database.cc b/chrome/browser/history/android/android_cache_database.cc
new file mode 100755
index 0000000000000000000000000000000000000000..77edac13c4ad98f34d4e03e4f4a84a3925f6c74c
--- /dev/null
+++ b/chrome/browser/history/android/android_cache_database.cc
@@ -0,0 +1,198 @@
+// Copyright (c) 2012 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 "chrome/browser/history/android/android_cache_database.h"
+
+#include "base/file_util.h"
+#include "sql/statement.h"
+
+using base::Time;
+using base::TimeDelta;
+
+namespace history {
+
+namespace {
+
+int64 ToMilliseconds(const Time& time) {
+  TimeDelta delta = time - Time::UnixEpoch();
+  return delta.InMilliseconds();
+}
+
+}  // namespace.
+
+AndroidCacheDatabase::AndroidCacheDatabase() {

[sky, 2012/03/02 15:59:50]

You should either initialize db_attached_, or nuke it if you don't need it (you
only set it in one place, so I'm not sure if you need it).

[michaelbai, 2012/03/02 20:04:43]

That needs nuked, thanks

On 2012/03/02 15:59:50, sky wrote:

+}
+
+AndroidCacheDatabase::~AndroidCacheDatabase() {
+  if (file_util::PathExists(db_name_)) {

[sky, 2012/03/02 15:59:50]

Don't you need to close first?

[michaelbai, 2012/03/02 20:04:43]

Thank for finding this, I shouldn't delete the file here because the database
needs close/detach first, then remove the file.

But I can't access the GetDB in destructor as it is virtual method, so the best
way seemes delete file in HistoryBackend's destructor after the db closed.

On 2012/03/02 15:59:50, sky wrote:

+     bool result = file_util::Delete(db_name_, false);

[sky, 2012/03/02 15:59:50]

spacing

[michaelbai, 2012/03/02 20:04:43]

Code removed.

On 2012/03/02 15:59:50, sky wrote:

+     DCHECK(result);
+  }
+}
+
+sql::InitStatus AndroidCacheDatabase::InitAndroidCacheDatabase(
+    const FilePath& db_name) {
+  if (!CreateDatabase(db_name))
+    return sql::INIT_FAILURE;
+
+  if (!Attach())
+    return sql::INIT_FAILURE;
+
+  if (!CreateBookmarkCacheTable())
+    return sql::INIT_FAILURE;
+
+  return sql::INIT_OK;
+}
+
+bool AndroidCacheDatabase::AddBookmarkCacheRow(const Time& created_time,
+                                               const Time& last_visit_time,
+                                               URLID url_id) {
+  sql::Statement statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "INSERT INTO android_cache_db.bookmark_cache (created_time, "
+      "last_visit_time, url_id) VALUES (?, ?, ?)"));
+
+  statement.BindInt64(0, ToMilliseconds(created_time));

[sky, 2012/03/02 15:59:50]

Why are you using milliseconds instead of InternalValue like the rest of the
code?

[michaelbai, 2012/03/02 20:04:43]

It because the Android use the milliseconds to query, that's the reason I cache
the time in this table.

On 2012/03/02 15:59:50, sky wrote:

[sky, 2012/03/02 20:51:28]

Can't you convert when necessary? If not, add a comment as to why you use ms
here.

[michaelbai, 2012/03/02 21:16:14]

I can't, as the client could just write a SQL as 'last_visit_time > 1234567'
without using sql arguments, event it uses sql arguements, I don't prefer to try
to know which argument is time.

Added the comment in header file's CreateBookmarkCacheTable


On 2012/03/02 20:51:28, sky wrote:

+  statement.BindInt64(1, ToMilliseconds(last_visit_time));
+  statement.BindInt64(2, url_id);
+
+  if (!statement.Run()) {
+    LOG(ERROR) << GetDB().GetErrorMessage();
+    return 0;
+  }
+
+  return GetDB().GetLastInsertRowId();

[sky, 2012/03/02 15:59:50]

Do you want the return value to be a bool or the ID?

[michaelbai, 2012/03/02 20:04:43]

Return true

On 2012/03/02 15:59:50, sky wrote:

+}
+
+bool AndroidCacheDatabase::ClearAllBookmarkCacheRow() {
+  if (!GetDB().Execute("DELETE FROM android_cache_db.bookmark_cache")) {

[sky, 2012/03/02 15:59:50]

Don't you want a cached statement?

[michaelbai, 2012/03/02 20:04:43]

Done.

+    LOG(ERROR) << GetDB().GetErrorMessage();
+    return false;
+  }
+  return true;
+}
+
+bool AndroidCacheDatabase::CacheBookmarkBits(
+    const std::vector<URLID>& url_ids) {
+  bool has_id = false;
+  std::ostringstream oss;

[sky, 2012/03/02 15:59:50]

Why not just build up a string?

[michaelbai, 2012/03/02 20:04:43]

Did you mean use 'string.append()', if so, I need to convert int64 to string
explicitly, using stream seems simple.

On 2012/03/02 15:59:50, sky wrote:

+  for (std::vector<URLID>::const_iterator i = url_ids.begin();
+      i != url_ids.end(); ++i) {
+    if (has_id)
+      oss << ", ";
+    else
+      has_id = true;
+    oss << *i;
+  }
+
+  if (!has_id)
+    return true;
+
+  std::string sql("UPDATE android_cache_db.bookmark_cache "
+                  "SET bookmark = 1 WHERE url_id in (");
+  sql.append(oss.str());
+  sql.append(")");
+  if (!GetDB().Execute(sql.c_str())) {
+    LOG(ERROR) << GetDB().GetErrorMessage();
+    return false;
+  }
+  return true;
+}
+
+bool AndroidCacheDatabase::CacheFaviconID(URLID url_id, FaviconID favicon_id) {
+  sql::Statement update_statement(GetDB().GetCachedStatement(SQL_FROM_HERE,
+      "UPDATE android_cache_db.bookmark_cache "
+      "SET favicon_id = ? WHERE url_id = ? "));
+
+  update_statement.BindInt64(0, favicon_id);
+  update_statement.BindInt64(1, url_id);
+  if (!update_statement.Run()) {
+    LOG(ERROR) << GetDB().GetErrorMessage();
+    return false;
+  }
+  return true;
+}
+
+bool AndroidCacheDatabase::CreateDatabase(const FilePath& db_name) {
+  db_name_ = db_name;
+  if (file_util::PathExists(db_name_))
+    file_util::Delete(db_name_, false);
+
+  // Using a new connection, otherwise we can not create the database.
+  sql::Connection connection;
+
+  // The db doesn't store too much data, so we don't need that big a page
+  // size or cache.
+  connection.set_page_size(2048);
+  connection.set_cache_size(32);
+
+  // Run the database in exclusive mode. Nobody else should be accessing the
+  // database while we're running, and this will give somewhat improved perf.
+  connection.set_exclusive_locking();
+
+  if (!connection.Open(db_name_)) {
+    LOG(ERROR) << connection.GetErrorMessage();
+    return false;
+  }
+  connection.Close();
+  return true;
+}
+
+bool AndroidCacheDatabase::CreateBookmarkCacheTable() {
+  const char* name = "android_cache_db.bookmark_cache";
+  DCHECK(!GetDB().DoesTableExist(name));
+
+  std::string sql;
+  sql.append("CREATE TABLE ");
+  sql.append(name);
+  sql.append("("
+             "id INTEGER PRIMARY KEY,"
+             "created_time INTEGER NOT NULL,"     // Time in millisecond.
+             "last_visit_time INTEGER NOT NULL,"  // Time in millisecond.
+             "url_id INTEGER NOT NULL,"           // url id in urls table.
+             "favicon_id INTEGER DEFAULT NULL,"   // favicon id.

[sky, 2012/03/02 15:59:50]

Don't you want 0 here?

[michaelbai, 2012/03/02 20:04:43]

No, this is the tricky thing. The client think the favicon is blob, it could
query the all url without favicon by 'WHERE favicon IS NULL', using the NULL
support this query.

Add more comments in header file.

On 2012/03/02 15:59:50, sky wrote:

+             "bookmark INTEGER DEFAULT 0"         // whether is bookmark.
+             ")");
+  if (!GetDB().Execute(sql.c_str())) {
+    LOG(ERROR) << GetDB().GetErrorMessage();
+    return false;
+  }
+
+  sql.assign("CREATE INDEX ");
+  sql.append("android_cache_db.bookmark_cache_url_id_idx ON "
+             "bookmark_cache(url_id)");
+  if (!GetDB().Execute(sql.c_str())) {
+    LOG(ERROR) << GetDB().GetErrorMessage();
+    return false;
+  }
+  return true;
+}
+
+bool AndroidCacheDatabase::Attach() {
+  if (GetDB().transaction_nesting())
+    GetDB().CommitTransaction();

[sky, 2012/03/02 15:59:50]

Document why this is needed.

[michaelbai, 2012/03/02 20:04:43]

Done.

+
+  // Attach AndroidCacheDatabase.
+  {
+    // This block is needed because otherwise the attach statement is
+    // never cleared from cache and we can't close the DB :P
+    std::string sql("ATTACH ? AS android_cache_db");
+    sql::Statement attach(GetDB().GetUniqueStatement(sql.c_str()));
+    if (!attach.is_valid()) {
+      // Keep the transaction open, even though we failed.
+      GetDB().BeginTransaction();

[sky, 2012/03/02 15:59:50]

This code is fragile because of having to open the connection before the early
return. How about moving it to a separate function and having it return an error
status. The caller can then be resposible for beginning the transaction after
calling the method.

[michaelbai, 2012/03/02 20:04:43]

FYI, This code was copied from ThumbnailDatabase. I didn't change it because it
looks mysterious.

On 2012/03/02 15:59:50, sky wrote:

[sky, 2012/03/02 20:51:28]

Now is your chance to fix it;)

+      return false;
+    }
+
+    attach.BindString(0, db_name_.value());
+    if (!attach.Run()) {
+      GetDB().BeginTransaction();
+      return false;
+    }
+  }
+  GetDB().BeginTransaction();
+  db_attached_ = true;
+  return true;
+}
+
+}  // namespace history