Make favicons for about:history and about:bookmarks switch from lodpi to hidpi when dragging browser

Make favicons for about:history and about:bookmarks switch from lodpi to hidpi when dragging browser from lodpi to hidpi screen

BUG=152121
Test=Manual, see bug

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

[pkotwicz, 2013-01-04 19:50:58]

kevers@, can you please take a look?

[pkotwicz, 2013-01-04 19:57:21]

Kevin, can you please take a look?

[kevers, 2013-01-04 20:21:10]

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/o...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(left):

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/o...
chrome/browser/resources/options/search_engine_manager_engine_list.js:104:
engine.iconURL);
Has this change been thoroughly tested? It appears that components in the image
set will be missing the iconurl portion, which may be problematic.  Perhaps the
trailing optional parameter in getFaviconImageSet for denoting a session favicon
should be an optional string type to allow handling of 'favicon',
'session-favicon' and 'favicon/iconurl'.

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/util.js (right):

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/s...
chrome/browser/resources/shared/js/util.js:45: var supported_scale_factors = new
Array();
Using C++ naming convention here and below.  Should use camel-case naming in
Javascript.  Underscores appear in optional parameters that start with the
"opt_" prefix and at the end of private data members.

Also prefer "[]" to "new Array()", but this just a style preference.

[pkotwicz, 2013-01-04 21:21:39]

Evan, can you please take a look?

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/o...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(left):

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/o...
chrome/browser/resources/options/search_engine_manager_engine_list.js:104:
engine.iconURL);
Thanks for catching this Kevin! chrome://favicon/iconurl does have different
behavior

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/util.js (right):

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/s...
chrome/browser/resources/shared/js/util.js:45: var supported_scale_factors = new
Array();
On 2013/01/04 20:21:10, kevers wrote:
> Using C++ naming convention here and below.  Should use camel-case naming in
> Javascript.  Underscores appear in optional parameters that start with the
> "opt_" prefix and at the end of private data members.
> 
> Also prefer "[]" to "new Array()", but this just a style preference.

Done.

[Dan Beam, 2013-01-04 21:34:28]

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/s...
File chrome/browser/resources/shared/js/util.js (right):

https://codereview.chromium.org/11762004/diff/5002/chrome/browser/resources/s...
chrome/browser/resources/shared/js/util.js:45: var supported_scale_factors = new
Array();
On 2013/01/04 20:21:10, kevers wrote:
> Using C++ naming convention here and below.  Should use camel-case naming in
> Javascript.  Underscores appear in optional parameters that start with the
> "opt_" prefix and at the end of private data members.
> 
> Also prefer "[]" to "new Array()", but this just a style preference.

This is not just style, [] always invokes the builtin Array constructor whereas
if somebody redefines window.Array new Array() invokes that code.  This was used
in a famous Gmail contacts hack. 
http://jeremiahgrossman.blogspot.com/2006/01/advanced-web-attack-techniques-u...

[Evan Stade, 2013-01-04 21:43:44]

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/shared/js/util.js (right):

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
chrome/browser/resources/shared/js/util.js:95: path.substr(replaceStartIndex +
11);
what is this magic 11? use 'scalefactor'.length

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
chrome/browser/resources/shared/js/util.js:97: s += url(pathWithScaleFactor) + '
' + scaleFactor + 'x';
indentation is wrong here

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
chrome/browser/resources/shared/js/util.js:299: function get1xFaviconUrl(url,
opt_size, opt_sessionFavicon) {
not a fan of this function name. Can you call it Unscaled, or some other English
word, instead of 1x?

[pkotwicz, 2013-01-04 23:06:53]

Evan, can you please take another look?

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/shared/js/util.js (right):

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
chrome/browser/resources/shared/js/util.js:95: path.substr(replaceStartIndex +
11);
On 2013/01/04 21:43:44, Evan Stade wrote:
> what is this magic 11? use 'scalefactor'.length

Done.

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
chrome/browser/resources/shared/js/util.js:97: s += url(pathWithScaleFactor) + '
' + scaleFactor + 'x';
On 2013/01/04 21:43:44, Evan Stade wrote:
> indentation is wrong here

Done.

https://codereview.chromium.org/11762004/diff/12001/chrome/browser/resources/...
chrome/browser/resources/shared/js/util.js:299: function get1xFaviconUrl(url,
opt_size, opt_sessionFavicon) {
Renamed to getFaviconUrlForDevicePixelRatio().
Didn't use getUnscaleFaviconUrl() as a favicon for the 2x scale factor is not
scaled either.

Added a comment warning about the limitations of the function

[Evan Stade, 2013-01-05 02:18:35]

lgtm

[commit-bot: I haz the power, 2013-01-06 17:06:58]

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

[commit-bot: I haz the power, 2013-01-06 17:07:06]

Presubmit check for 11762004-3018 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/docs

Presubmit checks took 3.7s to calculate.

[pkotwicz, 2013-01-06 18:21:16]

kalman@ for chrome/common/extensions/docs
I removed an unused method from
chrome/common/extensions/docs/examples/api/fontSettings/js/util.js

[not at google - send to devlin, 2013-01-07 05:17:50]

lgtm

[commit-bot: I haz the power, 2013-01-10 01:28:16]

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

[commit-bot: I haz the power, 2013-01-10 04:35:00]

Change committed as 176019