1320775 - [AS] [TopSites] Default top sites need good icons

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[:Grisha Kruglov]

Currently we're displaying basic generated icons. Ideally we want to display real favicons for the default top sites, similar to what the "classic" top sites do.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

This is bug 1312026. I'll close the other one.

Comment 3

[Andrzej Hunt :ahunt]

AFAICS suggested sites can also be provided by distributions: we can easily provide good icons for our own suggested-sites, but distributions would have to change to provide icons (and I'm guessing that's not something we want to force).

I'm wondering if we should instead reuse the touch-tiles to generate favicons (the touch tiles are actually a combination of an image plus a background - and we could generate a "favicon" to insert into the cache from there)? That could be done when suggested sites are loaded in SuggestedSites.java.

Comment 4

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #3)
> AFAICS suggested sites can also be provided by distributions: we can easily
> provide good icons for our own suggested-sites, but distributions would have
> to change to provide icons (and I'm guessing that's not something we want to
> force).
> 
> I'm wondering if we should instead reuse the touch-tiles to generate
> favicons (the touch tiles are actually a combination of an image plus a
> background - and we could generate a "favicon" to insert into the cache from
> there)? That could be done when suggested sites are loaded in
> SuggestedSites.java.

The touchicons don't really fit well in AS topsites (they're designed for bigger spaces), so we definitely want normal icons for those - I'm still not sure what to do about distribution-owned suggested sites.

Comment 5

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #4)
> The touchicons don't really fit well in AS topsites (they're designed for
> bigger spaces), so we definitely want normal icons for those - I'm still not
> sure what to do about distribution-owned suggested sites.

But that's what iOS is doing, at least for old topsites:
https://support.cdn.mozilla.net/media/uploads/gallery/images/2016-03-21-15-00-34-9aac9a.png

So I guess we could at least try this.

Comment 6

[Andrzej Hunt :ahunt]

Attachment: as_suggested_sites_tileIcons.png

In conjunction with Bug 1324028, using the tile resources doesn't look too bad, see attached screenshot (aspect ratios need fixing...). I think I'll stick with this approach.

Comment 7

[Andrzej Hunt :ahunt]

Attachment: as_suggested_sites_tileicons2.png

And an updated screenshot: the facebook icon is what is shown once facebook.com has actually been visited (i.e. site-supplied icons override the bundled icons). The remaining icons are the bundled ones: they look a bit small - it would be nice to boost the app-wide favicon size to improve this, especially since we're now showing favicons larger than before, but that's probably best to discuss in a separate bug (the SuggestedSiteLoader actually produces favicons that are more than large enough to fill the whole card, the Favicon pipeline and layouts then scale that down).

Comment 8

[Andrzej Hunt :ahunt]

Attachment: Bug 1320775 - Pre: move favicon colour fading to color generator

In order to allow for a background which merges into a favicon, we
need to allow for solid (non faded) colours. It is simplest to do this
by letting the colour generator (i.e. either the colour extractor,
or the favicon generator) fade the dominant colour as it wishes.
That also means the colour generator can in future choose to
not fade the colour if appropriate.

Review commit: https://reviewboard.mozilla.org/r/109392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109392/

Comment 9

[Andrzej Hunt :ahunt]

Attachment: Bug 1320775 - Use bundled touch-tiles as favicons for suggested sites

There are a number of ways in which we could supply favicons for the default suggested
sites. Reusing the touch tiles has the advantage that it works for both our own suggested
sites, and also distribution-supplied suggested sites. If we were to add yet another
icon source, distribution supplied sites would end up having no nice icon in AS topsites.

The priority ordering of the SuggestedSitePreparer means icons will be overriden as soon
as a site-supplied favicon is available - these icons will only be used up until the point
where a site has been visited.

Review commit: https://reviewboard.mozilla.org/r/109394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109394/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Andrzej Hunt :ahunt from comment #7)
> And an updated screenshot: the facebook icon is what is shown once
> facebook.com has actually been visited (i.e. site-supplied icons override
> the bundled icons).

Let's file a separate bug for considering using our shipped icons over the downloaded ones: The ones we ship should be always good.

> The remaining icons are the bundled ones: they look a
> bit small - it would be nice to boost the app-wide favicon size to improve
> this, especially since we're now showing favicons larger than before

There should be a bug for updating our shipped icosn to match the new AS aspect ratio.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8833163 [details]
Bug 1320775 - Pre: move favicon colour fading to color generator

https://reviewboard.mozilla.org/r/109392/#review111534

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8833163 [details]
Bug 1320775 - Pre: move favicon colour fading to color generator

https://reviewboard.mozilla.org/r/109392/#review111536

