[AppList] Use local favicons/icons with URLSuggestionResult

[AppList] Use local favicons/icons with URLSuggestionResult

If available, will use local icons. Otherwise fallback to server provided image.

BUG=440487
TBR=stevenjb

Committed: https://crrev.com/1d9783347527cd7d1936c410adca28632524cf7f
Cr-Commit-Position: refs/heads/master@{#307973}

[Mathieu, 2014-12-11 03:32:07]

mathp@chromium.org changed reviewers:
+ mgiuca@chromium.org

[Mathieu, 2014-12-11 03:32:51]

Hi Matt, 

Can you have a look? Thanks.

[Matt Giuca, 2014-12-11 05:59:59]

+benwells for comment on use of favicons.

lgtm because all of my comments are speculative. The one thing that I think
requires attention is whether to change kMinimumDesiredSizePixels to the
existing kTileIconSize constant. I will leave that up to you.

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
(right):

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:26: const
int kMinimumDesiredSizePixels = 64;
Is this for display on the Launcher start page suggestions? Then it probably
should be using kTileIconSize (in ui/app_list/app_list_constants.h), which is
48.

Is there a reason to get it in a 64px size? (Besides, it looks like this value
is almost meaningless to GetLargestRawFaviconForPageURL.)

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:73:
favicon_service_->GetLargestRawFaviconForPageURL(
This implies that for websites that only have 16x16 icons (a lot), we'll have
16x16 icons on the start page, scaled up to 48x48. That's really ugly.

The bookmark apps team did a lot of work on producing high quality icons for
websites (for example, using high-res favicons if available, or if not, using a
coloured letter in a square). You should make use of their work. Adding
+benwells for comment.

Also, we may want to differentiate URL suggestions from pre-installed apps. For
bookmark apps, if an app would open in a browser tab, we add a small white arrow
to the corner of the icon; perhaps you should adopt that convention too.

Note: I don't expect any of this for this CL. I'm not very fussed about letting
things like poor-quality icons go in, because this is behind a field trial. But
these are things to think about as you build out this feature (hint: file bugs).

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:87:
weak_ptr_factory_.GetWeakPtr()));
Is this implying that if the icon cannot be found, we'll just use the page
thumbnail in the suggestions on the start page? That seems quite unhelpful
(packing a page thumbnail into a 48x48 pixel image). It's probably better just
to use a generic default icon. But I'm happy to submit this as-is and see what
it's like.

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:97:
image_skia.AddRepresentation(gfx::ImageSkiaRep(bitmap, 1.0f));
Have you tested this on a hidpi display? I don't think simply adding the proper
scale in place of 1.0f will do the trick. All ImageSkias need a 1.0 scale image.

You should not have a place in your code that behaves differently based on
whether it is regular or hidpi (as your TODO implies you plan to do). Instead,
you should build an ImageSkia with as many representations as your image sources
support. (For example, regardless of whether this is a 100% or 200% display, you
build an ImageSkia with a 100P and 200P representation.) Then the display system
will automatically choose the correct representation.

As it looks like the FaviconService is just giving a single "largest" image, I
guess this is not really feasible right now. So I'm happy to go ahead with this.
But change the TODO to be something more generic like "Support hidpi displays by
building an ImageSkia with as many representations as possible".

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h
(right):

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:32: // is
expected to live longer that |suggestion|.
s/that/than/

[Mathieu, 2014-12-11 14:27:28]

mathp@chromium.org changed reviewers:
+ huangs@chromium.org

[Mathieu, 2014-12-11 14:27:55]

Sam, 

You will be intereted in Matt's comments below RE:icons.

You should probably review this as well if you don't mind!

Thanks

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
(right):

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:73:
favicon_service_->GetLargestRawFaviconForPageURL(
On 2014/12/11 05:59:58, Matt Giuca wrote:
> This implies that for websites that only have 16x16 icons (a lot), we'll have
> 16x16 icons on the start page, scaled up to 48x48. That's really ugly.
> 
> The bookmark apps team did a lot of work on producing high quality icons for
> websites (for example, using high-res favicons if available, or if not, using
a
> coloured letter in a square). You should make use of their work. Adding
> +benwells for comment.
> 
> Also, we may want to differentiate URL suggestions from pre-installed apps.
For
> bookmark apps, if an app would open in a browser tab, we add a small white
arrow
> to the corner of the icon; perhaps you should adopt that convention too.
> 
> Note: I don't expect any of this for this CL. I'm not very fussed about
letting
> things like poor-quality icons go in, because this is behind a field trial.
But
> these are things to think about as you build out this feature (hint: file
bugs).

Yep, we have those on our radar but it's great to have your perspective! I'll
file a few bugs.

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:87:
weak_ptr_factory_.GetWeakPtr()));
On 2014/12/11 05:59:58, Matt Giuca wrote:
> Is this implying that if the icon cannot be found, we'll just use the page
> thumbnail in the suggestions on the start page? That seems quite unhelpful
> (packing a page thumbnail into a 48x48 pixel image). It's probably better just
> to use a generic default icon. But I'm happy to submit this as-is and see what
> it's like.

