1234558 - Use icon/title from app manifest for "Add to home screen"

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[:Margaret Leibovic]

When adding a page to the user's Android home screen, we should use an icon from the site's app manifest, if it exists:
https://developer.mozilla.org/en-US/docs/Web/Manifest

Bug 826400 adds support for apple-touch-icons, this is another step towards better home screen shortcuts.

Comment 1

[Dale Harvey (:daleharvey)]

Hi Marcos, in https://bugzilla.mozilla.org/show_bug.cgi?id=997779 you have a patch in progress to add w3c manifest support within gecko, is there any chance you could give me a quick overview of how you were planning to expose this in a way that I can pick up the correct icons within Android? cheers

Comment 2

[Dale Harvey (:daleharvey)]

So just to update, beginning to figure out the API I think, but hitting issues, just after http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4004

I have:

    } else if (list.indexOf("[manifest]") != -1 && aEvent.type == "DOMLinkAdded") {
      dump('WTF browser.js GOT A MANIFEST');
      var result = ManifestObtainer.browserObtainManifest(this.browser);
      result.then(function(res) {
        dump('WTF browser.js browserObtainManifest inside then' + res);
      }).catch(function(err) {
        dump('WTF browser.js browserObtainManifest inside catch' + err);
      });

and browserObtainManifest is hanging with the promise never being resolved, trying to track it down now, it gets passed the |!isXULBrowser(aBrowser)| check but gets no return from the parent (the exact same happened when I previously tried using ManifestFinder.hasManifestLink although I dont think this code will need to use that)

Comment 3

[Dale Harvey (:daleharvey)]

ok figured it out, manifestMessages.js needs to be loaded to be listening from the content side

Comment 4

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #3)
> ok figured it out, manifestMessages.js needs to be loaded to be listening
> from the content side

Ok, cool - sorry for not replying earlier (timezone difference means your Friday is already my Saturday:(). I don't now much about how Fennec is set up... is there an equivalent of browser.js where you now load manifestMessages.js?

Comment 5

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #1)
> Hi Marcos, in https://bugzilla.mozilla.org/show_bug.cgi?id=997779 you have a
> patch in progress to add w3c manifest support within gecko, is there any
> chance you could give me a quick overview of how you were planning to expose
> this in a way that I can pick up the correct icons within Android? cheers

My initial plan was to leverage a `HTMLPictureElement` to pick the correct icon from the content process. That would allow the CSP machinery + Service Workers to intervene correctly - while also picking the best image for a given set of constraints (e.g., content type, or even a media query). Once the image has been fetched,  I was planning on transferring it to the parent process (directly through a transfer, if possible, or worst case as a "data:" URL string that can be re-hydrated to a blob on the parent process). 

Then, on Android, you could create an image PNG from that to write to disk.

Alternative would be for the parent process to provide a set of constraints (content type, size pref, etc.), and we filter out the icons that don't match. Then, in the child process, we would leverage `fetch()` to obtain the best image (+security checks) and basically again do what I said above to send the image back to the parent process... which Android could then use. 

I'm more than happy to code that up, but need your guidance on how to get the image back to you after it's gone through the DOM/Platform code and security checks. I'd also need to know what kind of constraints there might be. For example, Google Chrome only allows PNG and refuses to convert any other format to PNG to write to Android (thus forcing developers to only use PNGs, which is bit sad - but understandable). IIRC, Chrome also discards anything small than 144px or so... Would we want to do the same in Fennec? 

WDYT?

Comment 6

[Dale Harvey (:daleharvey)]

No bother, yeh the timezones are as far away as they can be (I am UK based) :) Yup Android has its own browser.js (http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js) 

I am not certain that having manifest code to fetch the icon would be helpful / usable, in Android (as it was on fxos) there is already support for consuming icons from various places (usually meta tags, sometimes generated) so I am assuming Android already has code in place to fetch the icon (+ post process it), is there any thing specific to manifest icons that would require them to be fetched specially? Assuming the CSP controls etc apply equally to manifest icons as ones defined within meta tags.

Working on this now and not blocked on anything so will update if I hit up against anything

Comment 7

[Marcos Caceres [:marcosc]]

>  is there any thing specific to manifest icons that would require them to be fetched specially? 

No. So long as the request goes through the appropriate CSP security checks and that a service worker can intervene the fetch request.  

> Assuming the CSP controls etc apply equally to manifest icons as ones defined within meta tags.

In theory, yes... but I've not checked. 

> Working on this now and not blocked on anything so will update if I hit up against anything. 

Ok, I'm happy to try to help. Looking forward to seeing what you end up putting together.

Comment 8

[Dale Harvey (:daleharvey)]

Attachment: WIP Use icons from app manifest

This is a working patch for icon manifests, currently needs some work before being landable, most of the issues are mentioned in TODO's but could do with some guidance here.

First issue is the annotations, as pointed out in the commends insertAnnotation doesnt seem particularly designed for the situation of having a single MANIFEST key, should I go back to a db migration and adding a manifest field or put an custom INSERT OR REPLACE only for manifest annotations?

Second issue is where the manifest fetcher should go? this is mostly just code organisation there should be some Manifest class that deals with fetching / caching etc, where should I put it?

Another issue is I can see icon requests getting hit a lot, it seems like we would only want ManifestIcons to do its business when the launcher has specifically been requested but cant see how to do that?

Thanks in advance for the feedback, adding you Sebastian but if theres anyone else more suited please forward it on, cheers

Comment 9

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #8) 
> Second issue is where the manifest fetcher should go? this is mostly just
> code organisation there should be some Manifest class that deals with
> fetching / caching etc, where should I put it?

We already have a fully-conforming manifest fetcher :) It's in:

dom/manifest/

And consists of 2 classes:

ManifestFinder.jsm - finds manifests in content. 
ManifestObtainer.jsm - fetches manifest from a URL, CORS allowing. 

(There is also a broken icon fetcher in there... which we could fix up)

It's used like this (assuming there is something like browser.js for Android):

XPCOMUtils.defineLazyModuleGetter(this, "ManifestFinder",
  "resource://gre/modules/ManifestFinder.jsm");

XPCOMUtils.defineLazyModuleGetter(this, "ManifestFinder",
  "resource://gre/modules/ManifestObtainer.jsm");

const hasManifest = ManifestFinder.contentHasManifestLink(content);

if (hasManifest)
  const manifest = yield ManifestObtainer.contentObtainManifest(content);

You can see how it's done in Gecko:
https://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#1488
 
> Another issue is I can see icon requests getting hit a lot, it seems like we
> would only want ManifestIcons to do its business when the launcher has
> specifically been requested but cant see how to do that?

I was thinking we would only spin up a fetching icons when we kick off an PWA installation process. 

However, I've only thought about this from a specification perspective - so that might not match reality :)

> Thanks in advance for the feedback, adding you Sebastian but if theres
> anyone else more suited please forward it on, cheers

Comment 10

[Marcos Caceres [:marcosc]]

Some copy/pasta above... should be:

XPCOMUtils.defineLazyModuleGetter(this, "ManifestObtainer",
  "resource://gre/modules/ManifestObtainer.jsm");

Comment 11

[Dale Harvey (:daleharvey)]

Yup, I had an original version that used /dom/manifest/ code (there are still some remnants in that patch) however there are problems using that.

Fennec has a browser.js but its scope is much smaller than desktop, it looks like its for communicating with content primarily, Fennec's UI is largely in Java, the path from pressing "Add to Homescreen" to the homescreen icon being added is all in Java and while its possible to call between Java <-> JS as far as I can tell that is mostly done with async message passing which I am not sure can fit nicely into the method that /icons/preperation uses to get the icons setup. 

