Support touch icon in FaviconHelper

a Downloaded or retrieved favicon and touch in FaviconHelper.

b.ViewHostMsg_UpdateFaviconURL can update multiple icon urls 


BUG=71571
TEST=Tested with existing unit test and add 2 new unit tests

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

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

[michaelbai, 2011-03-17 00:11:36]

Could you help to give a quick review of the design of this draft version? 

It can not be compiled now.

[sky, 2011-03-17 14:43:47]

As I mentioned previously, I don't think this makes sense. Touch icons are too
big to be caching in memory like this.

  -Scott

[michaelbai, 2011-03-17 15:59:19]

Updated

a. Only cache FAVICON.
b. Disable fetching touch icon.
c. added some design questions

[michaelbai, 2011-03-18 00:09:08]

Added FaviconDelegate to only update FAVICON to NavigationEntry

Please check again if there any other design issue.

[sky, 2011-03-18 17:33:37]

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:110: i != candidates.end(); +i) {
++i

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:122: if (my_candidates.size() == 0)
How come you have both my_candidates and favicon_candidates_. Can't you just use
favicon_candidates_?

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:143: message_handled_ = true;
YUCK! There has to be a better way to handle this. Talk to John (jabdelmalek),
he's the IPC master.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:167: } else if (++current_candidate_idx_ <
favicon_candidates_.size()
&& should be on this line.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:169: FetchFavicon(Candidate()->icon_url,
Candidate()->icon_type);
indentation is wrong.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helper.h
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:71: struct FaviconCandidate {
This should exist in src/chrome/common/icon_messages. See the pattern of
similiar classes (like the automation_messages) for naming and what not. Also,
Candidate isn't very useful here. Maybe just Favicon? Or FaviconURL?

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:76: // The url of the icon.
newline between 75 and 76.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:80: history::IconType icon_type;
The renderer shouldn't use history types, so you'll have to create a mirror of
them somewhere the renderer can see (such as chrome/common).

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:93: void FetchFavicon(const GURL& url, int
icon_types);
Why does Fetch need to take icon_types when the constructor does?

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:111: std::vector<FaviconCandidate>& candidates);
References passed to methods need to be const.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:181: FaviconCandidate* Candidate() {
candidate()

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:207: const int supported_icon_types_;
No need for supported, just icon_types_.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:213: int current_candidate_idx_;
size_t

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/icon_delegate.h
File chrome/browser/icon_delegate.h (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/icon_delegate...
chrome/browser/icon_delegate.h:24: virtual void
UpdateFaviconImageData(NavigationEntry* entry,
I was hoping a delegate interface would allow FaviconHelper not to know about
NavigationEntry, but the current code still has that. It would be nice to
totally divorce FaviconHelper from NavigationEntry, but that isn't totally
necessary.
FaviconHelper knows the icontype and can just as easily only update
NavigationEntry if type is FAVICON_TYPE. So, nuke this class and make
FaviconHelper only do parts of code if type is FAVICON_TYPE. This isn't the
cleanest thing, but what you have here doesn't help it much.

http://codereview.chromium.org/6672065/diff/6001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:2187: // TODO : Get both favicon and touch icon
url, and send them to the browser.
This should only be done if needed.

http://codereview.chromium.org/6672065/diff/6001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6672065/diff/6001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:395: touch_icon_helper_.reset(new
FaviconHelper(this,
You'll want to define a constant in chrome/browser/defaults and set it to true
only when needed.

[jam, 2011-03-18 18:40:45]

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:143: message_handled_ = true;
On 2011/03/18 17:33:37, sky wrote:
> YUCK! There has to be a better way to handle this. Talk to John (jabdelmalek),
> he's the IPC master.

There isn't a super clean way of doing this (it's pretty rare).  I think it's
better to avoid a member variable.  What we've done in these places is to take
that message out of the message map and then afterwards do:

if (message.type()) == OnUpdateFaviconURL::ID)
and then unpack the message manually (i don't have the syntax infront of me, can
look it up later if we need it).

one question: where is this message handled if your return false here?  can't
the sender send different messages depending on where they want it to go?

[michaelbai, 2011-03-18 19:18:38]

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:143: message_handled_ = true;
On 2011/03/18 18:40:45, John Abd-El-Malek wrote:
> On 2011/03/18 17:33:37, sky wrote:
> > YUCK! There has to be a better way to handle this. Talk to John
(jabdelmalek),
> > he's the IPC master.
> 
> There isn't a super clean way of doing this (it's pretty rare).  I think it's
> better to avoid a member variable.  What we've done in these places is to take
> that message out of the message map and then afterwards do:
> 
> if (message.type()) == OnUpdateFaviconURL::ID)
> and then unpack the message manually (i don't have the syntax infront of me,
can
> look it up later if we need it).
> 
> one question: where is this message handled if your return false here?  can't
> the sender send different messages depending on where they want it to go?

Favicon is a TabContentsObserver. TabContents just forwards the message to each
observers until the message is handled(TabContents::OnMessageReceived).

I think TabContent shouldn't unpack the message (might split into 2 message) and
forward to the proper FaviconHelper.

Do you think it might a little over skill to unpack message manually for
avoiding a member variable. I am sorry if I feel about this for I am not
familiar with IPC.

[michaelbai, 2011-03-22 18:14:13]

1. Addressed all comments.
2. Added touch icon support in FaviconSource.
3. This version can be compiled, unit test passed

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:110: i != candidates.end(); +i) {
On 2011/03/18 17:33:37, sky wrote:
> ++i

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:122: if (my_candidates.size() == 0)
On 2011/03/18 17:33:37, sky wrote:
> How come you have both my_candidates and favicon_candidates_. Can't you just
use
> favicon_candidates_?

I thought the coming icons might not the one this FaviconHelper could handle.
But, it seems I could get ride of it

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:167: } else if (++current_candidate_idx_ <
favicon_candidates_.size()
On 2011/03/18 17:33:37, sky wrote:
> && should be on this line.

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.cc:169: FetchFavicon(Candidate()->icon_url,
Candidate()->icon_type);
On 2011/03/18 17:33:37, sky wrote:
> indentation is wrong.

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helper.h
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:71: struct FaviconCandidate {
On 2011/03/18 17:33:37, sky wrote:
> This should exist in src/chrome/common/icon_messages. See the pattern of
> similiar classes (like the automation_messages) for naming and what not. Also,
> Candidate isn't very useful here. Maybe just Favicon? Or FaviconURL?

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:76: // The url of the icon.
On 2011/03/18 17:33:37, sky wrote:
> newline between 75 and 76.

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:80: history::IconType icon_type;
On 2011/03/18 17:33:37, sky wrote:
> The renderer shouldn't use history types, so you'll have to create a mirror of
> them somewhere the renderer can see (such as chrome/common).

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:93: void FetchFavicon(const GURL& url, int
icon_types);
On 2011/03/18 17:33:37, sky wrote:
> Why does Fetch need to take icon_types when the constructor does?

Removed

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:111: std::vector<FaviconCandidate>& candidates);
On 2011/03/18 17:33:37, sky wrote:
> References passed to methods need to be const.

tried, got the compile error in, No std::vectors are defined as const & in
render_messages.h

Changed to std::vector<FaviconCandidate>

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:181: FaviconCandidate* Candidate() {
On 2011/03/18 17:33:37, sky wrote:
> candidate()

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:207: const int supported_icon_types_;
On 2011/03/18 17:33:37, sky wrote:
> No need for supported, just icon_types_.

Done.

http://codereview.chromium.org/6672065/diff/6001/chrome/browser/favicon_helpe...
chrome/browser/favicon_helper.h:213: int current_candidate_idx_;
On 2011/03/18 17:33:37, sky wrote:
> size_t

Done.

http://codereview.chromium.org/6672065/diff/6001/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6672065/diff/6001/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:395: touch_icon_helper_.reset(new
FaviconHelper(this,
On 2011/03/18 17:33:37, sky wrote:
> You'll want to define a constant in chrome/browser/defaults and set it to true
> only when needed.

Done.

[sky, 2011-03-22 19:48:48]

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/defaults.h
File chrome/browser/defaults.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/defaults.h#n...
chrome/browser/defaults.h:86: extern bool enable_touch_icon;
This should be above line 72, right?

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:32: preferred_icon_size_(preferred_icon_size) {
Initialize current_candidate_idx too

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:127: size_t candidates_count =
favicon_candidates_.size();
If candidates contains a FaviconURL whose type matches what you want don't you
need to remove the existing one in favicon_candidates_? Similarly wouldn't you
need to reset candidate_index_?

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:146: if (GetFaviconService() &&
got_favicon_from_history_ && (favicon_expired_ ||
wrap favicon_expired_ onto a new line so that this is easier to read.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:147: !history_icon_.is_valid() ||
!IsSameFaviconURL(*candidate(),
Don't you need to check entry->favicon().is_valid() too? At least when the type
is favicon.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:154: bool message_handled = true;
Do you need the message_handled stuff anymore?

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:206: if (favicon.known_icon &&
favicon.icon_type == history::FAVICON &&
This doesn't check !entry->favicon().is_valid()

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:220: if (candidate() &&
!IsSameFaviconURL(*candidate(),favicon.icon_url,
missing space at ,favicon....

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:71: // |icon_types| defines this FaviconHelper
supported icon types. It can be the
|icon_types| is a bitmask of the supported icon types (history::IconType).

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:74: // size is not matched; It can be 0 if there
is no preference, the icon from
; -> . or It -> it

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:77: int icon_types,
I don't think it makes sense to let folks specify an arbitrary set of types
here. Instead, define an enum in this class:

enum Types {
  FAVICON,
  TOUCH
};

and have the constructor take the enum. It shouldn't take a preferred_icon_size
either, that can be figured out from the type. Internally you'll map the type to
the set of IconTypes.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:100: std::vector<FaviconURL> candidates);
const std::vector<>&

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:104: DownloadRequest() {}
initialize icon_type and callback here.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:112: icon_type(icon_type) { }
{ } -> {}

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:120: static bool IsSameFaviconURL(const
FaviconURL& favicon_url,
This should be unix_hacker_style. Also, the name is a bit misleading. How about,
do_url_and_icon_match

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:189: FaviconURL* candidate() {
current_candidate()

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:218: std::vector<FaviconURL>
favicon_candidates_;
How about naming this urls_ ? If you change it, update current_candidate_idx_
too.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:221: size_t current_candidate_idx_;
idx -> index

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:204: void OnUpdateFaviconURL(int32
page_id, std::vector<FaviconURL> urls);
const&

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.h:23: FaviconSource(Profile* profile,
const std::string& favicon_host);
This isn't related to this patch, right? Please keep in a separate patch.

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages.cc
File chrome/common/icon_messages.cc (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages...
chrome/common/icon_messages.cc:42: 
remove one of these lines.

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages.h
File chrome/common/icon_messages.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages...
chrome/common/icon_messages.h:13: // The icon type in a page. The definition
must be same as history::IconType.
You should pull ViewHostMsg_UpdateFaviconURL into this file.

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages...
chrome/common/icon_messages.h:18: TOUCH_PRECOMPOSED_ICON = 1 << 2
Add a test that makes sure the values match.

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:1995: if (touch_icon_helper_.get())
{
no parens

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:804: std::vector<FaviconURL>
candidates);
const&

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:1089: scoped_ptr<FaviconHelper>
touch_icon_helper_;
Document may be NULL.

http://codereview.chromium.org/6672065/diff/12029/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6672065/diff/12029/content/renderer/render_vie...
content/renderer/render_view.cc:2125: // TODO : Get both favicon and touch icon
url, and send them to the browser.
You should only do this if the browser cares about them. You'll need something
similar to defaults, or perhaps you can pass it down as a pref. I'm not sure
what's the preferred approach.

[michaelbai, 2011-03-22 23:59:03]

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/defaults.h
File chrome/browser/defaults.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/defaults.h#n...
chrome/browser/defaults.h:86: extern bool enable_touch_icon;
On 2011/03/22 19:48:48, sky wrote:
> This should be above line 72, right?

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:32: preferred_icon_size_(preferred_icon_size) {
On 2011/03/22 19:48:48, sky wrote:
> Initialize current_candidate_idx too

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:127: size_t candidates_count =
favicon_candidates_.size();
On 2011/03/22 19:48:48, sky wrote:
> If candidates contains a FaviconURL whose type matches what you want don't you
> need to remove the existing one in favicon_candidates_? Similarly wouldn't you
> need to reset candidate_index_?

No, I don't think we should reset favicon_candidates_, according previous logic,
the url will be ignored if FetchFavicon hasn't been called.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:146: if (GetFaviconService() &&
got_favicon_from_history_ && (favicon_expired_ ||
On 2011/03/22 19:48:48, sky wrote:
> wrap favicon_expired_ onto a new line so that this is easier to read.

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:147: !history_icon_.is_valid() ||
!IsSameFaviconURL(*candidate(),
On 2011/03/22 19:48:48, sky wrote:
> Don't you need to check entry->favicon().is_valid() too? At least when the
type
> is favicon.

Rewrote according previous logic.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:154: bool message_handled = true;
On 2011/03/22 19:48:48, sky wrote:
> Do you need the message_handled stuff anymore?

Yes, ViewHostMsg_DidDownloadFavicon still need to be handled

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:206: if (favicon.known_icon &&
favicon.icon_type == history::FAVICON &&
On 2011/03/22 19:48:48, sky wrote:
> This doesn't check !entry->favicon().is_valid()

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:220: if (candidate() &&
!IsSameFaviconURL(*candidate(),favicon.icon_url,
On 2011/03/22 19:48:48, sky wrote:
> missing space at ,favicon....

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:71: // |icon_types| defines this FaviconHelper
supported icon types. It can be the
On 2011/03/22 19:48:48, sky wrote:
> |icon_types| is a bitmask of the supported icon types (history::IconType).

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:74: // size is not matched; It can be 0 if there
is no preference, the icon from
On 2011/03/22 19:48:48, sky wrote:
> ; -> . or It -> it

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:77: int icon_types,
On 2011/03/22 19:48:48, sky wrote:
> I don't think it makes sense to let folks specify an arbitrary set of types
> here. Instead, define an enum in this class:
> 
> enum Types {
>   FAVICON,
>   TOUCH
> };
> 
> and have the constructor take the enum. It shouldn't take a
preferred_icon_size
> either, that can be figured out from the type. Internally you'll map the type
to
> the set of IconTypes.

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:100: std::vector<FaviconURL> candidates);
On 2011/03/22 19:48:48, sky wrote:
> const std::vector<>&

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:104: DownloadRequest() {}
On 2011/03/22 19:48:48, sky wrote:
> initialize icon_type and callback here.

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:112: icon_type(icon_type) { }
On 2011/03/22 19:48:48, sky wrote:
> { } -> {}

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:120: static bool IsSameFaviconURL(const
FaviconURL& favicon_url,
On 2011/03/22 19:48:48, sky wrote:
> This should be unix_hacker_style. Also, the name is a bit misleading. How
about,
> do_url_and_icon_match

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:189: FaviconURL* candidate() {
On 2011/03/22 19:48:48, sky wrote:
> current_candidate()

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:218: std::vector<FaviconURL>
favicon_candidates_;
On 2011/03/22 19:48:48, sky wrote:
> How about naming this urls_ ? If you change it, update current_candidate_idx_
> too.

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:221: size_t current_candidate_idx_;
On 2011/03/22 19:48:48, sky wrote:
> idx -> index

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:204: void OnUpdateFaviconURL(int32
page_id, std::vector<FaviconURL> urls);
On 2011/03/22 19:48:48, sky wrote:
> const&

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.h:23: FaviconSource(Profile* profile,
const std::string& favicon_host);
On 2011/03/22 19:48:48, sky wrote:
> This isn't related to this patch, right? Please keep in a separate patch.

Move them to another CL

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages.cc
File chrome/common/icon_messages.cc (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages...
chrome/common/icon_messages.cc:42: 
On 2011/03/22 19:48:48, sky wrote:
> remove one of these lines.

Done.

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages.h
File chrome/common/icon_messages.h (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages...
chrome/common/icon_messages.h:13: // The icon type in a page. The definition
must be same as history::IconType.
On 2011/03/22 19:48:48, sky wrote:
> You should pull ViewHostMsg_UpdateFaviconURL into this file.

Probably not easy. Got error: redefinition of ‘class
ViewHostMsg_UpdateFaviconURL’

ViewHostMsg_UpdateFaviconURL needs to be defined out of '#ifdef #endif'guard,
though icon_messages.h is only included in favicon_helper.h which is only
included in tab_contents.h and prerender_content.h; but tab_contents.h is
included by a lot of files, it is not easy to find the loop.

I added a TODO

http://codereview.chromium.org/6672065/diff/12029/chrome/common/icon_messages...
chrome/common/icon_messages.h:18: TOUCH_PRECOMPOSED_ICON = 1 << 2
On 2011/03/22 19:48:48, sky wrote:
> Add a test that makes sure the values match.

Done.

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:1995: if (touch_icon_helper_.get())
{
On 2011/03/22 19:48:48, sky wrote:
> no parens

Done.

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:804: std::vector<FaviconURL>
candidates);
On 2011/03/22 19:48:48, sky wrote:
> const&

Done.

http://codereview.chromium.org/6672065/diff/12029/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:1089: scoped_ptr<FaviconHelper>
touch_icon_helper_;
On 2011/03/22 19:48:48, sky wrote:
> Document may be NULL.

Done.

http://codereview.chromium.org/6672065/diff/12029/content/renderer/render_vie...
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6672065/diff/12029/content/renderer/render_vie...
content/renderer/render_view.cc:2125: // TODO : Get both favicon and touch icon
url, and send them to the browser.
On 2011/03/22 19:48:48, sky wrote:
> You should only do this if the browser cares about them. You'll need something
> similar to defaults, or perhaps you can pass it down as a pref. I'm not sure
> what's the preferred approach.

I will revisit it after I get the webkit part done.

Thanks

[sky, 2011-03-23 00:04:35]

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:127: size_t candidates_count =
favicon_candidates_.size();
On 2011/03/22 23:59:03, michaelbai wrote:
> On 2011/03/22 19:48:48, sky wrote:
> > If candidates contains a FaviconURL whose type matches what you want don't
you
> > need to remove the existing one in favicon_candidates_? Similarly wouldn't
you
> > need to reset candidate_index_?
> 
> No, I don't think we should reset favicon_candidates_, according previous
logic,
> the url will be ignored if FetchFavicon hasn't been called. 

What happens if OnUpdateFaviconURL is invoked twice with different URLs?

[michaelbai, 2011-03-23 15:49:43]

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/12029/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:127: size_t candidates_count =
favicon_candidates_.size();
On 2011/03/23 00:04:35, sky wrote:
> On 2011/03/22 23:59:03, michaelbai wrote:
> > On 2011/03/22 19:48:48, sky wrote:
> > > If candidates contains a FaviconURL whose type matches what you want don't
> you
> > > need to remove the existing one in favicon_candidates_? Similarly wouldn't
> you
> > > need to reset candidate_index_?
> > 
> > No, I don't think we should reset favicon_candidates_, according previous
> logic,
> > the url will be ignored if FetchFavicon hasn't been called. 
> 
> What happens if OnUpdateFaviconURL is invoked twice with different URLs?

The new URL is appended to the list, the first URL has priority.
In which case the OnUpdateFaviconURL() could be invoked twice? What's the
expected behavior if so?

[michaelbai, 2011-03-23 17:09:01]

Got you, I updated CL, please check it again

[sky, 2011-03-23 22:49:31]

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:200: } else if (++current_url_index_ <
urls_.size() &&
What happens if between the time we ask for an icon the set of icons changes
(OnUpdateFaviconURL). Wouldn't we incorrectly increment current_url_index_?

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:202:
FetchFavicon(current_candidate()->icon_url);
If we get here you've incremented current_url_index_, but FetchFavicon resets it
and clears out the list.

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:103: : callback(NULL),
Move the constructor and destructor to favicon_helper.cc

http://codereview.chromium.org/6672065/diff/17004/ui/gfx/favicon_size.h
File ui/gfx/favicon_size.h (right):

http://codereview.chromium.org/6672065/diff/17004/ui/gfx/favicon_size.h#newco...
ui/gfx/favicon_size.h:17: static void calc_favicon_target_size(int target_size,
Do we really need these changes? Aren't you still always passing in
kFaviconSize?

[michaelbai, 2011-03-23 23:26:49]

Addressed all issues, please check it again.

Thanks

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:200: } else if (++current_url_index_ <
urls_.size() &&
On 2011/03/23 22:49:31, sky wrote:
> What happens if between the time we ask for an icon the set of icons changes
> (OnUpdateFaviconURL). Wouldn't we incorrectly increment current_url_index_?

Fixed, thanks very much!

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:202:
FetchFavicon(current_candidate()->icon_url);
On 2011/03/23 22:49:31, sky wrote:
> If we get here you've incremented current_url_index_, but FetchFavicon resets
it
> and clears out the list.

Sorry, I should pass the page_url in.

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/17004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:103: : callback(NULL),
On 2011/03/23 22:49:31, sky wrote:
> Move the constructor and destructor to favicon_helper.cc

Done.

http://codereview.chromium.org/6672065/diff/17004/ui/gfx/favicon_size.h
File ui/gfx/favicon_size.h (right):

http://codereview.chromium.org/6672065/diff/17004/ui/gfx/favicon_size.h#newco...
ui/gfx/favicon_size.h:17: static void calc_favicon_target_size(int target_size,
On 2011/03/23 22:49:31, sky wrote:
> Do we really need these changes? Aren't you still always passing in
> kFaviconSize?

I thought it might useful for touch_icon in the future. Reverted.

[sky, 2011-03-23 23:45:04]

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:70: urls_.empty();
clear

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:153: urls_.empty();
clear.

I can only catch so much in a review. You need to add some tests to make sure
this all works.

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:219: FetchFavicon(url_);
spacing is still off.

But I still don't get what you're doing here. If errored is true why do you want
to start the whole process over again? Worse yet, this is going to set
got_favicon_from_history_ to false, which will most likely never get set back to
true (well, it will if the page dynamically changes the urls, but that is rather
unlikely). What am I missing?

[michaelbai, 2011-03-25 22:52:30]

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:70: urls_.empty();
On 2011/03/23 23:45:04, sky wrote:
> clear

Done.

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:153: urls_.empty();
On 2011/03/23 23:45:04, sky wrote:
> clear.
> 
> I can only catch so much in a review. You need to add some tests to make sure
> this all works.

Done.

http://codereview.chromium.org/6672065/diff/27020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:219: FetchFavicon(url_);
On 2011/03/23 23:45:04, sky wrote:
> spacing is still off.
> 
> But I still don't get what you're doing here. If errored is true why do you
want
> to start the whole process over again? Worse yet, this is going to set
> got_favicon_from_history_ to false, which will most likely never get set back
to
> true (well, it will if the page dynamically changes the urls, but that is
rather
> unlikely). What am I missing?

Done.

[sky, 2011-03-28 15:02:58]

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:169: if (!favicon_expired_ &&
current_candidate()->icon_type == ::FAVICON &&
You have the check for favicon and !favicon a number of times here. Make this:

if (xxx == :FAVICON) {
  // the if you have on 169
  // then 175
} else 179
}

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:257: if (!errored)
{} around this.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:260: std::vector<FaviconURL>
new_candidates(++urls_.begin(), urls_.end());
Add a comment here as to why you do this.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:317:
static_cast<history::IconType>(current_candidate()->icon_type));
I know you have the test, but this cast still makes me nervous. Add a
TypeToHistoryType switch or something.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:374:
static_cast<history::IconType>(current_candidate()->icon_type), NULL);
Same thing with this cast.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:71: enum Types {
Types -> Type

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:106: virtual NavigationEntry* GetEntry();
You need to move all the implementations to match the header.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:108: // Asks the render to download favicon.
Document return value.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:142: friend class DownloadHandler; // For
testing
Instead of having so many friends, which is pain to maintain. Make one of your
classes (say TestFaviconHelper) a friend and have it expose everything you need.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:265: int preferred_icon_size_;
Nuke this and calculate it from icon_types_ when you need it. Create a
preferred_icon_size() function if you want, but no need to create the field. In
fact you should really create cache the Type specified, and calculate
icon_types_ when you need it too.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper_unittest.cc (right):

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:5: #include
"chrome/browser/favicon_helper.h"
The style of including the header first, then a newline and the rest of the
includes only applies to favicon_helper.cc, not the test file. This header
should be sorted with the rest of them.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:15: void FillDataToBitmap(int w, int
h, SkBitmap* bmp) {
Static or put in a namespace and add a description.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:29: void FillBitmap(int w, int h,
std::vector<unsigned char>* output) {
Same here.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:35: class DownloadHandler {
Move into anonymous namespace, and add description.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:64: int download_id_;
Descriptions?

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:67: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:69: class HistoryRequestHandler {
Move into anonymous namespace, and add description.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:103: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:113: entry_.set_url(page_url);
Initialize download_image_size_.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:123: void
set_history_handler(HistoryRequestHandler* handler) {
Method ownership of these setters.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:140: virtual void
UpdateFaviconMappingAndFetch(
Add OVERRIDE on all these methods.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:198: NavigationEntry entry_;
Descriptions?

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:203: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6672065/diff/22004/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6672065/diff/22004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:96: #include
"ui/gfx/favicon_size.h"
Is this include needed?

http://codereview.chromium.org/6672065/diff/22004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:395: if
(browser_defaults::kEnableTouchIcon) {
no {}

[michaelbai, 2011-03-29 00:00:50]

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:169: if (!favicon_expired_ &&
current_candidate()->icon_type == ::FAVICON &&
On 2011/03/28 15:02:58, sky wrote:
> You have the check for favicon and !favicon a number of times here. Make this:
> 
> if (xxx == :FAVICON) {
>   // the if you have on 169
>   // then 175
> } else 179
> }

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:257: if (!errored)
On 2011/03/28 15:02:58, sky wrote:
> {} around this.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:260: std::vector<FaviconURL>
new_candidates(++urls_.begin(), urls_.end());
On 2011/03/28 15:02:58, sky wrote:
> Add a comment here as to why you do this.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:317:
static_cast<history::IconType>(current_candidate()->icon_type));
On 2011/03/28 15:02:58, sky wrote:
> I know you have the test, but this cast still makes me nervous. Add a
> TypeToHistoryType switch or something.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:374:
static_cast<history::IconType>(current_candidate()->icon_type), NULL);
On 2011/03/28 15:02:58, sky wrote:
> Same thing with this cast.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:71: enum Types {
On 2011/03/28 15:02:58, sky wrote:
> Types -> Type

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:106: virtual NavigationEntry* GetEntry();
On 2011/03/28 15:02:58, sky wrote:
> You need to move all the implementations to match the header.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:108: // Asks the render to download favicon.
On 2011/03/28 15:02:58, sky wrote:
> Document return value.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:142: friend class DownloadHandler; // For
testing
On 2011/03/28 15:02:58, sky wrote:
> Instead of having so many friends, which is pain to maintain. Make one of your
> classes (say TestFaviconHelper) a friend and have it expose everything you
need.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:265: int preferred_icon_size_;
On 2011/03/28 15:02:58, sky wrote:
> Nuke this and calculate it from icon_types_ when you need it. Create a
> preferred_icon_size() function if you want, but no need to create the field.
In
> fact you should really create cache the Type specified, and calculate
> icon_types_ when you need it too.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
File chrome/browser/favicon_helper_unittest.cc (right):

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:5: #include
"chrome/browser/favicon_helper.h"
On 2011/03/28 15:02:58, sky wrote:
> The style of including the header first, then a newline and the rest of the
> includes only applies to favicon_helper.cc, not the test file. This header
> should be sorted with the rest of them.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:15: void FillDataToBitmap(int w, int
h, SkBitmap* bmp) {
On 2011/03/28 15:02:58, sky wrote:
> Static or put in a namespace and add a description.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:29: void FillBitmap(int w, int h,
std::vector<unsigned char>* output) {
On 2011/03/28 15:02:58, sky wrote:
> Same here.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:35: class DownloadHandler {
On 2011/03/28 15:02:58, sky wrote:
> Move into anonymous namespace, and add description.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:64: int download_id_;
On 2011/03/28 15:02:58, sky wrote:
> Descriptions?

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:67: };
On 2011/03/28 15:02:58, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:69: class HistoryRequestHandler {
On 2011/03/28 15:02:58, sky wrote:
> Move into anonymous namespace, and add description.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:103: };
On 2011/03/28 15:02:58, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:113: entry_.set_url(page_url);
On 2011/03/28 15:02:58, sky wrote:
> Initialize download_image_size_.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:123: void
set_history_handler(HistoryRequestHandler* handler) {
On 2011/03/28 15:02:58, sky wrote:
> Method ownership of these setters.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:140: virtual void
UpdateFaviconMappingAndFetch(
On 2011/03/28 15:02:58, sky wrote:
> Add OVERRIDE on all these methods.

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:198: NavigationEntry entry_;
On 2011/03/28 15:02:58, sky wrote:
> Descriptions?

Done.

http://codereview.chromium.org/6672065/diff/22004/chrome/browser/favicon_help...
chrome/browser/favicon_helper_unittest.cc:203: };
On 2011/03/28 15:02:58, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6672065/diff/22004/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6672065/diff/22004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:96: #include
"ui/gfx/favicon_size.h"
On 2011/03/28 15:02:58, sky wrote:
> Is this include needed?

Done.

http://codereview.chromium.org/6672065/diff/22004/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:395: if
(browser_defaults::kEnableTouchIcon) {
On 2011/03/28 15:02:58, sky wrote:
> no {}

Done.

[sky, 2011-03-29 16:07:22]

LGTM with the following nits fixed.

  -Scott

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:98: preferred_icon_size() == image.height())) ?
align with p on previous line, eg:
   (preferred_icon_size() == image.width() &&
    preferred_icon_ ...

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:173: if (!favicon_expired_ &&
current_candidate()->icon_type != ::FAVICON &&
This should be an else from 163, eg:

if (current_candidate()->icon_type == ::FAVICON) {
  ...
} else if (!favicon_expired_ && got_favicon_from_history_  ..

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:231: int preferred_icon_size() {
const

[michaelbai, 2011-04-08 22:33:34]

Hi Avi,

Would you like help to review this CL

Thanks

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.cc (right):

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:98: preferred_icon_size() == image.height())) ?
On 2011/03/29 16:07:22, sky wrote:
> align with p on previous line, eg:
>    (preferred_icon_size() == image.width() &&
>     preferred_icon_ ...

Done.

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.cc:173: if (!favicon_expired_ &&
current_candidate()->icon_type != ::FAVICON &&
On 2011/03/29 16:07:22, sky wrote:
> This should be an else from 163, eg:
> 
> if (current_candidate()->icon_type == ::FAVICON) {
>   ...
> } else if (!favicon_expired_ && got_favicon_from_history_  ..

Done.

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
File chrome/browser/favicon_helper.h (right):

http://codereview.chromium.org/6672065/diff/44020/chrome/browser/favicon_help...
chrome/browser/favicon_helper.h:231: int preferred_icon_size() {
On 2011/03/29 16:07:22, sky wrote:
> const

Done.

[michaelbai, 2011-04-08 22:51:22]

Hi Brettw,

As you are the owner of content/renderer/render_view.cc, Could you help to
review the CL?

[Avi (use Gerrit), 2011-04-09 02:48:50]

I feel extremely awkward here; as I'm a little confused by the design, and
having been asked to comment I feel like I'm bikeshedding.

You're instantiating two copies of the FaviconHelper, each identical save the
fact that they're fetching two different types of icons. Because of the fact
that they need to coordinate, you had to hoist an IPC message out of the class
that used to handle it (the FaviconHelper) into a class that contains it, and
have it know intimate details of what used to be an implementation detail.

Why do we have two FaviconHelpers? Why can't we just have one that knows how to
do both types of icons?

(Apologies if I'm missing something...)

[michaelbai, 2011-04-11 16:04:53]

I think you may worried about the favicon message handler in TabContent. This
issue has been discussed, we were agreed to handling it when someone works on
decoupling the favicon from TabContent as I already ran out of time for this
feature.

Thanks for your understanding.


On 2011/04/09 02:48:50, Avi wrote:
> I feel extremely awkward here; as I'm a little confused by the design, and
> having been asked to comment I feel like I'm bikeshedding.
> 
> You're instantiating two copies of the FaviconHelper, each identical save the
> fact that they're fetching two different types of icons. Because of the fact
> that they need to coordinate, you had to hoist an IPC message out of the class
> that used to handle it (the FaviconHelper) into a class that contains it, and
> have it know intimate details of what used to be an implementation detail.
> 
> Why do we have two FaviconHelpers? Why can't we just have one that knows how
to
> do both types of icons?
> 
> (Apologies if I'm missing something...)

[Avi (use Gerrit), 2011-04-11 16:24:52]

On 2011/04/11 16:04:53, michaelbai wrote:
> I think you may worried about the favicon message handler in TabContent.

I am.

> we were agreed to handling it when someone works on
> decoupling the favicon from TabContent as I already ran out of time for this
> feature.

_I_ am the one who's decoupling the favicon from TabContent. I had committed a
patch to do so (and then had to revert it for unrelated dependency issues I'm
working on now). What are your plans to restore this encapsulation? I'll handle
moving the favicon to TCW, but would appreciate your help in restoring the
broken encapsulation first. (If you want to commit this CL, OK, but please then
proceed to make a new CL to fix this.)

Avi

[michaelbai, 2011-04-11 17:00:09]

Sure, I will have a new CL to fix this.


On 2011/04/11 16:24:52, Avi wrote:
> On 2011/04/11 16:04:53, michaelbai wrote:
> > I think you may worried about the favicon message handler in TabContent.
> 
> I am.
> 
> > we were agreed to handling it when someone works on
> > decoupling the favicon from TabContent as I already ran out of time for this
> > feature.
> 
> _I_ am the one who's decoupling the favicon from TabContent. I had committed a
> patch to do so (and then had to revert it for unrelated dependency issues I'm
> working on now). What are your plans to restore this encapsulation? I'll
handle
> moving the favicon to TCW, but would appreciate your help in restoring the
> broken encapsulation first. (If you want to commit this CL, OK, but please
then
> proceed to make a new CL to fix this.)
> 
> Avi

[Avi (use Gerrit), 2011-04-11 17:50:46]

On 2011/04/11 17:00:09, michaelbai wrote:
> Sure, I will have a new CL to fix this.

I mean immediately after committing this. Not after two weeks.

[Avi (use Gerrit), 2011-04-11 17:53:57]

As discussed in chat, a new CL will be forthcoming to address the encapsulation
issue. If everyone else's issues are addressed, we'll call this one LGTM.

[brettw, 2011-04-11 18:06:58]

content LGTM