Local salient image storage for enhanced bookmark experiment.

chrome/browser/bookmarks/enhanced/image_store.h

Index: chrome/browser/bookmarks/enhanced/image_store.h
diff --git a/chrome/browser/bookmarks/enhanced/image_store.h b/chrome/browser/bookmarks/enhanced/image_store.h
new file mode 100644
index 0000000000000000000000000000000000000000..550ccc02a39138d7e91cd4c68598919707b01dc3
--- /dev/null
+++ b/chrome/browser/bookmarks/enhanced/image_store.h
@@ -0,0 +1,108 @@
+// 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.
+#ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_H_

[sky, 2014/04/30 18:09:13]

Shouldn't all this code live in components now?

[sky, 2014/04/30 18:09:13]

nit: newline between 3/4.

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+#define CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_H_
+
+#include <map>
+#include <vector>
+
+#include "base/files/file_path.h"

[sky, 2014/04/30 18:09:13]

nit: forward declare as much as you can.

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+#include "sql/connection.h"
+#include "sql/init_status.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/image/image.h"
+#include "url/gurl.h"
+
+// The ImageStore keeps an image for each URL.
+class ImageStore : public base::RefCounted<ImageStore> {

[sky, 2014/04/30 18:09:13]

What thread is all this code used from?

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Thanks for pointing this out!

ImageStore will be used only in BookmarkImageService class
https://chrome-internal.googlesource.com/bling/chromium/+/master/ios_internal...

On Android, we construct and destruct BookmarkImageService on UI thread which
has |scoped_refptr<ImageStore> store_;| as a member variable, and all the
function calls to |store_| will be by calling
content::BrowserThread::PostBlockingPoolSequencedTask(kSequenceToken, ...) on UI
thread. So potential collision would be a function call on kSequenceToken thread
and BookmarkImageService's destructor on UI thread. But BookmarkImageService is
KeyedService, and on Android, we don't delete it for regular profile, but only
delete for incognito profile (discussed with tedchoc@). Since
BookmarkImageService won't be used with incognito profile, so I think
technically it doesn't go wrong. But I think it's better to make it thread safe
because that relies on many subtle assumptions. noyau@ : is this correct?

[sky, 2014/05/01 20:11:56]

Doesn't that mean you could potentially be destroying on the UI thread? Which
would invoke closing a db on the UI thread?

+ public:
+  ImageStore() {}
+
+  // Returns true if there is an image for this url.
+  virtual bool HasKey(const GURL& page_url) = 0;
+
+  // Inserts an image and its url in the store for the the given page url. The
+  // image can be null indicating that the download of the image at this URL
+  // failed previously.
+  virtual void Insert(const GURL& page_url,
+                      const GURL& image_url,
+                      const gfx::Image& image) = 0;
+
+  // Removes an image from the store.
+  virtual void Erase(const GURL& page_url) = 0;
+
+  // Returns the image associated with this url. Returns nil if there are no
+  // image for this url. It also returns the imageUrl where the image was

[sky, 2014/04/30 18:09:13]

imageUrl->image_url

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+  // downloaded from or failed to be downloaded from.
+  virtual gfx::Image Get(const GURL& page_url, GURL& image_url) = 0;

[sky, 2014/04/30 18:09:13]

style guide says all refs should be const. If you need to have an out param it
should be a pointer.

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+
+  // Returns the size of the image stored for this URL or CGSizeZero if no

[sky, 2014/04/30 18:09:13]

CGSizeZero->empty size

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+  // images are present.
+  virtual gfx::Size GetSize(const GURL& page_url) = 0;
+
+  // Returns all the urls that have an image in the store.
+  virtual std::vector<GURL> AllKeys() = 0;

[sky, 2014/04/30 18:09:13]

From the name it isn't clear if this is intended to be all the imageurls, or all
the pageurls. Change name to better reflect that. Maybe should like
GetAllPageURLs. Also, I think this should take a std::vector<GURL>*

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+
+  // Moves an image from one url to another.
+  virtual void ChangeImageURL(const GURL& from, const GURL& to);
+
+  // Removes all images.
+  void ClearAll();
+
+ protected:
+  virtual ~ImageStore() {}
+ private:

[sky, 2014/04/30 18:09:13]

nit: newline between 54/55.

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+  friend class base::RefCounted<ImageStore>;
+
+  DISALLOW_COPY_AND_ASSIGN(ImageStore);

[sky, 2014/04/30 18:09:13]

I don't believe this is necessary for a pure virtual class.

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+};
+
+// The MemoryImageStore is an implementation of ImageStore that keeps all its
+// data in memory. When deallocated all the associations are lost.
+// Used in tests.
+class MemoryImageStore : public ImageStore {

[sky, 2014/04/30 18:09:13]

Move these into their own files.

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+ public:
+  MemoryImageStore();
+  virtual bool HasKey(const GURL& page_url) OVERRIDE;
+  virtual void Insert(const GURL& page_url,
+                      const GURL& image_url,
+                      const gfx::Image& image) OVERRIDE;
+  virtual void Erase(const GURL& page_url) OVERRIDE;
+  virtual gfx::Image Get(const GURL& page_url, GURL& image_url) OVERRIDE;
+  virtual gfx::Size GetSize(const GURL& page_url) OVERRIDE;
+  virtual std::vector<GURL> AllKeys() OVERRIDE;
+ protected:

[sky, 2014/04/30 18:09:13]

nit: newline before new section headers (except for public).

[Kibeom Kim (inactive), 2014/05/01 19:02:26]

Done.

+  virtual ~MemoryImageStore();
+ private:
+  typedef std::map<const GURL, std::pair<gfx::Image, const GURL> > ImageMap;
+  ImageMap store_;
+  DISALLOW_COPY_AND_ASSIGN(MemoryImageStore);
+};
+
+// The PersistentImageStore is an implementation of ImageStore that persists its
+// data on disk.
+class PersistentImageStore : public ImageStore {
+ public:
+  // Creates a PersistentImageStore in the directory at the given path.
+  explicit PersistentImageStore(const base::FilePath& path);
+  virtual bool HasKey(const GURL& page_url) OVERRIDE;
+  virtual void Insert(const GURL& page_url,
+                      const GURL& image_url,
+                      const gfx::Image& image) OVERRIDE;
+  virtual void Erase(const GURL& page_url) OVERRIDE;
+  virtual gfx::Image Get(const GURL& page_url, GURL& image_url) OVERRIDE;
+  virtual gfx::Size GetSize(const GURL& page_url) OVERRIDE;
+  virtual std::vector<GURL> AllKeys() OVERRIDE;
+ protected:
+  virtual ~PersistentImageStore();
+ private:
+  sql::InitStatus OpenDatabase();
+  bool InitTables();
+  bool InitIndices();
+  const base::FilePath path_;
+  sql::Connection db_;
+  DISALLOW_COPY_AND_ASSIGN(PersistentImageStore);
+};
+
+#endif  // CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_H_