Adds support for saving favicon size into history database.

Modifies history backend to be able to take advantage of storing the pixel size of favicons into the database.
Changed FaviconData to allow retrieving favicon bitmaps for multiple pixel sizes from the database simultaneously.
Added History::GetFavicons() and History::GetFaviconsForURL() to do this.
Changed History::SetFavicon() to History::SetFavicons() such that multiple favicon bitmaps for a page URL can be set simultaneously.

BUG=138553
TEST=ThumbnailDatabaseTest.*, HistoryBackendTest.*

Depends on http://codereview.chromium.org/10815068

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

[pkotwicz, 2012-07-25 19:33:58]

Adds support for saving favicon size into history database.

In this CL the requested size is passed in as the pixel size and stored where
the pixel size would be stored. I will fix   this in short order, however I want
to keep it simple for M22.

[stevenjb, 2012-07-30 23:58:16]

I want to look at the history changes in more detail, but a little initial
feedback.

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/andr...
File chrome/browser/history/android/favicon_sql_handler.cc (right):

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/andr...
chrome/browser/history/android/favicon_sql_handler.cc:25: const gfx::Size
kDefaultFaviconSize =
We should define this with gfx::kFaviconSize, i.e. gfx::kDefaultFaviconSize and
use it everywhere.

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/andr...
chrome/browser/history/android/favicon_sql_handler.cc:91: // Since the URL was
delete, we delete all types of icon mappings.
s/was delete/was deleted/

