Closed Bug 1238656 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
firefox46 + fixed
firefox47 + verified

People

(Reporter: miketaylr, Assigned: ahunt)

References

()

Details

Attachments

(6 files, 1 obsolete file)

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">
Assignee: nobody → ahunt
Priority: -- → P2
[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.
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.
Flags: qe-verify+
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?
Attachment #8706539 - Flags: review?(michael.l.comella) → review+
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.
https://hg.mozilla.org/integration/fx-team/rev/153202eeee86cb746d6b284638b868773e074941
Bug 1238656 - Bypass cache for touchIcons to avoid needless downscaling. r=mcomella
https://hg.mozilla.org/mozilla-central/rev/153202eeee86
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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.)
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
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/
Attachment #8706539 - Attachment description: MozReview Request: Bug 1238656 - Bypass cache for touchIcons to avoid needless downscaling. r=mcomella → MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE to avoid icon downscaling r=mcomella
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.
Attachment #8706539 - Flags: review+
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)
Attachment #8714490 - Flags: review?(michael.l.comella)
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.)
Blocks: 1245311
Attachment #8706539 - Attachment description: MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE to avoid icon downscaling r=mcomella → MozReview Request: Bug Bug 1238656 - Part 2: move best icon size selection into its own method r=mcomella
Attachment #8706539 - Flags: review?(michael.l.comella)
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 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/
Attachment #8714490 - Attachment description: MozReview Request: Bug 1238656 - Part 3: bypass cache for homescreen icons r=mcomella → MozReview Request: Bug 1238656 - Part 3: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella
Attachment #8714490 - Flags: review?(michael.l.comella)
Attachment #8706539 - Attachment description: MozReview Request: Bug Bug 1238656 - Part 2: move best icon size selection into its own method r=mcomella → MozReview Request: Bug 1238656 - Part 2: move best icon size selection into its own method r=mcomella
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 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 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/
What happened to part 1 here? Should I just review 2-4?
Flags: needinfo?(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/
Attachment #8706539 - Attachment description: MozReview Request: Bug 1238656 - Part 2: move best icon size selection into its own method r=mcomella → MozReview Request: Bug 1238656 - Part 1: move best icon size selection into its own method r=mcomella
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/
Attachment #8714490 - Attachment description: MozReview Request: Bug 1238656 - Part 3: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella → MozReview Request: Bug 1238656 - Part 2: introduce FLAG_BYPASS_CACHE_WHEN_DOWNLOADING_ICONS to avoid icon downscaling r=mcomella
Attachment #8715100 - Attachment description: MozReview Request: Bug 1238656 - Part 4: bypass cache for homescreen icons r=mcomella → MozReview Request: Bug 1238656 - Part 3: bypass cache for homescreen icons r=mcomella
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/
(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!
Flags: needinfo?(ahunt)
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
Attachment #8706539 - Flags: review?(michael.l.comella) → review+
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.
Attachment #8714490 - Flags: review?(michael.l.comella) → review+
Attachment #8715100 - Flags: review?(michael.l.comella) → review+
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!
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?
Flags: needinfo?(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.
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
Flags: needinfo?(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!
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Just wanted to say thanks, this looks much nicer!
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.
Flags: needinfo?(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?
Flags: needinfo?(ahunt)
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.
Attachment #8732368 - Flags: approval-mozilla-beta?
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 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.
Attachment #8732368 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Didn't apply cleanly to beta for me.
Flags: needinfo?(ahunt)
(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...).
Attachment #8732368 - Attachment is obsolete: true
Flags: needinfo?(ahunt)
It seems like this bug is already fixed in Fx47.
Verified as fixed using:
Device: Nexus 9 (Android 6.0)
Build: Firefox for Android 47 Beta 1
Depends on: 1270348
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.