1174813 - Support for site icons provided by the web manifest

Status: RESOLVED FIXED

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

Description

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

Extend AppWindow / AppChrome to define and associate an icon with web sites ("apps") that provide details in a web manifest

Comment 1

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

In progress at gmarty's branch: 
https://github.com/gmarty/gaia/tree/Bug-1168944-View-Pin-Badge

Comment 2

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

So I'm not sure it makes sense to have marionette tests for this patch - its all library-level stuff with no user-facing features to test. We'll need integration tests for the parent bug 1168944 and the other features which use IconsHelper. 

One thing I think we should resolve before review and landing is the overlap with WebManifestHelper and its iconURLForSize method: https://github.com/mozilla-b2g/gaia/blob/master/shared/js/web_manifest_helper.js#L47:L65

This uses a slightly different signature, return value and algorithm for selecting the nearest size icon. As an experiment I tried calling WebManifestHelper.iconURLForSize() from IconsHelper.getBestIconFromWebManifest() to see which unit tests broke. The algorithm prefers the nearest size using Math.abs(parsedSize - size), which gives a different result to the implementation in IconsHelper. Actually, from past discussions with UX, I think neither is correct - ideally we would pick the icon which *equals* the preferred size or the next biggest. 

Details aside, we still need to figure out if
a) the duplication is ok and they are different use cases. This seems like a recipe for future confusion and we should bite the bullet now and get a good API worked out. 
b) iconURLForSize should move out of WebManifestHelper and get integrated into IconsHelper. WebManifestHelper would then depend on IconsHelper
c) getBestIconFromWebManifest should be removed from IconsHelper, or gutted so it just calls WebManifestHelper.iconURLForSize. IconsHelper would then depend on WebManifestHelper. 
d) Some other refactoring to tease apart the icons and webmanifest aspects...

I really don't know which way to go. On the one hand IconsHelper is likely to be more widely used and provides slightly lower-level facilities. Neither method is async friendly though so lazy-loading either library would pose a problem.

Comment 3

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

Ben, can you chime in on comment #2?

Comment 4

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

Ok, talked with gmarty and here's the plan: 

In WebManifestHelper, it will only ensure a sanitized icons list is returned. I'm thinking it might pre-process the xhr.response before resolving with it, and provide a helper to do the same for raw js/json manifests (esp. for tests)

In IconsHelper, it will provide the preferred-size functionality exclusively. For icons in web manifests, this will be via the getBestIconFromWebManifest. We'll merge any logic from WebManifestHelper here as necessary. 

All consumers of WMH.iconURLForSize will be updated to use IconHelper.getBestIconfromWebManifest.

I'm working on this and the associated tests right now. Its a bit more work but a sane plan going forward I think.

Comment 5

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

Assigning to me for me.

Comment 6

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

Attachment: PR: web manifest icons handling

See PR for details, this is getting close I think

Comment 7

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

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

tldr; 
Ben - Can you review the app_chrome changes (js, css, default icons)
Christian - can you review the Bookmarks and Collection changes
Etienne - can you review the AppWindow changes
All - can you look over the /shared/ changes. (Not sure if I need to loop in :djf?)
 
This lays groundwork for bug 1174813 and the other pinning-the-web tasks by adding methods for getting and setting icons on the AppWindow and AppChrome respectively. This appears non-symetrical, but actually getSiteIcon will be used by AppChrome, TaskManager and possibly elsewhere, whereas setSiteIcon is specifically for AppChrome to set (and display) its site icon. 

IconsHelper became a lot more helpful. There should be few follow-ups to remove duplication across Gaia now it has fetchIcon and the best-size functionality. WebManifestHelper has a new processRawManifest method - which may disappear into the platform - all that stuff is copied from the relevant jsm. It guarantees that the icons list will have been sanitized and resolved to real URL instances with the manifestURL as base. Removing the icon size functionality from WebManifestHelper meant some light refactoring - so the patch touches grid_item.js, and the bookmarks_editor and the relevant HTML to load the new helper. 

One thing to call out in the IconsHelper. The best-size algorithm is ported from https://dxr.mozilla.org/mozilla-central/source/dom/manifest/ManifestProcessor.js, where it looks for the nearest size (smaller or larger). This broke a couple of tests, which I've amended to agree with the implementation.