[stevenjb, 2012-08-01 00:00:31]

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1948: if (closest_pixel_size.GetArea()
< desired_pixel_size.GetArea() &&
Area may not be the best method for comparison. For example area would give
16x64 the same priority as 32x32. 
Exact matches should always be given highest priority. 
After that I thin we are better off just comparing first height, then width (if
heights are equal).

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:2095: //          but either of them
is ICON_TOUCH or ICON_PRECOMPOSED_TOUCH.
Do we care about differentiating between touch and touch_precomposed for Chrome?
i.e. could we just treat those as the same type?

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:2100: //    elsewhere as well.
This seems a bit arbitrary. Have we considered defining a fixed set of icon
sizes (which might vary by platform) and constraining pixel_size to that list?

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:2122: *replaced_icon =
icon_mappings[0].icon_id;
Shouldn't this logic be in AddIconMapping? It's not clear here what |mapping_id|
refers to?

[pkotwicz, 2012-08-02 19:47:52]

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:1948: if (closest_pixel_size.GetArea()
< desired_pixel_size.GetArea() &&
Agree

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:2095: //          but either of them
is ICON_TOUCH or ICON_PRECOMPOSED_TOUCH.
I'll look into this.

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:2100: //    elsewhere as well.
We are actually going to be doing this too.
It still feels like a good idea to constrain the maximum amount of bitmaps we
store for a single page.

http://codereview.chromium.org/10802066/diff/9004/chrome/browser/history/hist...
chrome/browser/history/history_backend.cc:2122: *replaced_icon =
icon_mappings[0].icon_id;
I would rather not add extra SQL queries to AddIconMapping which is why I put
this logic here.
It is true that the picking of icon_mappings[0].mapping_id is completely
arbitrary. (Though all |icon_mappings| are mappings for the same page) However,
I don't think there are any negative consequences of this being the case.

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

Finished implementing the design suggested by Scott. Sorry for the delay.
Steven and sky, can you please take a look and give me your feedback?

[Nico, 2012-08-15 20:56:58]

I didn't look at the code. A few questions about the design (for you, or for
sky).

As far as I understand, this keeps just a single size in the db. How does this
handle dragging a window between low and hidpi screens? It'll have every favicon
only in the resolution of the monitor chrome was on when the favicon was
requested?

How will we handle webui? I guess the chrome://favicon handler will always
return the full size, and the webui css needs to make sure to explicity set the
size of every favicon img element to 16x16?

I suppose there's some place where we convert history favicons to gfx::Image
objects. Can you use  chrome/browser/favicon/select_favicon_frames.h in these
places (if you aren't already)?

[pkotwicz, 2012-08-15 21:39:52]

Nico, to answer your question, this patch along with
http://codereview.chromium.org/10815068/ implement support for having multiple
favicon bitmaps for the same page URL and icon type.

[stevenjb, 2012-08-15 22:59:18]

Some feedback, mostly nits.

I'm not familiar with the history/ code, so will mostly rely on sky's review,
but I will try to take a closer look soon.

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_model.cc:787:
favicon_data.elements[0].bitmap_data;
nit: use constructor instead of assignment

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_handler.cc:495: history::FaviconDataElement
element = favicon_data.elements[0];
Why do a copy here?

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_handler.cc:508: favicon_data.elements.empty() ||
!(DoUrlAndIconMatch(
nit: put !(DoUrlAndIconMatch( on a new line

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_handler_unittest.cc (right):

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_handler_unittest.cc:53:
scoped_refptr<base::RefCountedBytes> data = new base::RefCountedBytes();
nit: use constructor

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_handler_unittest.cc:574:
scoped_refptr<base::RefCountedBytes> data = new base::RefCountedBytes();
nit: use constructor

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/and...
File chrome/browser/history/android/android_history_types.h (right):

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/and...
chrome/browser/history/android/android_history_types.h:120: void
set_favicon(scoped_refptr<base::RefCountedMemory> data) {
Shouldn't the arg be a reference to avoid an extra copy?

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/and...
chrome/browser/history/android/android_history_types.h:125: return favicon_;
Same here?

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/and...
File chrome/browser/history/android/favicon_sql_handler.cc (right):

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/and...
chrome/browser/history/android/favicon_sql_handler.cc:83: // Since the URL was
delete, we delete all types of icon mappings.
nit: was deleted

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

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2007: GURL icon_url =
elements[i].icon_url;
You could use a GURL& or skip local to avoid an extra copy here.

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2178: if (desired_pixel_size) {
Couldn't this just check on desired_pixel_size.IsEmpty()?

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
chrome/browser/history/history_backend.h:613: const gfx::Size*
desired_pixel_size);
Why a const* here instead of a const&?

[pkotwicz, 2012-08-16 02:58:36]

Fixed nits as per Steven

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

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2178: if (desired_pixel_size) {
I think that it is clearer when desired_pixel_size == NULL. But if you think
that it is clearer to use desired_pixel_size.IsEmpty(), I can change this.

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

I'm going to hold off until the other review lands, then I'll look at this one.

  -Scott

On Wed, Aug 15, 2012 at 7:58 PM,  <pkotwicz@chromium.org> wrote:
> Fixed nits as per Steven
>
>
>
>
http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
> File chrome/browser/history/history_backend.cc (right):
>
>
http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
> chrome/browser/history/history_backend.cc:2178: if (desired_pixel_size)
> {
> I think that it is clearer when desired_pixel_size == NULL. But if you
> think that it is clearer to use desired_pixel_size.IsEmpty(), I can
> change this.
>
> http://codereview.chromium.org/10802066/

[stevenjb, 2012-08-20 17:20:27]

Still wrapping my head around the history backend code, but so far that looks OK
to me.

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

http://codereview.chromium.org/10802066/diff/12003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2178: if (desired_pixel_size) {
On 2012/08/16 02:58:37, pkotwicz wrote:
> I think that it is clearer when desired_pixel_size == NULL. But if you think
> that it is clearer to use desired_pixel_size.IsEmpty(), I can change this.

I guess it's a tradeoff between clarity here and consistency in the API - mostly
we use const gfx::Size& desired_pixel_size, so I think we should use it here
also. Additionally, if &gfx::Size() was passed as an argument here (and we use
that for 'default' elsewhere in this CL so it would be an easy enough mistake),
this would do the wrong thing, so I think we should do an IsEmpty() check
regardless.

All that said, I'm OK with this as-is also.

[pkotwicz, 2012-08-21 16:40:41]

Merged this CL with changes in previous CL.
Removed methods in history service which get data for a target size as it seems
to be more natural to handle this in the favicon_service (in a separate CL).

[pkotwicz, 2012-08-21 16:41:25]

Scott, can you take a look when you have time?

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

I got hung up on the structures you're creating. Seems like they should better
match the db.

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history.h:696: // from the history backend.
Document icon_types

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history.h:702: // backend for favicons of type FAVICON.
This takes an icon_type.

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

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2137: if
(favicon_data->icon_url_sizes.size() == 0) {
!empty

Why does this method care about the icon_url_sizes? Shouldn't it always reset
favicon_data?

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_backend.h:637: bool AddToFaviconData(FaviconID
favicon_id,
Using 'Add' in the name makes it sound as though this adds to the database, but
that isn't the case. This should be something like: GetFavicondData

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

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_types.h:761: // Sizes represents the sizes that
the thumbnail database knows a favicon is
Sizes -> FaviconSizes

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_types.h:771: class FaviconSizes {
I think this code should use std::set<gfx::Size> and the functions that
encode/decode the string should be else where.

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_types.h:774: FaviconSizes(const std::string&
string_representation);
explicit on these two. Do we need so many constructors?

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_types.h:804: struct FaviconDataElement {
How about naming this FaviconDataBitmap?

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_types.h:833: bool expired;
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;
};

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_types.h:844: IconURLSizesMap icon_url_sizes;
sizes?

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

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:466: return
!bitmap_id_size_listing->empty();
In this one you return whether non-empty, but not in GetFaviconBitmaps. I has
asked GetFaviconBitmaps to look like this last review and you didn't change it.
How come you're not making this one match that pattern? I think what you have
here is the right pattern and GetFaviconBitmaps should change.

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:528:
scoped_refptr<base::RefCountedMemory> icon_data,
Shouldn't this take the size too?

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:659: bool result = false;
Same comment as in GetFaviconBitmapIDSizeListing: get rid of result and check
filtered_mapping_data->empty instead.

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

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:105: bool
GetFaviconBitmapIDSizeListing(
GetFaviconBitmapIDSizes

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

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

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/his...
chrome/browser/history/history_types.h:833: bool expired;
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?

[pkotwicz, 2012-08-23 23:12:56]

Scott can you give me early feedback on
HistoryBackend::GetFaviconFromDB HistoryBackend::GetFaviconDataForBestMatch and
how select_favicon_frames is used?

[sky, 2012-08-24 16:35:41]

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

http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2141: std::vector<FaviconID>
favicon_ids;
Add a description as to what this does.

http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2166:
scoped_ptr<std::vector<FaviconBitmapID> > best_bitmap_ids(NULL);
Why do you need a scoped_ptr here and below?

http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2183:
best_bitmap_ids.swap(candidate_bitmap_ids);
This seems like it only picks bitmaps for a unique faviconid.

Don't you need to track the best score for each size you need? By size I mean
the combination of size and scale factors.

[sky, 2012-08-24 16:36:02]

These comments are only from the two functions you asked me to look at.

  -Scott

On Fri, Aug 24, 2012 at 9:35 AM,  <sky@chromium.org> wrote:
>
>
http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
> File chrome/browser/history/history_backend.cc (right):
>
>
http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
> chrome/browser/history/history_backend.cc:2141: std::vector<FaviconID>
> favicon_ids;
> Add a description as to what this does.
>
>
http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
> chrome/browser/history/history_backend.cc:2166:
> scoped_ptr<std::vector<FaviconBitmapID> > best_bitmap_ids(NULL);
> Why do you need a scoped_ptr here and below?
>
>
http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
> chrome/browser/history/history_backend.cc:2183:
> best_bitmap_ids.swap(candidate_bitmap_ids);
> This seems like it only picks bitmaps for a unique faviconid.
>
> Don't you need to track the best score for each size you need? By size I
> mean the combination of size and scale factors.
>
> http://codereview.chromium.org/10802066/

[michaelbai, 2012-08-28 15:23:16]

It seemed that you didn't run the Android related tests, Could you make sure
they are passed also?

[pkotwicz, 2012-09-04 16:18:34]

Changes as requested. Scott can you please take a look?
I still need to update the comments in history.h though.

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

http://codereview.chromium.org/10802066/diff/19006/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:528:
scoped_refptr<base::RefCountedMemory> icon_data,
In my opinion a favicon bitmap is uniquely identified by the FaviconID it is
associated with and it's pixel size.
So if the pixel size or FaviconID changes, we would add a new FaviconBitmap and
delete the old one as opposed to setting it.

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

http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2183:
best_bitmap_ids.swap(candidate_bitmap_ids);
Yes this is true. I added a TODO to make this different but I think it is best
to keep it this way for now till we update FaviconStatus.

To reply to your comment above, I use the scoped_ptr to avoid the vector copy
from assigning |candidate_bitmap_ids| to |best_bitmap_ids|.

[sky, 2012-09-04 17:18:07]

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

http://codereview.chromium.org/10802066/diff/25001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2183:
best_bitmap_ids.swap(candidate_bitmap_ids);
On 2012/09/04 16:18:36, pkotwicz wrote:
> Yes this is true. I added a TODO to make this different but I think it is best
> to keep it this way for now till we update FaviconStatus.
> 
> To reply to your comment above, I use the scoped_ptr to avoid the vector copy
> from assigning |candidate_bitmap_ids| to |best_bitmap_ids|.

Huh? Use swap if you're really worried.

[sky, 2012-09-04 17:22:31]

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history.cc:498: int desired_size_in_dip,
What is desired_size_in_dip? Why is it an int?

[pkotwicz, 2012-09-04 19:47:54]

Changes as requested. Updated comments in history.h.

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history.cc:498: int desired_size_in_dip,
I have updated the comments in history.h.
|desired_size_in_dip| is an int because SelectFaviconFrames does not require a
gfx::Size.

[sky, 2012-09-04 20:52:28]

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:106: icon_urls, icon_type,
gfx::kFaviconSize, ui::GetSupportedScaleFactors());
> 80

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/favicon/sel...
File chrome/browser/favicon/select_favicon_frames.cc (right):

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/favicon/sel...
chrome/browser/favicon/select_favicon_frames.cc:21: void
SizesFromFaviconBitmapIDSizes(
Add description.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history.h:768: void MergeFavicon(
I don't understand the use case for this, and especially the need to verify the
size. Could you explain that?

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:1874:
favicon_sizes.push_back(merge_pixel_size);
How does an empty size end up getting mapped correctly?

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2059:
favicon_ids.push_back(favicon_id);
It feels cumbersome to have to index into icon_url_id_map. Is it worth using a
pair <id, url>?

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2063: // else case, haven't cached
entry yet. Caller is responsible for
This comment doesn't really match anymore. Nuke it.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2113: // The merge is valid if the
favicon sizes for |favicon_id| are unkown.
unknown

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2261: float highest_score = -1.0f;
nit: add a kInvalidScore some where and use it here.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2275: best_favicon_id =
candidate_favicon_ids[i],
does this compile?

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2290: favicon_bitmap_results->clear();
Shouldn't this be early on in the function?

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.h:638: // the favicon bitmap for the
merge to be valid.
Document its set to empty if new.

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_types.h:660: INVALID_ICON = 0x0,
Add a comment that order matters here.

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:451: bool
ThumbnailDatabase::GetFaviconBitmapIDSizes(
Is it possible to land the changes in this file outside of the rest?

[michaelbai, 2012-09-04 21:18:53]

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:213: // TODO(pkotwicz): Pass in real
pixel size to SetFavicons.
Not sure what did you mean pass in real pixel size?

Will you add 'pixel_size' parameter? If so, will this method requires the client
parse the images before set the favicon?

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history.h:702: // from the history backend.
From the comments it is not clear to me that, how *all* favicon mappings could
be gotten?

Why did icon_typs, desired_size_in_dip and desired_scale_factors exist if I
could get all favicon mappings from this method?

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history.h:720: // |page_url| whose icon URL is not in
|icon_urls| will be deleted.
The comment didn't say how the parameters icon_types and desired_size_in_dip
will be used?

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history.h:744: // |elements| is a listing of additional
favicon bitmaps to store for
There is no elements argument in method's definition.

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

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2166: const IconURLSizesMap&
icon_url_sizes) const {
In Android, there is no url associated with the icon. This will break the
Android provider.

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2206:
thumbnail_db_->DeleteFaviconBitmap(bitmap_id_sizes[i].bitmap_id);
Not sure, I fully understand the design here, could this method deletes all the
favicons finally?

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history_backend.h:651: // SetFavicons are valid.
You may explain what the valid params for SetFavicon.

[pkotwicz, 2012-09-04 22:40:54]

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:213: // TODO(pkotwicz): Pass in real
pixel size to SetFavicons.
I am unsure as to whether the client should parse the images before calling
SetFavicons.
Practically it does so today.
Realistically, we may use the HTML5 sizes parameter if it is available, and
otherwise create a mock list of pixel sizes based on the bitmap data which was
downloaded.

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history.h:702: // from the history backend.
You cannot get all of the favicon bitmaps from this method. I have updated the
comments in this file in patch set #13.

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history.h:720: // |page_url| whose icon URL is not in
|icon_urls| will be deleted.
I have updated the comments in this file in patch set #13

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

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2166: const IconURLSizesMap&
icon_url_sizes) const {
It is still valid for IconURLSizesMap to have a key which is an empty GURL.

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2206:
thumbnail_db_->DeleteFaviconBitmap(bitmap_id_sizes[i].bitmap_id);
This method will delete any favicon bitmaps whose pixel sizes are not in
|favicon_sizes|. So yes it CAN delete all favicon bitmaps for a FaviconID but it
doesn't always do so.
The idea is that the entire state of the world is passed in the IconURLSizesMap
for a particular |page_url|. Any old data which is outside of that state of the
world is removed.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history.h:768: void MergeFavicon(
The use case for this is sync to use this function instead of SetFavicons in
bookmark_change_processor.cc
Imagine a user who has two devices.
a) Windows machine.
b) Retina macbook.
Sync syncs only 16x16 bitmaps.
The idea is that sync updates from the Windows box don't delete 2x bitmaps from
the history backend on the retina macbook.

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:1874:
favicon_sizes.push_back(merge_pixel_size);
I am unsure as to what you are asking

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:451: bool
ThumbnailDatabase::GetFaviconBitmapIDSizes(
Sure. I will however wait for the HistoryBackend to be almost LGTMed before
splitting up this file into a separate patch.

[sky, 2012-09-05 00:15:50]

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2053: if (icon_type_out >
selected_icon_type) {
This is subtle enough that you should add a comment.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.h:700: bool BuildIconURLSizesMap(const
std::vector<FaviconID>& favicon_ids,
nit, when you wrap wrap all arguments (like you did in
SetFaviconMappingsForPageAndRedirects).

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:561: bool
ThumbnailDatabase::SetFaviconBitmap(
Make order match header.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:574: statement.BindInt64(1,
time.ToTimeT());
Shouldn't this be ToInternalValue? Or are they all wrong in this file?

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.h:105: // |icon_id|.
Document this is the actual sizes we have, which may be a subset of those
available for the id.

[michaelbai, 2012-09-05 19:08:17]

https://chromiumcodereview.appspot.com/10802066/diff/38003/chrome/browser/his...
File chrome/browser/history/history_backend.h (right):

https://chromiumcodereview.appspot.com/10802066/diff/38003/chrome/browser/his...
chrome/browser/history/history_backend.h:651: // SetFavicons are valid.
Could you add comment for what the valid params are?

https://chromiumcodereview.appspot.com/10802066/diff/38003/chrome/browser/his...
chrome/browser/history/history_backend.h:687: // Returns true on success.
From this comment, it is not clear to me that, 
- how many favicons could be returned?
- what's the best match?

Why don't you return all favicons, and find out the best match in
favicon_service, just like what you did for resize.

[pkotwicz, 2012-09-05 22:39:25]

Added a whole bunch of additional comments.
Changes as requested. If I have not replied to the comment, it means that I have
done the change suggested. (I don't think I have missed any)
Other changes are as described in the comments below.

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/10802066/diff/35001/chrome/browser/history/his...
chrome/browser/history/history_backend.h:651: // SetFavicons are valid.
On 2012/09/04 21:18:53, michaelbai wrote:
> You may explain what the valid params for SetFavicon.

Done.

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:1874:
favicon_sizes.push_back(merge_pixel_size);
I have updated MergeFavicon().
GetDefaultFaviconSizes() now returns an empty FaviconSizes vector.
However, GetDefaultFaviconSizes() is not valid input to SetFavicons().
Practically, any uses where we would want to call SetFavicons() with
GetDefaultFaviconSizes() should use MergeFavicon() instead.
However, this slightly inconvenient for unittests.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2059:
favicon_ids.push_back(favicon_id);
Replaced this with a std::vector<FaviconID>

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2275: best_favicon_id =
candidate_favicon_ids[i],
On 2012/09/04 20:52:29, sky wrote:
> does this compile?

Done.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.cc:2290: favicon_bitmap_results->clear();
Done.

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
File chrome/browser/history/history_backend.h (right):

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_backend.h:687: // Returns true on success.
Updated the comment to address your concerns.

The reason that I don't return all favicons is to avoid retreiving favicons
which we don't use. (I actually had a CL up previously where I returned all
favicons).
I don't mind going back to the previous state though.

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/his...
chrome/browser/history/history_types.h:660: INVALID_ICON = 0x0,
On 2012/09/04 20:52:29, sky wrote:
> Add a comment that order matters here.

Done.

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

http://codereview.chromium.org/10802066/diff/38003/chrome/browser/history/thu...
chrome/browser/history/thumbnail_database.cc:574: statement.BindInt64(1,
time.ToTimeT());
Fixed this in separate CL.

[sky, 2012-09-05 22:58:12]

LGTM

[michaelbai, 2012-09-05 23:24:59]

Hi Sky,

Currently the favicon was selected and resized by desired_size_in_dip and
desired_scale_factors in two different location, The selection is in
HistoryBackend and the transformation happened in FaviconService.

Would it be good to put them together in either HistoryBackend or
FaviconService?

If we put them in FaviconService, HistoryBackend will return all the bitmaps for
a FaviconID, then the bitmaps were selected and transformed in FaviconService.
The downside is it used a little more memory, and take a more time of UI thread.

If we do the selection and transformation in HistoryBackend,  we need to provide
a way to get all bitmaps or first bitmaps from HistoryBackend as Android
requires this feature.

[sky, 2012-09-06 04:11:21]

On Wed, Sep 5, 2012 at 4:24 PM,  <michaelbai@chromium.org> wrote:
> Hi Sky,
>
> Currently the favicon was selected and resized by desired_size_in_dip and
> desired_scale_factors in two different location, The selection is in
> HistoryBackend and the transformation happened in FaviconService.
>
> Would it be good to put them together in either HistoryBackend or
> FaviconService?
>
> If we put them in FaviconService, HistoryBackend will return all the bitmaps
> for
> a FaviconID, then the bitmaps were selected and transformed in
> FaviconService.
> The downside is it used a little more memory, and take a more time of UI
> thread.

I don't want to select images that aren't going to be necessary.
Especially if we end up adding other sizes.

I would like to see the sharing if possible too.

  -Scott

> If we do the selection and transformation in HistoryBackend,  we need to
> provide
> a way to get all bitmaps or first bitmaps from HistoryBackend as Android
> requires this feature.
>
> http://codereview.chromium.org/10802066/

[commit-bot: I haz the power, 2012-09-09 19:06:43]

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

[commit-bot: I haz the power, 2012-09-09 23:26:41]

Change committed as 155645