Make getFaviconImageSet work with all expected URL types.

Make getFaviconImageSet work with all expected URL types.

BUG=606445
Committed: https://crrev.com/e34687cdc6fed5fd7d1eede2fb6a0aed5f26d466
Cr-Commit-Position: refs/heads/master@{#390137}

[dpapad, 2016-04-27 00:46:55]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-04-27 00:58:41]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[dpapad, 2016-04-27 00:58:42]

The old Options code was not using the getFaviconImageSet() helper method,
because it was expecting only origin URLs, not favicon URLs. Instead of adding a
3rd optional parameter to that method, I am deducing the urlType from the URL
itself.

As a bonus, I am adding tests for old and new functionality too. Once this
lands, then https://codereview.chromium.org/1918053002 can also land. Remaining
issues I have encountered with favicons not updating seem to be residing in C++
and will investigate separately.

[Dan Beam, 2016-04-27 02:18:31]

lgtm, but you should probably try this at different DPIs as well (if you haven't
already)

see --force-device-scale-factor for much fun

https://codereview.chromium.org/1914073005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(right):

https://codereview.chromium.org/1914073005/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/search_engine_manager_engine_list.js:127: }
no curlies

https://codereview.chromium.org/1914073005/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/util_test.html (right):

https://codereview.chromium.org/1914073005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/util_test.html:23: /* Test the case where the origin URL
is passed. */
nit: single-line comments, i.e. // Test ...

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:333: var FAVICON_URL_REGEX = /\.ico$/;
nit: /i

[dpapad, 2016-04-27 17:21:47]

Tried with --force-device-scale-factor=2 and verified that the correct URLs were
produced.

https://codereview.chromium.org/1914073005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/options/search_engine_manager_engine_list.js
(right):

https://codereview.chromium.org/1914073005/diff/20001/chrome/browser/resource...
chrome/browser/resources/options/search_engine_manager_engine_list.js:127: }
On 2016/04/27 at 02:18:31, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/1914073005/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/util_test.html (right):

https://codereview.chromium.org/1914073005/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/util_test.html:23: /* Test the case where the origin URL
is passed. */
On 2016/04/27 at 02:18:31, Dan Beam wrote:
> nit: single-line comments, i.e. // Test ...

Done.

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:333: var FAVICON_URL_REGEX = /\.ico$/;
On 2016/04/27 at 02:18:31, Dan Beam wrote:
> nit: /i

Are you suggesting  modifying the regex to /ico$/? This would make it more
likely that it will result in a false positive, imagine a URL like
www.something.com/fantastico which is not a favicon URL. I prefer to keep the
dot matching for this reason.

[dpapad, 2016-04-27 17:24:25]

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:333: var FAVICON_URL_REGEX = /\.ico$/;
On 2016/04/27 at 17:21:47, dpapad wrote:
> On 2016/04/27 at 02:18:31, Dan Beam wrote:
> > nit: /i
> 
> Are you suggesting  modifying the regex to /ico$/? This would make it more
likely that it will result in a false positive, imagine a URL like
www.something.com/fantastico which is not a favicon URL. I prefer to keep the
dot matching for this reason.

On second thought, you probably meant adding /i at the end for case
insensitivity. Done.

[dpapad, 2016-04-27 17:33:07]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-04-27 17:33:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1914073005/#ps60001
(title: "case insensitive")

[commit-bot: I haz the power, 2016-04-27 17:33:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1914073005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1914073005/60001

[Dan Beam, 2016-04-27 18:46:57]

still lgtm

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/1914073005/diff/20001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:333: var FAVICON_URL_REGEX = /\.ico$/;
On 2016/04/27 17:24:25, dpapad wrote:
> On 2016/04/27 at 17:21:47, dpapad wrote:
> > On 2016/04/27 at 02:18:31, Dan Beam wrote:
> > > nit: /i
> > 
> > Are you suggesting  modifying the regex to /ico$/? This would make it more
> likely that it will result in a false positive, imagine a URL like
> http://www.something.com/fantastico which is not a favicon URL. I prefer to
keep the
> dot matching for this reason.
> 
> On second thought, you probably meant adding /i at the end for case
> insensitivity. Done.

yes, that ^

[commit-bot: I haz the power, 2016-04-27 18:51:57]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:11:34]

Patchset 3 (id:??) landed as
https://crrev.com/e34687cdc6fed5fd7d1eede2fb6a0aed5f26d466
Cr-Commit-Position: refs/heads/master@{#390137}

[commit-bot: I haz the power, 2016-04-30 17:51:03]

Description was changed from

==========
Make getFaviconImageSet work with all expected URL types.

BUG=606445
==========

to

==========
Make getFaviconImageSet work with all expected URL types.

BUG=606445

Committed: https://crrev.com/e34687cdc6fed5fd7d1eede2fb6a0aed5f26d466
Cr-Commit-Position: refs/heads/master@{#390137}
==========