::: mobile/android/base/java/org/mozilla/gecko/icons/processing/ColorProcessor.java:31
(Diff revision 1)
>              return;
>          }
>  
>          try {
>              final Palette palette = Palette.from(response.getBitmap()).generate();
> -            response.updateColor(palette.getVibrantColor(DEFAULT_COLOR));
> +            response.updateColor(palette.getVibrantColor(DEFAULT_COLOR) & 0x7FFFFFFF);

Do we need to update TestColorProcessor?

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8833164 [details]
Bug 1320775 - Use bundled touch-tiles as favicons for suggested sites

https://reviewboard.mozilla.org/r/109394/#review111538

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/SuggestedSiteLoader.java:22
(Diff revision 1)
> +import org.mozilla.gecko.icons.IconRequest;
> +import org.mozilla.gecko.icons.IconResponse;
> +
> +import java.io.IOException;
> +
> +public class SuggestedSiteLoader implements IconLoader {

Can we add unit tests?

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/SuggestedSiteLoader.java:39
(Diff revision 1)
> +        }
> +
> +        return null;
> +    }
> +
> +    private IconResponse prepareIcon(final Context context, final String siteURL) {

The name 'prepareIcon' is a bit missleading in a "loader" and not a "preparer".

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/SuggestedSiteLoader.java:60
(Diff revision 1)
> +            // Our favicon pipeline and layout code assumes a square icon - so we hack our supplied
> +            // icons into a square shape
> +            final int heightWidth = Math.max(foreground.getHeight(), foreground.getWidth());
> +
> +            final Bitmap output = Bitmap.createBitmap(heightWidth, heightWidth, foreground.getConfig());
> +            final Canvas canvas = new Canvas(output);
> +            canvas.drawColor(backgroundColor);
> +
> +            // And we draw the icon in the middle of that square:
> +            final int heightDelta = heightWidth - foreground.getHeight();
> +            final int widthDelta = heightWidth - foreground.getWidth();
> +            canvas.drawBitmap(foreground, widthDelta / 2, heightDelta / 2, null);

We could avoid creating a bitmap that is larger than request.getTargetSize() so that we do not have to downsize it later again.

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:19
(Diff revision 1)
> +import org.mozilla.gecko.icons.loader.SuggestedSiteLoader;
> +
> +import java.util.HashSet;
> +import java.util.Set;
> +
> +public class SuggestedSitePreparer implements Preparer {

Can we add unit tests?

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:21
(Diff revision 1)
> +import java.util.HashSet;
> +import java.util.Set;
> +
> +public class SuggestedSitePreparer implements Preparer {
> +
> +    boolean initialised = false;

I wonder if our guidelines mention US english vs. British english. :)

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:21
(Diff revision 1)
> +    boolean initialised = false;
> +    final Set<String> siteFaviconMap = new HashSet<>();

private?

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:43
(Diff revision 1)
> +            cursor.close();
> +        }
> +    }
> +
> +    @Override
> +    public void prepare(final IconRequest request) {

This should exit early if request.shouldSkipDisk() returns true.

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/SuggestedSitePreparer.java:54
(Diff revision 1)
> +
> +        final String siteURL = request.getPageUrl();
> +
> +        if (siteFaviconMap.contains(siteURL)) {
> +            request.modify()
> +                    .icon(IconDescriptor.createGenericIcon(SuggestedSiteLoader.SUGGESTED_SITE_TOUCHTILE + request.getPageUrl()))

Instead of using "generic" I think we should give this its own type so that we can rank it against the other available icons. I feel like we should start with giving it a high value (15?).

Comment 14

[Andrzej Hunt :ahunt]

Attachment: Bug 1320775 - Add tests for SuggestedSiteLoader/Preparer

Review commit: https://reviewboard.mozilla.org/r/110898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110898/

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8833163 [details]
Bug 1320775 - Pre: move favicon colour fading to color generator

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109392/diff/1-2/

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8833164 [details]
Bug 1320775 - Use bundled touch-tiles as favicons for suggested sites

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109394/diff/1-2/

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8835277 [details]
Bug 1320775 - Add tests for SuggestedSiteLoader/Preparer

https://reviewboard.mozilla.org/r/110898/#review113810

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/04d7cacd46f9
Pre: move favicon colour fading to color generator r=sebastian
https://hg.mozilla.org/integration/autoland/rev/aeab7252c116
Use bundled touch-tiles as favicons for suggested sites r=sebastian
https://hg.mozilla.org/integration/autoland/rev/0cfcc10ad05c
Add tests for SuggestedSiteLoader/Preparer r=sebastian

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04d7cacd46f9
https://hg.mozilla.org/mozilla-central/rev/aeab7252c116
https://hg.mozilla.org/mozilla-central/rev/0cfcc10ad05c

Comment 20

[Carmen Fat [:cfat] - Ecosystem QA]

Tested on latest Nightly build 54.0a1 (2017-02-20) using:
- Asus ZenPad 8 (Android 6.0.1);
- Oneplus Two (Android 6.0.1).

The new tile icons for the default websites in Top sites area are looking good, as in the following screenshot: http://imgur.com/a/gu2bN

I'm marking this as Verified.