1042027 - [Larger icons in homescreen] Given a set of icons pick up the best suitable one

Status: RESOLVED FIXED

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

Description

[Francisco Jordano [:arcturus] [:francisco]]

A web page can define several icons, we will be fetching all of them but we need to select from that whole list which one suites better the one of the current device screen.

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

Here is an example: http://www.wordpress.com

<link rel="shortcut icon" type="image/vnd.microsoft.icon" href="//s1.wp.com/i/favicon-stacked.ico?m=1311976025g" sizes="16x16 32x32 48x48">
<link rel="shortcut icon" type="image/x-icon" href="//s2.wp.com/i/favicon.ico?m=1311976025g" sizes="16x16">
<link rel="icon" type="image/x-icon" href="//s2.wp.com/i/favicon.ico?m=1311976025g" sizes="16x16">
<link rel="apple-touch-icon-precomposed" type="image/png" href="//0.gravatar.com/blavatar/653166773dc88127bd3afe0b6dfe5ea7?s=114&amp;d=http%3A%2F%2Fs0.wp.com%2Fi%2Fwebclip.png" sizes="114x114">


So far, check bug 921014, we are not supporting apple touch icons, so from that input we will get the following array:

[
    {'href':'//s1.wp.com/i/favicon-stacked.ico?m=1311976025g', sizes: '16x16 32x32 48x48'},
    {'href':'//s2.wp.com/i/favicon.ico?m=1311976025g', sizes: '16x16'},
    {'href':'//s2.wp.com/i/favicon.ico?m=1311976025g', sizes: '16x16'}
]

We will need to provide the best icon based on current device dpi, also taking into account that a single ico file can provide several icon resolutions

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Final format as aggreed with Dale on IRC:

{
  '//s1.wp.com/i/favicon-stacked.ico?m=1311976025g': {
    sizes: ['16x16 32x32 48x48']
  }, 
  '//s2.wp.com/i/favicon.ico?m=1311976025g' { 
    sizes: ['16x16']
  }
}

Comment 3

[Francisco Jordano [:arcturus] [:francisco]]

Attachment: Pointer to pr 22043

This will need still to be applied to the Places and Bookmars, but selects what should be the icon based on sizes.

That list of sizes will be provided once we land bug 968156.

This is just a helper library to be used in Places and Bookmarks

Comment 4

[Dale Harvey (:daleharvey)]

francisco, I know you mentioned following up with this, however since the fetching (and handling the failure to do so) of the icons is going to be a big part of this modules responsibility and will heavily effect the api (its gonna have to be async) I think its worth the initial implementation actually coming back with a blob here, do you think so? otherwise we write tests and possibly start integrating it where the implementation will change.

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

Hei Dale,

this first version came to me so simple since the integration with Bookmarks and Places could require just the correct url.

I've been looking to the case of ico files, and after a while, yes we have the wordpress.com between the top 500 alexa, but nothing else (and wordpress support other icons as well).

That's why I decided to go for the simplier version that doesn't fetch the icon, and it will just replace the config.favicon for IconHelper.getBestIcon(config.icons).

What do you think?

Comment 6

[Dale Harvey (:daleharvey)]

Yeh since we probably dont want to change the bookmark activity to use blobs quite yet, I think you are right, the fetch and fallback code can be introduced as new functions in here when we need them to, will review properly today, thanks

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8460338 [details] [review]
Pointer to pr 22043

Awesome, one tiny nit before landing :)

Comment 8

[Francisco Jordano [:arcturus] [:francisco]]

Waited till gai-try green ... and now landed:

https://github.com/arcturus/gaia/commit/65acf5f0385f4dff544aaa0fc8b18d8251c00547

Thanks folks!