Restore favicons to chrome://extensions and chrome://bookmarks page (among probably other things).

Restore favicons to chrome://extensions and chrome://bookmarks page (among probably other things).

This regressed with r81258 (http://codereview.chromium.org/6672065/).

BUG=79296
TEST=Open chrome://extensions and note the right favicon (puzzle piece, not the globe).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88652

[Finnur, 2011-06-08 14:19:11]

I did a search through the code base and I think I caught all the cases...

[Finnur, 2011-06-09 11:40:35]

ping

[michaelbai, 2011-06-09 15:49:44]

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

http://codereview.chromium.org/7046049/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_backend.cc:1719: favicon.icon_type =
history::FAVICON;
The favicon.icon_type should be returned from line 1708. Did you see the
favicon.icon_type was zero?

http://codereview.chromium.org/7046049/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_backend.cc:1757: favicon.icon_type =
history::FAVICON;
The favicon.icon_type should be set in line 1755.

http://codereview.chromium.org/7046049/diff/1/chrome/browser/ui/webui/chrome_...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/7046049/diff/1/chrome/browser/ui/webui/chrome_...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:290: favicon.icon_type =
history::FAVICON;
Would the issue be fixed by only adding this line?

[Finnur, 2011-06-09 16:01:56]

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

http://codereview.chromium.org/7046049/diff/1/chrome/browser/history/history_...
chrome/browser/history/history_backend.cc:1757: favicon.icon_type =
history::FAVICON;
Ah, yes. You are right. I'll remove this file entirely from the CL.

http://codereview.chromium.org/7046049/diff/1/chrome/browser/ui/webui/chrome_...
File chrome/browser/ui/webui/chrome_web_ui_factory.cc (right):

http://codereview.chromium.org/7046049/diff/1/chrome/browser/ui/webui/chrome_...
chrome/browser/ui/webui/chrome_web_ui_factory.cc:290: favicon.icon_type =
history::FAVICON;
On 2011/06/09 15:49:44, michaelbai wrote:
> Would the issue be fixed by only adding this line?

Yes. The other two were found by searching. I guess I didn't look closely
enough.

I have uploaded a new CL.

[michaelbai, 2011-06-09 16:10:50]

LGTM, Thanks, but you probably need to get somebody else's approval.

[Finnur, 2011-06-09 16:16:54]

Evan, can you give an owner's LG on this?

On 2011/06/09 16:10:50, michaelbai wrote:
> LGTM, Thanks, but you probably need to get somebody else's approval.

[Evan Stade, 2011-06-09 22:26:43]

lgtm