1238656 - Highest quality apple-touch-icon not chosen for homescreen shortcut icon

Status: VERIFIED FIXED

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

Description

[Mike Taylor [:miketaylr]]

STR:

In Nightly:
1) Go to https://webcompat.com
2) Menu > Page > Add to Home Screen

In Chrome Mobile:
1) Go to https://webcompat.com
2) Menu > Add to Home screen 
3) Give it a different name to compare

Expected: both choose high quality icons
Actual: Fennec's looks less nice.

Note this is on a Nexus 6P which has a DPR of like 3.5 or something. Unsure if that makes a difference


We have the following markup for icons on webcompat:

<link rel="apple-touch-icon" sizes="57x57" href="/favicon/apple-touch-icon-57x57.png">
<link rel="apple-touch-icon" sizes="114x114" href="/favicon/apple-touch-icon-114x114.png">
<link rel="apple-touch-icon" sizes="72x72" href="/favicon/apple-touch-icon-72x72.png">
<link rel="apple-touch-icon" sizes="144x144" href="/favicon/apple-touch-icon-144x144.png">
<link rel="apple-touch-icon" sizes="60x60" href="/favicon/apple-touch-icon-60x60.png">
<link rel="apple-touch-icon" sizes="120x120" href="/favicon/apple-touch-icon-120x120.png">
<link rel="apple-touch-icon" sizes="76x76" href="/favicon/apple-touch-icon-76x76.png">
<link rel="apple-touch-icon" sizes="152x152" href="/favicon/apple-touch-icon-152x152.png">

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: Screenshot_20160111-114751.png

Comment 2

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

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

Comment 3

[Andrzej Hunt :ahunt]

[Tracking Requested - why for this release]:
We introduced apple-touch-icon support for 46 (currently aurora?), this fix should be backported there so we're not shipping low-quality touch-icons.

Comment 4

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking for 46 and 47 since we just shipped touch-icon support in 46. 
Once this lands on Nightly, let's test the fix and then please request uplift to aurora.

Comment 5

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8706539 [details]
MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

https://reviewboard.mozilla.org/r/30365/#review29627

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:840
(Diff revision 1)
> -        if (touchIconURL != null) {
> -            // We have the favicon data (base64) decoded on the background thread, callback here, then
> -            // call the other createShortcut method with the decoded favicon.
> +        // Retrieve the icon while bypassing the cache. We explicitly bypass the cache as we limit the size of icons in
> +        // the cache. If touchIconURL is null, then Favicons falls back to finding the best possible favicon for
> +        // the site URI, hence we can use this call even when there is no touchIcon defined.

nit: As we discussed a bit in person, I'd add to this comment to include that we don't need to use the cache because we only get this icon once, when we add the shortcut.

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:843
(Diff revision 1)
> -            // This is slightly contrived, but makes the images available to the favicon cache.
> +        Favicons.getPreferredSizeFaviconForPage(getApplicationContext(), aURI, touchIconURL, listener);

I notice this method accesses `GeckoAppShell.getPreferredIconSize` and presumably limits the returned favicon for this size – will that always be big enough for the icons we want to display?

Comment 6

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/30365/#review29627

> I notice this method accesses `GeckoAppShell.getPreferredIconSize` and presumably limits the returned favicon for this size – will that always be big enough for the icons we want to display?

Yes: getPreferredIconSize tries to retrieve the launcher icon size from Android, i.e. we get exactly the right size (except on Android < 11, where we do some guessing):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java#

I've updated the comment in getPreferredSizeFaviconForPage to hopefully explain this better.

Comment 7

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/153202eeee86cb746d6b284638b868773e074941
Bug 1238656 - Bypass cache for touchIcons to avoid needless downscaling. r=mcomella

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/153202eeee86

Comment 9

[Andrzej Hunt :ahunt]

This isn't completely fixed, I didn't realise that we're also going through the in-memory cache (regardless of whether we're using the main cache) - that downscales the images to 128px. I'm not entirely sure what the correct solution for this is - we could either bypass that cache if the input is > 128px, or just increase the cache size, or bypass the cache only for touchIcons (but that would require extra flags / new codepaths, just for touch-icons).

(The issue was most obvious with the Nexus 9 which has large xxdpi icons, but is still at the lower end of density for xxdpi.)

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8706539 [details]
MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

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

Comment 11

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella

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

Comment 12

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8706539 [details]
MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

https://reviewboard.mozilla.org/r/30365/#review29895

I think it's unfortunate `LoadFaviconTask` is so inherently tied to the cache – it both makes this code hard to understand and (obviously) modify to not use the cache. I think a better architecture might be:

`DownloadFaviconTask` – only downloads Favicons
`LoadFaviconTask extends DownloadFaviconTask` – checks in cache for Favicon cache and if it's not there, downloads the Favicon
`LoadAndCacheFaviconTask extends LoadFaviconTask` – gets the favicon above and then caches the result

But perhaps it's unrealistic to refactor it in this way for this bug.

Can you think of a more intuitive way to do this? For more context, see my comments below.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:456
(Diff revision 2)
> +            if ((flags & FLAG_BYPASS_CACHE) == 0) {

There are other places we access the cache (e.g. the first way we try to load a favicon is by decoding a data uri, which could get pushed to the cache) so the name of this flag seems like it could lead to confusion in the future.

Maybe `FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_FAVICONS`? It makes fewer guarantees about what it does. But then again, the next use would also confusing with this flag name.

I'm having difficulty quantifying this, probaby because the code is so hard to understand. Do you see what I'm saying?

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:463
(Diff revision 2)
> +                return loadedBitmaps.getBitmaps().next();

If we receive multiple bitmaps, how do we know the top-most bitmap is the one we want?

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:465
(Diff revision 2)
> +                return null;

Is this what `pushToCacheAndGetResult` would return if `getBitmaps().hasNext()` returns false?

We should make sure the existing branch doesn't change.

The best I can tell, we do – we try to put it in the cache, but there's nothing to put in there so we don't iterate over `toInsert.favicons` in `putFavicons` and thus don't put it in the cache. When we try to return it from the cache, it's not there, so we return null.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:562
(Diff revision 2)
>          Bitmap scaled = image;

nit: I prefer `final Bitmap` here and set `scaled = image` in the else clause.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:566
(Diff revision 2)
> +            scaled = Bitmap.createScaledBitmap(image, targetWidth, targetWidth, true);

fyi, we shouldn't be scaling this on the UiThread, but that's not your fault: I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1244945 to fix this.

Comment 13

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8714490 [details]
MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella

(Clearing review until we fix up first part)

Comment 14

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/30365/#review29895

> There are other places we access the cache (e.g. the first way we try to load a favicon is by decoding a data uri, which could get pushed to the cache) so the name of this flag seems like it could lead to confusion in the future.
> 
> Maybe `FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_FAVICONS`? It makes fewer guarantees about what it does. But then again, the next use would also confusing with this flag name.
> 
> I'm having difficulty quantifying this, probaby because the code is so hard to understand. Do you see what I'm saying?

Changing the flag is definitely worthwhile, although I wonder if it's worth adapting all these places to also respect the flag? However right now I think it makes most sense to stick with the flag, and file a followup to investigate if we can simplify all of this?

> If we receive multiple bitmaps, how do we know the top-most bitmap is the one we want?

I believe the multi-sized icons are only possible for .ico (which would only be used for actual favicons), whereas it seems apple-touch-icon's should be png (there's no real spec for this though). Hence I assumed we would only have a single bitmap in this case. This obviously isn't bulletproof, but we also shouldn't ever end up having more than one bitmap here.

However I realise that we could actually end up using a favicon icon for the homescreen shortcuts (if no apple-touch-icon is provided), so maybe we do need to walk through the list to find the largest icon?

> Is this what `pushToCacheAndGetResult` would return if `getBitmaps().hasNext()` returns false?
> 
> We should make sure the existing branch doesn't change.
> 
> The best I can tell, we do – we try to put it in the cache, but there's nothing to put in there so we don't iterate over `toInsert.favicons` in `putFavicons` and thus don't put it in the cache. When we try to return it from the cache, it's not there, so we return null.

I think so too: we return null if we end up with an invalid Bitmap (i.e. no Bitmaps in the list). We also return null in various other cases where we can't load an icon, so I think it's safe to return null here.

