963047 - Choose the best available icon based on size when adding a bookmark to the homescreen

Status: RESOLVED DUPLICATE of bug 1041482

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Ben Francis [:benfrancis]]

In bug 962630 I discovered that we actually now get multiple mozbrowsericonchange events for multiple <link rel=icon> tags in a web page. The event contains not only a href property (the icon URL), but also a sizes property.

In order to choose the best quality icon to add to the homescreen we should handle all of the mozbrowsericonchange events fired by a mozbrowserwindow for a given URL and then parse the sizes attribute to try to determine which icon we want to use.

The event doesn't contain the type of the image (or the intended media), so we can only decide based on size for now.

The W3C HTML spec provides some guidance on how to parse the sizes attribute and select an icon http://www.w3.org/TR/html5/links.html#rel-icon

If we later add support for the W3C Manifest spec as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=962630#c1 then we should use an icon specified in the manifest in favour of one provided in a link tag.

Comment 1

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

The database currently only saves the last reported icon for a given icon URL, so I guess we should first change the way we store icons to allow several icons per page. We could then add some convenience methods in browserdb to retrieve particular sizes, biggest size or the default icon. Does that make sense?

Comment 2

[Peter Dolanjski [:pdol]]

Ben, would this apply to icons added to homescreen via the bookmark migration or does it only apply to net new ones which are added by the user?

Comment 3

[Ben Francis [:benfrancis]]

I think this can only apply to new bookmarks because discovering new icons depends on events which are only fired when you actually load the page.

What we haven't figured out is whether we update the icons of bookmarks on the homescreen over time. In the browser app all icons in the places database (including bookmarks) are updated when they're changed by web page authors, after an expiry period of one day if I remember correctly.

Comment 4

[Ben Francis [:benfrancis]]

Sergi,

To answer your question (which I missed before, sorry), I don't think it makes sense to store *all* the icons provided by a web page. I think we probably need to store just one or at most two of them for use on the homescreen and in the Rocketbar results list. Some Firefox OS devices have significant constraints on available storage and storing multiple icons for every single site you visit, some of which may never be used, is not an efficient use of that storage. We should be using the "sizes" metadata provided to choose the most appropriate icon(s). This data can be provided via meta tags or manifest files.

Comment 5

[Sam Foster [:sfoster] (he/him)]

in the case of a .ico iconUri, we can only guess what's in there. We can request a particular size with #-moz-resolution=n,n, but we wont know what size we actually got until we assign to an img.src and measure. In practice we probably wont know at all, the user will just see a more or less nicely rendered icon image. This means that #-moz-resolution should be a fallback, last-resort strategy. 

So I think we should try something like:

1) look at the manifest for an icons object where possible

2) listen for all the mozbrowsericonchange events, building up a lookup of size : iconUri pairs. When we can be confident we're not expecting anymore mozbrowsericonchange events (is that mozbrowserloadend? we'll lose the ability to start loading the icon early unfortunately) 

3) Guess at host/favicon.ico.

We'll want to know (via a setting?) which sizes we prefer. This may vary from device to device.
Given this list (which may be a list of one: 32x32) we walk the collection we've accumulated and get the best matches. Those should be fetched and - when successful - stored.

Right now we have 1:1 of place:icon. Basically, where today we treat icon as a scalar uri, we would need to treat it as a collection. So each place has one icon collection associated with it, and the collection can be queried by size. I think that aligns with http://www.w3.org/2012/sysapps/manifest/#icons-member, it also analogous interestingly with how #-moz-resolution lets us address a particular size in a .ico. 

If that's the right direction, I guess the next steps would be to figure out what this means for the icons store and upgrade/migration implications. It starts to sound like quite a lot of work. We should probably re-examine the requirement for >1 icon - Where exactly is this necessary and is it valuable enough to justify the development cost?

Comment 6

[Sam Foster [:sfoster] (he/him)]

One other thought I had while looking at our existing implementation: Does the MAX_ICON_SIZE represent the max pixel size we want to deal with, or the max number of bytes? if the icon we've requested is a .ico, testing that the blob size doesn't exceed our maximum size is ambiguous (there may be a 128x128 image in there, but we don't have to use that one). We could move that check to the img.onload and use var src = window.URL.createObjectURL(blob)+'#-moz-resolution=32,32'. Is the harm already done at that point? ISTM we've got the blob in memory already anyhow?

Comment 7

[Ben Francis [:benfrancis]]

Yep I agree with this approach.

(In reply to Sam Foster [:sfoster] from comment #5)
> 2) listen for all the mozbrowsericonchange events, building up a lookup of
> size : iconUri pairs. When we can be confident we're not expecting anymore
> mozbrowsericonchange events (is that mozbrowserloadend? we'll lose the
> ability to start loading the icon early unfortunately) 

One problem I'm not sure how to solve is that mozbrowsericonchange events could come at any time during or after the page is loaded so we have no way of knowing when the list is complete. We should do some testing to find out whether it's common for mozbrowsericonchange events triggered by rel=icon link elements which are included in the original markup HTTP response to fire after mozbrowserloadend, because I'm not sure. Additional link elements also can be added into the DOM at any time after the page loads using JavaScript which would trigger further events, but I'm not sure how common this use case is.

(In reply to Sam Foster [:sfoster] from comment #6)
> One other thought I had while looking at our existing implementation: Does
> the MAX_ICON_SIZE represent the max pixel size we want to deal with, or the
> max number of bytes?

It's the maximum number of bytes, and this limitation is a fairly naive precaution to prevent the main process of the phone crashing because it runs out of memory when trying to decode a very large image. There was also a concern that because the browser runs in the main process that a specially crafted image could exploit bugs in the image decoder to gain escalated privileges and crack into the device.

Comment 8

[Ben Francis [:benfrancis]]

(In reply to Sam Foster [:sfoster] from comment #5)
> http://www.w3.org/2012/sysapps/manifest/#icons-member

This is a deprecated specification btw, you want the one written by the web apps working group:
http://w3c.github.io/manifest/#icons-member

Comment 9

[Dale Harvey (:daleharvey)]

Sorry blocked a bunch on the wrong bug

Comment 10

[Ben Francis [:benfrancis]]

Is this work now being tracked by a different bug or is it still valid?

Comment 11

[Dale Harvey (:daleharvey)]

Yeh this is referenced from 1041482 and I think the actual implementation bugs are all under that on