Closed Bug 1265708 Opened 8 years ago Closed 8 years ago

No apple-touch-icon for some sites

Categories

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

Product:
Firefox for Android Graveyard
Component:
Favicon Handling
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48+ verified, firefox49+ verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox48 + verified
firefox49 + verified

People

(Reporter: sebastian, Assigned: ahunt)

References

Details

Attachments

(6 files)

Bug 1265708 - Pre: remove unused variable
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1265708 - Pre: Implement URLUtils.stripRefFromURL
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1265708 - Pre: make getForURLs arguments more generic
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1265708 - Strip ref from URL when adding homescreen icons
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1265708 - Set URL metadata based on URL without ref
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1265708 - Post: query topsites metadata by URL without ref too
58 bytes, text/x-review-board-request
sebastian
: review+
Sylvestre
: approval-mozilla-beta+
Details 
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">
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.
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.
Assignee: nobody → ahunt
Renaming since we aren't processing apple-touch-icons for some sites (but we do use -precomposed when available / not dropped)
Summary: Use apple-touch-icon-precomposed icons → No apple-touch-icon for some sites
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)?
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.
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/
Attachment #8759366 - Flags: review?(s.kaspari)
Attachment #8759367 - Flags: review?(s.kaspari)
Attachment #8759368 - Flags: review?(s.kaspari)
Attachment #8759369 - Flags: review?(s.kaspari)
Attachment #8759370 - Flags: review?(s.kaspari)
Attachment #8759371 - Flags: review?(s.kaspari)
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/
Status: NEW → ASSIGNED
Comment on attachment 8759366 [details]
Bug 1265708 - Pre: remove unused variable

https://reviewboard.mozilla.org/r/57372/#review54356
Attachment #8759366 - Flags: review?(s.kaspari) → review+
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()?
Attachment #8759367 - Flags: review?(s.kaspari) → review+
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.
Attachment #8759368 - Flags: review?(s.kaspari) → review-
Comment on attachment 8759369 [details]
Bug 1265708 - Strip ref from URL when adding homescreen icons

https://reviewboard.mozilla.org/r/57378/#review54364
Attachment #8759369 - Flags: review?(s.kaspari) → review+
Comment on attachment 8759370 [details]
Bug 1265708 - Set URL metadata based on URL without ref

https://reviewboard.mozilla.org/r/57380/#review54366
Attachment #8759370 - Flags: review?(s.kaspari) → review+
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.
Attachment #8759368 - Flags: review- → review+
Comment on attachment 8759371 [details]
Bug 1265708 - Post: query topsites metadata by URL without ref too

https://reviewboard.mozilla.org/r/57382/#review54368
Attachment #8759371 - Flags: review?(s.kaspari) → review+
[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).
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
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
Flags: needinfo?(ahunt)
Backout by cbook@mozilla.com:
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
(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.
Flags: needinfo?(ahunt)
Already resolved, just tracking for 49 in case it reopens.  
Are you aiming to uplift this to beta?  or might that be risky?
status-firefox48: --- → affected
tracking-firefox49: ? → +
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8759371 - Flags: approval-mozilla-beta?
Attachment #8759371 - Flags: approval-mozilla-aurora?
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.
Attachment #8759371 - Flags: approval-mozilla-beta?
Attachment #8759371 - Flags: approval-mozilla-beta+
Attachment #8759371 - Flags: approval-mozilla-aurora?
Verified as fixed in build Aurora 49.0a2 (2016-07-06);
Device: Nexus 5 (Android 6.0.1)
status-firefox49: fixed → verified
Verified as fixed in build Firefox 48 Beta 6;
Device: Asus ZenPad 8 (Android 5.0.2) and Nexus 5 (Android 6.0.1).
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: