1265708 - No apple-touch-icon for some sites

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

It looks like we do not use icons that use "apple-touch-icon-precomposed" as value for the rel attribute.

YouTube's mobile site uses them and we fallback to use the ugly 16x16 ico.

Example:
<link rel="apple-touch-icon-precomposed" href="https://s.ytimg.com/yts/mobile/img/apple-touch-icon-114x114-precomposed-vflLGbwkc.png" sizes="114x114">

Comment 1

[Andrzej Hunt :ahunt]

I briefly dug into this: for some reason we're not receiving DOMLinkAdded events for apple-touch-icons for _some_ sites at:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4042

This affects only some sites (and we're still receiving the other <link> tags here). It might be less noticeable if the site supplies a high-resolution favicon, but it's quite bad for sites with old style icons.

Sites that work:
facebook.com

Sites that don't work:
nzz.ch
youtube.com

I can reproduce this on beta (where touch-icons first landed) so it's not a regression.

Comment 2

[Andrzej Hunt :ahunt]

I can try to look into this in the next week or two. It looks like I might need to do some native debugging (and I already have that configured for another bug), so it's probably most efficient for me to jump into this.

Comment 3

[Andrzej Hunt :ahunt]

Renaming since we aren't processing apple-touch-icons for some sites (but we do use -precomposed when available / not dropped)

Comment 4

[Andrzej Hunt :ahunt]

This seems to be working for me now on nightly/self-built. I'm guessing this was fixed on the platform side - we should try and figure out where this was fixed for uplifting though (might be too late for 47, but we could at least get 48 working)?

Comment 5

[Andrzej Hunt :ahunt]

This was actually caused by refs in the URL, youtube redirects to https://m.youtube.com/# on my N6P (but stays as https://m.youtube.com/ on an N4 and N7). As far as I can tell this is consistent across releases.

Comment 6

[Andrzej Hunt :ahunt]

Attachment: Bug 1265708 - Pre: remove unused variable

We already pass the URL inside the JSON object.

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

Comment 7

[Andrzej Hunt :ahunt]

Attachment: Bug 1265708 - Pre: Implement URLUtils.stripRefFromURL

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

Comment 8

[Andrzej Hunt :ahunt]

Attachment: Bug 1265708 - Pre: make getForURLs arguments more generic

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

Comment 9

[Andrzej Hunt :ahunt]

Attachment: Bug 1265708 - Strip ref from URL when adding homescreen icons

We add URL metadata based on the base URL, we should retrieve it this way too.

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

Comment 10

[Andrzej Hunt :ahunt]

Attachment: Bug 1265708 - Set URL metadata based on URL without ref

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

Comment 11

[Andrzej Hunt :ahunt]

Attachment: Bug 1265708 - Post: query topsites metadata by URL without ref too

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

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759366 [details]
Bug 1265708 - Pre: remove unused variable

https://reviewboard.mozilla.org/r/57372/#review54356

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759367 [details]
Bug 1265708 - Pre: Implement URLUtils.stripRefFromURL

https://reviewboard.mozilla.org/r/57374/#review54358

::: mobile/android/base/java/org/mozilla/gecko/util/URLUtils.java:19
(Diff revision 1)
> +     * Strip the ref from a URL, if present
> +     *
> +     * @return The base URL, without the ref. The original String is returned if it has no ref,
> +     *         of if the input is malformed.
> +     */
> +    public static String stripRefFromURL(final String inputURL) {

A bunch of URL related helper methods are in StringUtils. Let's move this into StringUtils too; even though that's not the best name.

::: mobile/android/base/java/org/mozilla/gecko/util/URLUtils.java:21
(Diff revision 1)
> +            final URL url = new URL(inputURL);
> +
> +            final String ref = url.getRef();
> +            if (ref != null) {
> +                // Remove the ref (-ref.length()), and the # separator (-1).
> +                return inputURL.substring(0, inputURL.length() - ref.length() - 1);
> +            }

If I understand the RFC correctly then the first # will always be the one of the fragment/ref. So you can just use indexOf().

See ABNF and especially the regex:
https://tools.ietf.org/html/rfc3986#appendix-A

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/util/TestURLUtils.java:21
(Diff revision 1)
> +    static {
> +        URLS = new ArrayList<>();
> +
> +        URLS.add(new Pair<String, String>(null, null));
> +        URLS.add(new Pair<>("", ""));
> +        URLS.add(new Pair<>("??AAABBBCCC", "??AAABBBCCC"));
> +
> +        URLS.add(new Pair<>("https://mozilla.org", "https://mozilla.org"));
> +        URLS.add(new Pair<>("https://mozilla.org#BBBB", "https://mozilla.org"));
> +        URLS.add(new Pair<>("https://mozilla.org/#BBBB", "https://mozilla.org/"));
> +    }

Why is this static and not just part of the test method?

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/util/TestURLUtils.java:41
(Diff revision 1)
> +            final String in = urlPair.first;
> +            final String expected = urlPair.second;
> +
> +            final String out = URLUtils.stripRefFromURL(in);
> +
> +            assertEquals(expected, out);

I don't know if it's worth having this dynamic. Why not just write 6 lines of assertEquals()?

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759368 [details]
Bug 1265708 - Pre: make getForURLs arguments more generic

https://reviewboard.mozilla.org/r/57376/#review54362

::: mobile/android/base/java/org/mozilla/gecko/db/StubBrowserDB.java:46
(Diff revision 1)
>      public Map<String, Map<String, Object>> getForURLs(final ContentResolver cr,
> -                                                       final List<String> urls,
> +                                                       final Collection<String> urls,
>                                                         final List<String> columns) {

Is this really needed? In the next patch you use singletonList() and this should return a list? You use singletonList() for the third parameter as well and this is still a List.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759369 [details]
Bug 1265708 - Strip ref from URL when adding homescreen icons

https://reviewboard.mozilla.org/r/57378/#review54364

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759370 [details]
Bug 1265708 - Set URL metadata based on URL without ref

https://reviewboard.mozilla.org/r/57380/#review54366

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759368 [details]
Bug 1265708 - Pre: make getForURLs arguments more generic

https://reviewboard.mozilla.org/r/57376/#review54370

Ah, the last patch is going to need this.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8759371 [details]
Bug 1265708 - Post: query topsites metadata by URL without ref too

https://reviewboard.mozilla.org/r/57382/#review54368

Comment 19

[Andrzej Hunt :ahunt]

[Tracking Requested - why for this release]: We don't use apple-touch icons for homescreen shortcuts if the URL contains a ref in all releases. This fix is probably upliftable, so we might as well put it in 48 (it's probably too late for 47 now).

Comment 20

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/f685593ac3cc4ea3b8d6545e42eb7d225095161a
Bug 1265708 - Pre: remove unused variable r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/974090a524da56fad3b8045b19461e9ae6817529
Bug 1265708 - Pre: Implement StringUtils.stripRef r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/047961bdb4adf91179be3b8a983cffc0d8e9debb
Bug 1265708 - Pre: make getForURLs arguments more generic r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/e994f70de0490e77a2309793852434f2f1641ad7
Bug 1265708 - Strip ref from URL when adding homescreen icons r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/10fa130289d70f0adb8ad8379d3454976c922049
Bug 1265708 - Set URL metadata based on URL without ref r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/67c37ed1a25f92cac6294cb70af97e95515c30e2
Bug 1265708 - Post: query topsites metadata by URL without ref too r=sebastian

Comment 21

[Carsten Book [:Tomcat]]

sorry had to back this out since it seems this broke android builds like https://treeherder.mozilla.org/logviewer.html#?job_id=9762906&repo=fx-team

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9f454bf15a8a
Backed out changeset 67c37ed1a25f 
https://hg.mozilla.org/integration/fx-team/rev/dbaa543cfb37
Backed out changeset 10fa130289d7 
https://hg.mozilla.org/integration/fx-team/rev/130b03d8de24
Backed out changeset e994f70de049 
https://hg.mozilla.org/integration/fx-team/rev/710bda4e42c6
Backed out changeset 047961bdb4ad 
https://hg.mozilla.org/integration/fx-team/rev/5cdc84e272d5
Backed out changeset 974090a524da 
https://hg.mozilla.org/integration/fx-team/rev/e1fb7ee96e4e
Backed out changeset f685593ac3cc for android bustage

Comment 23

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07ac0500a230

Comment 24

[Andrzej Hunt :ahunt]

(In reply to Carsten Book [:Tomcat] from comment #21)
> sorry had to back this out since it seems this broke android builds like
> https://treeherder.mozilla.org/logviewer.html#?job_id=9762906&repo=fx-team

Sorry for causing that!

I forgot to update moz.build after removing a file during review-fixup, and never noticed since I didn't do a full-build/verification after that. I'll hopefully be able to repush if try is happy with my updated patch-series.

Comment 25

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/1a529039f1a746a54ce1e727ad80595723d302ba
Bug 1265708 - Pre: remove unused variable r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/dc03fe0bad2a83df21395abee0a0574387642982
Bug 1265708 - Pre: Implement StringUtils.stripRef r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/1888dfa820b765ff63f7a5c1c223857100fe9d98
Bug 1265708 - Pre: make getForURLs arguments more generic r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/971e29d3b65c788142eb09b900c98279fd38a732
Bug 1265708 - Strip ref from URL when adding homescreen icons r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/3eaa539ca29456dbe29a2467638e0fe8f08dec88
Bug 1265708 - Set URL metadata based on URL without ref r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/ce9080e6765eb54ba1d65df446e4d917edb18677
Bug 1265708 - Post: query topsites metadata by URL without ref too r=sebastian

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a529039f1a7
https://hg.mozilla.org/mozilla-central/rev/dc03fe0bad2a
https://hg.mozilla.org/mozilla-central/rev/1888dfa820b7
https://hg.mozilla.org/mozilla-central/rev/971e29d3b65c
https://hg.mozilla.org/mozilla-central/rev/3eaa539ca294
https://hg.mozilla.org/mozilla-central/rev/ce9080e6765e

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Already resolved, just tracking for 49 in case it reopens.  
Are you aiming to uplift this to beta?  or might that be risky?

Comment 28

[Andrzej Hunt :ahunt]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #27)
> Already resolved, just tracking for 49 in case it reopens.  
> Are you aiming to uplift this to beta?  or might that be risky?

The changes are quite simple, and there don't seem to be any reported issues on nightly, so I'll request uplift on this.

Comment 29

[Andrzej Hunt :ahunt]

Comment on attachment 8759371 [details]
Bug 1265708 - Post: query topsites metadata by URL without ref too

Approval Request Comment
[Feature/regressing bug #]: Bug 826400
[User impact if declined]: apple-touch-icons aren't used for homescreen shortcuts if the page URL contains a ref (#foo). Some sites automatically redirect to such a URL on certain devices, e.g. youtube on an N6P will automatically redirect, resulting in a low quality homescreen icon.
[Describe test coverage new/current, TreeHerder]: manual testing, in addition to automated tests for the new URL processing code.
[Risks and why]: medium risk: touch-icon handling code now strips refs on both the javascript side (low-risk: we reuse standard URL handling methods), and java side (medium-risk: new URL handling methods).
[String/UUID change made/needed]: none.

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8759371 [details]
Bug 1265708 - Post: query topsites metadata by URL without ref too

Has been in nightly & aurora for 3 weeks, let's take it.

Comment 31

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

https://hg.mozilla.org/releases/mozilla-beta/rev/46dd314af6c5
https://hg.mozilla.org/releases/mozilla-beta/rev/92ebc47f8c12
https://hg.mozilla.org/releases/mozilla-beta/rev/432620991dab
https://hg.mozilla.org/releases/mozilla-beta/rev/fc565360b361
https://hg.mozilla.org/releases/mozilla-beta/rev/a46e0c70d190
https://hg.mozilla.org/releases/mozilla-beta/rev/dd7af1fa4ece

Comment 32

[Sorina Florean [:sflorean]]

Verified as fixed in build Aurora 49.0a2 (2016-07-06);
Device: Nexus 5 (Android 6.0.1)

Comment 33

[Sorina Florean [:sflorean]]

Verified as fixed in build Firefox 48 Beta 6;
Device: Asus ZenPad 8 (Android 5.0.2) and Nexus 5 (Android 6.0.1).