[Icons NTP] Add chrome://large-icon host, which resizes icon and renders fallback.

[Icons NTP] Add chrome://large-icon host, which resizes icon and renders fallback.

The chrome://large-icon endpoint is meant to serve (square) large icons
to the NTP to replace thumbnails. If icon is available, it is resized
first. Otherwise a fallback of the requested size is returned. Therefore
a custom NTP should always gets an image back, and cannot deduce whether
user has visited a site by examining icon presence or icon size.

BUG=467712
Committed: https://crrev.com/28365cabceeefe79bc02f0a7882f8889e6c58b8d
Cr-Commit-Position: refs/heads/master@{#321464}

[huangs, 2015-03-17 20:24:16]

huangs@chromium.org changed reviewers:
+ beaudoin@chromium.org, dbeam@chromium.org, jhawkins@chromium.org,
rogerm@chromium.org

[huangs, 2015-03-17 20:24:16]

This CL has two prerequisites:

- https://codereview.chromium.org/1014853004 for LargeIconParser.
- https://codereview.chromium.org/1018733002 for
FaviconService::GetGenericLargeIconForPageURL().

PTAL.

jhawkins@:
  chrome\browser\search\instant_service.cc
  chrome\browser\ui\webui\large_icon_source.cc
  chrome\browser\ui\webui\large_icon_source.h
  chrome\browser\ui\webui\ntp\most_visited_handler.cc
dbeam@:
  chrome\browser\ui\webui\large_icon_source.cc
  chrome\browser\ui\webui\large_icon_source.h
  chrome\browser\ui\webui\ntp\most_visited_handler.cc

(there's also GYPI and url_constants.* changes).

[Roger McFarlane (Chromium), 2015-03-17 20:41:02]

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
File chrome/browser/ui/webui/large_icon_source.h (right):

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.h:20: // All of the parameters except
for the url are optional. However, the order of
abortive comment?

[beaudoin, 2015-03-17 23:08:36]

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:31: int kMaxLargeIconSize = 512; 
// Arbitrary bound to safegard endpoint.
Looks really big. I'd be fine limiting it more. According to the Sebastien
Gabriel's spec we never need to go above 192x192.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:52:
render_fallback_on_failure_(true) {
Overdesign? All constructors set it to true: take it out until you need it.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:57: font_list.push_back("Helvetica
Neue");
Isn't there a better way to access the platform's default font? This looks
hardcoded...

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:80: || parser.size_in_pixels() >
kMaxLargeIconSize) {
More conventional in Chrome to have the || on the previous line. (And the style
guide recommends it.)

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:88: SendNotFoundResponse(callback);
Do we want to send a fallback icon in this case? (Given the URL is valid.)

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:134: // Resize the retrieved large
icon.
Indentation.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
File chrome/browser/ui/webui/large_icon_source.h (right):

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.h:71: // Renders and returns a
fallback icon.
I don't like "returns" here since the method doesn't return anything. I would
use: "invokes the request callback with..."

[huangs, 2015-03-18 22:54:12]

Updated, PTAL.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:31: int kMaxLargeIconSize = 512; 
// Arbitrary bound to safegard endpoint.
On 2015/03/17 23:08:36, beaudoin wrote:
> Looks really big. I'd be fine limiting it more. According to the Sebastien
> Gabriel's spec we never need to go above 192x192.

Done.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:52:
render_fallback_on_failure_(true) {
On 2015/03/17 23:08:36, beaudoin wrote:
> Overdesign? All constructors set it to true: take it out until you need it.

Done.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:57: font_list.push_back("Helvetica
Neue");
That's the purpose of my refactoring CL
https://chromiumcodereview.appspot.com/313053007/

However, I'm deferring that CL to get prototype through.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:80: || parser.size_in_pixels() >
kMaxLargeIconSize) {
On 2015/03/17 23:08:36, beaudoin wrote:
> More conventional in Chrome to have the || on the previous line. (And the
style
> guide recommends it.)

Done.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:88: SendNotFoundResponse(callback);
|favicon_service| this is an exceptional case.  Better to send broken result to
amplify bug, than to hide it.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:134: // Resize the retrieved large
icon.
Can remove resizing code thanks to your insight in
https://codereview.chromium.org/1018733002/ :)

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
File chrome/browser/ui/webui/large_icon_source.h (right):

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.h:20: // All of the parameters except
for the url are optional. However, the order of
On 2015/03/17 20:41:01, Roger McFarlane (Chromium) wrote:
> abortive comment?

Done.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.h:71: // Renders and returns a
fallback icon.
Replacing "returns" with "sends".  It's shorter, and is also used above.

[beaudoin, 2015-03-19 01:50:18]

w00t on removing resizing code. :)

LGTM

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
File chrome/browser/ui/webui/large_icon_source.cc (right):

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:57: font_list.push_back("Helvetica
Neue");
On 2015/03/18 22:54:11, huangs wrote:
> That's the purpose of my refactoring CL
> https://chromiumcodereview.appspot.com/313053007/
> 
> However, I'm deferring that CL to get prototype through.

Acknowledged.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:88: SendNotFoundResponse(callback);
On 2015/03/18 22:54:11, huangs wrote:
> |favicon_service| this is an exceptional case.  Better to send broken result
to
> amplify bug, than to hide it.

Acknowledged.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.cc:134: // Resize the retrieved large
icon.
On 2015/03/18 22:54:11, huangs wrote:
> Can remove resizing code thanks to your insight in
> https://codereview.chromium.org/1018733002/ :)

Acknowledged.

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
File chrome/browser/ui/webui/large_icon_source.h (right):

https://codereview.chromium.org/1016833003/diff/1/chrome/browser/ui/webui/lar...
chrome/browser/ui/webui/large_icon_source.h:71: // Renders and returns a
fallback icon.
On 2015/03/18 22:54:12, huangs wrote:
> Replacing "returns" with "sends".  It's shorter, and is also used above.

Acknowledged.

[huangs, 2015-03-19 14:53:43]

Now this CL is unblocked!  Going to sync.

[James Hawkins, 2015-03-19 18:47:31]

LGTM with nit.

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

https://codereview.chromium.org/1016833003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/large_icon_source.cc:26: int kMaxLargeIconSize = 192; 
// Arbitrary bound to safegard endpoint.
safeguard

[huangs, 2015-03-19 21:15:36]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-19 21:15:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
beaudoin@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/1016833003/#ps80001
(title: "Fix typo.")

[commit-bot: I haz the power, 2015-03-19 21:19:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1016833003/80001

[commit-bot: I haz the power, 2015-03-19 23:05:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-19 23:06:22]

Patchset 5 (id:??) landed as
https://crrev.com/28365cabceeefe79bc02f0a7882f8889e6c58b8d
Cr-Commit-Position: refs/heads/master@{#321464}