Pass in a set of requested favicon pixel sizes to the HistoryService

Pass in a set of requested favicon pixel sizes to the HistoryService instead of a desired size in DIP and a set of desired scale factors.

BUG=None
TEST=None

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

[pkotwicz, 2014-06-18 04:10:54]

Scott, can you please take a look?

I was motivated to write this CL as a result of
https://codereview.chromium.org/335233003/

It should be possible for most of the FaviconService methods if not all not to
need a ui::ScaleFactor parameter (and not need a float scale parameter either).
I am hoping that oshima@ will look into that (given that he is working on
removing the usage of ui::ScaleFactor from favicon related code)

https://codereview.chromium.org/336423006/diff/40001/components/favicon_base/...
File components/favicon_base/select_favicon_frames.cc (right):

https://codereview.chromium.org/336423006/diff/40001/components/favicon_base/...
components/favicon_base/select_favicon_frames.cc:70: const int kHugeEdgeSize =
desired_size * 8;
The old cold is correct while the old comment is not.

When 16x16 and 32x32 favicon bitmaps are requested the old code avoids returning
huge bitmaps (128x128 and larger) in the majority of cases.

[oshima, 2014-06-18 07:03:03]

Thank you for doing this. 

I'm actually wondering if ChromeWebUIController should keep using scales
instead. I'll look into this tomorrow again.

Can you review my change btw? Conflicts looks simple and will rebase after your
change is landed.

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

https://codereview.chromium.org/336423006/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:66: std::vector<int> GetPixelSizes(
looks like all calls are just passing GetFaviconScaleFactors().
Can you just do

std::vector<int> GetPixelSizesForFaviconScales(int size_in_dip); ?

https://codereview.chromium.org/336423006/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/336423006/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:601:
std::vector<ui::ScaleFactor> scale_factors = ui::GetSupportedScaleFactors();
Does this have to be GetFaviconScaleFactors() ?

[oshima, 2014-06-18 16:04:31]

On 2014/06/18 07:03:03, oshima wrote:
> Thank you for doing this. 
> 
> I'm actually wondering if ChromeWebUIController should keep using scales
> instead. I'll look into this tomorrow again.

I think it's better to use scale factor in ChromeWebUIController because it's
simpler and
it doesn't seem to affect history backend change.

> 
> Can you review my change btw? Conflicts looks simple and will rebase after
your
> change is landed.
> 
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/favicon/f...
> File chrome/browser/favicon/favicon_service.cc (right):
> 
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/favicon/f...
> chrome/browser/favicon/favicon_service.cc:66: std::vector<int> GetPixelSizes(
> looks like all calls are just passing GetFaviconScaleFactors().
> Can you just do
> 
> std::vector<int> GetPixelSizesForFaviconScales(int size_in_dip); ?
> 
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):
> 
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:601:
> std::vector<ui::ScaleFactor> scale_factors = ui::GetSupportedScaleFactors();
> Does this have to be GetFaviconScaleFactors() ?

[pkotwicz, 2014-06-18 17:31:05]

I have addressed Oshima's comments. Oshima can you please take another look?

[oshima, 2014-06-18 17:36:40]

On 2014/06/18 16:04:31, oshima wrote:
> On 2014/06/18 07:03:03, oshima wrote:
> > Thank you for doing this. 
> > 
> > I'm actually wondering if ChromeWebUIController should keep using scales
> > instead. I'll look into this tomorrow again.
> 
> I think it's better to use scale factor in ChromeWebUIController because it's
> simpler and
> it doesn't seem to affect history backend change.

You probably missed this comment?

