Local salient image storage for enhanced bookmark experiment.

components/bookmarks_enhanced/image_store_unittest.cc

+// 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/image_store.h"
+
+#include "base/files/scoped_temp_dir.h"
+#include "components/bookmarks_enhanced/image_store_util.h"
+#include "components/bookmarks_enhanced/memory_image_store.h"
+#include "components/bookmarks_enhanced/persistent_image_store.h"
+#include "testing/platform_test.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "url/gurl.h"
+
+namespace {
+
+const SkBitmap CreateBitmap(int width, int height, int a, int r, int g, int b) {

[noyau (Ping after 24h), 2014/05/05 18:09:50]

The images had randomness in them to make sure that the image will be different
if it is rotated/scaled/resized in any way. Using a flat color image tends to
hide to off by one pixel errors. When the image is not symmetrical in any
dimension, those issues are immediately visible. The iOS test uses an off
centered circular gradient image from one color to another as it's really easy
to do with UIImage.

[Kibeom Kim (inactive), 2014/05/05 18:21:34]

IIRC, I think the original code randomly chose one color to clear. I thought
that should be enough since this doesn't perform any image manipulation. Do you
think it's worth to have a non-symmetrical image for testing? (I can do it)

+  SkBitmap bitmap;
+  bitmap.setConfig(SkBitmap::kARGB_8888_Config, width, height);
+  bitmap.allocPixels();
+  bitmap.eraseARGB(a, r, g, b);
+  return bitmap;
+}
+
+gfx::Image GenerateWhiteImage() {
+  return gfx::Image::CreateFrom1xBitmap(
+      CreateBitmap(42, 24, 255, 255, 255, 255));
+}
+
+gfx::Image GenerateBlackImage() {
+  return gfx::Image::CreateFrom1xBitmap(CreateBitmap(42, 24, 255, 0, 0, 0));
+}
+
+// Returns true if the two images are identical.
+bool CompareImages(const gfx::Image& image_1, const gfx::Image& image_2) {
+  if (image_1.IsEmpty() && image_2.IsEmpty())
+    return true;
+
+  if (image_1.IsEmpty() || image_2.IsEmpty())
+    return false;
+
+  scoped_refptr<base::RefCountedMemory> image_1_png =
+      image_store_util::BytesForImage(image_1);
+  scoped_refptr<base::RefCountedMemory> image_2_png =
+      image_store_util::BytesForImage(image_2);
+
+  if (image_1_png->size() != image_2_png->size())
+    return false;
+
+  return !memcmp(image_1_png->front(),
+                 image_2_png->front(),
+                 image_1_png->size());
+}
+
+// Factory functions for creating instances of the implementations.
+template <class T>
+scoped_refptr<ImageStore> createStore(base::ScopedTempDir& folder);
+
+template <>
+scoped_refptr<ImageStore> createStore<MemoryImageStore>(
+    base::ScopedTempDir& folder) {
+  return scoped_refptr<ImageStore>(new MemoryImageStore());
+}
+
+template <>
+scoped_refptr<ImageStore> createStore<PersistentImageStore>(
+    base::ScopedTempDir& folder) {
+  return scoped_refptr<ImageStore>(new PersistentImageStore(folder.path()));
+}
+
+// Methods to check if persistence is on or not.
+template <class T> bool shouldPersist();
+template <> bool shouldPersist<MemoryImageStore>() { return false; }
+template <> bool shouldPersist<PersistentImageStore>() { return true; }
+
+// Test fixture class template for the abstract API.
+template <class T>
+class ImageStoreUnitTest : public PlatformTest {
+ protected:
+  ImageStoreUnitTest() {
+    bool success = tempDir_.CreateUniqueTempDir();
+    EXPECT_TRUE(success);

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

Seems like this should be an ASSERT. And I would move it to SetUp.

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

Done.

+    store_ = createStore<T>(tempDir_);
+  }
+
+  void TearDown() OVERRIDE {
+    if (store_ && usePersistentStore())
+      store_->ClearAll();
+  }
+
+  bool usePersistentStore() { return shouldPersist<T>(); }
+  void resetStore() { store_ = createStore<T>(tempDir_); }
+  virtual ~ImageStoreUnitTest() {}

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

destructor should be right after constructor.

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

Done.

+  // The directory the database is saved into.
+  base::ScopedTempDir tempDir_;
+  // The object the fixture is testing, via its base interface.
+  scoped_refptr<ImageStore> store_;
+};

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

DISALOW...

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

Done.

+
+// The list of implementations of the abstract API that are going to be tested.
+typedef testing::Types<MemoryImageStore,
+                       PersistentImageStore> Implementations;
+
+TYPED_TEST_CASE(ImageStoreUnitTest, Implementations);
+
+// All those tests are run on all the implementations.
+TYPED_TEST(ImageStoreUnitTest, startsEmpty) {
+  std::vector<GURL> all_urls;
+  this->store_->GetAllPageUrls(&all_urls);
+  EXPECT_EQ(0ul, all_urls.size());

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

You shouldn't need the 'l' in any of these, '0u' should be fine.

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

Done.

+}
+
+TYPED_TEST(ImageStoreUnitTest, storeOne) {
+  this->store_->Insert(GURL("foo://bar"), GURL("a.jpg"), GenerateBlackImage());
+
+  std::vector<GURL> all_urls;
+  this->store_->GetAllPageUrls(&all_urls);
+  EXPECT_EQ(1ul, all_urls.size());
+  EXPECT_EQ(GURL("foo://bar"), all_urls[0]);
+  EXPECT_TRUE(this->store_->HasKey(GURL("foo://bar")));
+}
+
+TYPED_TEST(ImageStoreUnitTest, retrieve) {
+  gfx::Image src_image = GenerateBlackImage();
+  const GURL url("foo://bar");
+  const GURL image_url("a.jpg");
+  this->store_->Insert(url, image_url, src_image);
+  GURL result;
+  gfx::Image dst_image = this->store_->Get(url, &result);
+
+  EXPECT_EQ(image_url, result);
+  EXPECT_TRUE(CompareImages(src_image, dst_image));
+}
+
+TYPED_TEST(ImageStoreUnitTest, erase) {
+  gfx::Image src_image = GenerateBlackImage();
+  const GURL url("foo://bar");
+  const GURL image_url("a.jpg");
+  this->store_->Insert(url, image_url, src_image);
+  this->store_->Erase(url);
+
+  EXPECT_FALSE(this->store_->HasKey(url));
+  std::vector<GURL> all_urls;
+  this->store_->GetAllPageUrls(&all_urls);
+  EXPECT_EQ(0ul, all_urls.size());
+}
+
+TYPED_TEST(ImageStoreUnitTest, update) {
+  gfx::Image src_image1 = GenerateWhiteImage();
+  gfx::Image src_image2 = GenerateBlackImage();
+  const GURL url("foo://bar");
+  const GURL image_url1("1.jpg");
+  this->store_->Insert(url, image_url1, src_image1);
+
+  const GURL image_url2("2.jpg");
+  this->store_->Insert(url, image_url2, src_image2);
+
+  GURL result;
+  gfx::Image dst_image = this->store_->Get(url, &result);
+
+  EXPECT_TRUE(this->store_->HasKey(url));
+  std::vector<GURL> all_urls;
+  this->store_->GetAllPageUrls(&all_urls);
+  EXPECT_EQ(1ul, all_urls.size());
+  EXPECT_EQ(image_url2, result);
+  EXPECT_TRUE(CompareImages(src_image2, dst_image));
+}
+
+TYPED_TEST(ImageStoreUnitTest, persistence) {
+  gfx::Image src_image = GenerateBlackImage();
+  const GURL url("foo://bar");
+  const GURL image_url("a.jpg");
+  this->store_->Insert(url, image_url, src_image);
+
+  this->resetStore();
+  if (this->usePersistentStore()) {
+    std::vector<GURL> all_urls;
+    this->store_->GetAllPageUrls(&all_urls);
+    EXPECT_EQ(1ul, all_urls.size());
+    EXPECT_EQ(GURL("foo://bar"), all_urls[0]);
+    EXPECT_TRUE(this->store_->HasKey(GURL("foo://bar")));
+    GURL result;
+    gfx::Image dst_image = this->store_->Get(url, &result);
+
+    EXPECT_EQ(image_url, result);
+    EXPECT_TRUE(CompareImages(src_image, dst_image));
+  } else {
+    std::vector<GURL> all_urls;
+    this->store_->GetAllPageUrls(&all_urls);
+    EXPECT_EQ(0ul, all_urls.size());
+    EXPECT_FALSE(this->store_->HasKey(GURL("foo://bar")));
+  }
+}
+
+}  // namespace