Convert ui::ScaleFactor -> float in favicon/history code

Convert ui::ScaleFactor -> float in favicon/history code

First stop to fix favicon for fractional scale factor.

I'll convert the rest of ui::ScaleFactor in 38

BUG=376367, 381601
R=pkotwicz@chromium.org, sky@chromium.org

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

[pkotwicz, 2014-06-17 17:01:41]

I am looking into how hard it is to pass in the desired pixel sizes to
chrome/browser/history (v.s. desired DIP size + scale factors)

[oshima, 2014-06-17 17:10:08]

On 2014/06/17 17:01:41, pkotwicz wrote:
> I am looking into how hard it is to pass in the desired pixel sizes to
> chrome/browser/history (v.s. desired DIP size + scale factors)

That'd be separate CL.

[pkotwicz, 2014-06-17 20:04:59]

It turns out to be pretty easy to do. I am going to post a CL which does this
and on which this in a couple of hours. This CL should depend on that CL.

[pkotwicz, 2014-06-17 20:05:34]

It turns out to be pretty easy to do. I am going to post a CL which does this
 in a couple of hours. This CL should depend on that CL.

[pkotwicz, 2014-06-17 20:05:37]

It turns out to be pretty easy to do. I am going to post a CL which does this
 in a couple of hours. This CL should depend on that CL.

[pkotwicz, 2014-06-18 15:46:34]

Some initial comments.  Correct me if I am wrong. This CL will not make favicons
 in WebUI look any better for fractional scale factors than the way they look
today.

As such, I am going to wait to review this CL given
thathttps://codereview.chromium.org/336423006/ will reduce the size of the CL by
half

https://codereview.chromium.org/335233003/diff/140001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/335233003/diff/140001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.cc:128: int edge_size_in_pixel =
floor(desired_size_in_dip * favicon_scales[i]);
This should be ceil()

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
File components/favicon_base/favicon_util.cc (left):

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.cc:162: ui::SCALE_FACTOR_100P);
The TODO for ios should stay

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
File components/favicon_base/favicon_util.cc (right):

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.cc:58: int pixel_size =
std::floor(favicon_size * favicon_scales[i]);
We should ceil instead.

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.cc:185: for (size_t i = 0; i <
png_reps.size(); ++i) {
You can use std::find() here now?

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.h:20: // different from scales derived from
ui::GetSupportedScaleFactors()
Nit: scales from (not scales derived from)

"scales retrieved from" is also ok

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

Yes, I purposely made this CL not to change the behavior.
I actually have additional change (to do so) in
chrome_web_ui_controller_factory.cc

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
File components/favicon_base/favicon_util.cc (left):

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.cc:162: ui::SCALE_FACTOR_100P);
On 2014/06/18 15:46:33, pkotwicz wrote:
> The TODO for ios should stay

Done.

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
File components/favicon_base/favicon_util.cc (right):

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.cc:58: int pixel_size =
std::floor(favicon_size * favicon_scales[i]);
On 2014/06/18 15:46:33, pkotwicz wrote:
> We should ceil instead.

Done.

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.cc:185: for (size_t i = 0; i <
png_reps.size(); ++i) {
On 2014/06/18 15:46:33, pkotwicz wrote:
> You can use std::find() here now?

let me do it in separate CL. I'd like to keep this CL pure ui::ScaleFactor ->
float conversion.

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
components/favicon_base/favicon_util.h:20: // different from scales derived from
ui::GetSupportedScaleFactors()
On 2014/06/18 15:46:33, pkotwicz wrote:
> Nit: scales from (not scales derived from)
> 
> "scales retrieved from" is also ok

Done.

[oshima, 2014-06-18 21:24:37]

On 2014/06/18 17:17:20, oshima wrote:
> Yes, I purposely made this CL not to change the behavior.
> I actually have additional change (to do so) in
> chrome_web_ui_controller_factory.cc

I take this word back. I did keep the CL simple conversion, and this should
create the correct sized favicon
for fractional scale factor when singgle specific scale factor is requested
(like 1.5).
It used to be trancated to 2.0 (or 1.0).

> 
>
https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
> File components/favicon_base/favicon_util.cc (left):
> 
>
https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
> components/favicon_base/favicon_util.cc:162: ui::SCALE_FACTOR_100P);
> On 2014/06/18 15:46:33, pkotwicz wrote:
> > The TODO for ios should stay
> 
> Done.
> 
>
https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
> File components/favicon_base/favicon_util.cc (right):
> 
>
https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
> components/favicon_base/favicon_util.cc:58: int pixel_size =
> std::floor(favicon_size * favicon_scales[i]);
> On 2014/06/18 15:46:33, pkotwicz wrote:
> > We should ceil instead.
> 
> Done.
> 
>
https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
> components/favicon_base/favicon_util.cc:185: for (size_t i = 0; i <
> png_reps.size(); ++i) {
> On 2014/06/18 15:46:33, pkotwicz wrote:
> > You can use std::find() here now?
> 
> let me do it in separate CL. I'd like to keep this CL pure ui::ScaleFactor ->
> float conversion.
> 
>
https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
> File components/favicon_base/favicon_util.h (right):
> 
>
https://codereview.chromium.org/335233003/diff/140001/components/favicon_base...
> components/favicon_base/favicon_util.h:20: // different from scales derived
from
> ui::GetSupportedScaleFactors()
> On 2014/06/18 15:46:33, pkotwicz wrote:
> > Nit: scales from (not scales derived from)
> > 
> > "scales retrieved from" is also ok
> 
> Done.

[pkotwicz, 2014-06-19 21:18:28]

Ping me when you rebase the CL and I will look at it then.

Just looked at the implementation of getSupportedScaleFactors() in util.js. I
now see how this code is helpful for web UI

[oshima, 2014-06-19 23:23:30]

uploaded rebased patch. ptal

[pkotwicz, 2014-06-20 00:35:41]

Looks good.

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/extension_web_ui.cc (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_web_ui.cc:427: const std::vector<float>&
favicon_scales = favicon_base::GetFaviconScales();
This should not be a reference because favicon_base::GetFaviconScales() does not
return a reference.

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.cc:122: // - Favicons for imported
bookmarks.
Can you update this comment? Can you please also grep the codebase for any stray
references to favicon_base::GetFaviconScaleFactors()

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_service.h:67: // the requested scales. All of the
scale factors supported by the
Find and replace 'scale factors' -> 'scales'

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:172: IconRequest(callback, GURL(), 16,
ui::SCALE_FACTOR_100P));
Noooooooooooooooooo

ui::SCALE_FACTOR_100P -> 1.0f
In the past, I have caught mistakes where we passed in an enum value to code
which expects a float by temporarily making ui::ScaleFactor a class.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/favicon_util.cc (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:29: // |scale_factors| for which the
image reps can be created without resizing
|scale_factors| -> |favicon_scales|

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:56: // supported by the platform.
(ui::GetSupportedScaleFactor() cannot be used.)
This comment is no longer valid. We want to keep map to avoid loss of precision.

ceil(favicon_size * scale) / favicon_size != scale

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:149: std::vector<float>
favicon_scales(1, kScale1x);
You can use std::find here

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:168: // Create image reps for as many
scale factors as possible without resizing
scale factors -> scales

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.h:32: // a favicon of size favicon_size (in
DIPs) at the desired ui scale factors.
I think that this is a better comment:
"Takes a vector of PNG-encoded data, and converts it to a gfx::Image of size
|favicon_size| in DIPs. The return gfx::Image has an entry for each of
|favicon_scales|.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/select_favicon_frames.h (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/select_favicon_frames.h:23: // function adds a
representation at every desired scales.
Nit: "at every desired scales" -> "for each entry in |favicon_scales|."

[oshima, 2014-06-20 04:16:34]

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/extension_web_ui.cc (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_web_ui.cc:427: const std::vector<float>&
favicon_scales = favicon_base::GetFaviconScales();
On 2014/06/20 00:35:41, pkotwicz wrote:
> This should not be a reference because favicon_base::GetFaviconScales() does
not
> return a reference.
This is actually a valid. const reference can prolong the lifetime of temp
object.

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.cc:122: // - Favicons for imported
bookmarks.
On 2014/06/20 00:35:41, pkotwicz wrote:
> Can you update this comment? Can you please also grep the codebase for any
stray
> references to favicon_base::GetFaviconScaleFactors()

Done.

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_service.h:67: // the requested scales. All of the
scale factors supported by the
On 2014/06/20 00:35:41, pkotwicz wrote:
> Find and replace 'scale factors' -> 'scales'

I intentionally left them as scale factors as they're ones defined in
GetSupoprtedScaleFactors not arbitrary scales. I changed it to "resource scale
factors" to be more explicit.

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/335233003/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:172: IconRequest(callback, GURL(), 16,
ui::SCALE_FACTOR_100P));
On 2014/06/20 00:35:41, pkotwicz wrote:
> Noooooooooooooooooo
> 
> ui::SCALE_FACTOR_100P -> 1.0f
> In the past, I have caught mistakes where we passed in an enum value to code
> which expects a float by temporarily making ui::ScaleFactor a class.

yeah, that's why I didn't want to use float, and also wanted to make this  a
class but anyway fixed.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/favicon_util.cc (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:29: // |scale_factors| for which the
image reps can be created without resizing
On 2014/06/20 00:35:41, pkotwicz wrote:
> |scale_factors| -> |favicon_scales|

Done.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:56: // supported by the platform.
(ui::GetSupportedScaleFactor() cannot be used.)
On 2014/06/20 00:35:41, pkotwicz wrote:
> This comment is no longer valid. We want to keep map to avoid loss of
precision.
> 
> ceil(favicon_size * scale) / favicon_size != scale

Updated the comment.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:149: std::vector<float>
favicon_scales(1, kScale1x);
On 2014/06/20 00:35:41, pkotwicz wrote:
> You can use std::find here

How?

If you're talking about the code at 190, I have a separate CL for it.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.cc:168: // Create image reps for as many
scale factors as possible without resizing
On 2014/06/20 00:35:41, pkotwicz wrote:
> scale factors -> scales

Done.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.h:32: // a favicon of size favicon_size (in
DIPs) at the desired ui scale factors.
On 2014/06/20 00:35:41, pkotwicz wrote:
> I think that this is a better comment:
> "Takes a vector of PNG-encoded data, and converts it to a gfx::Image of size
> |favicon_size| in DIPs. The return gfx::Image has an entry for each of
> |favicon_scales|.

Updated to the slightly modified version.

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/select_favicon_frames.h (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/select_favicon_frames.h:23: // function adds a
representation at every desired scales.
On 2014/06/20 00:35:41, pkotwicz wrote:
> Nit: "at every desired scales" -> "for each entry in |favicon_scales|."

Done.

[pkotwicz, 2014-06-20 14:22:37]

LGTM

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.h:32: // a favicon of size favicon_size (in
DIPs) at the desired ui scale factors.
This method does not always decode the PNG encoded data which is why I was keen
on removing the "decodes them" part.

[oshima, 2014-06-20 15:21:50]

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
File components/favicon_base/favicon_util.h (right):

https://codereview.chromium.org/335233003/diff/180001/components/favicon_base...
components/favicon_base/favicon_util.h:32: // a favicon of size favicon_size (in
DIPs) at the desired ui scale factors.
On 2014/06/20 14:22:36, pkotwicz wrote:
> This method does not always decode the PNG encoded data which is why I was
keen
> on removing the "decodes them" part.

I see, done.

[oshima, 2014-06-20 15:23:22]

sky -> chrome/ owners

[sky, 2014-06-20 19:31:16]

https://codereview.chromium.org/335233003/diff/340001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/335233003/diff/340001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.cc:129: int edge_size_in_pixel =
std::ceil(desired_size_in_dip * favicon_scales[i]);
Why the change from floor to ciel here? I'm not sure I get why one is better
than the other, but curious.

[oshima, 2014-06-20 19:59:46]

https://codereview.chromium.org/335233003/diff/340001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/335233003/diff/340001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.cc:129: int edge_size_in_pixel =
std::ceil(desired_size_in_dip * favicon_scales[i]);
On 2014/06/20 19:31:16, sky wrote:
> Why the change from floor to ciel here? I'm not sure I get why one is better
> than the other, but curious.

I believe this should be consistent with what c/b/favicon/favicon_service.cc
uses (it uses ceil to build sizes)

[sky, 2014-06-20 22:55:12]

Fair enough, LGTM

[oshima, 2014-06-20 23:21:13]

Committed patchset #8 manually as r278853 (presubmit successful).