I've not flattened the patch yet - its a joint effort from gmarty and myself. I hate to lose attribution by collapsing into one commit, but as it wouldnt make sense to back out one piece but not the other, I'll do that before landing.

Comment 8

[Guillaume Marty [:gmarty]]

Just a quick note regarding this patch:
* We need a cache mechanism for web manifest files. AFAIU, the plan is to move the helper to the platform where this can be taken care of. But in the meantime, we still want to avoid downloading the manifest at each page load.
* The icons cached in the data store need to be invalidated after some time (let's say 24 hours). Also, maybe we need a way to clear the cache manually (e.g. via Settings > Browsing Privacy > Clear browsing history). This will be useful for developers who update the icon and want to test it.
* The icon retrieving logic from the manifest seems not to be completely on par with the spec. The `type` and `density` properties should be used.
* Finally, what about an optimisation that would map a webpage URL to an icon blob? That would save computational time on the device (no more getting the web manifest from data store and executing the icon finding algorithm). Such a mechanism would allow to display the icon much earlier in the web page lifespan if it is already in the datastore. What do you think?

Let's get this patch landed asap and then we can open followup bugs for the points above.

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

Only looked at the WindowManager part, but it looks good :)

I know it's a pain but I'd like to add unit test coverage for
- mozbrowsermanifestchange eventually leads to a correctly set `appWindow.webManifest` by calling a mock web manifest helper with the correct url
- getSiteIcon resolve with the result of a mock IconHelper

or, alternatively, we can use the fake server machinery from the homescreen integration tests [1] to add one or two integration tests covering everything! :)

[1] https://github.com/mozilla-b2g/gaia/blob/74480f23df14152054fb6b21a7c9bb0d8cd6c5d2/apps/verticalhome/test/marionette/app_appcache_pending_test.js#L17-21

Comment 10

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

> I know it's a pain but I'd like to add unit test coverage for
> - mozbrowsermanifestchange eventually leads to a correctly set
> `appWindow.webManifest` by calling a mock web manifest helper with the
> correct url
> - getSiteIcon resolve with the result of a mock IconHelper

Not a pain at all - I had this in an earlier local version of the patch and it got lost along the way. 

> or, alternatively, we can use the fake server machinery from the homescreen
> integration tests [1] to add one or two integration tests covering
> everything! :)

I do want to add integration tests, but I think it makes more sense to add them to the user-facing features, not the underlying library code - which is better unit tested. So they will likely land with bug 1168944.

Comment 11

[Etienne Segonzac (:etienne)]

