Closed Bug 826400 Opened 12 years ago Closed 9 years ago

Use apple-touch-icon for homescreen shortcuts

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox46 fixed, relnote-firefox 46+)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed
relnote-firefox --- 46+

People

(Reporter: wesj, Assigned: ahunt)

References

Details

(Keywords: dev-doc-needed)

Attachments

(4 files)

I think we need to do a bit better at looking for high res art, both for thumbnails in our product and homescreen shortcuts. Junior uses some nice icons that are hardcoded, but pages provide some. Some examples: 1.) many sites have high res art in random meta tags (qz.com lists http://app.qz.com/img/qz_og_img.png in a twitter card and opengraph data). These might be useful for about:home thumbnails, tab thumbnails (using images for twitter cards?), and as shortcut icons. How do we feel about creating a white list of tags that we're comfortable using and prioritizing them? 2.) RSS feeds (where NYT links to http://graphics8.nytimes.com/images/misc/NYT_logo_rss_250x40.png). These tend to be less square, but are often much more detailed.
Another example: CNN provides a <meta name="msapplication-TileImage" content="http://i.cdn.turner.com/cnn/2012/images/10/15/cnn_logo_144_144.png"/> along with a TileColor for Windows 8. Wonder if we could share a "parser" with the Metro team.
<link rel="apple-touch-icon"> are probably the most widely used format for homescreen shortcut icons. Maybe search for any images in link/meta tags inside <head> and take the biggest of those where x=y?
Fixed by Bug 914296 and 748100. Pretty picture: https://bugzilla.mozilla.org/attachment.cgi?id=800002 (Top row with, bottom row without) (Assuming the solution you're after is "Use higher resolution Favicons).
Assignee: nobody → ckitching
Assignee: chriskitching → nobody
See Also: → 1029524
Google has started pushing a WebApp Manifest, which supports a set of high resolution assets. We should consider looking for the manifest. Gecko has code to read a manifest: http://mxr.mozilla.org/mozilla-central/source/dom/manifest/ManifestObtainer.jsm
Summary: Try harder to find useful art for thumbnails and homescreen shortcuts → Try harder to find useful art for homescreen shortcuts
Blocks: progressive-apps
No longer blocks: fennec-polish
Does this also include having a sensible fallback as well? For example, if the icon isn't "useful enough"/of a certain size, could we wrap it around a nicer button UI?
(In reply to Anthony Lam (:antlam) from comment #9) > Does this also include having a sensible fallback as well? > > For example, if the icon isn't "useful enough"/of a certain size, could we > wrap it around a nicer button UI? We already have logic to do that with the dominant color, but that could potentially stand to be improved. If you have ideas about what we could do to make that fallback better, I would file a new bug about it (and link it back to bug 1212648).
What I'm hearing is that there a number of places websites stick their art that we could use, so maybe let's pick one and see how much of a win that gets us. https://bugzilla.mozilla.org/show_bug.cgi?id=826400#c3 names apple-touch-icons so why don't we start there.
Assignee: nobody → ally
Assignee: a.m.naaktgeboren → nobody
Assignee: nobody → ahunt
Would it be possible to reuse the design in bug 1228680? This would keep it consistent and help with recall but also be more useful as a fallback when we don't have perfectly nice icons.
(In reply to Anthony Lam (:antlam) from comment #12) > Would it be possible to reuse the design in bug 1228680? > > This would keep it consistent and help with recall but also be more useful > as a fallback when we don't have perfectly nice icons. I think that would be a different bug. This bug is not about creating a new fallback. This one is about finding better images so we can avoid the fallback. The fallback code is a separate, more narrow scope.
I've got this working for the apple-touch-icon now - see preliminary patch above. There are still some issues I need to address: 1. Using the "sizes" attribute - makeFavIconMessage parses the sizes for us, however we discard that and overwrite touchIcon with whichever apple-touch-icon was listed last in the page source. (For favicons, each size is sent separately to Tab, and later handled in the Favicon code which lets us retrieve different sizes as needed.) It would probably be simplest to filter the sizes during pageload, and store just one touchIcon url, however I don't think we have access to the preferred home screen icon size at that point - this might require more work on the database side to store multiple sizes, and then filter them when creating the homescreen shortcut? (That would probably needed for app manifests that provide multiple icon sizes too.) 2. Fix the URLMetadataTable caching. Currently we push data into the cache on any request to LocalURLMetaData - regardless of what columns were actually requested (and thus cached). E.g. on startup we retrieve tileImage/tileColor for all top-sites (for the top-sites panel), if we then try to add a top-site to the home screen, we have a cache hit, but the result doesn't contain the touchIcon. I've added an ugly check to re-retrieve rows with missing data to the current patch. I think it makes most sense to enable the cache only on requests for tileImage+tileColor, and not bother caching touchIcon requests (the former are retrieved every time we show the top-sites panel, the latter is used rarely), but that would make the code quite a bit more complex. Alternatively we could just cache all columns (but that means we'd need to retrieve all columns on every request).
Attachment #8698716 - Flags: feedback?(nalexander)
https://reviewboard.mozilla.org/r/28097/#review25183 ::: mobile/android/chrome/content/browser.js:4195 (Diff revision 1) > + // TODO: we can have multiple sizes of touch icons (similary to favicons) - handle that somehow? I think we hold the metadata until the page is loaded. Once loaded, we send the "best match" to Java. We could use: this.METADATA_GOOD_MATCH + message.size That would make bigger sizes preferred.
https://reviewboard.mozilla.org/r/28097/#review25239 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1230 (Diff revision 1) > - Integer.MAX_VALUE, > + GeckoAppShell.createShortcut(title, url); nit: indentation is off. ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:755 (Diff revision 1) > - static void createShortcut(final String aTitle, final String aURI, final String aIconData) { > + public static void createShortcut(final String aTitle, final String aURI) { My expectation is that this will fail when you build it in automation, since this is an entrypoint accessed from the compiled C++ code. This is a limitation of artifact builds. For testing, keep the signature the same and ignore the parameter. For landing, you'll need to address consumers, like https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/build/nsShellService.cpp#28. ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:759 (Diff revision 1) > - Favicons.getSizedFavicon(getApplicationContext(), aURI, aIconData, Integer.MAX_VALUE, 0, > + // The columns list will be mutated by metadata, hence we can't use Collections.singletonList Consider making the implementation not require this, since it's almost never what you want. That is, make the method copy the list if needed and add the URL column. ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:764 (Diff revision 1) > + final Map<String, Map<String, Object>> metadata = db.getURLMetadata().getForURLs(cr, Wow, this API is funky. ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:771 (Diff revision 1) > + String iconURL = null; nit: touchIconURL. Let's be explicit. ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:788 (Diff revision 1) > + Favicons.getSizedFavicon(getApplicationContext(), aURI, iconURL, Integer.MAX_VALUE, 0, listener nit: The indentation and line breaks here some off. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:117 (Diff revision 1) > - // Cache hit! > + // Cache hit - however the cache might not contain all the needed columsn. E.g. the top sites panel nit: columsn. This comment needs some editorial love. Full sentences, please. Explain why this is ugly and explain how these things would improve matters rather than stating that it is ugly. ::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:225 (Diff revision 1) > + GeckoAppShell.createShortcut(info.getDisplayTitle(), info.url); There's a slight change in semantics here, since before we captured the title earlier. Perhaps do that? ::: mobile/android/chrome/content/browser.js:4195 (Diff revision 1) > + // TODO: we can have multiple sizes of touch icons (similary to favicons) - handle that somehow? nit: similary. I think we should try to surface all the relevant metadata to the Java table, and do the filtering on that side, where we have the most complete information. Nice patch! I have some nits, a structural request, and a substantive request. Structural request: the smaller your patches, the easier they are to review. For this patch, split it into two parts: one, database changes (preserving existing functionality); two, the meat. Here, they're intertwingled. Substantive request: the API at https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java#97 is pretty rough. As a part 0, make that not mutate its inputs (and update callers). Larger question: Are we really gaining anything from this general system of `Map<String, *>`, considering this is all getting backed by actual SQL columns?
Comment on attachment 8698716 [details] MozReview Request: Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander I left comments via RB.
Attachment #8698716 - Flags: feedback?(nalexander) → feedback+
Comment on attachment 8698716 [details] MozReview Request: Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28097/diff/1-2/
Attachment #8698716 - Attachment description: MozReview Request: Bug 826400 - use apple-touch-icon for homescreen shortcuts → MozReview Request: Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander
Attachment #8698716 - Flags: feedback+ → review?(nalexander)
I've just pushed updated/split patches (see above)! Thank you for the review. Some notes: In the current implementation we grab all touchIcon sizes on the JS side, send the list to the Java side, filter these sizes (based on Android's preferred homescreen icon size), and finally write one optimal icon size to the DB - this icon is then used for homescreen shortcut creation. We've discussed storing all the sizes in the DB, however currently the only use for these icons is homescreen/launcher icons - these have a fixed size for any given device, hence storing one size should be sufficient here. We could potentially use the multiple touchIcon sizes to improve other parts of the browser ("top sites, rich history visits, better sharing UX, notifications") - however I'm not sure this is sensible given that some sites (e.g. twitter) use completely different designs for their favicons vs touchIcons (the favicon has a clear background / is optimised for showing on a white surface, whereas the touchIcon has a solid black background and seems to be optimised for being part of a launcher). It's already possible for sites to supply larger favicons (and if I understand the code correctly, we will use those as appropriate) - it seems like we're more likely to make the experience worse if we try to use the touchIcon outside of the homescreen?
Comment on attachment 8698716 [details] MozReview Request: Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28097/diff/2-3/
Comment on attachment 8700871 [details] MozReview Request: Bug 826400 - Part 2: Extract and store optimal apple-touch-icon during page load r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28821/diff/1-2/
Comment on attachment 8700872 [details] MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28823/diff/1-2/
Blocks: 1234558
Summary: Try harder to find useful art for homescreen shortcuts → Use apple-touch-icon for homescreen shortcuts
Attachment #8698716 - Flags: review?(nalexander) → review+
Comment on attachment 8698716 [details] MozReview Request: Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander https://reviewboard.mozilla.org/r/28097/#review25763 ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:146 (Diff revision 3) > + List<String> columns = requestedColumns; OK. wfm. Do we have JUnit 3 unit tests of this DB? If not, please file a ticket to exercise the functionality.
Attachment #8700871 - Flags: review?(nalexander)
Comment on attachment 8700871 [details] MozReview Request: Bug 826400 - Part 2: Extract and store optimal apple-touch-icon during page load r=nalexander https://reviewboard.mozilla.org/r/28821/#review25767 nits, including one substantive request. Be careful with the JSON types -- you haven't really guaranteed string values and integer (or long) keys. (In this situation, I don't think you need to guarantee it, but you do need to handle bad inputs.) ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:83 (Diff revision 2) > + // Try to select the closest icon that is larger than (or equal to) the target size - this Move this comment up and include a sketch of what the data looks like. Something like: // Incoming data should look like: {touchIconList:{100:"http://x.com/100.png", 200:"http://x.com/200.png"}}. And then explain your strategy. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:85 (Diff revision 2) > + if ((iconSize < preferredSize && iconSize > bestSizeFound) || This is hard to read. Can you express it more simply? I think I'd sort the list in ascending order, and then look for the cases sequentially: i.e., find the first icon >= than the preferred size; if there isn't one, just return the last size icon (it's the largest, and it's smallest overall.) ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:91 (Diff revision 2) > + } catch (JSONException e) { This isn't particularly robust, since some icon sizes may be malformed and other sizes may be well formed. Since we produce the incoming data, that's probably fine. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:92 (Diff revision 2) > + e.printStackTrace(); Use `Log.w(LOGTAG, message, e)`. This shouldn't be fatal -- you pretty much have to ignore it and move on. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:95 (Diff revision 2) > + Assert.isNotNull(iconURL); What do you want to do on failure? Assert's don't crash in beta or release, so prefer to log and adapt rather than possibly crash. ::: mobile/android/chrome/content/browser.js:3942 (Diff revision 2) > - if (!this.metatags[type] || this.metatags[type + "_quality"] < quality) { > + if (type === "touchIconList") { I don't think we use `===` often. Just `==`. ::: mobile/android/chrome/content/browser.js:3944 (Diff revision 2) > + this.metatags['touchIconList'] = {}; Consider setting metatags['touchIconList_quality'] as well. (Since we're changing the API.)
Attachment #8700872 - Flags: review?(nalexander) → review+
Comment on attachment 8700872 [details] MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen https://reviewboard.mozilla.org/r/28823/#review25773 Fine by me, but let's get snorp's stamp. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1107 (Diff revision 2) > - Bitmap favicon = tab.getFavicon(); > + final String favicon = tab.getFaviconURL(); It looks like `favicon` is unused: remove it. ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:756 (Diff revision 2) > - static void createShortcut(final String aTitle, final String aURI, final String aIconData) { > + public static void createShortcut(final String aTitle, final String aURI) { Ah, I see this. I'm going to redirect to snorp for two questions: 1) is this OK, since I don't know the JNI interfaces well; 2) are we confident that we can bump nsIShellService like this? (It may be used by other parts of the code base and also add-ons.) ::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java:762 (Diff revision 2) > + Collections.singletonList(URLMetadataTable.TOUCH_ICON_COLUMN) Oh, neat -- I wasn't aware of this helper. ::: mobile/android/components/build/nsIShellService.idl:22 (Diff revision 2) > * @param aIconData a base64-encoded data: URI representation of the shortcut's icon, as accepted by the favicon decoder. Update the comment, please.
Comment on attachment 8700872 [details] MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen snorp, jchen: can one of you vet the JNI changes here? It's not clear to me that we can bump this IDL without repurcussions.
Attachment #8700872 - Flags: review?(snorp)
Attachment #8700872 - Flags: review?(nchen)
Comment on attachment 8700872 [details] MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen https://reviewboard.mozilla.org/r/28823/#review25949 ::: mobile/android/components/build/nsIShellService.idl:25 (Diff revision 2) > - void createShortcut(in AString aTitle, in AString aURI, in AString aIconData, in AString aIntent); > + void createShortcut(in AString aTitle, in AString aURI, in AString aIntent); You should change the UUID as well if you make a non-trivial change to the IDL interface. I couldn't find our code actually using nsIShellService, but this change could affect addons. The safer way is probably to deprecate the parameter but keep it there, like what we did for the aIntent parameter.
Attachment #8700872 - Flags: review?(nchen)
Blocks: 1235599
https://reviewboard.mozilla.org/r/28821/#review25767 > This is hard to read. Can you express it more simply? I think I'd sort the list in ascending order, and then look for the cases sequentially: i.e., find the first icon >= than the preferred size; if there isn't one, just return the last size icon (it's the largest, and it's smallest overall.) I've implemented this, I'm just not sure I'm doing this in a sane way: My code iterates over the list keys (JSONObject.keys(), which gives us an Iterator) to copy them into a sortable ArrayList - that feels a bit clunky to me, but there doesn't seem to be any better way of getting a sortable list from the JSONObject - does that seem right? > Consider setting metatags['touchIconList_quality'] as well. (Since we're changing the API.) Would you want this to be an additional list of qualities/sizes (touchIconList_quality:{"200","300"}), or to replace the current storage of size within touchIconList (so touchIconList:{"x.com/200.png", "x.com/300.png"}, touchIconList_quality:{"200","300"}), or something else (e.g. storing just the largest quality obtained so far)? (I'd be worried about splitting the list into separate icon and size lists, especially since we're going to need a map from size->icon later - I'm not sure if that's what you want?)
Comment on attachment 8698716 [details] MozReview Request: Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28097/diff/3-4/
Comment on attachment 8700871 [details] MozReview Request: Bug 826400 - Part 2: Extract and store optimal apple-touch-icon during page load r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28821/diff/2-3/
Attachment #8700871 - Flags: review?(nalexander)
Comment on attachment 8700872 [details] MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28823/diff/2-3/
Attachment #8700872 - Attachment description: MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander → MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen
Attachment #8700872 - Flags: review?(snorp) → review?(nchen)
Comment on attachment 8700872 [details] MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen https://reviewboard.mozilla.org/r/28823/#review26049
Attachment #8700872 - Flags: review?(nchen) → review+
Attachment #8700871 - Flags: review?(nalexander) → review+
Comment on attachment 8700871 [details] MozReview Request: Bug 826400 - Part 2: Extract and store optimal apple-touch-icon during page load r=nalexander https://reviewboard.mozilla.org/r/28821/#review26421 lgtm. Sorry for the delay! ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:92 (Diff revisions 2 - 3) > - // should hopefully give us the highest quality favicon possible > + for (int size: sizes) { nit: house style is to space the colon -- like: ``` for (int size : sizes) { } ``` ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:102 (Diff revisions 2 - 3) > + bestSizeFound = sizes.get(sizes.size() - 1); Guard against empty collections here. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalURLMetadata.java:107 (Diff revisions 2 - 3) > - e.printStackTrace(); > + Log.w(LOGTAG, "Exception processing touchIconList for LocalURLMetadata", e); The important thing is to log what you're doing, to save passers-by reading the code: "for ...; ignoring."
https://reviewboard.mozilla.org/r/28821/#review25767 > I've implemented this, I'm just not sure I'm doing this in a sane way: > > My code iterates over the list keys (JSONObject.keys(), which gives us an Iterator) to copy them into a sortable ArrayList - that feels a bit clunky to me, but there doesn't seem to be any better way of getting a sortable list from the JSONObject - does that seem right? Sadly, I don't see `.keySet`, which would be better :(
Comment on attachment 8698716 [details] MozReview Request: Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28097/diff/4-5/
Comment on attachment 8700871 [details] MozReview Request: Bug 826400 - Part 2: Extract and store optimal apple-touch-icon during page load r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28821/diff/3-4/
Comment on attachment 8700872 [details] MozReview Request: Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28823/diff/3-4/
https://hg.mozilla.org/integration/fx-team/rev/0bbc5f3f9c671de8b9d6a2a054e226ecc4b3398e Bug 826400 - Part 1: Add touchIcon to URLMetadataTable r=nalexander https://hg.mozilla.org/integration/fx-team/rev/f21ce36d052486948d2712a2632612e7fa6a506f Bug 826400 - Part 2: Extract and store optimal apple-touch-icon during page load r=nalexander https://hg.mozilla.org/integration/fx-team/rev/cc103b8b1009effb6e4cc576174b70c0153c0b23 Bug 826400 - Part 3: Simplify homescreen shortcut creation, and use apple-touch-icon if available r=nalexander,jchen
Hi, I tested it with the Nightly from 2015-01-10 on my HTC One A9 with Android 6.0 and my blog www.soeren-hentzschel.at - it does not work, the apple-touch-icon is not used as home screen icon.
Strange. It seems to be working for me when adding Twitter to my home screen. Might be worth opening a new bug.
See Also: → 1238656
(In reply to Sören Hentzschel from comment #44) > I tested it with the Nightly from 2015-01-10 on my HTC One A9 with Android > 6.0 and my blog www.soeren-hentzschel.at - it does not work, the > apple-touch-icon is not used as home screen icon. The issue here is that we can't load icons from sites needing SNI for their SSL cert. See Bug 507641 for the main bug, there's also some discussion about http libraries in Bug 765064. As a result we can't load both favicons and touchIcons for your site (or any SNI using site), meaning we just show the default star on the homescreen, and also don't show a favicon during normal browsing.
OS: Linux → Android
Hardware: x86 → All
I've just learned that we should be updating the Bug number beside DATABASE_VERSION, here's a patch that does that, and also adds an explanation to hopefully avoid this not happening in future.
Attachment #8717043 - Flags: review?(liuche)
Attachment #8717043 - Flags: review?(liuche) → review+
https://hg.mozilla.org/integration/fx-team/rev/de8bfa116f66406ac3ca874a1eb4f560057cc7d4 Bug 826400 - Post: update DATABASE_VERSION comment, and clarify why we do this r=liuche
Noted as "Clearer homescreen shortcut icons"
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

Created:
Updated:
Size: