Add support for favicon scale factor in WebUI

Add support for favicon scale factor.

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

[kevers, 2012-09-17 17:44:29]

Hi Peter,

PTAL.

[pkotwicz, 2012-09-17 19:32:06]

LGTM with nits

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:59: path.substr(0, 8) == "iconurl@"))
{
Can you put " (path.substr(0, 8) == iconurl/ ||" on a separate line for clarity?

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:87: if (scale_delimiter != path.npos
&& scale_delimiter < slash) {
Can you use std::string::npos instead?

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:92: request_size_map_[request_id] =
IconSize(pixel_size, scale_factor);
Can you use std::map::insert() and thus avoid needing to define the empty
constructor for IconSize() ?

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:175: ui::ScaleFactor scale =
request_size_map_[request_id].scale_factor;
Nit: can you call this variable scale_factor?
In general whenever I see a variable named scale I think it is a float.

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.h (right):

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.h:57: : pixel_size(16),
scale_factor(ui::SCALE_FACTOR_NONE) {
I don't think you need this constructor.

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.h:63: :  pixel_size(size),
scale_factor(scale) {
As you are using multiple lines for the initializer list (ie it is not on the
same line as IconSize(...), you have to put
pixel_size and scale_factor on different lines

[kevers, 2012-09-17 21:13:02]

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:59: path.substr(0, 8) == "iconurl@"))
{
On 2012/09/17 19:32:06, pkotwicz wrote:
> Can you put " (path.substr(0, 8) == iconurl/ ||" on a separate line for
clarity?

Done.

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:87: if (scale_delimiter != path.npos
&& scale_delimiter < slash) {
On 2012/09/17 19:32:06, pkotwicz wrote:
> Can you use std::string::npos instead?

Done.

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:92: request_size_map_[request_id] =
IconSize(pixel_size, scale_factor);
On 2012/09/17 19:32:06, pkotwicz wrote:
> Can you use std::map::insert() and thus avoid needing to define the empty
> constructor for IconSize() ?

Using insert does not appear to eliminate the need for an empty constructor and
the [] operator has a much cleaner syntax.

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:175: ui::ScaleFactor scale =
request_size_map_[request_id].scale_factor;
On 2012/09/17 19:32:06, pkotwicz wrote:
> Nit: can you call this variable scale_factor?
> In general whenever I see a variable named scale I think it is a float.

Done.

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.h (right):

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.h:57: : pixel_size(16),
scale_factor(ui::SCALE_FACTOR_NONE) {
On 2012/09/17 19:32:06, pkotwicz wrote:
> I don't think you need this constructor.

An empty constructor is required even if using insert instead of [].  Updated
formatting based on comment below.

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.h:63: :  pixel_size(size),
scale_factor(scale) {
On 2012/09/17 19:32:06, pkotwicz wrote:
> As you are using multiple lines for the initializer list (ie it is not on the
> same line as IconSize(...), you have to put
> pixel_size and scale_factor on different lines

Explicit copy constructor removed since not needed.

[kevers, 2012-09-18 12:39:24]

Hi Evan,

Can you please take a look at this CL.

[Evan Stade, 2012-09-18 13:54:05]

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/resources/h...
File chrome/browser/resources/history/history.js (right):

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/resources/h...
chrome/browser/resources/history/history.js:223: 'url(chrome://favicon/size/16@'
+ window.devicePixelRatio + 'x/' +
could you put this in util.js? something like GetFaviconUrl(size, url);

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fa...
chrome/browser/ui/webui/favicon_source.cc:92: request_size_map_[request_id] =
IconSize(pixel_size, scale_factor);
is it possible to delete the map (therefore fixing my todo :) and instead bind
the struct to cancelable consumer in SetClientData? (this also requires making
the struct hold the request id)

[kevers, 2012-09-18 17:31:03]

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/resources/hi...
File chrome/browser/resources/history/history.js (right):

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/resources/hi...
chrome/browser/resources/history/history.js:223: 'url(chrome://favicon/size/16@'
+ window.devicePixelRatio + 'x/' +
On 2012/09/18 13:54:05, Evan Stade wrote:
> could you put this in util.js? something like GetFaviconUrl(size, url);

Done.

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.cc (right):

http://codereview.chromium.org/10909236/diff/6001/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:92: request_size_map_[request_id] =
IconSize(pixel_size, scale_factor);
On 2012/09/18 13:54:05, Evan Stade wrote:
> is it possible to delete the map (therefore fixing my todo :) and instead bind
> the struct to cancelable consumer in SetClientData? (this also requires making
> the struct hold the request id)

Done.

[Evan Stade, 2012-09-19 12:59:39]

thanks, lgtm

http://codereview.chromium.org/10909236/diff/13001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/util.js (right):

http://codereview.chromium.org/10909236/diff/13001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/util.js:237: var size = opt_size ? opt_size :
16;
var size = opt_size || 16;

[kevers, 2012-09-19 13:36:57]

http://codereview.chromium.org/10909236/diff/13001/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/util.js (right):

http://codereview.chromium.org/10909236/diff/13001/chrome/browser/resources/s...
chrome/browser/resources/shared/js/util.js:237: var size = opt_size ? opt_size :
16;
On 2012/09/19 12:59:40, Evan Stade wrote:
> var size = opt_size || 16;

Done.

[commit-bot: I haz the power, 2012-09-19 13:55:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/10909236/19001

[commit-bot: I haz the power, 2012-09-19 14:47:32]

Retried try job too often for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-09-19 20:10:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/10909236/17003

[commit-bot: I haz the power, 2012-09-19 22:11:17]

Change committed as 157622