There will need to be some Manifest.java class that deals with the icons interaction with the manifest, whether it can call into js and share the fetcher code I am not sure, but will look into

Comment 12

[Dale Harvey (:daleharvey)]

btw forgot to mention I have been using http://junk.arandomurl.com/testapp.html to test, it doesnt have any link rel or touch icons, only a manifest with an icons property

Comment 13

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #11)
> Yup, I had an original version that used /dom/manifest/ code (there are
> still some remnants in that patch) however there are problems using that.
> 
> Fennec has a browser.js but its scope is much smaller than desktop, it looks
> like its for communicating with content primarily, Fennec's UI is largely in
> Java, the path from pressing "Add to Homescreen" to the homescreen icon
> being added is all in Java and while its possible to call between Java <->
> JS as far as I can tell that is mostly done with async message passing which
> I am not sure can fit nicely into the method that /icons/preperation uses to
> get the icons setup. 

The requirement is that the image request needs to have the ability to pass through the service worker and through the CSP machinery. 

The use cases being: 

 * an application should be installable when the user is using the app offline: where the icon is served from the Service Worker's cache. 
 * the icon might be synthesized but the service worker prior to install. 


> There will need to be some Manifest.java class that deals with the icons
> interaction with the manifest, whether it can call into js and share the
> fetcher code I am not sure, but will look into

Ok, let me know I can help. Given the re-org, we are all now part of the same team (pod?) :) It would be great for us to come up with an implementation plan.

Comment 14

[Dale Harvey (:daleharvey)]

Yeh thats a good point, gonna take a look now to see how to make that possible, thanks

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8798073 [details] [diff] [review]
WIP Use icons from app manifest

Review of attachment 8798073 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dale Harvey (:daleharvey) from comment #8)
> First issue is the annotations, as pointed out in the commends
> insertAnnotation doesnt seem particularly designed for the situation of
> having a single MANIFEST key, should I go back to a db migration and adding
> a manifest field or put an custom INSERT OR REPLACE only for manifest
> annotations?

I think INSERT OR REPLACE is a good choice here. We probably want to rename MANIFEST to something like MANIFEST_URL because we are only storing the URL and we might want to store the whole manifest there later too. This is almost what we do with the FEED* annotations: FEED is a mapping from URL to feed URL (RSS/ATOM) and FEED_SUBSCRIPTION is a mapping from feed URL to a tiny JSON object describing the feed and the last state we fetched. I could see us building something like that for the manifest too (Mapping from URL to Manifest URL [n:1] and mapping from Manifest URL to Manifest JSON [1:1]).
 

> Second issue is where the manifest fetcher should go? this is mostly just
> code organisation there should be some Manifest class that deals with
> fetching / caching etc, where should I put it?

I guess we are going to need even more code as soon as we are going to implement more of the manifest features. So a completely new package might be helpful. Maybe org.mozilla.gecko.webappmanifest or org.mozilla.gecko.pwa? We can move things around at any time so this is not critical.

> Another issue is I can see icon requests getting hit a lot, it seems like we
> would only want ManifestIcons to do its business when the launcher has
> specifically been requested but cant see how to do that?

Yeah, this code should only run whenever we actually need an icon for a launcher. Code that needs an icon will use an IconRequestBuilder instance to build the request. There's already a method forLauncherIcon() that currently only modifies the target size of the icon. Additionally we could save this boolean in the request. Every 'Preparer' implementation gets the request passed in an you could exist early if not request.isForLauncher() or something like that.

