Applied the IconType.

Populated the IconType in FavIconService.
Changed the FavIconService callback to have the IconType parameter.
Changed the callers to use FAV_ICON as default type

BUG=71571
TEST=Tested with existing unit test

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

[michaelbai, 2011-03-09 02:08:51]

A simple cl, please review it.

[sky, 2011-03-09 21:41:07]

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_html_writer.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_html_writer.cc:443: GURL(url),
Generally folks put as many args on a line as will fit.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_html_writer.cc:461: history::IconType) {
From the style guide: 'All parameters should be named, with identical names in
the declaration and implementation.'

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:741: history::IconType) {
Include name.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:199: history::IconType) {
include name

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service.h
File chrome/browser/favicon_service.h (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service....
chrome/browser/favicon_service.h:41: typedef Callback6<Handle,                  
        // handle
6 parameters with 2 booleans is pretty confusing. Put this into a structure.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service....
chrome/browser/favicon_service.h:70: // notified when the bits have been
fetched. |icon_type| could be any
could->can

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service....
chrome/browser/favicon_service.h:78: int icon_type,
icon_types.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history.cc
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history....
chrome/browser/history/history.cc:454: Schedule(PRIORITY_NORMAL,
&HistoryBackend::GetFavIcon, NULL, request,
See how there is multiple arguments on a single line here. You should be
consistent and use this style every where.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history.h
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history....
chrome/browser/history/history.h:659: int icon_type);
icon_types

http://codereview.chromium.org/6651014/diff/1/chrome/browser/possible_url_mod...
File chrome/browser/possible_url_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/possible_url_mod...
chrome/browser/possible_url_model.cc:193: history::IconType) {
include name

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/search_engine...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/search_engine...
chrome/browser/ui/search_engines/template_url_table_model.cc:107:
history::IconType) {
name

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/extensi...
File chrome/browser/ui/webui/extension_icon_source.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/extensi...
chrome/browser/ui/webui/extension_icon_source.cc:216: history::IconType) {
name

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/favicon...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/favicon...
chrome/browser/ui/webui/favicon_source.cc:71: history::IconType) {
name

[michaelbai, 2011-03-09 23:11:45]

Went through the whole CL and fixed the common style issues.

Just one thing, I prefer to leave the callback signature as it. Please consider
about it.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_html_writer.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_html_writer.cc:443: GURL(url),
On 2011/03/09 21:41:08, sky wrote:
> Generally folks put as many args on a line as will fit.

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_html_writer.cc:461: history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> From the style guide: 'All parameters should be named, with identical names in
> the declaration and implementation.'

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_model.cc:741: history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> Include name.

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/custom_home_page...
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:199: history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> include name

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/custom_home_page...
chrome/browser/custom_home_pages_table_model.cc:199: history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> include name

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service.h
File chrome/browser/favicon_service.h (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service....
chrome/browser/favicon_service.h:41: typedef Callback6<Handle,                  
        // handle
On 2011/03/09 21:41:08, sky wrote:
> 6 parameters with 2 booleans is pretty confusing. Put this into a structure.

To be a structure makes a lot of code change. And 2 booleans were not introduced
in this CL.

Can we leave it as it?

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service....
chrome/browser/favicon_service.h:70: // notified when the bits have been
fetched. |icon_type| could be any
On 2011/03/09 21:41:08, sky wrote:
> could->can

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service....
chrome/browser/favicon_service.h:78: int icon_type,
On 2011/03/09 21:41:08, sky wrote:
> icon_types.

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history.cc
File chrome/browser/history/history.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history....
chrome/browser/history/history.cc:454: Schedule(PRIORITY_NORMAL,
&HistoryBackend::GetFavIcon, NULL, request,
On 2011/03/09 21:41:08, sky wrote:
> See how there is multiple arguments on a single line here. You should be
> consistent and use this style every where.

I have gone though the whole CL and kept the style consistent as much as
possible.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history.h
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/history/history....
chrome/browser/history/history.h:659: int icon_type);
On 2011/03/09 21:41:08, sky wrote:
> icon_types

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/possible_url_mod...
File chrome/browser/possible_url_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/possible_url_mod...
chrome/browser/possible_url_model.cc:193: history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> include name

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/search_engine...
File chrome/browser/ui/search_engines/template_url_table_model.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/search_engine...
chrome/browser/ui/search_engines/template_url_table_model.cc:107:
history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> name

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/extensi...
File chrome/browser/ui/webui/extension_icon_source.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/extensi...
chrome/browser/ui/webui/extension_icon_source.cc:216: history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> name

Done.

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/favicon...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/ui/webui/favicon...
chrome/browser/ui/webui/favicon_source.cc:71: history::IconType) {
On 2011/03/09 21:41:08, sky wrote:
> name

Done.

[sky, 2011-03-09 23:34:52]

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service.h
File chrome/browser/favicon_service.h (right):

http://codereview.chromium.org/6651014/diff/1/chrome/browser/favicon_service....
chrome/browser/favicon_service.h:41: typedef Callback6<Handle,                  
        // handle
On 2011/03/09 23:11:45, michaelbai wrote:
> On 2011/03/09 21:41:08, sky wrote:
> > 6 parameters with 2 booleans is pretty confusing. Put this into a structure.
> 
> To be a structure makes a lot of code change. And 2 booleans were not
introduced
> in this CL.
> 
> Can we leave it as it?
> 

Is it really that many more code changes than you're already doing? That you had
to create a new Callback6 type is a strong indication that the callback should
be supplied a structure and not a bunch of args.

[michaelbai, 2011-03-10 01:24:32]

Add new struct, please review it.

[sky, 2011-03-10 17:54:44]

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
File chrome/browser/favicon_service.h (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
chrome/browser/favicon_service.h:31: struct FaviconData {
Put this in history_types

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
chrome/browser/favicon_service.h:62: FaviconData>::Type          // the type of
favicon
Can this take a const FaviconData& ?

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
chrome/browser/favicon_service.h:98: // Marks the all type of favicon for the
page as being out of date.
marks all icon types for ...

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history.h:646: const GURL& icon_url, history::IconType
icon_type);
Once you wrap a function declaration (or definition) put each arg on its own
line.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history.h:658: int icon_type);
icon_types

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history.h:675: history::IconType icon_types);
icon_type

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1714: scoped_refptr<RefCountedBytes>
data;
Does this really work? Don't you need to set favicon.image_data = new
RefCountedBytes() ? Also, why use data here, just use the one in
favicon.image_data.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1751: scoped_refptr<RefCountedBytes>
data;
Same comment as above here.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/ui/webui/exte...
File chrome/browser/ui/webui/extension_icon_source.cc (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/ui/webui/exte...
chrome/browser/ui/webui/extension_icon_source.cc:219:
!favicon.image_data->size()) {
This code is in nearly every callback. Create a method in FaviconData, say
is_valid, that does this.

[michaelbai, 2011-03-10 19:52:06]

On 2011/03/10 17:54:44, sky wrote:
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
> File chrome/browser/favicon_service.h (right):
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
> chrome/browser/favicon_service.h:31: struct FaviconData {
> Put this in history_types
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
> chrome/browser/favicon_service.h:62: FaviconData>::Type          // the type
of
> favicon
> Can this take a const FaviconData& ?
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
> chrome/browser/favicon_service.h:98: // Marks the all type of favicon for the
> page as being out of date.
> marks all icon types for ...
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
> File chrome/browser/history/history.h (right):
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
> chrome/browser/history/history.h:646: const GURL& icon_url, history::IconType
> icon_type);
> Once you wrap a function declaration (or definition) put each arg on its own
> line.
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
> chrome/browser/history/history.h:658: int icon_type);
> icon_types
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
> chrome/browser/history/history.h:675: history::IconType icon_types);
> icon_type
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
> File chrome/browser/history/history_backend.cc (right):
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
> chrome/browser/history/history_backend.cc:1714: scoped_refptr<RefCountedBytes>
> data;
> Does this really work? Don't you need to set favicon.image_data = new
> RefCountedBytes() ? Also, why use data here, just use the one in
> favicon.image_data.
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
> chrome/browser/history/history_backend.cc:1751: scoped_refptr<RefCountedBytes>
> data;
> Same comment as above here.
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/ui/webui/exte...
> File chrome/browser/ui/webui/extension_icon_source.cc (right):
> 
>
http://codereview.chromium.org/6651014/diff/7004/chrome/browser/ui/webui/exte...
> chrome/browser/ui/webui/extension_icon_source.cc:219:
> !favicon.image_data->size()) {
> This code is in nearly every callback. Create a method in FaviconData, say
> is_valid, that does this.

I will cover the issue in next patch, before that I want to confirm using 'const
FaviconData&'

I tried to keep the callback as before, we pushed all parameters in stack, the
data is still valid after callback return if the caller want to use it later.

We can live with 'const FaviconData&' by copying the data in the callback if we
need it later. 

Let me know this is exactly what you want.

Thanks

[michaelbai, 2011-03-11 01:11:28]

Addressed all comments, please check it again

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
File chrome/browser/favicon_service.h (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
chrome/browser/favicon_service.h:31: struct FaviconData {
On 2011/03/10 17:54:44, sky wrote:
> Put this in history_types

Done.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
chrome/browser/favicon_service.h:62: FaviconData>::Type          // the type of
favicon
On 2011/03/10 17:54:44, sky wrote:
> Can this take a const FaviconData& ?

I think we can't use '&', callback is posted to message loop, and will be
executed after returned.

It seemed that we can't use const. compiler can not find the proper method to
call.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/favicon_servi...
chrome/browser/favicon_service.h:98: // Marks the all type of favicon for the
page as being out of date.
On 2011/03/10 17:54:44, sky wrote:
> marks all icon types for ...

Done.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
File chrome/browser/history/history.h (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history.h:658: int icon_type);
On 2011/03/10 17:54:44, sky wrote:
> icon_types

Done.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history.h:675: history::IconType icon_types);
On 2011/03/10 17:54:44, sky wrote:
> icon_type

Done.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1714: scoped_refptr<RefCountedBytes>
data;
On 2011/03/10 17:54:44, sky wrote:
> Does this really work? Don't you need to set favicon.image_data = new
> RefCountedBytes() ? Also, why use data here, just use the one in
> favicon.image_data.

The new RefCountedBytes is actually in next line, moved it up.

Using data avoids casting back to RefCountedBytes in line 1719.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1751: scoped_refptr<RefCountedBytes>
data;
On 2011/03/10 17:54:44, sky wrote:
> Same comment as above here.

Moved new RefCountedBytes up

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/ui/webui/exte...
File chrome/browser/ui/webui/extension_icon_source.cc (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/ui/webui/exte...
chrome/browser/ui/webui/extension_icon_source.cc:219:
!favicon.image_data->size()) {
On 2011/03/10 17:54:44, sky wrote:
> This code is in nearly every callback. Create a method in FaviconData, say
> is_valid, that does this.

Done.

[sky, 2011-03-11 01:19:26]

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1714: scoped_refptr<RefCountedBytes>
data;
On 2011/03/11 01:11:28, michaelbai wrote:
> On 2011/03/10 17:54:44, sky wrote:
> > Does this really work? Don't you need to set favicon.image_data = new
> > RefCountedBytes() ? Also, why use data here, just use the one in
> > favicon.image_data.
> 
> The new RefCountedBytes is actually in next line, moved it up.

My mistake.

> Using data avoids casting back to RefCountedBytes in line 1719.

I don't understand this. Can't 1719 be &(favicon.image_data->data) ?

http://codereview.chromium.org/6651014/diff/11001/chrome/browser/history/hist...
File chrome/browser/history/history_types.cc (right):

http://codereview.chromium.org/6651014/diff/11001/chrome/browser/history/hist...
chrome/browser/history/history_types.cc:423: return known_icon &&
!image_data.get() && image_data->size();
Shouldn't that be image_data.get()?

http://codereview.chromium.org/6651014/diff/11001/chrome/browser/history/hist...
chrome/browser/history/history_types.cc:425: }  // namespace history
newline between 424 and 425.

[michaelbai, 2011-03-11 16:56:19]

Please check again.

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1714: scoped_refptr<RefCountedBytes>
data;
On 2011/03/11 01:19:26, sky wrote:
> On 2011/03/11 01:11:28, michaelbai wrote:
> > On 2011/03/10 17:54:44, sky wrote:
> > > Does this really work? Don't you need to set favicon.image_data = new
> > > RefCountedBytes() ? Also, why use data here, just use the one in
> > > favicon.image_data.
> > 
> > The new RefCountedBytes is actually in next line, moved it up.
> 
> My mistake.
> 
> > Using data avoids casting back to RefCountedBytes in line 1719.
> 
> I don't understand this. Can't 1719 be &(favicon.image_data->data) ?


data is the member of RefCountedBytes, the type of favicon.image_data is
scoped_refptr<RefCountedMemory> which doesn't have std::vector<unsigned char>
data member.

http://codereview.chromium.org/6651014/diff/11001/chrome/browser/history/hist...
File chrome/browser/history/history_types.cc (right):

http://codereview.chromium.org/6651014/diff/11001/chrome/browser/history/hist...
chrome/browser/history/history_types.cc:423: return known_icon &&
!image_data.get() && image_data->size();
On 2011/03/11 01:19:26, sky wrote:
> Shouldn't that be image_data.get()?

Done.

http://codereview.chromium.org/6651014/diff/11001/chrome/browser/history/hist...
chrome/browser/history/history_types.cc:425: }  // namespace history
On 2011/03/11 01:19:26, sky wrote:
> newline between 424 and 425.

Done.

[sky, 2011-03-11 17:56:45]

LGTM with the following changes.

As I said, I would love to see the callback take a const&, but there it may be
problematic with callbacks. Thanks for trying it.

  -Scott

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
File chrome/browser/history/history_backend.cc (right):

http://codereview.chromium.org/6651014/diff/7004/chrome/browser/history/histo...
chrome/browser/history/history_backend.cc:1714: scoped_refptr<RefCountedBytes>
data;
On 2011/03/11 16:56:19, michaelbai wrote:
> On 2011/03/11 01:19:26, sky wrote:
> > On 2011/03/11 01:11:28, michaelbai wrote:
> > > On 2011/03/10 17:54:44, sky wrote:
> > > > Does this really work? Don't you need to set favicon.image_data = new
> > > > RefCountedBytes() ? Also, why use data here, just use the one in
> > > > favicon.image_data.
> > > 
> > > The new RefCountedBytes is actually in next line, moved it up.
> > 
> > My mistake.
> > 
> > > Using data avoids casting back to RefCountedBytes in line 1719.
> > 
> > I don't understand this. Can't 1719 be &(favicon.image_data->data) ?
> 
> 
> data is the member of RefCountedBytes, the type of favicon.image_data is
> scoped_refptr<RefCountedMemory> which doesn't have std::vector<unsigned char>
> data member.

GAH! Sorry. I get it.
I'm tempted to say the callback should take a RefCountedBytes, but we don't
really want to expose the implementation so what you have is right.

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/custom_home_...
File chrome/browser/custom_home_pages_table_model.cc (right):

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/custom_home_...
chrome/browser/custom_home_pages_table_model.cc:202: if (favicon.known_icon &&
favicon.image_data.get() &&
favicon.is_valid

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/fav_icon_hel...
File chrome/browser/fav_icon_helper.cc (right):

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/fav_icon_hel...
chrome/browser/fav_icon_helper.cc:203: if (favicon.image_data.get() &&
favicon.image_data->size())
is_valid

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/favicon_serv...
File chrome/browser/favicon_service.h (right):

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/favicon_serv...
chrome/browser/favicon_service.h:65: // Requests any |icon_type| of favicon for
a web page URL. |consumer| is
icon_types

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/favicon_serv...
chrome/browser/favicon_service.h:66: // notified when the bits have been
fetched. |icon_type| can be any
icon_types

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/favicon_serv...
chrome/browser/favicon_service.h:78: // Marks all type of favicon for the page
as being out of date.
all types of favicons

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/possible_url...
File chrome/browser/possible_url_model.cc (right):

http://codereview.chromium.org/6651014/diff/16001/chrome/browser/possible_url...
chrome/browser/possible_url_model.cc:192: if (favicon.known_icon) {
is_valid

[Avi (use Gerrit), 2011-03-16 17:48:36]

http://codereview.chromium.org/6651014/diff/13004/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/6651014/diff/13004/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model.cc:740: SkBitmap fav_icon;
FYI, it's favicon, not fav_icon. See crbug.com/76073. I'll fix.