(I actually ended up in this situation previously when I'd borked our Bitmap retrieval - trying to add a homescreen icon just showed a star in place of the unavailable icon: we're able to handle this.)

Comment 15

[Andrzej Hunt :ahunt]

Comment on attachment 8706539 [details]
MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

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

Comment 16

[Andrzej Hunt :ahunt]

Comment on attachment 8714490 [details]
MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella

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

Comment 17

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1238656 - Part 3: bypass cache for homescreen icons r=mcomella

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

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8706539 [details]
MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30365/diff/3-4/

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8714490 [details]
MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella

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

Comment 20

[Andrzej Hunt :ahunt]

Comment on attachment 8715100 [details]
MozReview Request: Bug 1238656 - Part 3: bypass cache for homescreen icons r=mcomella

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

Comment 21

[Michael Comella (:mcomella) [needinfo or I won't see it]]

What happened to part 1 here? Should I just review 2-4?

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8706539 [details]
MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30365/diff/4-5/

Comment 23

[Andrzej Hunt :ahunt]

Comment on attachment 8714490 [details]
MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33111/diff/3-4/

Comment 24

[Andrzej Hunt :ahunt]

Comment on attachment 8715100 [details]
MozReview Request: Bug 1238656 - Part 3: bypass cache for homescreen icons r=mcomella

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

Comment 25

[Andrzej Hunt :ahunt]

(In reply to Michael Comella (:mcomella) from comment #21)
> What happened to part 1 here? Should I just review 2-4?

Ooops... There's no part 1, so I've renumbered Parts 2-4 as Parts 1-3 now!

Comment 26

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8706539 [details]
MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

https://reviewboard.mozilla.org/r/30365/#review32515

Cool cleanup. That code was gross.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:92
(Diff revision 5)
> -                for (int size : sizes) {
> +                String iconURL = icons.getString(Integer.toString(bestSize));

nit: `final` while we're touching this line anyway

::: mobile/android/base/java/org/mozilla/gecko/favicons/Favicons.java:169
(Diff revision 5)
> +            return -1;

I recognize you're just copy-pasta-ing the code from before but returning -1 doesn't appear to be the best error case here.

Consider:
1) Adding a comment to say mention it's not the best but you don't you'd rather not spend the time/avoid making changes to the existing code (or whatever applies)
2) If you understand the code well enough and know that it won't be an issue, throwing in the error case

Comment 27

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8714490 [details]
MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella

https://reviewboard.mozilla.org/r/33111/#review29913

Just clear

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:468
(Diff revision 4)
> +                Map<Integer, Bitmap> iconMap = new HashMap<>();
> +                List<Integer> sizes = new ArrayList<>();

nit: `final`.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:471
(Diff revision 4)
> +                while (loadedBitmaps.getBitmaps().hasNext()) {

It's unfortunate there are no sorting guarantees in loadedBitmaps. But I suppose the list is small so we don't need to worry about it.

::: mobile/android/base/java/org/mozilla/gecko/favicons/LoadFaviconTask.java:473
(Diff revision 4)
> +                    iconMap.put(b.getWidth(), b);
> +                    sizes.add(b.getWidth());

This could be slightly more efficient if we used `iconMap.keySet` rather than creating a size List but it's probably not worth refactoring given the tiny list sizes and how infrequently we'll call this branch.

Comment 28

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8715100 [details]
MozReview Request: Bug 1238656 - Part 3: bypass cache for homescreen icons r=mcomella

https://reviewboard.mozilla.org/r/33337/#review32529

Thanks for being patient!

Comment 29

[Michael Comella (:mcomella) [needinfo or I won't see it]]

comment 9: not completely fixed.

Comment 30

[:Margaret Leibovic]

What's going on with this bug? It looks like one commit landed but the others didn't. Is there anything stopping us from landing the last parts? Will we need to uplift this?

Comment 31

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d8e370f99cb

Comment 32

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #30)
> What's going on with this bug? It looks like one commit landed but the
> others didn't. Is there anything stopping us from landing the last parts?
> Will we need to uplift this?

I'll hopefully be landing this later today (currently waiting for try) - we'll need to uplift  to 46 and 47 since that's where the touch-icons (including this bug) landed.

Comment 33

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/7ccac48f64f2d9897d630a193e7becebdd87eaf7
Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/d85800041473ed4765ca38e4c4cc351747e07998
Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/12e0bd2ffb918ce19b046fcddc81c665a55e2498
Bug 1238656 - Part 3: bypass cache for homescreen icons r=mcomella

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ccac48f64f2
https://hg.mozilla.org/mozilla-central/rev/d85800041473
https://hg.mozilla.org/mozilla-central/rev/12e0bd2ffb91

Comment 35

[Andrzej Hunt :ahunt]

(In reply to Carsten Book [:Tomcat] from comment #34)
> https://hg.mozilla.org/mozilla-central/rev/7ccac48f64f2
> https://hg.mozilla.org/mozilla-central/rev/d85800041473
> https://hg.mozilla.org/mozilla-central/rev/12e0bd2ffb91

Shouldn't need leave-open anymore now that these have landed!

Comment 36

[Mike Taylor [:miketaylr]]

Just wanted to say thanks, this looks much nicer!

Comment 37

[Andrzej Hunt :ahunt]

We'll probably want to uplift this to wherever apple-touch-icons first landed, I think that commit has made it all the way to beta now (I'll double check that when I'm more awake in the morning). However a crash slipped in, see Bug 125335, so we'd have to either uplift together with its fix once available, or not uplift.

(If not uplifting, then we could just disable apple-touch-icons, but I don't think there's any effective difference either way, since we blur favicons in the same way when using them for homescreen shortcuts. We probably wouldn't want to advertise apple-touch-icon support until the release where this bug lands.)

NI'ing myself as a reminder.

Comment 38

[Andrzej Hunt :ahunt]

I've landed a fix for Bug 1255335 now - we could potentially uplift this in conjunction with that crash-fix so that apple-touch-icon support is complete in 46?

Comment 39

[Andrzej Hunt :ahunt]

Attachment: touchicon_scaling_1238656_1255335_beta.patch

Requesting uplift to Beta so we actually have fully functional apple-touch-icons (we're advertising this in the release notes, see "Clearer homescreen shortcut icons" in https://www.mozilla.org/en-US/firefox/android/46.0beta/releasenotes/)

I've created one patch containing all 5 commits that would need to land simultaneously:
4 commits from this bug, all of which are on mozilla-central and aurora
1 commit from Bug 1255335 (crashfix) which has landed on mozilla-central, and is approved for aurora (this one required fixup for aurora due to variable renaming).


Approval Request Comment
[Feature/regressing bug #]: Bug 826400 introduced apple-touch-icons, but with a scaling issue.
[User impact if declined]: Homescreen icons are blurred on devices with higher pixel densities (noticeable e.g. on Nexus 5 and newer).
[Describe test coverage new/current, TreeHerder]: Manual testing.
[Risks and why]: Medium risk: we now bypass the favicon cache with a specific flag to avoid downscaling of images. Normal favicon processing should be unaffected, apple-touch-icon processing no longer accesses the cache.
[String/UUID change made/needed]: none.

Note: these patches introduced a crash for some websites, this was fixed in Bug 1255335. The attached patch contains the crashfix commit. This crash has no new traces on nightly since the crashfix landed.

Comment 40

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

We can try this out in beta 5. If it causes problems we can back it out and remove the bit about icons from the release notes.

Comment 41

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8732368 [details] [diff] [review]
touchicon_scaling_1238656_1255335_beta.patch

May be risky, icon fix plus crash fix. This should land for beta 5.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Didn't apply cleanly to beta for me.

Comment 43

[Andrzej Hunt :ahunt]

Attachment: touchicon_scaling_1238656_1255335_beta_correctordering.patch

(In reply to Wes Kocher (:KWierso) from comment #42)
> Didn't apply cleanly to beta for me.

Here's a fixed patch.

Sorry, I exported the patches in reverse order (hg export -r tip:-5) which is why they wouldn't apply, I've now made sure I can reimport this patch series. (I'm still figuring out if there's a better way to do this...).

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/21e3f38556a3

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

It seems like this bug is already fixed in Fx47.

Comment 46

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2016-04-26 16:06:34.png

Verified as fixed using:
Device: Nexus 9 (Android 6.0)
Build: Firefox for Android 47 Beta 1