Change FaviconData to be able to return data for multiple bitmaps for same icon URL

Change FaviconData to be able to return data for multiple bitmaps for same icon URL.
Add methods to FaviconService to get gfx::Image more easily
from Favicon.

BUG=138553
Test=Compiles, FaviconHandlerTest.* pass.

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

[sky, 2012-08-23 15:50:37]

The only think keeping me from approving is the change in
HistoryBackend::GetFaviconFromDB.

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

http://codereview.chromium.org/10870022/diff/1/chrome/browser/favicon/favicon...
chrome/browser/favicon/favicon_service.h:46: typedef base::Callback<
As discussed over IM, this is temporary. Long term we should have a separate
callback for the common case that doesn't pass in the vector. In fact the common
case is likely a gfx::Image and that's it.

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

http://codereview.chromium.org/10870022/diff/1/chrome/browser/history/history...
chrome/browser/history/history_backend.cc:2462: favicon_data->known_icon = true;
How come this is set even if we don't have a favicon?

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

http://codereview.chromium.org/10870022/diff/1/chrome/browser/history/history...
chrome/browser/history/history_types.cc:434: return bitmap_data.get() &&
bitmap_data->size();
inline this.

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

http://codereview.chromium.org/10870022/diff/1/chrome/browser/history/history...
chrome/browser/history/history_types.h:802: // The sizes at which the favicon is
available at from the web. For instance
'available at' -> available

http://codereview.chromium.org/10870022/diff/1/chrome/browser/history/history...
chrome/browser/history/history_types.h:805: // have entries for each entry in
|sizes|.
Document ordering of sizes.

http://codereview.chromium.org/10870022/diff/1/chrome/browser/history/history...
chrome/browser/history/history_types.h:808: // The bitmaps whose pixel sizes
best match the optimal favicon size of 16x16
Won't this depend upon the query?

http://codereview.chromium.org/10870022/diff/1/chrome/browser/ui/cocoa/histor...
File chrome/browser/ui/cocoa/history_menu_bridge.mm (right):

http://codereview.chromium.org/10870022/diff/1/chrome/browser/ui/cocoa/histor...
chrome/browser/ui/cocoa/history_menu_bridge.mm:483: if
(gfx::PNGCodec::Decode(bitmap_data->front(),
Would be nice if there was a method on favicondata for this, but that can be
added later.

http://codereview.chromium.org/10870022/diff/1/chrome/browser/ui/webui/ntp/ap...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/10870022/diff/1/chrome/browser/ui/webui/ntp/ap...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:775: web_app->icons[0].url =
GURL();
I realize your copying code, but is this line necessary?

[pkotwicz, 2012-08-27 02:24:35]

Scott, can you please take a look? I ended up changing
FaviconService::GetFavicon() and FaviconService::GetFaviconURL() to return more
usable callback params.

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

http://codereview.chromium.org/10870022/diff/1/chrome/browser/history/history...
chrome/browser/history/history_types.h:805: // have entries for each entry in
|sizes|.
My implementation doesn't specify an ordering of sizes. I guess I should mention
that?

[sky, 2012-08-27 14:52:01]

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_service.h (right):

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_service.h:37: // |desired_size_in_dip| at the
scale factors supported by this platform
this -> the current

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_service.h:127: // supported by this plaform (eg
MacOS) are requested for
this -> the current

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/jumplist_win.h
File chrome/browser/jumplist_win.h (right):

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/jumplist_win...
chrome/browser/jumplist_win.h:54: SkBitmap data() const { return data_; }
const SkBitmap&

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (left):

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:776: web_app->icons.clear();
Don't you need this else branch?

[pkotwicz, 2012-08-27 19:35:03]

Changes as requested. Improved a couple of comments.

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (left):

http://codereview.chromium.org/10870022/diff/6003/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:776: web_app->icons.clear();
|web_app| is constructed on line 761, so I don't think we need the else branch.

[sky, 2012-08-27 20:50:11]

LGTM

[commit-bot: I haz the power, 2012-08-27 21:02:01]

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

[commit-bot: I haz the power, 2012-08-27 21:22:57]

Try job failure for 10870022-7013 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-28 14:18:59]

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

[commit-bot: I haz the power, 2012-08-28 17:51:05]

Change committed as 153695

[commit-bot: I haz the power, 2012-08-29 23:58:29]

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

[commit-bot: I haz the power, 2012-08-30 04:40:27]

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

[commit-bot: I haz the power, 2012-08-30 06:19:52]

Try job failure for 10870022-28001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-30 06:23:32]

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

[commit-bot: I haz the power, 2012-08-30 06:23:46]

Failed to apply patch for chrome/browser/history/history_types.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/history/history_types.cc
Hunk #1 FAILED at 90.
Hunk #2 succeeded at 402 (offset -27 lines).
Hunk #3 succeeded at 430 (offset -27 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/history/history_types.cc.rej

[commit-bot: I haz the power, 2012-08-30 13:23:40]

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

[commit-bot: I haz the power, 2012-08-30 15:43:53]

Change committed as 154157

[michaelbai, 2012-08-31 21:41:14]

https://chromiumcodereview.appspot.com/10870022/diff/28001/chrome/browser/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://chromiumcodereview.appspot.com/10870022/diff/28001/chrome/browser/fav...
chrome/browser/favicon/favicon_service.h:159: const FaviconRawCallback&
callback);
I don't understand this method, Is favicon_id the unique ID of a favicon?

If I request a specific favicon by ID, what's the purpose of desired_size_in_dip
and desired_scale_factor?