1196809 - Pick best available icon for pinned sites on homescreen

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P2)

Description

[Ben Francis [:benfrancis]]

Currently bookmarks use the .icon property of an object stored in the bookmarks DataStore which is set to a single icon when the bookmark is saved.

In bug 1168960 we are saving pinned sites in the bookmarks DataStore using a siteObject which may include multiple .icons specified in <link rel=icon> tags and also .webManifest metadata if provided by the site, which may also specify multiple icons of different sizes.

Instead of using the .icon property, the (new) homescreen should use the IconsHelper to extract, fetch and cache the best available icon from the site object for the size required at runtime.

It may need to fall back to the .icon property for existing bookmarks if no migration is carried out.

Comment 1

[Guillaume Marty [:gmarty]]

I've started working on it. The very early work in progress is in this branch:
https://github.com/gmarty/gaia/tree/Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen

Comment 2

[Guillaume Marty [:gmarty]]

The patch is looking good but now the homescreen crashes whenever a bookmark is opened (repro 100% of the time).
Chris, it's very likely to be a Gecko bug, can you help me debugging this? I need to decrypt the logcat.

Comment 3

[:gerard-majax]

(In reply to Guillaume Marty [:gmarty] from comment #2)
> The patch is looking good but now the homescreen crashes whenever a bookmark
> is opened (repro 100% of the time).
> Chris, it's very likely to be a Gecko bug, can you help me debugging this? I
> need to decrypt the logcat.

Are you running a debug build ? If so, then the leak in bug 1202592 on debug build will produce this kind of crash. It should be explicit when looking at logcat.

Comment 4

[Guillaume Marty [:gmarty]]

That's a regular build but the logcat show these lines:
I/Gecko   (  318): -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR dev.gaia.pinning_the_web: 23 FROMaddObserver@jar:file:///system/b2g/omni.ja!/components/SettingsManager.js:382:48
I/Gecko   (  318): sl_observe@app://system.gaiamobile.org/shared/js/settings_listener.js:58:5
I/Gecko   (  318): BrowserContextMenu@app://system.gaiamobile.org/js/browser_context_menu.js:21:1
...
I/Gecko   (  318): -*- SettingsManager: WARNING: MORE THAN 10 OBSERVERS FOR dev.gaia.pinning_the_web: 24 FROMaddObserver@jar:file:///system/b2g/omni.ja!/components/SettingsManager.js:382:48
I/Gecko   (  318): sl_observe@app://system.gaiamobile.org/shared/js/settings_listener.js:58:5
I/Gecko   (  318): ac__registerEvents@app://system.gaiamobile.org/js/app_chrome.js:608:7

And there's no way there are more than 2 of these observers when on startup there only a couple instances of appChrome objects.
Also commenting out the lines using SettingsListener prevents the crash. So there's definitely an issue here.

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

Comment 6

[Guillaume Marty [:gmarty]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

I can't reproduce the crash in the latest master, so it's ready to review.

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

I've added comments on github - there are outstanding issues/questions before I can r+ this.

Comment 8

[:gerard-majax]

(In reply to Guillaume Marty [:gmarty] from comment #2)
> The patch is looking good but now the homescreen crashes whenever a bookmark
> is opened (repro 100% of the time).
> Chris, it's very likely to be a Gecko bug, can you help me debugging this? I
> need to decrypt the logcat.

I am wondering if you are not just seeing another instance of bug 1206485. Attaching a gdb might help.

Comment 9

[Guillaume Marty [:gmarty]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

I reworked the logic as discussed with Alberto.
Before going deeper in refining the code and writing test, can you take a look to see if the patch is going in the right direction?

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

This looks good to me - left a couple of comments, but likely just over WIP stuff.

Comment 11

[Guillaume Marty [:gmarty]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

Chris, how does it look like now?

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

I think this is mostly ok, but:

- This doesn't actually switch over homescreen from using gaia-app-icon in its bower_components to gaia-site-icon?

- If the former is done, the icon-loaded handler will need to be modified so that it doesn't cache icons that we just set (because Alberto changed gaia-site-icon so that icon-loaded is always fired, even when the icon is user-set)

- We also need to not cache bookmark icons I guess, if they're cached by IconsHelper?

Comment 13

[Guillaume Marty [:gmarty]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

Ready for another round of review.
I made the changes you pointed and reworked the dependencies to avoid useless duplication with /shared/elements.

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 8662956 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen > mozilla-b2g:master

One nit, but lgtm :) Re-flag me if there are failing tests that you need to make significant changes to get to pass.

Comment 15

[Carsten Book [:Tomcat]]

seems there are merge conflicts, could you take a look, thanks!

Comment 16

[Chris Lord [:cwiiis]]

Merged: https://github.com/mozilla-b2g/gaia/commit/8f02e2b33ff13656066137cae70abf6b22b0de99

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Reverted this in https://github.com/mozilla-b2g/gaia/commit/356d5fabeb4e06097ae595f01f4b78c0af93d770 for gij(7) failures.

https://treeherder.mozilla.org/logviewer.html#?job_id=2970204&repo=b2g-inbound

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen-new > mozilla-b2g:master

Comment 19

[Guillaume Marty [:gmarty]]

Comment on attachment 8671825 [details] [review]
[gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen-new > mozilla-b2g:master

Carrying r+ from Chris.

Comment 20

[Guillaume Marty [:gmarty]]

I double checked and the tests are green on treeherder. Also the failing Gij test passes locally.

Comment 21

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/537898c4fa889134a78b37936f4499b64685a07c

Comment 22

[Michael Henretty [:mikehenrty][:mhenretty]]

Hmm, looking at the tests on the PR, it looks like we have a failing test:
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=37ff7d9cd0c8b163d1dc4ee058ca6de432d10e82

And this test is now failing on gaia-master:
https://treeherder.mozilla.org/#/jobs?repo=gaia-master&revision=8e503650edcf7392349f68d845f36f35ddfb91fb

Comment 23

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1196809-Pick-best-available-icon-for-pinned-sites-on-homescreen-hotfix > mozilla-b2g:master

Comment 24

[Carsten Book [:Tomcat]]

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