We're interested in your fallback icon, too! Hopefully the server image is just
a last resort, and soon we may have a way for the server to send icons, which
would be through this same code here.

https://codereview.chromium.org/795863002/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:97:
image_skia.AddRepresentation(gfx::ImageSkiaRep(bitmap, 1.0f));
On 2014/12/11 05:59:58, Matt Giuca wrote:
> Have you tested this on a hidpi display? I don't think simply adding the
proper
> scale in place of 1.0f will do the trick. All ImageSkias need a 1.0 scale
image.
> 
> You should not have a place in your code that behaves differently based on
> whether it is regular or hidpi (as your TODO implies you plan to do). Instead,
> you should build an ImageSkia with as many representations as your image
sources
> support. (For example, regardless of whether this is a 100% or 200% display,
you
> build an ImageSkia with a 100P and 200P representation.) Then the display
system
> will automatically choose the correct representation.
> 
> As it looks like the FaviconService is just giving a single "largest" image, I
> guess this is not really feasible right now. So I'm happy to go ahead with
this.
> But change the TODO to be something more generic like "Support hidpi displays
by
> building an ImageSkia with as many representations as possible".

Done.

[huangs, 2014-12-11 19:18:54]

Suggest also to get michaelbai@ to take a look.

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
(right):

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:34:
weak_ptr_factory_(this),
Make |weak_ptr_factory_| last, for consistency.

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:62:
icon_types.push_back(favicon_base::IconType::FAVICON |
NIT: These all get OR'ed together down the stream.  Perhaps the intended usage
is

icon_types.push_back(favicon_base::IconType::FAVICON);
icon_types.push_back(favicon_base::IconType::TOUCH_ICON);
icon_types.push_back(favicon_base::IconType::TOUCH_PRECOMPOSED_ICON);

?

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:83:
return;
Would you need to call set_display_type(DISPLAY_NONE); here?

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h
(right):

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:21: }
} // namespace favicon_base

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:60:
base::WeakPtrFactory<URLSuggestionResult> weak_ptr_factory_;
|weak_ptr_factory_| should always be the last member.

[Mathieu, 2014-12-11 19:27:14]

Thanks, I think it's simple enough that we don't need extra reviewers.

Another look?

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
(right):

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:34:
weak_ptr_factory_(this),
On 2014/12/11 19:18:54, huangs1 wrote:
> Make |weak_ptr_factory_| last, for consistency.

Done.

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:62:
icon_types.push_back(favicon_base::IconType::FAVICON |
On 2014/12/11 19:18:54, huangs1 wrote:
> NIT: These all get OR'ed together down the stream.  Perhaps the intended usage
> is
> 
> icon_types.push_back(favicon_base::IconType::FAVICON);
> icon_types.push_back(favicon_base::IconType::TOUCH_ICON);
> icon_types.push_back(favicon_base::IconType::TOUCH_PRECOMPOSED_ICON);
> 
> ?

Done.

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:83:
return;
On 2014/12/11 19:18:54, huangs1 wrote:
> Would you need to call set_display_type(DISPLAY_NONE); here?

Done.

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h
(right):

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:21: }
On 2014/12/11 19:18:54, huangs1 wrote:
> } // namespace favicon_base

Done.

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.h:60:
base::WeakPtrFactory<URLSuggestionResult> weak_ptr_factory_;
On 2014/12/11 19:18:54, huangs1 wrote:
> |weak_ptr_factory_| should always be the last member.

Done.

[huangs, 2014-12-11 19:35:25]

lgtm

[Mathieu, 2014-12-11 19:35:49]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2014-12-11 19:35:53]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2014-12-11 19:37:06]

Thanks, 

TBR stevenjb because of DEPS presubmit asking for a review of DEPS owner.

[Mathieu, 2014-12-11 19:37:09]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-12-11 19:37:36]

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

[tfarina, 2014-12-11 20:42:39]

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
(right):

https://codereview.chromium.org/795863002/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:34:
weak_ptr_factory_(this),
On 2014/12/11 19:18:54, huangs1 wrote:
> Make |weak_ptr_factory_| last, for consistency.

Not just for consistency. See crbug.com/303818

[benwells, 2014-12-11 20:51:18]

benwells@chromium.org changed reviewers:
+ benwells@chromium.org

[benwells, 2014-12-11 20:51:18]

Yep, agree with Matt that 16x16 icons scaled up look terrible. There is code in
c/b/extensions/bookmark_app_helper that will generate nice icons based on a
favicon color and the first letter of the origin, feel free to move it somewhere
more general and use it.

[benwells, 2014-12-11 20:52:11]

On 2014/12/11 20:51:18, benwells wrote:
> Yep, agree with Matt that 16x16 icons scaled up look terrible. There is code
in
> c/b/extensions/bookmark_app_helper that will generate nice icons based on a
> favicon color and the first letter of the origin, feel free to move it
somewhere
> more general and use it.

BTW you still need a chrome/browser/ui owner for the DEPS change.

[commit-bot: I haz the power, 2014-12-11 21:04:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-11 21:05:58]

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