> 
> > 
> > Can you review my change btw? Conflicts looks simple and will rebase after
> your
> > change is landed.
> > 
> >
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/favicon/f...
> > File chrome/browser/favicon/favicon_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/favicon/f...
> > chrome/browser/favicon/favicon_service.cc:66: std::vector<int>
GetPixelSizes(
> > looks like all calls are just passing GetFaviconScaleFactors().
> > Can you just do
> > 
> > std::vector<int> GetPixelSizesForFaviconScales(int size_in_dip); ?
> > 
> >
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/ui/webui/...
> > File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/336423006/diff/40001/chrome/browser/ui/webui/...
> > chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:601:
> > std::vector<ui::ScaleFactor> scale_factors = ui::GetSupportedScaleFactors();
> > Does this have to be GetFaviconScaleFactors() ?

[pkotwicz, 2014-06-18 18:00:51]

I think that it is actually simpler not to use scale factor in
ChromeWebUIController. The desired scale factors are no longer passed to
FaviconService::GetFaviconForPageURLImpl().

My hope is that all of the public methods in FaviconService would either:
- take in a desired pixel size.
  e.g. FaviconService::GetRawFavicon(icon_url, icon_type, desired_size_in_dip,
desired_scale_factor, callback, tracker)
       becomes
       FaviconService::GetRawFavicon(icon_url, icon_type, desired_size_in_pixel,
callback, tracker)

- take in neither a desired dip size nor a desired size in DIP
  e.g. FaviconService::GetFaviconForPageURL(params, callback, tracker)
       becomes
       FaviconService::GetFaviconForPageURL(page_url, icon_types, callback,
tracker)

[oshima, 2014-06-18 18:17:03]

lgtm

Ok, after reading through the code, I think this is more correct thing to do.

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:627:
bitmap_result.pixel_size = candidate_sizes[selected_index];
I believe someone has to scale this to the desired size in order to get the
expected result. Are we doing this now? If not, can you add TODO?

[pkotwicz, 2014-06-18 18:38:50]

Scott for OWNERS

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:627:
bitmap_result.pixel_size = candidate_sizes[selected_index];
Yes, we do resizing in
FaviconService::RunFaviconImageCallbackWithBitmapResults() and
FaviconService::RunFaviconImageCallbackWithBitmapResults()

[pkotwicz, 2014-06-18 18:39:56]

Correction, we do resizing in
FaviconService::RunFaviconRawBitmapCallbackWithBitmapResults() and
FaviconService::RunFaviconImageCallbackWithBitmapResults()

[sky, 2014-06-18 22:16:51]

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

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:124:
static_cast<int>(desired_size_in_dip * desired_scale + 0.5);
Why do add .5 here? I see this comes from old code, but can you add a comment
please.

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:203:
static_cast<int>(params.desired_size_in_dip * desired_scale + 0.5f);
Same comment here about .5. In fact how about factory the two into a common
function, GetDesiredSizeInPixel().

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:691: // most closely match
|desired_sizes|. If |desired_sizes| has a '0' entry, the
Would an empty vector be a better way to indicate wanting the biggest entry? Do
we ever have the case where 0 is in the vector too?

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/create_application_shortcut_view.cc (right):

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/create_application_shortcut_view.cc:504:
static_cast<int>(requested_size * scale + 0.5f));
Same question about the .5 here.

[pkotwicz, 2014-06-18 23:32:02]

Scott, can you please take another look?

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

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_service.cc:124:
static_cast<int>(desired_size_in_dip * desired_scale + 0.5);
I believe that rounding up is the convention when resizing a bitmap. (e.g. in
gfx::Canvas we round up to get the canvas size from the DIP size. Same in
ImageSkiaOperations::CreateResizedImage())

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/history/h...
File chrome/browser/history/history_backend.h (right):

https://codereview.chromium.org/336423006/diff/60001/chrome/browser/history/h...
chrome/browser/history/history_backend.h:691: // most closely match
|desired_sizes|. If |desired_sizes| has a '0' entry, the
Yes! But in a subsequent CL. I added a TODO in select_favicon_frames.h

[sky, 2014-06-19 15:05:55]

LGTM

[pkotwicz, 2014-06-19 20:39:30]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-06-19 20:41:50]

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

[commit-bot: I haz the power, 2014-06-20 01:56:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-20 02:02:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 02:02:36]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[oshima, 2014-06-20 04:28:19]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-06-20 04:31:17]

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

[commit-bot: I haz the power, 2014-06-20 07:08:14]

Change committed as 278640