(In reply to Sam Foster [:sfoster] from comment #10)
> I do want to add integration tests, but I think it makes more sense to add
> them to the user-facing features, not the underlying library code - which is
> better unit tested. So they will likely land with bug 1168944.

Makes perfect sense, thanks!

Comment 12

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

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

A bit of cleanup in the updated PR, and I've added unit tests for the new AppWindow functionality. I renamed getSiteIcon to getSiteIconUrl as it returns the url not some kind of icon object.

Comment 13

[Ben Francis [:benfrancis]]

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

This is looking awesome.

One thing I noticed when trying it out is that I never see the default icon, did it not commit properly? Also, not showing an icon during load looks a bit broken. I've been wondering for a while whether we should use a spinner in place of the icon during page load instead of the current chrome's progress bar.

Eric, what do you think about that? Could you provide us with a great looking default icon and a spinner?

I'm leaving feedback+ until we figure out the visuals but I'm keen for Etienne to review the AppWindow parts, I'm not sure whether all the methods on AppWindow belong on AppWindow.

Sam I think we should consider "weighting for a larger vs. a smaller icon" in the icon helper as it says in the comments. Nice work slipping the #-moz-resolution thing in there ;)

Comment 14

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

 > One thing I noticed when trying it out is that I never see the default icon,
> did it not commit properly? 

Yeah looks like those files got messed up along the way. I've pushed a new PR which should restore them. 

> I'm leaving feedback+ until we figure out the visuals but I'm keen for
> Etienne to review the AppWindow parts, I'm not sure whether all the methods
> on AppWindow belong on AppWindow.

I'd argue getSiteIconUrl does belong on AppWindow - unless we create some object to represent a site in which case it could go on there. I'm less sure about getIconBlob - that's basically a wrapper around a lazy-loaded IconsHelper.getIconBlob to avoid a hard dependency on IconsHelper. I'll take any suggestions on a better way to slice and dice this. 

> Sam I think we should consider "weighting for a larger vs. a smaller icon"
> in the icon helper as it says in the comments. 

Yeah, we need to balance un-blocking folks working on pin-the-web features and landing stable & high-quality code. This and all gmarty's suggestions in comment #8 clearly need to happen. I'll look at that algorithm, but would suggest this is a potential candidate for a follow-up as the current implementation matches existing behavior.  

> Nice work slipping the
> #-moz-resolution thing in there ;)

Gmarty gets credit for that - and for a lot of the implementation here. Our branches got too entangled to preserve attribution though and I had to flatten to simplify.

Comment 15

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

Oh, also noticed I have Gij failures, I'll look into those.

Comment 16

[Etienne Segonzac (:etienne)]

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

2 small comments on github, apart from that I can't wait to see the feature (and the integration tests ;)) move forward!

Comment 17

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

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

Working on Etienne's comments right now.

Comment 18

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

PR is updated with 50% border radius by default on the .urlbar  .site-icon, and an empty click event listener per Etienne's request.

Comment 19

[Eric Pang [:epang] UX]

Jacqueline,

What do you think of 

'I've been wondering for a while whether we should use a spinner in place of the icon during page load instead of the current chrome's progress bar.'

I'm trying to flash my devices to get more clear picture of how this will work.  I'm worried it might break interaction patterns.

Comment 20

[Ben Francis [:benfrancis]]

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

Sam, this looks great. I've left a couple of comments on the pull request that I'd like to address before landing. My only concerns are not taking pixel density into account and a potential race condition during loading.

Let's leave the progress bar discussion for a follow up in bug 1179908 in the interests of getting this landed, but Eric is working on a nicer default icon which I'd like to get in this patch if it doesn't take too long.

Thanks!

Comment 21

[Eric Pang [:epang] UX]

Attachment: Pin_icons.zip

updated pin icon

Comment 22

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

* updated the PR with the new icons. 
* left a comment on the PR about the devicePixelRatio - IconsHelper.getIcon does that for us

Ben's right about the race with WebManifestHelper.getManifest and handleLoadEnd. I started looking at stashing a promise while that is in-flight, and rolling that promise into getSiteIconUrl, but ran into the weeds trying to unit test it, and ran out of time. Much as I would like to see this land, I think that should be resolved first. I have long weekend + PTO until Tuesday; feel free to push it over the finish line if this is blocking you. If its still unresolved when I'm back I'll pick it back up.

Comment 23

[Ben Francis [:benfrancis]]

Comment on attachment 8624628 [details] [review]
PR: web manifest icons handling

Etienne, the patch lost your review flag, could you add it again? Looks like we still have one failing integration test but I'd like to figure out how to get this landed today if we can.

Comment 24

[Ben Francis [:benfrancis]]

Oh, the integration test was a known intermittent being tracked in bug 1179580.

Let's get this landed because it's blocking all the other Pin the Web work, and I'll file a follow up for the icon race. The only side effect is that the user might get a different icon.

Comment 25

[Ben Francis [:benfrancis]]

Merged into master https://github.com/mozilla-b2g/gaia/commit/83927a21d3ad081e9ffb0c853ee52890d85dc37d

Filed follow-up bug 1180297 for manifest which finish loading after the page's loadend event.

Clearing NEEDINFO on Jacqueline, will follow up on progress bar changes in bug 1179908.

Thanks everyone!

Comment 26

[Etienne Segonzac (:etienne)]

(In reply to Ben Francis [:benfrancis] from comment #23)
> Comment on attachment 8624628 [details] [review]
> PR: web manifest icons handling
> 
> Etienne, the patch lost your review flag, could you add it again? Looks like
> we still have one failing integration test but I'd like to figure out how to
> get this landed today if we can.

No worries, you can always "carry" the r+ to another patch by setting the + flag and mentioning in the comment that you're "carrying the r+ from Comment X"