Adds support for saving favicon size into history database.

chrome/browser/history/history_types.h

Index: chrome/browser/history/history_types.h
diff --git a/chrome/browser/history/history_types.h b/chrome/browser/history/history_types.h
index 2408d4c5165a3c88034330705098abca9486ec2b..9f2eb9fb8681166c21455171f2e37bd6cb61b3fb 100644
--- a/chrome/browser/history/history_types.h
+++ b/chrome/browser/history/history_types.h
@@ -274,23 +274,6 @@ typedef std::vector<VisitRow> VisitVector;
 // used by HistoryBackend::AddVisits() to create new visits for a URL.
 typedef std::pair<base::Time, content::PageTransition> VisitInfo;
 
-// Favicons -------------------------------------------------------------------
-
-// Used by the importer to set favicons for imported bookmarks.
-struct ImportedFaviconUsage {
-  ImportedFaviconUsage();
-  ~ImportedFaviconUsage();
-
-  // The URL of the favicon.
-  GURL favicon_url;
-
-  // The raw png-encoded data.
-  std::vector<unsigned char> png_data;
-
-  // The list of URLs using this favicon.
-  std::set<GURL> urls;
-};
-
 // PageVisit ------------------------------------------------------------------
 
 // Represents a simplified version of a visit for external users. Normally,
@@ -768,32 +751,109 @@ struct IconMapping {
   // The unique id of the icon.
   FaviconID icon_id;
 
+  // The url of the icon.
+  GURL icon_url;
+
   // The type of icon.
   IconType icon_type;
 };
 
+// Sizes represents the sizes that the thumbnail database knows a favicon is

[sky, 2012/08/21 20:16:56]

Sizes -> FaviconSizes

+// available at from the web. This can be different from the pixel sizes of
+// the entries in the favicon_bitmaps table for a favicon. For instance, if
+// a web page has a .ico favicon with bitmaps of pixel sizes
+// (16x16, 24x24, 32x32), FaviconSizes::has_size will be true for all three
+// pixel sizes regardless of whether bitmaps for all three pixel sizes are
+// stored in the favicon_bitmaps database table. However, the pixel sizes of
+// favicon bitmaps stored for a FaviconID must be a subset of the FaviconSizes
+// for the favicon.
+// FaviconSizes intentionally supports copy and assign.
+class FaviconSizes {

[sky, 2012/08/21 20:16:56]

I think this code should use std::set<gfx::Size> and the functions that
encode/decode the string should be else where.

+ public:
+  FaviconSizes();
+  FaviconSizes(const std::string& string_representation);

[sky, 2012/08/21 20:16:56]

explicit on these two. Do we need so many constructors?

+  FaviconSizes(const std::vector<gfx::Size>& vector_representation);
+  ~FaviconSizes();
+
+  // Inserts size to the list of sizes supported by the favicon.
+  void InsertSize(const gfx::Size& size);
+
+  // Returns true if |size| is supported by the favicon.
+  bool has_size(const gfx::Size& size) const {
+    return sizes_.find(size) != sizes_.end();
+  }
+
+  size_t num_sizes() const { return sizes_.size(); }
+
+  // Returns string representation of sizes.
+  // Format:
+  //   Each widthxheight pair is separated by a space.
+  //   Width and height are separated by a space.
+  // For instance, if sizes contains pixel sizes (16x16, 32x32), the
+  // string representation is "16 16 32 32".
+  std::string ToString() const;
+
+  // Returns vector representation of sizes.
+  std::vector<gfx::Size> ToVector() const;
+
+ private:
+  std::set<gfx::Size> sizes_;
+};
+
+// Defines a favicon bitmap stored in the history backend.
+struct FaviconDataElement {

[sky, 2012/08/21 20:16:56]

How about naming this FaviconDataBitmap?

+  FaviconDataElement();
+  ~FaviconDataElement();
+
+  // The bits of the bitmap.
+  scoped_refptr<base::RefCountedMemory> bitmap_data;
+
+  // The pixel dimensions of |bitmap_data|.
+  gfx::Size pixel_size;
+
+  // The icon's URL.
+  GURL icon_url;
+};
+
+// Mapping of icon URL to the sizes the favicon is available at from the web.
+typedef std::map<GURL, FaviconSizes> IconURLSizesMap;
+
 // Defines the favicon stored in history backend.
 struct FaviconData {
   FaviconData();
   ~FaviconData();
 
-  // Returns true if the icon is known and image has data.
+  // Returns true if the icon is known, and all |elements| have valid bitmaps.
   bool is_valid();
 
   // Indicates whether the icon is known by the history backend.
   bool known_icon;
 
-  // The bits of image.
-  scoped_refptr<base::RefCountedMemory> image_data;
-
-  // Indicates whether image is expired.
+  // Indicates whether at least one of the bitmaps in |elements| is expired.
   bool expired;

[sky, 2012/08/21 20:16:56]

Shouldn't expired and icon_url_sizes be moved to another structore that has the
FaviconDataElement? Something like:

struct FaviconBitmapData {
  gfx::Size size;
  scoped_refptr<base::RefCountedMemory> bitmap_data;
};

struct FaviconURL {
  GURL url;
  bool expired;
  history::IconType icon_type;
  std::vector<gfx::Size> sizes;
  std::vector<FaviconBitmapData> bitmaps;
};

struct FaviconData {
  std::vector<FaviconURL> urls;
};

[pkotwicz, 2012/08/22 15:32:10]

Thanks for the suggestion Scott. From looking at select_favicon_frames and
talking to Nico briefly it looks like only a single FaviconURL in FaviconData
will ever have valid bitmap data.
I was wondering whether it would be possible to simplify FaviconData to
something like this:

struct FaviconData {
  // Result portion of FaviconData
  GURL url;
  history::IconType icon_type;
  std::vector<gfx::Size> sizes;
  std::vector<FaviconBitmapData> bitmaps;

  // Data to use in FaviconHandler::DoUrlAndIconMatch
  std::vector<GURL> matched_icon_urls;
};

What do you think?

 
-  // The icon's URL.
-  GURL icon_url;
-
   // The type of favicon.
   history::IconType icon_type;
+
+  // Each entry in |elements| represents a favicon bitmap at a different pixel
+  // size.
+  std::vector<FaviconDataElement> elements;
+
+  // Mapping of icon URL to the sizes that the favicon at the icon URL is
+  // available at from the web.
+  IconURLSizesMap icon_url_sizes;

[sky, 2012/08/21 20:16:56]

sizes?

+};
+
+// Defines a favicon bitmap and its associated pixel size.
+struct FaviconBitmapIDSize {
+  FaviconBitmapIDSize();
+  ~FaviconBitmapIDSize();
+
+  // The unique id of the favicon bitmap.
+  FaviconBitmapID bitmap_id;
+
+  // The pixel dimensions of the associated bitmap.
+  gfx::Size pixel_size;
 };
 
 // Defines a favicon bitmap stored in the history backend.
@@ -817,6 +877,21 @@ struct FaviconBitmap {
   gfx::Size pixel_size;
 };
 
+// Used by the importer to set favicons for imported bookmarks.
+struct ImportedFaviconUsage {
+  ImportedFaviconUsage();
+  ~ImportedFaviconUsage();
+
+  // The URL of the favicon.
+  GURL favicon_url;
+
+  // The raw png-encoded data.
+  std::vector<unsigned char> png_data;
+
+  // The list of URLs using this favicon.
+  std::set<GURL> urls;
+};
+
 // Abbreviated information about a visit.
 struct BriefVisitInfo {
   URLID url_id;