Changes favicon database to support storing bitmaps of different sizes for the same icon_url

Changes favicon database to support storing bitmaps of different sizes for the same icon_url.

This moves the favicon bitmap data to a new table: favicon_frames.
The favicon_frame table contains columns:
icon_id
bitmap_data
last_updated
width
height

For favicons of type .ico, a single entry in the favicons table will have multiple associated frames.
For favicons of other image types, there should be a single frame for each favicon id.

Bug=138553
Test=ThumbnailDatabaseTest.*, HistoryBackendTest.* pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152367

[sky, 2012-07-24 04:38:47]

http://codereview.chromium.org/10815068/diff/2001/chrome/browser/history/thum...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/10815068/diff/2001/chrome/browser/history/thum...
chrome/browser/history/thumbnail_database.cc:236: "size INTEGER)");
Doesn't this assume the size is a rectangle?

[pkotwicz, 2012-07-24 16:05:57]

The idea is that
FaviconHandler::preferred_icon_size() * scale_factor == size in the favicon
database table.

I think that this is sufficient for all current uses of the thumbnail database.
For favicons of type FAVICON, we resize the favicon to be square before sending
it over to the history service.
For other types, we would be inserting the favicon with a size of 0 as the
favicon is unresized and FaviconHandler::preferred_icon_size == 0.
From briefly talking to michaelbai@, it seems that a single
TOUCH_ICON/TOUCH_PRECOMPOSED_ICON is sufficient.

We will need to do something better (Have an additional database table) if we
want to add favicons of sizes other than gfx::kFaviconSize * scale_factor into
the database in order to avoid storing duplicate bitmaps.
However, I am unsure as to the need for large favicons. And I would rather do it
in a separate patch.

[sky, 2012-07-24 16:34:31]

The value in the table should correspond to pixel size, and making it
width x height string gives us more flexibility. We already know folks
want richer icons here, we shouldn't artificially constrain this.

  -Scott

On Tue, Jul 24, 2012 at 9:05 AM,  <pkotwicz@chromium.org> wrote:
> The idea is that
> FaviconHandler::preferred_icon_size() * scale_factor == size in the favicon
> database table.
>
> I think that this is sufficient for all current uses of the thumbnail
> database.
> For favicons of type FAVICON, we resize the favicon to be square before
> sending
> it over to the history service.
> For other types, we would be inserting the favicon with a size of 0 as the
> favicon is unresized and FaviconHandler::preferred_icon_size == 0.
> From briefly talking to michaelbai@, it seems that a single
> TOUCH_ICON/TOUCH_PRECOMPOSED_ICON is sufficient.
>
> We will need to do something better (Have an additional database table) if
> we
> want to add favicons of sizes other than gfx::kFaviconSize * scale_factor
> into
> the database in order to avoid storing duplicate bitmaps.
> However, I am unsure as to the need for large favicons. And I would rather
> do it
> in a separate patch.
>
> http://codereview.chromium.org/10815068/

[pkotwicz, 2012-07-25 19:49:27]

Changes as requested. I am keeping this CL simple.
I will add a separate table as we discussed yesterday in a separate CL in short
order

[pkotwicz, 2012-08-02 18:25:57]

Changes as per Scott's suggestions.
I am keeping this CL as small as possible.
Thus, the thumbnail_database API is missing a number of functions.

[sky, 2012-08-02 19:50:31]

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/his...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2339: if
(!thumbnail_db_->InitTemporaryFaviconFramesTable())
I think it less error prone and easier to maintain if ThumbnailDatabase exposes
a single InitTemporaryTables method that takes care of all the necessary tables.

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:29: static void
FillIconMapping(const sql::Statement& statement,
Add a comment here detailing the tables that are created here and how they
relate. See chrome/browser/webdata/autofill_table.h for an example of what I'm
looking for.

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:515: success &= statement.Run();
If this fails, shouldn't this return?

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:702: "SELECT icon_id, last_updated,
image_data, width, height "
This is inserting the old id when it needs to insert the new id. Additionally
it's querying using 'id' when it should be using icon_id. Make sure you have
test coverage for this.

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:727: // Init favicons table - same
schema as the thumbnails.
tables

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:883: "SELECT id, last_updated,
image_data FROM favicons") &&
You need to update width/height here too.

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database.h (right):

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:108: bool AddFaviconFrame(FaviconID
icon_id,
Can you use Image or Bitmap instead of Frame in all of these? Including the
table name -> favicon_images.

This function needs to take a gfx::Size for the size of the image.

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:137: bool GetFaviconFrame(FaviconID
icon_id,
Won't this need to take something that returns a vector of png_icon_data and
sizes and probably the sizes string?

[sky, 2012-08-02 19:57:01]

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database_unittest.cc (right):

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database_unittest.cc:374:
EXPECT_EQ(std::string(""), statement.ColumnString(3));
no ""

http://codereview.chromium.org/10815068/diff/12002/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database_unittest.cc:376: // Query width and
height to ensure that the columns exist.
Can you also verify the size is set to 16x16 in the db after migration.

[pkotwicz, 2012-08-15 00:49:02]

Changes as requested. I will upload the followup CL later today / early morning
tomorrow

[pkotwicz, 2012-08-15 20:47:51]

Updated this CL as per Scott's comments. Have also uploaded the followup patch
at http://codereview.chromium.org/10802066/

[sky, 2012-08-16 16:12:57]

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/his...
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/his...
chrome/browser/history/history_types.h:801: // The unique id of the bitmap.
Add constructor/destructor and put implementations in .cc file. Make sure
bitmap_id and icon_id are initializes to 0.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:235: "sizes LONGVARCHAR DEFAULT '0
0')");
why 0 0 as a default?

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:412: bool result = false;
Why do we need a return value here? Can't they check the size of
favicon_bitmaps?

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:466: "UPDATE favicon_bitmaps SET
image_data=?, last_updated=? "
This might reset multiple rows. Shouldn't the where be on id? Or if you really
need icon_id, shouldn't this remove then insert so that we know there is only
one row?

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:528: if (favicon_bitmaps.size() ==
0)
empty

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:747: 
DCHECK_EQ(arraysize(main_tables), arraysize(temporary_tables))

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:965: "SELECT id, last_updated,
image_data FROM favicons") &&
Don't you need to set the width/height?

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:969: "icon_type INTEGER DEFAULT 1,"
Document what 1 is here.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:970: "sizes LONGVARCHAR DEFAULT '0
0')") &&
Why default to 0x0?

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:976:
meta_table_.SetVersionNumber(6);
Early return if execute fails on the previous lines.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database.h (right):

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:44: // Description of tables:
Move this to the .cc since its an implementation detail.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:49: //   icon_id
Document icon_id too.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:52: //                    |page_url|
in the |icon_mapping| table. This should
Not 'should', but does. Eg This is the defualt favicon plus any ...

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:56: //                    The |id|
should match the |icon_id| field in the
Same here, 'The id matches the icon_id field...

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:79: //                    Note that
it is not necessary for there to be a row for
There is not necessarily a row for each size specified in the sizes attribute.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database_unittest.cc (right):

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database_unittest.cc:472:
TEST_F(ThumbnailDatabaseTest, DeleteFavicon) {
Add a short description for tests.

[pkotwicz, 2012-08-16 19:22:46]

Changes as requested.
Also made icon_id field in favicon_bitmaps NOT NULL

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:235: "sizes LONGVARCHAR DEFAULT '0
0')");
The default is 0x0 because it is a very recognizable value.
It is very clear that 0x0 is not the size of the bitmap whereas any other value
can be mistaken as such.
In http://codereview.chromium.org/10802066/, we store all bitmaps at 0x0 so that
we only put a single bitmap per favicon into the database. Using 0x0 will make
transitioning into a state where we store the real bitmap size easy.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:412: bool result = false;
We use return values in ThumbnailDatabase so that we can be lazy in unit tests.
In particular some unit tests do this:
EXPECT_FALSE(thumbnail_db_.GetFaviconBitmaps(icon_id, NULL)).
This is consistent with what we do for ThumbnailDatabase::GetIconMappings()

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:466: "UPDATE favicon_bitmaps SET
image_data=?, last_updated=? "
Changed this to remove and insert as you suggested.
Yes, we should be passing in and using FaviconBitmapID. This is what the
followup CL does.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:528: if (favicon_bitmaps.size() ==
0)
On 2012/08/16 16:12:57, sky wrote:
> empty

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:747: 
On 2012/08/16 16:12:57, sky wrote:
> DCHECK_EQ(arraysize(main_tables), arraysize(temporary_tables))

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:965: "SELECT id, last_updated,
image_data FROM favicons") &&
On 2012/08/16 16:12:57, sky wrote:
> Don't you need to set the width/height?

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:969: "icon_type INTEGER DEFAULT 1,"
On 2012/08/16 16:12:57, sky wrote:
> Document what 1 is here.

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:970: "sizes LONGVARCHAR DEFAULT '0
0')") &&
The default is 0x0 because it is a very recognizable value.
It is very clear that 0x0 is not the size of the bitmap whereas any other value
can be mistaken as such.
In http://codereview.chromium.org/10802066/, we store all bitmaps at 0x0 so that
we only put a single bitmap per favicon into the database. Using 0x0 will make
transitioning into a state where we store the real bitmap size easy.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:976:
meta_table_.SetVersionNumber(6);
On 2012/08/16 16:12:57, sky wrote:
> Early return if execute fails on the previous lines.

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database.h (right):

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:44: // Description of tables:
On 2012/08/16 16:12:57, sky wrote:
> Move this to the .cc since its an implementation detail.

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:49: //   icon_id
On 2012/08/16 16:12:57, sky wrote:
> Document icon_id too.

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:52: //                    |page_url|
in the |icon_mapping| table. This should
On 2012/08/16 16:12:57, sky wrote:
> Not 'should', but does. Eg This is the defualt favicon plus any ...

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:56: //                    The |id|
should match the |icon_id| field in the
On 2012/08/16 16:12:57, sky wrote:
> Same here, 'The id matches the icon_id field...

Done.

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:79: //                    Note that
it is not necessary for there to be a row for
On 2012/08/16 16:12:57, sky wrote:
> There is not necessarily a row for each size specified in the sizes attribute.

Done.

[sky, 2012-08-16 20:01:41]

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
File chrome/browser/history/thumbnail_database.cc (right):

http://codereview.chromium.org/10815068/diff/10004/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:235: "sizes LONGVARCHAR DEFAULT '0
0')");
On 2012/08/16 19:22:46, pkotwicz wrote:
> The default is 0x0 because it is a very recognizable value.
> It is very clear that 0x0 is not the size of the bitmap whereas any other
value
> can be mistaken as such.
> In http://codereview.chromium.org/10802066/, we store all bitmaps at 0x0 so
that
> we only put a single bitmap per favicon into the database. Using 0x0 will make
> transitioning into a state where we store the real bitmap size easy.

0x0 implies there is one known size, 0x0. That isn't the case though, there are
no known sizes. I think it should be an empty string.

[sky, 2012-08-16 21:27:01]

LGTM

[Nico, 2012-08-17 20:14:21]

You can add a BUG= line.

Is this ready to land?

[commit-bot: I haz the power, 2012-08-20 16:04:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10815068/23007

[commit-bot: I haz the power, 2012-08-20 18:46:39]

Change committed as 152367