::: mobile/android/base/java/org/mozilla/gecko/Tab.java
@@ +526,5 @@
>  
> +    // TODO: This is going to create an annotation for every time a
> +    // website with a manifest is visited, we only want to create one
> +    // annotation
> +    public void addManifest(String manifestUrl) {

We needed to do the same for the feeds I mentioned. There's an open bug to make it INSERT/REPLACE in one statement, see bug 1259170 for hints.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ +655,5 @@
> +             *
> +             * Key:   manifest
> +             * Value: URL of manifest
> +             */
> +            MANIFEST("manifest"),

As mentioned above we might want to rename it so that we can use "MANIFEST" for actually storing the manifest here (Assuming we might want to store just the JSON blob and not normalize the data).

::: mobile/android/base/java/org/mozilla/gecko/icons/preparation/ManifestIcons.java
@@ +42,5 @@
> +/**
> + * Preparer implementation that checks if the page has a reference to a manifest and if so finds the
> + * appropriate icon and adds it to the list of available icons.
> + */
> +public class ManifestIcons implements Preparer {

Matching AddDefaultIconUrl we might want to name this AddManifestIconUrls or something like that.

@@ +45,5 @@
> + */
> +public class ManifestIcons implements Preparer {
> +    @Override
> +    public void prepare(IconRequest request) {
> +

As mentioned earlier: We should exist early if this is not for a launcher icon. This should be a flag of the request.

@@ +69,5 @@
> +            String iconSrc = getCorrectSizeIcon(targetSize, manifest);
> +
> +            // TODO: probably want to construct the url nicer?
> +            URL origin = new URL(pageUrl);
> +            String iconUrl = origin.getProtocol() + "://" + origin.getAuthority() + "/" + iconSrc;

That's how we construct the "default URL" (/favicon.ico):
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons/IconsHelper.java#109

@@ +81,5 @@
> +        } catch (Exception e) {
> +            // If JSON is invalid or we fail to fetch the manifest
> +            // then we dont get a fancy icon
> +        } finally {
> +            cursor.close();

You close the cursor but you never call disconnect() on the connection.

@@ +88,5 @@
> +
> +    /*
> +     *
> +     */
> +    private String getCorrectSizeIcon(int targetSize, JSONObject manifest) throws JSONException {

This looks like the code we use in FaviconLoadResult to pick the best icon from a list icons in an .ico favicon.

But you actually don't need it: Just add all icons that are in the manifest to the request. IconTask/IconRequest already do a ranking (based on type, size etc.) and you do not need to implement one here.

Comment 16

[Dale Harvey (:daleharvey)]

Marcos, switching this to use ManifestObtainer etc, however having problems trying to figure out the api, given a manifest fetched by 

   ManifestObtainer.browserObtainManifest(browser).then(manifest => {

What is the right way to pick the correct icon? I can see ManifestProcessor.process but it is expecting to be given the manifest json as a string and has a few assumptions I do not quite understand

Comment 17

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #16)
> Marcos, switching this to use ManifestObtainer etc, however having problems
> trying to figure out the api, given a manifest fetched by 
> 
>    ManifestObtainer.browserObtainManifest(browser).then(manifest => {
> 
> What is the right way to pick the correct icon? I can see
> ManifestProcessor.process but it is expecting to be given the manifest json
> as a string and has a few assumptions I do not quite understand

Sorry, should write a proper usage guide for this :( You should never need to use ManifestProcessor on it's own.

you would normally do:

```JS
async.task(function*(){
   const hasManifest = yield ManifestFinder.browserHasManifestLink(aBrowser);
   if (!hasManifest) return;
   const manifest = yield ManifestObtainer.browserObtainManifest(browser);
   // manifest.icons should be here
});
```

If the above is not returning icons, then there is a bug somewhere. Do you have some sample code?

Comment 18

[Marcos Caceres [:marcosc]]

That should be: 
```
const hasManifest = yield ManifestFinder.browserHasManifestLink(browser);

```

Should test code before writing it :)

Comment 19

[Dale Harvey (:daleharvey)]

I get the icon details, I was looking for the code to pick the correct icon by size and to do the actual fetching (so it goes via the SW etc), Ill just write the code to do that for now and can clean it up once its working

Comment 20

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #19)
> I get the icon details, I was looking for the code to pick the correct icon
> by size and to do the actual fetching (so it goes via the SW etc), Ill just
> write the code to do that for now and can clean it up once its working

My idea for image selection was to reuse Gecko's `<picture>` element infrastructure to do the selection (by mapping the manifest icons into <source srcset="" media=""> element/attributes). That way, we get the correct security checks when loading the images and the service worker can intervene if necessary. Then you get the right image out based on `img.currentSrc`.

Comment 21

[Dale Harvey (:daleharvey)]

Attachment: WIP Use icons from app manifest

So again this is a working patch but instead of doing all the work in Java I am going back into Gecko for the manifest and icon fetching. 

Marcos: I dont have the code in there for picking the correct size, I was assuming I would just parse the manifest `sizes` field and pick appropriately, but if you think its a good idea to create a picture element is there any chance you could give a little code sample on how? Not very familiar with constructing the dom outside of the context of a body

Sebastian: Assuming the sizes code is correct, how does the rest look? I am going to do a little cleaning up / handling error cases, I am not currently storing the manifest as we dont need to use it again, but planning on picking a feature for the next patch that requires storing of the manifest

As far as I could tell from looking quickly ContentProviders need to have UNIQUE clauses to do an INSERT OR REPLACE so I punted on that for this patch and did the extra query, that can be cleaned up.

For the passing the icon, being able to directly transfer the blob would be ideal but I figured that would likely be very hard between gecko and java, Mike has mentioned Desktop uses datauri's between processes a lot and since its a one off I figured a datauri was the simplest solution here. 

Maybe want to rename doCreateShortcut to just createShortcut with a different sig? wasnt sure on the best standards for that.

Cheers
Dale

Comment 22

[Marcos Caceres [:marcosc]]

Attachment: ToResposiveImage.js

(In reply to Dale Harvey (:daleharvey) from comment #21)
> Created attachment 8804219 [details] [diff] [review]
> WIP Use icons from app manifest
> 
> So again this is a working patch but instead of doing all the work in Java I
> am going back into Gecko for the manifest and icon fetching. 
> 
> Marcos: I dont have the code in there for picking the correct size, I was
> assuming I would just parse the manifest `sizes` field and pick
> appropriately, but if you think its a good idea to create a picture element
> is there any chance you could give a little code sample on how? Not very
> familiar with constructing the dom outside of the context of a body

I found some old code I was playing around with (I wrote it last year, so I honestly don't even know if it works - and it was literally just me playing in Firefox's scratch pad, so apologies in advance!)... effectively, something like the attached would go into "dom/ipc/manifestMessages.js" (or equivalent for Fennec).

In any case, it shows some of the rough conversion to a <picture> element. The only thing that would be need to get it work with a frame script would be to change:

var i = toResponsiveImage(imageObjects, document);

To use "content.", which is the window object in the frame script: 

var i = toResponsiveImage(imageObjects, content.document);

I'll see if I can dig up any other code... I vaguely recall having coded something else up for getting the right icon, but it never landed. 

Hope that helps!

Comment 23

[Marcos Caceres [:marcosc]]

Comment on attachment 8804219 [details] [diff] [review]
WIP Use icons from app manifest

Review of attachment 8804219 [details] [diff] [review]:
-----------------------------------------------------------------

Generally, looks like a good start.

Comment 24

[Marcos Caceres [:marcosc]]

I was looking at ToResposiveImage.js some more, and noticed it still uses "density" - that was dropped from the spec: it always defaults to 1x.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8804219 [details] [diff] [review]
WIP Use icons from app manifest

Review of attachment 8804219 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this is looking pretty good!

(In reply to Dale Harvey (:daleharvey) from comment #21)
> Marcos: I dont have the code in there for picking the correct size, I was
> assuming I would just parse the manifest `sizes` field and pick
> appropriately, but if you think its a good idea to create a picture element
> is there any chance you could give a little code sample on how? Not very
> familiar with constructing the dom outside of the context of a body

Btw. if helpful: We return the system's preferred launcher icon size from GeckoAppShell.getPreferredIconSize(). This method can be accessed via JNI from JavaScript:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_jni.html#23

> Sebastian: Assuming the sizes code is correct, how does the rest look? I am
> going to do a little cleaning up / handling error cases, I am not currently
> storing the manifest as we dont need to use it again, but planning on
> picking a feature for the next patch that requires storing of the manifest

Sounds good! No need to store the manifest now. For implementing activity stream we are adding a more generic way of extracting metadata (like the manifest URL) using page-metadata-parse [1] / fathom [2].

[1] https://github.com/mozilla/page-metadata-parser
[2] https://github.com/mozilla/fathom

There's some first code here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/WebsiteMetadata.jsm

Nothing to worry about now. But something we could switch to in the future.

> As far as I could tell from looking quickly ContentProviders need to have
> UNIQUE clauses to do an INSERT OR REPLACE so I punted on that for this patch
> and did the extra query, that can be cleaned up.

Okay. We can do this in a follow-up bug too.

> For the passing the icon, being able to directly transfer the blob would be
> ideal but I figured that would likely be very hard between gecko and java,
> Mike has mentioned Desktop uses datauri's between processes a lot and since
> its a one off I figured a datauri was the simplest solution here.

Yeah, that's absolutely fine and that's what we are doing to when the favicon is set to a data URI.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +749,5 @@
>              "Sanitize:ClearHistory",
>              "Sanitize:ClearSyncedTabs",
>              "Settings:Show",
>              "Telemetry:Gather",
> +            "Browser:ManifestLoaded",

nit: As this will always trigger creating a shortcut I feel like "ManifestLoaded" is a bit misleading. Maybe something like "ManifestLoadedForShortcut" is more descriptive for now?

@@ +1953,5 @@
>                  break;
>          }
>      }
>  
> +    private Bitmap dataUriToBitmap(String dataUri) {

Instead of this method you can use FaviconDecoder.decodeDataURI(). BitmapFactory does not support the ICO format for example.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +1899,5 @@
>  
>      @Override
>      public void createShortcut(final String title, final String url) {
> +
> +        final String manifestUrl = WebManifest.getManifestUrl(getApplicationContext(), url);

See other comment: BrowserDB could return a String directly and then you wouldn't need the helper method.

::: mobile/android/base/java/org/mozilla/gecko/db/UrlAnnotations.java
@@ +25,5 @@
>      void insertFeedSubscription(ContentResolver cr, FeedSubscription subscription);
> +
> +    void insertManifest(ContentResolver cr, String originUrl, String manifestUrl);
> +    boolean hasManifestForUrl(ContentResolver cr, String originUrl);
> +    Cursor getManifest(ContentResolver cr, String originUrl);

Those methods should all have a URL suffix: insertManifestURL, getManifestURL - because later we might actually insert/get the manifest.

getManifestUrl() could return a string - then callers will not need to deal with the Cursor object and you do not need helper methods.

::: mobile/android/base/java/org/mozilla/gecko/manifest/WebManifest.java
@@ +20,5 @@
> +
> +    public NativeJSObject manifest;
> +
> +    public WebManifest(NativeJSObject _manifest) {
> +        manifest = _manifest;

nit: this.manifest = manifest;

If possible we should extract all the data here in the constructur and not keep the NativeJSObject around.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
@@ +824,5 @@
>  
> +    // Creates a homescreen shortcut for a web page.
> +    // This is the entry point from nsIShellService.
> +    @WrapForJNI(calledFrom = "gecko")
> +    public static void doCreateShortcut(final String aTitle, final String aURI, Bitmap icon) {

This is called from BrowserApp and doesn't need to be accessible from JNI / native code - or did I miss something?

Comment 26

[Dale Harvey (:daleharvey)]

Attachment: Use icons from app manifest

Ok addressed that feedback, thanks a lot. I think this is mostly good although still have a few questions:

In ManifestIcons, is using the DOM like that going to have any effect on the users web content? (thinking like an onerror gettting fired or similiar), also due to using canvas these icons are going to subject to same origin restrictions, is that on purpose or should we work around that somehow?

And this should likely need tests, probably want to test it behaves that it works in a few edge cases, any chance someone could point me to an example of the type of test we would create for this type of code?

Cheers

Comment 27

[Marcos Caceres [:marcosc]]

Comment on attachment 8807136 [details] [diff] [review]
Use icons from app manifest

Review of attachment 8807136 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good... couple of comments and questions.

::: dom/manifest/ManifestIcons.jsm
@@ +5,5 @@
> +Components.utils.import("resource://gre/modules/JNI.jsm");
> +
> +this.ManifestIcons = {
> +
> +  browserFetchIcon: Task.async(function* (aBrowser, manifest) {

I'm wondering, what's the rationale for passing the manifest object around? Why not just get it straight from the content when needed?

@@ +17,5 @@
> +    return result;
> +  }),
> +
> +  contentFetchIcon: Task.async(function* (aWindow, manifest) {
> +    let jenv = JNI.GetForThread();

NIT: please use `const` unless the variable will be reassigned to something else. This applies to the rest of the file.

@@ +41,5 @@
> +    .map(width => `${image.src} ${width}w`)
> +    .join(", ");
> +}
> +
> +function iconJsonToSrcset(icons) {

The reducer seems to be filtering and mapping,. Also, the manifest processor assures the src, so maybe just:

function iconJsonToSrcset(icons) {
  return icons
    .filter(icon => icon.sizes) 
    .map(toWidthDescriptors)
    .join(", ");
}

@@ +43,5 @@
> +}
> +
> +function iconJsonToSrcset(icons) {
> +  return icons.reduce((acc, icon) => {
> +    if (icon.src && icon.sizes) {

With icons that don't have a size, they are effectively equivalent to `<img src="">`, where the intrinsic size can be derived after they load. 

Initially, I think it's fine for us to discard icons that lack a size - but we might need to consider allowing them later.

@@ +64,5 @@
> +    picker.onload = function() {
> +      let canvas = createElement(doc, "canvas", size);
> +      canvas.getContext("2d").drawImage(picker, 0, 0, size, size);
> +      resolve(canvas.toDataURL("image/png"));
> +    }

Probably want to reject .onerror, or else this will block.

@@ +65,5 @@
> +      let canvas = createElement(doc, "canvas", size);
> +      canvas.getContext("2d").drawImage(picker, 0, 0, size, size);
> +      resolve(canvas.toDataURL("image/png"));
> +    }
> +    picker.setAttribute('srcset', iconJsonToSrcset(manifest.icons));

NIT (easier to read): picker.srcset = iconJsonToSrcset(manifest.icons);

::: mobile/android/base/java/org/mozilla/gecko/manifest/WebManifest.java
@@ +29,5 @@
> +    public String launcherName() {
> +        if (manifestFields.get("name").length() <= 12) {
> +            return manifestFields.get("name");
> +        }
> +        if (manifestFields.containsKey("short_name") && manifestFields.get("short_name").length() <= 12) {

I think we should be biased towards the short_name first (i.e., make this the first if statement). Also, maybe don't even bother with the size check and just leave Android to deal with it (like at the end of this function). The spec says that "short_name" is preferred for underneath icons, and "name" can be used to search indexes, or as a fallback for when short_name is missing.

::: mobile/android/chrome/content/browser.js
@@ +4506,5 @@
>    ])
>  };
>  
> +var Manifest = {
> +  install(manifestUrl) {

Manifest.install() here seems to imply an installation process of some sort, but this just seems to be fetching the manifest. Maybe this should be renamed? 

However, if this does represent the installation process, then after, we should fire an install event ("DOM:Manifest:FireAppInstalledEvent" in manifestMessages.js).

Also, NIT: it is possible to use Task.async here? it would make this code easier to read.

Comment 28

[Marcos Caceres [:marcosc]]

> In ManifestIcons, is using the DOM like that going to have any effect on the users web content?
> (thinking like an onerror gettting fired or similiar), also due to using canvas these icons are 
> going to subject to same origin restrictions, is that on purpose or should we work around that somehow?


I don't think so, as we are not actually adding that stuff to the DOM... but we should double check with Bz.

> And this should likely need tests, probably want to test it behaves that it works in a few edge cases,
> any chance someone could point me to an example of the type of test we would create for this 
> type of code?

You might want to look in: 
https://github.com/mozilla/gecko-dev/tree/master/dom/manifest/test

There is lots of reusable infrastructure in there. There are bunch of icon related tests in there too (test_ImageObjectProcessor_* tests).

We should get CSP and Service Worker behavior for free, I think... but we might need to throw in a couple of tests in to make sure it's all working as expected.

Comment 29

[Marcos Caceres [:marcosc]]

Comment on attachment 8807136 [details] [diff] [review]
Use icons from app manifest

Review of attachment 8807136 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/manifest/ManifestIcons.jsm
@@ +52,5 @@
> +}
> +
> +function createElement(doc, tag, size) {
> +  let el = doc.createElement(tag);
> +  el.width = size;

for "img", we might want to set 
`
el.crossOrigin = "anonymous"; 
`

@@ +62,5 @@
> +  return new Promise(resolve => {
> +    let picker = createElement(doc, "img", size);
> +    picker.onload = function() {
> +      let canvas = createElement(doc, "canvas", size);
> +      canvas.getContext("2d").drawImage(picker, 0, 0, size, size);

If the icons start coming out blurry, we might need to do something like: https://www.html5rocks.com/en/tutorials/canvas/hidpi/

Comment 30

[Marcos Caceres [:marcosc]]

Sorry, didn't answer this part:

> also due to using canvas these icons are 
> going to subject to same origin restrictions, is that on purpose or should we work around that somehow?

So, just reading the spec again... 
https://www.w3.org/TR/appmanifest/#fetching-image-objects

So, actually I was wrong about the .crossOrigin setting... it should be "no-cors". 

We might need to do some gymnastics to correctly set the "initiator".

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

I was just testing the patch in a local build and see the gecko thread crash when trying to add web.telegram.org to my home 
screen:

>      GeckoCrashHandler  E  >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 11385 ("GeckoBackgroundThread")
>                         E  android.database.CursorIndexOutOfBoundsException: Index 0 requested, with a size of 0
>                         E      at android.database.AbstractCursor.checkPosition(AbstractCursor.java:460)
>                         E      at android.database.AbstractWindowedCursor.checkPosition(AbstractWindowedCursor.java:136)
>                         E      at android.database.AbstractWindowedCursor.getString(AbstractWindowedCursor.java:50)
>                         E      at android.database.CursorWrapper.getString(CursorWrapper.java:137)
>                         E      at org.mozilla.gecko.db.LocalUrlAnnotations.getManifestUrl(LocalUrlAnnotations.java:77)
>                         E      at org.mozilla.gecko.GeckoApp.createShortcut(GeckoApp.java:1886)
>                         E      at org.mozilla.gecko.GeckoAppShell.createShortcut(GeckoAppShell.java:831)
>                         E      at org.mozilla.gecko.BrowserApp$22.run(BrowserApp.java:1411)
>                         E      at android.os.Handler.handleCallback(Handler.java:751)
>                         E      at android.os.Handler.dispatchMessage(Handler.java:95)
>                         E      at android.os.Looper.loop(Looper.java:154)
>                         E      at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)
>                         E  Main thread (1) stack:
>                         E      android.os.MessageQueue.nativePollOnce(Native Method)
>                         E      android.os.MessageQueue.next(MessageQueue.java:323)
>                         E      android.os.Looper.loop(Looper.java:136)
>                         E      android.app.ActivityThread.main(ActivityThread.java:6088)
>                         E      java.lang.reflect.Method.invoke(Native Method)
>                         E      com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
>                         E      com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8807136 [details] [diff] [review]
Use icons from app manifest

Review of attachment 8807136 [details] [diff] [review]:
-----------------------------------------------------------------

Quick review. Overall this looks good to me. Please have a look at the error posted above. Just re-flag me again and I'll have a more closer look.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +11,5 @@
>  import android.os.Environment;
>  import android.os.Process;
>  import android.support.annotation.CheckResult;
>  import android.support.annotation.NonNull;
> +import android.graphics.BitmapFactory;

nit: Unused import

@@ +1897,5 @@
> +                final Bitmap icon = FaviconDecoder
> +                    .decodeDataURI(getContext(), message.getString("icon"))
> +                    .getBestBitmap(GeckoAppShell.getPreferredIconSize());
> +                final String startUrl = manifest.get("start_url");
> +                GeckoAppShell.doCreateShortcut(manifest.launcherName(), startUrl, icon);

Is it guaranteed that start_url is always set? Do we validate this on JS side?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +125,5 @@
>  import java.util.Map;
>  import java.util.Set;
>  import java.util.concurrent.TimeUnit;
>  
> +import android.database.Cursor;

nit: Unused import

@@ +1891,5 @@
> +                final JSONObject manifestObj = new JSONObject();
> +                manifestObj.put("src", manifestUrl);
> +                manifestObj.put("title", title);
> +                GeckoAppShell.notifyObservers("Browser:LoadManifest", manifestObj.toString());
> +            } catch (JSONException e) { }

nit: Let's log an error or if we except this to never happen then let's re-throw as AssertionError.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalUrlAnnotations.java
@@ +72,5 @@
> +                new String[] { Key.MANIFEST_URL.getDbValue(), originUrl },
> +                null);
> +
> +          try {
> +              result.moveToFirst();

This returns a boolean: False means that the cursor could not be moved (Empty cursor). In addition to that the cursor could be null (Let's exit early in this case).

Comment 33

[Marcos Caceres [:marcosc]]

Before I forget, the spec has the following warning:

            It is RECOMMENDED that user agents prevent other applications from
            determining which applications are installed on the system (e.g.,
            via a timing attack on the user agent's cache). This could be done
            by, for example, invalidating from the user agent's cache the
            resources linked to from the manifest (for example, icons) after a
            web application is installed - or by using an entirely
            different cache from that used for regular web browsing.

Basically, icons can be used in such a timing attack. We don't need to figure that out now... but we should figure that out at some point, as it will be an easy attack vector.

Comment 34

[Marcos Caceres [:marcosc]]

Comment on attachment 8807136 [details] [diff] [review]
Use icons from app manifest

Setting to feedback+ for now, as I expect this will be changing over next few weeks based on feedback thus far.

Comment 35

[Dale Harvey (:daleharvey)]

So I have had all the changes to this for a while just having a little problem creating the tests. I want to test the ManifestIcons returns the correct icon however as we process it through canvas (and possibly resize) we cant directly compare base64, so my idea is to have different sized icons in the tests that are all seperate colours, I can then pick a pixel from the returned icon and check that the correct icon was picked. However having problems with the canvas code from inside a mochitest-browser context.

The code is mostly @ https://gist.github.com/daleharvey/8e2ef60cff94981ef457e4bd31414ba3 with an example of the error, if I replace browser.contentWindow.document with the current global document I get |null| returned from ctx.getContext, guessing this is because the chrome has a document but not a drawing context? 

Mike (or anyone else) is there any chance you could give me a hint on how to sample a canvas element from within mochitest-browser context?

Cheers
Dale

Comment 36

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dale Harvey (:daleharvey) from comment #35)
> 
> Mike (or anyone else) is there any chance you could give me a hint on how to
> sample a canvas element from within mochitest-browser context?
> 
> Cheers
> Dale

I'll try to supply something useful here.

At least with e10s, accessing browser.contentWindow or browser.contentDocument (which is an alias to browser.contentWindow.document) is ill-advised, as these things are running in a different process. We "shim" the Mochitest add-on so that this synchronous access kinda works (via CPOWs), but it's not perfect, and sometimes falls over in spectacular ways (since CPOWs held in the parent don't keep the associated item in the content process alive, and vice-versa).

So when you want to mess around inside content inside a tab, you want to use ContentTask. ContentTask will take some script, serialize it, and run it in the content process, and can send messages back up to the parent with results. Example:

yield BrowserTestUtils.withNewTab({
  url: "http://example.com",
  gBrowser,
}, function*(browser) {
  // Going to use a ContentTask to manipulate the contents of this
  // example.com page. The second argument is serialized and sent as
  // arguments to the function that's eventually run in the content process.
  let someArg = "Hello";

  yield ContentTask.spawn(browser, someArg, function*(someArgInContent) {
    // someArg is not accessible in the closure because the script isn't
    // executed in the closure - it's serialized and sent to the content
    // process.
    Assert.ok(!someArg, "Shouldn't exist");
    // But we serialized it and sent it down with ContentTask
    Assert.equal(someArgInContent, "Hello");
    // We can access the DOMWindow via the "content" global in a ContentTask.

    let someHeader = content.document.querySelector("h1");
    someHeader.textContent = someArgInContent;

    // I can also return a Promise from the ContentTask if I need to do something
    // asynchronously. The Promise that ContentTask.spawn returns in the parent will
    // not resolve until any Promises that are returned by this serialized function
    // resolve.
    return new Promise((resolve) => {
      content.setTimeout(resolve, 5000);
    });
  });

  Assert.ok(true, "5 seconds passed in the content process.");
});

I bring up ContentTask because I wonder if the "shimmed" context you're getting when you use CPOWs just doesn't work. Very likely. CPOWs are shake-y ground.

So I suggest you attempt to modify getIconColor to use ContentTask instead. Note that ContentTask should work transparently when e10s is enabled or disabled.

Does that help at all?

Comment 37

[Dale Harvey (:daleharvey)]

That helps a bunch Mike, works great, new PR incoming

Comment 38

[Dale Harvey (:daleharvey)]

Attachment: Use icons from app manifest

> Manifest.install() here seems to imply an installation process of some 
> sort, but this just seems to be fetching the manifest. Maybe 
> this should be renamed? 

I was using this a starting base where in follow ups it will fire an appinstalled event etc (and possibly get moved into /dom/manifest)

> I was just testing the patch in a local build and see the gecko 
> thread crash when trying to add web.telegram.org to my home 

So web.telegram.org has an invalid manifest, The cursor checks you mentioned prevents the crash and now installing web.telegram will now fail with an error in the console pointing to what is wrong. I have put the manifest usage behind a pref (browser.pwa.enabled) so we do not regress bookmarking telegram while we discuss what we want to do in these cases since that is more of a product call.

> Is it guaranteed that start_url is always set? Do we validate this on JS side?

Yup

Thanks for all the comments, updated to fix them, this patch doesnt include the binary files used for the tests to make it easier to review but will attach them separately (and get on learning hg)

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8815318 [details] [diff] [review]
Use icons from app manifest

The new Android team in Taipei is going to work on a bunch of "Progressive Web Apps" related bugs and features in the coming quarter and so I'll add them as reviewers here so that they get familiar with the new code already. :)

Comment 40

[Nevin Chen(Not active on Bugzilla)]

Hi Dale
I've applied your patch but I can't get the right icon from
https://react-hn.appspot.com/#/?_k=igu0ur
and 
https://www.washingtonpost.com/
Can you please help me verify them?

btw, when I want to apply your patch to the latest central,I got some conflict in BrowserApp.java. Maybe you'll want to check that,too.

Comment 41

[Dale Harvey (:daleharvey)]

Hey Nevin

So I have been testing with http://junk.arandomurl.com/testapp.html since it has no default icon

https://www.washingtonpost.com/ Has 2 manifests, neither of which specify icons 
https://www.washingtonpost.com/pb/resources/json/desktop-notifications-manifest.json
https://www.washingtonpost.com/pb/resources/json/manifest.json
And do for that case we error out as currently expected

Testing https://react-hn.appspot.com/ - I am seeing it use the manifest icon about half the time, the other half it has not seen the manifest and gives us the default icon, I do see https://gist.github.com/daleharvey/0e917c80f2596f7c11639315e4238cb0 happen sometimes which may be related (although I have seen it work despite that error) it looks like android does not like writes to the filesystem in the UI thread, will investigate further and maybe look at spinning the db write into a new thread

Comment 42

[Nevin Chen(Not active on Bugzilla)]

There's recently some changes in central(mostly EventDispatcher in Java). Please rebase and update the patch again. Thank you!

Comment 43

[Marcos Caceres [:marcosc]]

Dale, if it's ok. I'll review in Hawaii ... by the beach, with a drink that has a little umbrella in it. Be great to discuss in person :) 

/me goes to pack!

Comment 45

[Marcos Caceres [:marcosc]]

FYI, I'm away on vacation till next week - so won't be able to do my review till I'm back. Dale, hope that's ok!

Comment 46

[Dale Harvey (:daleharvey)]

Marcos, is there any chance you could point me to someone else to do the review? have a new patch ready and next week being christmas (then new year). I will be working pretty heavily on this stuff so happy if you wanna do a post review and I can follow up if there are any changes you want in there. Cheers

Comment 47

[Dale Harvey (:daleharvey)]

My new patch started crashing when rebased on master, the changes to messaging dont seem to like anything I send. Jim what are the limitations of sendMessage, I am trying to send a fairly simple object with 4 string values (one of which is long, a base64 encoded image)

          Messaging.sendRequest({
            type: "Website:ManifestLoadedForShortcut",
            icon: icon,
            name: name,
            start_url: startUrl
          });

I have tried various changes, shorten the name or not include the icon data but cant seem to get it to not crash consistently, https://gist.github.com/daleharvey/36f628ec3a4daad09b4049f96eba9f85 is the log. Cheers

Comment 48

[Dale Harvey (:daleharvey)]

Unping, the issue was that due to a bug in some situations an undefined object could be put into the json, it didnt show up in dump() but crashed the bundler code, fixed now, thanks

Comment 49

[Dale Harvey (:daleharvey)]

Attachment: Use icons from app manifest

Ok so new patch ready, some changes + fixes from the original. 

  1. #fragments are stripped from urls before storing / querying
  2. I removed WebManifest.java, it was mostly unnecessary for this patch and I was looking ahead for when we store the manifest, but this will need to be done inside gecko so definitely not needed
  3. ManifestIcons has switched from using an |img| to use plain fetch as to set the correct initiator
  4. I switched from Browser:ManifestLoadedForShortcut to Website:AppInstalled, can bikeshed on exact name but I wanted it to be clear that this was the installation process
  5. Wrapped DB access called in |postToBackgroundThread| so Android doesnt complain about db access on the UI thread

pref("browser.pwa.enabled", true) is needed to test this, otherwise it does nothing, I tested with react-hn.appspot.com, http://junk.arandomurl.com/testapp.html, pokedex.org which all work web.telegram.org will fail with a console message as it does not have a valid manifest, washingtonpost.com will also fail as its manifest does not specify icons, I would like to address what we do to fallback in a follow up (and as this is behind a pref there is no worry of shipping regressions to users).

I asked Mike for review as Marcos is on PTO, he said he will forward it on if needed

Cheers

Comment 50

[Dale Harvey (:daleharvey)]

Attachment: Use icons from app manifest (with binaries)

This patch includes the binary files in case you want to run the dom tests

Comment 51

[Marcos Caceres [:marcosc]]

(In reply to Dale Harvey (:daleharvey) from comment #46)
> Marcos, is there any chance you could point me to someone else to do the
> review? have a new patch ready and next week being christmas (then new
> year). I will be working pretty heavily on this stuff so happy if you wanna
> do a post review and I can follow up if there are any changes you want in
> there. 

Baku could be good candidate, as he has been following/reviewing some of the manifest work on the past.

Comment 52

[Dale Harvey (:daleharvey)]

Comment on attachment 8818451 [details] [diff] [review]
Use icons from app manifest

So while working on a follow up patch to this I realised the android code is significantly more complicated than it needs to be, I started under the assumption that we would be storing the manifest data in Java (not sure why I assumed that). However we definitely cant do that, one reason being that it would be nice to share the code if pwa is developed on desktop, but more importantly the state of the manifest is going to affect other API's inside gecko, permissions and storage being the obvious ones and they cant be calling out to java every time they need to know whether a url is in an installed state or not.

This means the android code can be far simpler, we dont need to persist anything, all we need is a flag set on the currently loaded url to say whether a manifest exists or not

Clearing review and will reup a simple patch later today

Comment 53

[Dale Harvey (:daleharvey)]

Attachment: Use icons from app manifest

So without planning to store the manifest in java this becomes a much more trivial patch on the android side, it also avoids the potential problem of having to gc all those manifest urls we were storing.

On the dom side I have started the initial Manifest.install process, I plan to follow up with Manifest being responsible for persisting the manifest data.

https://bugzilla.mozilla.org/show_bug.cgi?id=1234558#c49 still applies in terms of the pref and where this will work

Comment 54

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Dale Harvey (:daleharvey) from comment #52)
> So while working on a follow up patch to this I realised the android code is
> significantly more complicated than it needs to be, I started under the
> assumption that we would be storing the manifest data in Java (not sure why
> I assumed that). However we definitely cant do that, one reason being that
> it would be nice to share the code if pwa is developed on desktop, but more
> importantly the state of the manifest is going to affect other API's inside
> gecko, permissions and storage being the obvious ones and they cant be
> calling out to java every time they need to know whether a url is in an
> installed state or not.

Note that sooner or later the Java-side might need to know some things about the manifest too and even before Gecko is loaded and JavaScript can be executed (e.g. display modes and if we decide to show a splash screen).

Comment 55

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8819653 [details] [diff] [review]
Use icons from app manifest

Review of attachment 8819653 [details] [diff] [review]:
-----------------------------------------------------------------

Last review before the holidays.... :)

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +1951,5 @@
> +                final String name = message.getString("name");
> +                final String startUrl = message.getString("start_url");
> +                final Bitmap icon = FaviconDecoder
> +                    .decodeDataURI(getContext(), message.getString("icon"))
> +                    .getBestBitmap(GeckoAppShell.getPreferredIconSize());

We want to do the decoding off the UI thread too.

@@ +1952,5 @@
> +                final String startUrl = message.getString("start_url");
> +                final Bitmap icon = FaviconDecoder
> +                    .decodeDataURI(getContext(), message.getString("icon"))
> +                    .getBestBitmap(GeckoAppShell.getPreferredIconSize());
> +                ThreadUtils.postToBackgroundThread(new Runnable() {

The event code has change a bit recently (Did you rebase recently?). You can now register to listen for an event on the background thread and then you do not need posting to the background thread anymore.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +1889,5 @@
> +            // If a page has associated manifest, lets install it
> +            try {
> +                final JSONObject manifestObj = new JSONObject();
> +                manifestObj.put("iconSize", GeckoAppShell.getPreferredIconSize());
> +                GeckoAppShell.notifyObservers("Browser:LoadManifest",

As mentioned earlier this API has changed recently and you should rebase and switch to the new style, more here:
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-December/002186.html

::: mobile/android/base/java/org/mozilla/gecko/db/UrlAnnotations.java
@@ +22,5 @@
>      void deleteFeedSubscription(ContentResolver cr, FeedSubscription subscription);
>      void updateFeedSubscription(ContentResolver cr, FeedSubscription subscription);
>      boolean hasFeedSubscription(ContentResolver cr, String feedUrl);
>      void insertFeedSubscription(ContentResolver cr, FeedSubscription subscription);
> +

nit: There's only this new line change left in this file. :)

::: mobile/android/chrome/content/browser.js
@@ +16,5 @@
>  Cu.import("resource://gre/modules/AsyncPrefs.jsm");
>  Cu.import("resource://gre/modules/DelayedInit.jsm");
> +Cu.import("resource://gre/modules/PromiseMessage.jsm");
> +
> +const { Manifest } = Cu.import('resource://gre/modules/Manifest.jsm', {});

Can/Shouldn't this be lazily loaded like the modules below? - Oh wait, this isn't really a module, right? Can it still be loaded only when needed?

@@ +1910,5 @@
>        case "gather-telemetry":
>          GlobalEventDispatcher.sendRequest({ type: "Telemetry:Gather" });
>          break;
>  
> +      case "Browser:LoadManifest":

See lazilyLoadedObserverScripts somewhere around line 150, you could let move this code into a separate file and let it load/dispatch automatically when this event comes in.

@@ +4004,5 @@
>          } else if (list.indexOf("[search]") != -1 && aEvent.type == "DOMLinkAdded") {
>            this.sendOpenSearchMessage(target);
> +        } else if (list.indexOf("[manifest]") != -1 &&
> +                   aEvent.type == "DOMLinkAdded" &&
> +                   Services.prefs.getBoolPref("browser.pwa.enabled")) {

Do we want to hide this behind a pref? Even with the time we get from the release trains?

If we want to have it: The name is maybe not the best? PWA is a set of things and this is "only" about the manifest part?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/BaseGeckoInterface.java
@@ +119,5 @@
>          // By default, do nothing.
>      }
>  
>      @Override
> +    public void doCreateShortcut(String title, String URI, Bitmap icon) {

Now that we are making this a public part of the interface I agree with what you wrote earlier: Let's rename this (basically removing the 'do' part).

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
@@ +1792,5 @@
> +         *
> +         * @param title of URI to link to.
> +         * @param URI to link to.
> +         */
> +        public void doCreateShortcut(String title, String URI, Bitmap icon);

Wait, why do we even need to make this public and add it to the interface. Afaik this is only called from BrowserApp and it should be able to call the now public method in its parent (GeckoApp) without this.

Comment 57

[Marcos Caceres [:marcosc]]

Comment on attachment 8819653 [details] [diff] [review]
Use icons from app manifest

Review of attachment 8819653 [details] [diff] [review]:
-----------------------------------------------------------------

Great stuff, Dale! I didn't review the Java, but the JS is looking good. Just a couple of little nits, etc.

::: dom/ipc/manifestMessages.js
@@ +83,5 @@
>      sendAsyncMessage("DOM:Manifest:FireAppInstalledEvent", response);
> +  },
> +
> +  /**
> +   */

Nit: it's pretty self evident what this does, but documenting methods is always helpful... specially if the ManifestIcons is doing some magic.

@@ +94,5 @@
> +    } catch (err) {
> +      response.result = serializeError(err);
> +    }
> +    sendAsyncMessage("DOM:WebManifest:fetchIcon", response);
> +  })

nit: add "," after ")"

::: dom/manifest/Manifest.jsm
@@ +1,1 @@
> +"use strict";

This seems quite useful in general... maybe add a preamble comment as to why others might want to bring in this module instead of the others?

@@ +14,5 @@
> +  this.data = null;
> +}
> +
> +Manifest.prototype.install = Task.async(function* () {
> +  this.data = yield ManifestObtainer.browserObtainManifest(this.browser);

question: Might this be the right time fire the install event at the content process? ... also, "install" seems like a bit of a misnomer here, as it's obtaining the manifest, but not actually doing the "install process". That's ok, but maybe we should add a note here that more magic will happen... and the "install" event will be dispatched. 

Thinking out loud here: before the "yield" there, that is probably where we want to add the "beforeinstallprompt" dispatch.

::: dom/manifest/ManifestIcons.jsm
@@ +10,5 @@
> +Cu.import("resource://gre/modules/PromiseMessage.jsm");
> +
> +this.ManifestIcons = {
> +
> +  browserFetchIcon: Task.async(function* (aBrowser, manifest, iconSize) {

nit: don't we support `async/await` now? If so, we don't need Task.async, just:

{
  
  async browserFetchIcon(aBrowser, manifest, iconSize) {
     await stuff;
  }
}

@@ +38,5 @@
> +}
> +
> +// Create an array of icons sorted by their size
> +function toIconArray(icons) {
> +  let iconBySize = [];

nit: const

@@ +48,5 @@
> +  });
> +  return iconBySize.sort((a, b) => a.size - b.size);
> +}
> +
> +function getIcon(aWindow, icons, expectedSize) {

Maybe this should be defined as an Task.async() function from the start (as it's using promises)... if we need to expand on it later, we will likely need to convert it to async/await pattern, so might as will do it now.

@@ +50,5 @@
> +}
> +
> +function getIcon(aWindow, icons, expectedSize) {
> +  if (!icons.length) {
> +    return Promise.reject("Could not find valid icon");

reject(new Error("Could not find valid icon")) ... or you won't have a `.stack` ... which will be sad :)

@@ +59,5 @@
> +  let index = icons.findIndex(icon => icon.size >= expectedSize);
> +  if (index === undefined) {
> +    index = icons.length - 1;
> +  }
> +  let icon = icons.splice(index)[0];

nit: try to avoid splice(), as changing the original list is not great (makes it harder to debug if things are not immutable)... maybe filter instead?

@@ +69,5 @@
> +    return getIcon(aWindow, icons, expectedSize);
> +  });
> +}
> +
> +function fetchIcon(aWindow, src) {

As above, regarding async.task

::: dom/manifest/test/browser_ManifestIcons_browserFetchIcon.js
@@ +32,5 @@
> +    const image = new content.Image();
> +    image.onload = function() {
> +      ctx.drawImage(image, 0, 0);
> +      resolve(ctx.getImageData(1, 1, 1, 1).data);
> +    };

Mabye add .onerror and a reject(new Error("could not create image")) or something here... been bitten too many times by Promise errors being swallowed and vanishing into the ether.

@@ +38,5 @@
> +  });
> +}
> +
> +add_task(function*() {
> +  let tabOptions = {gBrowser, url: makeTestURL(manifest)};

nit: const (same elsewhere if the variable wont be reassigned... at least, that's my convention... feel free to ignore :))

::: mobile/android/chrome/content/browser.js
@@ +14,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/AddonManager.jsm");
>  Cu.import("resource://gre/modules/AsyncPrefs.jsm");
>  Cu.import("resource://gre/modules/DelayedInit.jsm");
> +Cu.import("resource://gre/modules/PromiseMessage.jsm");

Do you need import this (PromiseMessage) here? I don't see it used below?

@@ +3797,5 @@
>          return null;
>      }
>    },
>  
> +  makeManifestMessage: function(eventTarget) {

nit: eventTarget is defined but never used?

Comment 58

[Dale Harvey (:daleharvey)]

> Do we want to hide this behind a pref? Even with the time we get from the release trains?

So I put this behind a pref for a few reasons, it currently regresses the case where sites to not have valid manifest icons (it doesnt currently fallback to the current bookmark icons) and also in follow up patches I will be introducing UI changes based on whether origins are installed or not that we are definitely going to want to experiment with and go via a UX review before releasing to users, this flag basically blocks anything from being recognized as an installed application, maybe call it "manifest.install.enabled" ?

> question: Might this be the right time fire the install event at the content process?

Yup, I starting this off one feature at a time but will be following up with firing the appropriate events etc, I have added comments to indicate that.

Thanks for the reviews, making the rest of the changes mentioned and will get a new patch up for review shortly.

Comment 59

[Dale Harvey (:daleharvey)]

Attachment: 1234558-7.patch

Ok new patch that takes all the changes into account aside from one: 

Jim I tried to replace the instance of |GeckoAppShell.notifyObservers| to |EventDispatcher.getInstance().dispatch("Browser:LoadManifest", message)| however I could not receive the event in browser.js using |EventDispatcher.instance.registerListener(this, "Browser:LoadManifest")| or any of the other event listeners, was I doing something wrong?

Carrying the r+ from Marcos as I made the changes there, Sebastian will change from notifyObservers to dispatch once I figure that out but if you could take a look at the rest that would be great, thanks

Comment 60

[Dale Harvey (:daleharvey)]

Comment on attachment 8825077 [details] [diff] [review]
1234558-7.patch

Clearing review, have a new patch

Comment 61

[Dale Harvey (:daleharvey)]

Attachment: Use icons from app manifest

Ok switched to EventDispatcher (thanks Jim)

Comment 62

[Dale Harvey (:daleharvey)]

Landed to inbound @ https://hg.mozilla.org/integration/mozilla-inbound/rev/89f920cb1ce2c4083cd999af18ea05506965f8d4

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89f920cb1ce2

Comment 64

[Florian Quèze [:florian]]

Are the new Manifest.jsm and ManifestIcons.jsm files meant to be shipped on all platforms if they are only used on Android?

Comment 65

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Barbara, NicoleYee pinged me regarding this one. Should we consider relnot'ing this one for Aurora53?

Comment 66

[Dale Harvey (:daleharvey)]

> Are the new Manifest.jsm and ManifestIcons.jsm files meant to be shipped on all platforms if 
> they are only used on Android?

There are no concrete plans for implementing the manifest on desktop, but there are vague ones, being they are small files and are designed to be eventually used in desktop is it worth removing them?

> Hi Barbara, NicoleYee pinged me regarding this one. Should we 
> consider relnot'ing this one for Aurora53?

Just mentioning this is preffed off by default and has very little end user impact, Its not really ready for users yet

Comment 67

[Florian Quèze [:florian]]

(In reply to Dale Harvey (:daleharvey) from comment #66)
> > Are the new Manifest.jsm and ManifestIcons.jsm files meant to be shipped on all platforms if 
> > they are only used on Android?
> 
> There are no concrete plans for implementing the manifest on desktop, but
> there are vague ones, being they are small files and are designed to be
> eventually used in desktop is it worth removing them?

If there are no concrete plans, then yes it's worth removing. Otherwise we accumulate many unused small files over the years. See bug 1316187 to the work I'm doing to identify these files.

Comment 68

[Dale Harvey (:daleharvey)]

Florian, ok that makes complete sense, similiarly all the previously included Manifest files to not need to be packaged for desktop, happy to do any work to help the build do the right thing

Comment 69

[Barbara Bermes [:barbara] - NI please!]

(In reply to Ritu Kothari (:ritu) from comment #65)
> Hi Barbara, NicoleYee pinged me regarding this one. Should we consider
> relnot'ing this one for Aurora53?

Yes please (sorry for the late reply)

Comment 70

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Barbara Bermes [:barbara] - NI please! from comment #69)
> (In reply to Ritu Kothari (:ritu) from comment #65)
> > Hi Barbara, NicoleYee pinged me regarding this one. Should we consider
> > relnot'ing this one for Aurora53?
> 
> Yes please (sorry for the late reply)

Thanks Barbara. Hi Gerry, Nicole, could you guys work together on the wording for this relnote and then add it to Release 51 Fennec notes in nucleus? Thanks!

Comment 71

[Gerry Chang [:gchang]]

I've put a draft of this bug in https://docs.google.com/document/d/1CNQpjA5nzWW-Ho0qwW0o7_jcM6tuUY04ATf8uphYrSc/edit#heading=h.obj2q96hevbo in Fennec section.

Comment 72

[Nicole Yee (:nicoleyee)]

lgtm :)

Comment 73

[Chris Mills [:cmills]]

I've mentioned this on the browser compat information for Web App Manifest:
https://developer.mozilla.org/en-US/docs/Web/Manifest#Browser_compatibility

I've also added a note into the Fx53 release notes (we don't have a Fennec specific one):
https://developer.mozilla.org/en-US/Firefox/Releases/53#Other

Let me know if this is OK, or if there are other details to add. Thanks!

I'll also e-mail Marcos separately about browser compat for Manifest features generally.

Comment 74

[Oana Horvath [:ohorvath]]

Verified as fixed on Nightly 55.0a1.
Devices:
-HTC 10 (Android 6.0.1)
-Motorola Nexus 6 (Android 7.0)
-Huawei Honor 5X (Android 5.1.1)
-Prestigio Grace X5 (Android 4.4.2)

Comment 75

[David S]

Hi Folks.
I have Firefox 57 on Android 5.0.1 (Samsung S4 - yes it's old :-)
When I visit the mentioned test app (http://junk.arandomurl.com/testapp.html) and tap Page/Save to Homescreen I get what I think is the default icon -- a brown square with the letter J in it on the homescreen with the words "Test App" below.  The testapp icon listed in the manifest.json is not shown nor is the shortname used for the icon text.

I also tried this on my new phone -- Firefox 56, Android 7.0 (Samsung S8) with the same result.

With Chrome 62.0.3202.84 on Android 5.0.1 (Samsung S4) all works as expected -- the manifest icon shows on the homescreen with the shortname ("yay app") as the icon text.

I am not sure if this should be a comment on this bug or a new one.
If you prefer a new one, please advise and I will happily file it.

Thx.
David.

Comment 76

[David S]

Ok, did some more testing and looks like this is fixed in 58.  I grabbed 58.0b3 and all worked as expected.
Thx.

Comment 77

[tespio]

For some reason it seems this bug is recurring. Currently on 65.0.1/Android while browsing a site with a valid manifest file, if i add to Home screen, the web app ignores the background_color and icon (theme_color works) in manifest.json making the splash screen a fully white page, until the website content is rendered.
However, when i touch the android button (which shows up near address bar and add to home button) it opens up the website as an app respecting all the instructions in manifest.json, including background_color and icons to create the splash screen.
This used to work last year around November i think when i last tested on Firefox.