Local salient image storage for enhanced bookmark experiment.

components/bookmarks_enhanced/persistent_image_store.cc

Index: components/bookmarks_enhanced/persistent_image_store.cc
diff --git a/components/bookmarks_enhanced/persistent_image_store.cc b/components/bookmarks_enhanced/persistent_image_store.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ddb4d2689ef795d3191017e4753c6da629905378
--- /dev/null
+++ b/components/bookmarks_enhanced/persistent_image_store.cc
@@ -0,0 +1,197 @@
+// Copyright 2014 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/bookmarks_enhanced/persistent_image_store.h"
+
+#include "components/bookmarks_enhanced/image_store_util.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+#include "ui/gfx/geometry/size.h"
+#include "url/gurl.h"
+
+namespace {
+
+bool InitTables(sql::Connection& db) {
+  const char kTableSql[] =
+      "CREATE TABLE IF NOT EXISTS images_by_url ("
+      "  page_url LONGVARCHAR NOT NULL,"
+      "  image_url LONGVARCHAR NOT NULL,"
+      "  image_data BLOB,"
+      "  width INTEGER,"
+      "  height INTEGER"
+      ")";
+  if (!db.Execute(kTableSql))
+    return false;
+  return true;
+}
+
+bool InitIndices(sql::Connection& db) {
+  const char kIndexSql[] =
+      "CREATE INDEX IF NOT EXISTS images_by_url_idx ON images_by_url(page_url)";
+  if (!db.Execute(kIndexSql))
+    return false;
+  return true;
+}
+
+sql::InitStatus OpenDatabaseImpl(sql::Connection& db,
+                                 const base::FilePath& db_path) {
+  DCHECK(!db.is_open());
+
+  db.set_histogram_tag("BookmarkImages");
+  // TODO(noyau): Set page and cache sizes?
+  // TODO(noyau): Set error callback?
+
+  // Run the database in exclusive mode. Nobody else should be accessing the
+  // database while we're running, and this will give somewhat improved perf.
+  db.set_exclusive_locking();
+
+  if (!db.Open(db_path))
+    return sql::INIT_FAILURE;
+
+  // Scope initialization in a transaction so we can't be partially initialized.
+  sql::Transaction transaction(&db);
+  if (!transaction.Begin())
+    return sql::INIT_FAILURE;
+
+  // Create the tables.
+  if (!InitTables(db) ||
+      !InitIndices(db)) {
+    return sql::INIT_FAILURE;
+  }
+
+  // Initialization is complete.
+  if (!transaction.Commit())
+    return sql::INIT_FAILURE;
+
+  return sql::INIT_OK;
+}
+
+}  // namespace
+
+PersistentImageStore::PersistentImageStore(const base::FilePath& path)
+    : ImageStore(), path_(path.Append("BookmarkImageAndUrlStore.db")) {}
+
+PersistentImageStore::~PersistentImageStore() {

[sky, 2014/05/06 16:14:20]

Order should match header.

[Kibeom Kim (inactive), 2014/05/07 00:09:46]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+bool PersistentImageStore::HasKey(const GURL& page_url) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  OpenDatabase();
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "SELECT COUNT(*) FROM images_by_url WHERE page_url = ?"));
+  statement.BindString(0, page_url.possibly_invalid_spec());
+
+  int count = statement.Step() ? statement.ColumnInt(0) : 0;
+
+  return !!count;
+}
+
+void PersistentImageStore::Insert(const GURL& page_url,
+                                  const GURL& image_url,
+                                  const gfx::Image& image) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  Erase(page_url);  // Remove previous image for this url, if any.
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "INSERT INTO images_by_url "
+      "    (page_url, image_url, image_data, width, height)"
+      "    VALUES (?, ?, ?, ?, ?)"));
+
+  statement.BindString(0, page_url.possibly_invalid_spec());
+  statement.BindString(1, image_url.possibly_invalid_spec());
+
+  scoped_refptr<base::RefCountedMemory> data =
+      image_store_util::BytesForImage(image);
+  statement.BindBlob(2, data->front(), data->size());
+
+  statement.BindInt64(3, image.Size().width());
+  statement.BindInt64(4, image.Size().height());
+  statement.Run();
+}
+
+void PersistentImageStore::Erase(const GURL& page_url) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  OpenDatabase();
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "DELETE FROM images_by_url WHERE page_url = ?"));
+  statement.BindString(0, page_url.possibly_invalid_spec());
+  statement.Run();
+}
+
+gfx::Image PersistentImageStore::Get(const GURL& page_url, GURL* image_url) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(image_url);
+
+  OpenDatabase();
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "SELECT image_data, image_url FROM images_by_url WHERE page_url = ?"));
+
+  statement.BindString(0, page_url.possibly_invalid_spec());
+
+  while (statement.Step()) {
+    if (statement.ColumnByteLength(0) > 0) {
+      scoped_refptr<base::RefCountedBytes> data(new base::RefCountedBytes());
+      statement.ColumnBlobAsVector(0, &data->data());
+      *image_url = GURL(statement.ColumnString(1));
+      return image_store_util::ImageForBytes(data);
+    }
+  }
+  return gfx::Image();
+}
+
+gfx::Size PersistentImageStore::GetSize(const GURL& page_url) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  OpenDatabase();
+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "SELECT width, height FROM images_by_url WHERE page_url = ?"));
+
+  statement.BindString(0, page_url.possibly_invalid_spec());
+
+  while (statement.Step()) {
+    if (statement.ColumnByteLength(0) > 0) {
+      int64 width = statement.ColumnInt64(0);

[sky, 2014/05/06 16:14:20]

Why the int64? gfx::Size is only int.

[Kibeom Kim (inactive), 2014/05/07 00:09:46]

Done. Also added GetSize() check in TYPED_TEST(ImageStoreUnitTest, retrieve).

Eric, could you confirm this is ok? I ran the above test with, saving by
|statement.BindInt64(3, image.Size().width());| and reading by
|statement.ColumnInt(0);| and it passed the test. So I think this change should
be fine with an existing sql file.

+      int64 height = statement.ColumnInt64(1);
+      return gfx::Size(width, height);
+    }
+  }
+  return gfx::Size();
+}
+
+void PersistentImageStore::GetAllPageUrls(std::vector<GURL>* urls) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(urls->empty());
+
+  OpenDatabase();

