1042730 - [Larger icons in homescreen] Use better icons

Status: RESOLVED FIXED

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

Description

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

Make Bookmarks use better icons once bug 1042027 and bug 968156 land

Comment 1

[Dale Harvey (:daleharvey)]

Gecko part has landed so this can be finished up, we also need to make sure we use the icon picker in the search app, and certainly integration tests

Comment 2

[(no longer active)]

Can you please explain how you're going to deal with this completely in gaia?  I'm not sure if that's possible...

Comment 3

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

Hi Ehsan,

now we will receive an object containing all icons detected (even the apple-touch ones), and we created a simple utility library to fetch the best icon size (if we have it) based on dpr, or by specifiying a desired match:

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/icons_helper.js

Comment 4

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

Attachment: Pointer to PR 22358

Comment 5

[(no longer active)]

(In reply to comment #3)
> Hi Ehsan,
> 
> now we will receive an object containing all icons detected (even the
> apple-touch ones), and we created a simple utility library to fetch the best
> icon size (if we have it) based on dpr, or by specifiying a desired match:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/shared/js/icons_helper.js

Thanks for the explanation.  Note that this won't completely implement the spec <http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#rel-icon> (note this section "If there are multiple equally appropriate icons, user agents must use the last one declared in tree order at the time that the user agent collected the list of icons."  Basically my concern is that by the time that gaia sees the icons detected, the information about the tree order is lost.  We could, of course, make sure that the list that Gecko provides through the mozbrowser API is sorted properly or something...

Comment 6

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

I've identified another 2 places where we create bookmarks:

- In the search app.

- Inside a smart collection

In both cases we don't follow the same path than when we are browsing, so we won't have an array of icons, we will just have an icon provided by the grid.

Comment 7

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> Thanks for the explanation.  Note that this won't completely implement the
> spec
> <http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.
> html#rel-icon> (note this section "If there are multiple equally appropriate
> icons, user agents must use the last one declared in tree order at the time
> that the user agent collected the list of icons."  Basically my concern is
> that by the time that gaia sees the icons detected, the information about
> the tree order is lost.  We could, of course, make sure that the list that
> Gecko provides through the mozbrowser API is sorted properly or something...

Hi Ehsan,

we do persist all info coming from gecko:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L904-L927

the problem is that we store it in an object, where we miss the order.

Comment 8

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

Just realised, since Vivien's patch landed you cannot test this patch, as long as we don't show the dialog to add a page as a bookmark :(

Comment 9

[(no longer active)]

(In reply to comment #7)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment
> #5)
> > Thanks for the explanation.  Note that this won't completely implement the
> > spec
> > <http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.
> > html#rel-icon> (note this section "If there are multiple equally appropriate
> > icons, user agents must use the last one declared in tree order at the time
> > that the user agent collected the list of icons."  Basically my concern is
> > that by the time that gaia sees the icons detected, the information about
> > the tree order is lost.  We could, of course, make sure that the list that
> > Gecko provides through the mozbrowser API is sorted properly or something...
> 
> Hi Ehsan,
> 
> we do persist all info coming from gecko:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L904-L927
> 
> the problem is that we store it in an object, where we miss the order.

IIRC Gecko doesn't advertize anything about where the link element is in the DOM.  The order in which Gecko reports these elements is based on the order in which it sees them (which will be different than tree order for dynamically inserted elements.)

Comment 10

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

Since we removed the previous browser chrome we will need to get it back to try this patch, blocking on bug 1045828

Comment 11

[Dale Harvey (:daleharvey)]

> In both cases we don't follow the same path than when we are browsing, 
> so we won't have an array of icons, we will just have an icon provided by the grid.

In the search app for places we use the places data to create the grid element, it should be a matter of plugging it into https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/providers/places.js#L265

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 8465414 [details] [review]
Pointer to PR 22358

Looking good so far, as mentioned we should also fix this for places results

Comment 13

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

Right, will rename the bug them to make it clear that we will use this bug to apply the new icon selection mechanism in any place needed.

Comment 14

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

Just updated the patch for making places use the best icon available.

Also realised that the work done by Chris in bug 1045828 wont affect us or the quality of the icons.

That bug is just for e.me apps, and those apps already come with a single icon, fortunately it goes through an editorial process so the content that we get is already good enough.

It was confusing for me to see that web content and e.me content are separated, but once I got the thing there is nothing much we can do in that part.

Comment 15

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

Comment on attachment 8465414 [details] [review]
Pointer to PR 22358

Hi Dale,

PR ready, it includes the following:

- Use of better icons in places
- Use of better icons while adding a bookmar to homescreen

There is a case, when adding to homescreen content from e.me, that already comes with an icon that we cannot improve (if bookmarked from the search app)

Comment 16

[Dale Harvey (:daleharvey)]

Reviewing this now, however I would really like to see an integration test written for this, it seems like it should be fairly simple, can create a page with a plain favicon links plus some sizes and make sure the correct icon gets picked up.

Comment 17

[Dale Harvey (:daleharvey)]

Comment on attachment 8465414 [details] [review]
Pointer to PR 22358

So I tested this, I got improved results for github and twitter (however it seemed like they used e.me type icons despite saving from the homescreen in the browser), and no improvements for bbc / nytimes / amazon which all have better icons, also reddit which has a large icon android doesnt use.

At the same time I seen E/GeckoConsole( 2493): [JavaScript Error: "TypeError: icons[uri].sizes is undefined" {file: "app://search.gaiamobile.org/shared/js/icons_helper.js" line: 64}] at the console a bunch with this applied

We may not be able to get the reddit one working, but the others should and this definitely needs an integration test, reflag me, thanks

Comment 18

[Kevin Grandon :kgrandon]

Hey Francisco - thanks for the work on this so far. Is this something you would be able to finish up this week, or should we get some more people on it? Let me know, thanks!

Comment 19

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

Sorry, my fault, actually I wanted to ask Dale if he tried the patch flashing just the search app. This patch modifies both search and system and will require both.

I'll be happy to finish this this week Kevin.

Dale, if you do a make reset-gaia still seeing the same error?

Comment 20

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

Just sync with Dale on IRC, he did a fresh build, so the patch has some bugs as he comments.

Comment 21

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

Comment on attachment 8465414 [details] [review]
Pointer to PR 22358

Hi folks,

I've found a weird issue, by the time this PR was done, we have landed in places the ability to cache icons, and when we build a GaiaGrid.Bookmark object, we pass the cached icon.

The current grid is able to accept either blobs or urls, and we are passing both (from different places -> e.me pass urls, places pass blobs, etc.).
Unfortunately when we pass the blob we don't keep a reference to the icon url.

What I've done here is just use the fact that when we build a GaiaGrid.Bookmark we pass an object that we keep, well, just did the trick in Places, now we pass an extra attribute that is the original icon url, that is prioritized when we are calling the web activity.

This is definitely a temporary solution. A nice fix for this, IMHO, will be modifying bookmarks to accept either urls or blobs, and store internally blobs (and the url of course), but this is a change that I think is out of 2.1 scope.

If you have a better alternative, please just point me to it and will be happy to have it ready ASAP.

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8465414 [details] [review]
Pointer to PR 22358

Woohoo, this is working really well for me! I've left a few comments on github. Thanks for knocking this out.

I think you may want to wait for Dale to also review this as he probably has more context than I do at this point for this area. Thanks!

Comment 23

[Dale Harvey (:daleharvey)]

Comment on attachment 8465414 [details] [review]
Pointer to PR 22358

Yeh I dont love the iconUrl, I think with the placesdb work and this in place we can start switching to an IconHelper.getIcon('http://google.com' type api and vastly clean up a lot of the having to pass through urls and data everywhere.

This doesnt work on nytimes and bbc for me still, but does improve reddit / github, I will file bugs for sites it isnt working on and we can follow up with them, this is looking great though.

Comment 24

[Dale Harvey (:daleharvey)]

Just a heads up, this is r+'d but needs a few follow ups, im gonna do the the follow ups and land this if I dont hear back otherwise cause francisco is likely done with work by now, and this blocks some other work

Comment 25

[Dale Harvey (:daleharvey)]

I needed to add a require to the unit tests and removed sensors.json

https://github.com/mozilla-b2g/gaia/commit/afcdd36f13e75adcdebe57d842a277fd587faf28