Closed Bug 1042730 Opened 10 years ago Closed 10 years ago

[Larger icons in homescreen] Use better icons

Categories

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

defect
Not set
normal

Tracking

(feature-b2g:2.1)

RESOLVED FIXED
2.1 S2 (15aug)
feature-b2g 2.1

People

(Reporter: arcturus, Assigned: arcturus)

References

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
daleharvey
: review+
kgrandon
: review+
daleharvey
: feedback+
Details | Review
Make Bookmarks use better icons once bug 1042027 and bug 968156 land
Depends on: 1042027, 968156
Blocks: 1041482
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
Can you please explain how you're going to deal with this completely in gaia?  I'm not sure if that's possible...
Assignee: nobody → 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
Attached file Pointer to PR 22358
Attachment #8465414 - Flags: feedback?(dale)
(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...
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.
(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.
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 :(
(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.)
Since we removed the previous browser chrome we will need to get it back to try this patch, blocking on bug 1045828
Depends on: 1045828
feature-b2g: --- → 2.1
Whiteboard: [systemsfe]
> 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 on attachment 8465414 [details] [review]
Pointer to PR 22358

Looking good so far, as mentioned we should also fix this for places results
Attachment #8465414 - Flags: feedback?(dale) → feedback+
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.
Summary: [Larger icons in homescreen] Use better icons in Bookmarks → [Larger icons in homescreen] Use better icons
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.
Target Milestone: --- → 2.1 S2 (15aug)
Depends on: 1048927
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)
Attachment #8465414 - Flags: review?(dale)
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 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
Attachment #8465414 - Flags: review?(dale)
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!
Flags: needinfo?(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?
Flags: needinfo?(francisco) → needinfo?(dale)
Just sync with Dale on IRC, he did a fresh build, so the patch has some bugs as he comments.
Flags: needinfo?(dale)
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.
Attachment #8465414 - Flags: review?(kgrandon)
Attachment #8465414 - Flags: review?(dale)
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!
Attachment #8465414 - Flags: review?(kgrandon) → review+
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.
Attachment #8465414 - Flags: review?(dale) → review+
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
Flags: needinfo?(francisco)
I needed to add a require to the unit tests and removed sensors.json

https://github.com/mozilla-b2g/gaia/commit/afcdd36f13e75adcdebe57d842a277fd587faf28
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(francisco)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: