[Refactor] Change FaviconService API to take in a desired pixel size

Refactor FaviconService API to take in a desired pixel size instead of a
desired size in DIP pixels and a desired scale.

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282840

[pkotwicz, 2014-07-01 00:56:17]

Scott, can you please take a look?

I will refactor FaviconHandler in an upcoming CL. (I will improve the comments
for the API methods used by FaviconHandler at that time)

I also changed the ordering of the methods in FaviconService such that the
methods that callers
other than FaviconHandler use are at the top.

[sky, 2014-07-09 17:33:45]

Can you remind me why this is better?

[pkotwicz, 2014-07-09 18:11:54]

I think that it makes the FaviconService API simpler to understand. Now that the
FaviconService API uses a float scale instead of a ui::ScaleFactor enum, passing
in both a desired size in DIP and a float scale is somewhat pointless.

[sky, 2014-07-10 00:03:39]

IMO the more confusing thing we have now is the methods names of FaviconService.
For example, GetFaviconImage means get the image of type favicon, but
GetFaviconForPageURL lets you specify the types you care about. Since this class
is named FaviconService having favicon in the name for anything is redundant.
I'm not too thrilled about rawimage either. I tend to favicon a simple GetImage,
but doing this implies lots of other renaming in FAviconService.

https://codereview.chromium.org/331163003/diff/60002/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java
(right):

https://codereview.chromium.org/331163003/diff/60002/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java:80:
int desiredSizeInPixel, FaviconImageCallback faviconImageCallback) {
Were all call sites already passing in pixels?

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/android/f...
File chrome/browser/android/favicon_helper.cc (right):

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/android/f...
chrome/browser/android/favicon_helper.cc:111: jint j_desired_size_in_pixel,
Were all call sites already passing in pixels?

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:253: 
nit: onlye one newline.

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.h:41: // |callback| is run when the
favicon has been fetched. It is always run
Move |callback| documentation to class description since it's generally
applicable.

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/notificat...
File chrome/browser/notifications/message_center_settings_controller.cc (left):

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/notificat...
chrome/browser/notifications/message_center_settings_controller.cc:276:
favicon_base::FAVICON | favicon_base::TOUCH_ICON,
Don't you lose the TOUCH_ICON with this change?

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:52: // Requests the largest
favicon of type favicon_base::TOUCH_ICON in the
Can we pass in favicon_base::IconType so that we only have one method?

[pkotwicz, 2014-07-11 01:23:09]

Scott, can you please take another look?

I will leave renaming the methods in FaviconService for a followup CL. (I do not
think that it makes sense to do in this CL)

Ideally, we would have
FaviconService::GetImageForIconURL(
    const GURL& icon_url,
    const favicon_base::FaviconImageCallback& callback,
    base::CancelableTaskTracker* tracker);

and

FaviconService::GetLargestPNGDataForIconURL(
    const GURL& icon_url,
    favicon_base::IconType icon_type,
    const favicon_base::FaviconRawBitmapCallback& callback,
    base::CancelableTaskTracker* tracker);

The performance difference in using FaviconService::GetFaviconImage() instead of
FaviconService::GetRawFavicon() when only the 1x data is used is negligible.

However, we need to keep FaviconService::GetRawFavicon() for Android

Wrt to FaviconService::GetFavicon(), in the short term it is worth making the
method name look scarier than it does now. It should only be used by
FaviconHandler and no one else. FaviconHandler needs to be improved....

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/android/f...
File chrome/browser/android/favicon_helper.cc (right):

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/android/f...
chrome/browser/android/favicon_helper.cc:111: jint j_desired_size_in_pixel,
In the implementation of OnLocalFaviconAvailable() on the left,
gfx::Image::AsBitmap() is used to get to the SkBitmap to return. AsBitmap()
returns the 1x bitmap.

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/notificat...
File chrome/browser/notifications/message_center_settings_controller.cc (left):

https://codereview.chromium.org/331163003/diff/60002/chrome/browser/notificat...
chrome/browser/notifications/message_center_settings_controller.cc:276:
favicon_base::FAVICON | favicon_base::TOUCH_ICON,
TOUCH_ICON is android and IOS only and based on build/common.gypi neither uses
the message center.

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:52: // Requests the largest
favicon of type favicon_base::TOUCH_ICON in the
The main reason that I split the method into two is because it was confusing to
explain that for a request of
favicon_base::FAVICON the returned gfx::Image is a multi-resolution 16x16 DIP
favicon
favicon_base::TOUCH_ICON the returned gfx::Image is a single resolution image
which is of the largest size available in the database.

[sky, 2014-07-11 16:26:51]

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:52: // Requests the largest
favicon of type favicon_base::TOUCH_ICON in the
On 2014/07/11 01:23:08, pkotwicz wrote:
> The main reason that I split the method into two is because it was confusing
to
> explain that for a request of
> favicon_base::FAVICON the returned gfx::Image is a multi-resolution 16x16 DIP
> favicon
> favicon_base::TOUCH_ICON the returned gfx::Image is a single resolution image
> which is of the largest size available in the database.
> 

This interface is very narrowly used, so I'm fine with cryptic wording.

[pkotwicz, 2014-07-11 18:47:45]

Scott, can you please take another look?

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_client.h (right):

https://codereview.chromium.org/331163003/diff/60002/components/bookmarks/bro...
components/bookmarks/browser/bookmark_client.h:52: // Requests the largest
favicon of type favicon_base::TOUCH_ICON in the
Done!

[sky, 2014-07-11 19:32:56]

LGTM

[pkotwicz, 2014-07-11 21:47:43]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-11 21:49:09]

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

[pkotwicz, 2014-07-11 23:39:32]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-11 23:40:29]

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

[pkotwicz, 2014-07-12 15:25:04]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-07-12 15:25:49]

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

[commit-bot: I haz the power, 2014-07-12 23:00:29]

Change committed as 282840