[sky, 2014/05/06 16:14:20]

Shouldn't you check the status every where?

[Kibeom Kim (inactive), 2014/05/07 00:09:46]

Actually, we just crash on OpenDatabase failure (#195) so I changed the return
type to void.

[sky, 2014/05/08 15:33:08]

Isn't it possible that openning the database can fail? Why do you want to crash?

[Kibeom Kim (inactive), 2014/05/08 17:56:33]

Done. (I will add failure returns for public functions as caller needs them in
the future)

[Scott Hess - ex-Googler, 2014/05/09 17:51:36]

Random peanut-gallery comment: When adding in error returns, be mindful of what
you expect the caller to do.  A common pattern our code unfortunately follows is
to delegate decisions to the caller, who delegates to their caller, who has no
idea what to do and the database fails to open forever.  Stuff stored on disk
breaks because of external factors, and when you have hundreds of millions of
users you just have to deal with it.

+  sql::Statement statement(db_.GetCachedStatement(SQL_FROM_HERE,
+      "SELECT page_url FROM images_by_url"));
+  while (statement.Step())
+    urls->push_back(GURL(statement.ColumnString(0)));
+}
+
+sql::InitStatus PersistentImageStore::OpenDatabase() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (db_.is_open())
+    return sql::INIT_OK;
+
+  const size_t kAttempts = 2;
+
+  sql::InitStatus status = sql::INIT_FAILURE;
+  for (size_t i = 0; i < kAttempts; ++i) {
+    status = OpenDatabaseImpl(db_, path_);
+    if (status == sql::INIT_OK)
+      return status;
+
+    // Can't open, raze().
+    if (db_.is_open())
+      db_.Raze();
+    db_.Close();
+  }
+  CHECK(false) << "Can't open image DB";
+  return sql::INIT_FAILURE;
+}