Andrew Overholt [:overholt] Reporter
	Description • 10 years ago

http://w3c.github.io/manifest/

	(no longer active)
	Comment 1 • 10 years ago

Discussing this with Marcos in person, I suggested that we handle the mozbrowsermanifestchange event on the parent side, do the processing of the manifest, and then expose the results through the mozbrowser API.  This way we can test the processing code in Gecko, and then the browser app in Gaia can use the API to get the manifest information from the web pages.

Does this sound sane?

	[:fabrice] Fabrice Desré
	Comment 2 • 10 years ago

Sounds fine to me, but what exactly will we expose to gaia? A sanitized manifest? something else?

	Marcos Caceres [:marcosc]
	Comment 3 • 10 years ago

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Sounds fine to me, but what exactly will we expose to gaia? A sanitized
> manifest? something else?

I guess it will just be a sanitized manifest (just hand you an Object somehow). However, if you require something more fancy/useful, we can come up with something better.

	Marcos Caceres [:marcosc]
	Comment 4 • 10 years ago

This is by no means done, just putting this up for initial review - just to get a sense of direction and feedback. 

I'm a workshop all this week so I figured I'd start gathering feedback. Remember I'm new to Gecko and don't know the internal classes - so if there is better stuff I should be using (e.g., I know the security check stuff is wrong), then let me know what I should be using. 

The code starts at ManifestObtainer.js. The ManifestObtainer relies on ManifestProcessor to process the manifest. The ManifestProcessor makes use of the IconsProcessor.js.

	Ben Francis [:benfrancis]
	Comment 5 • 10 years ago

Comment on attachment 8432815 [details] [diff] [review]
manifest.patch

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

Hi Marcos, I'm not the right person to review this patch, maybe Fabrice would like to take a look.

One question, what will be the difference between the manifest data Gecko retrieves and the manifest data that gets passed on to Gaia?

	Marcos Caceres [:marcosc]
	Comment 6 • 10 years ago

(In reply to Ben Francis [:benfrancis] from comment #5) 
> Hi Marcos, I'm not the right person to review this patch, maybe Fabrice
> would like to take a look.

No probs. 

> One question, what will be the difference between the manifest data Gecko
> retrieves and the manifest data that gets passed on to Gaia?

The data Gaia gets is a cleaned up manifest that includes defaults. Like:

{
  "name": "   test   ",
  "foo": "bar"
} 

Would return something like this to Gaia:

{
   "name": "test", 
   "icons": [], 
   "display": "browser"
}

	Marcos Caceres [:marcosc]
	Comment 7 • 10 years ago

Fabrice, now that you are back. Can you take a look at the general direction of the attached patch?

	Marcos Caceres [:marcosc]
	Comment 8 • 10 years ago

I'll keep hacking at it and see if I can find someone else to review it.

	[:fabrice] Fabrice Desré
	Comment 9 • 10 years ago

Comment on attachment 8432815 [details] [diff] [review]
manifest.patch

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

Sorry for taking so long - I did a first pass.

The overall design looks fine, with the exception of the error reporting from process().
From a coding style pov, please switch to 2 spaces per indent. We also usually use double quotes in chrome code for strings. There's a bunch of minor whitespace issues too (eg check all the |if () {| blocks)

::: dom/manifest/IconsProcessor.jsm
@@ +4,5 @@
> +/* exported IconsProcessor */
> +'use strict';
> +this.EXPORTED_SYMBOLS = ['IconsProcessor'];
> +
> +var extractValue;

s/var/let

@@ +5,5 @@
> +'use strict';
> +this.EXPORTED_SYMBOLS = ['IconsProcessor'];
> +
> +var extractValue;
> +function IconsProcessor(valueExtractor) {

you need this.IconsProcessor = function(...) to export it properly.

@@ +101,5 @@
> +                sizes = this.processSizesMember(potentialIcon),
> +                icon = {
> +                    'src': src,
> +                    'type': type,
> +                    'sizes': sizes

nit: no need for quotes around the property names.
you can also just write:
icons.append({
  src: this.process...
  type: this.process...
  sizes: this.process...
});

::: dom/manifest/ManifestObtainer.jsm
@@ +21,5 @@
> +Cu.importGlobalProperties(['XMLHttpRequest', 'URL']);
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +XPCOMUtils.importRelative(this, 'ManifestProcessor.jsm');
> +
> +function ManifestObtainer() {

you need this.ManifestObtainer = function() to export it properly.

@@ +24,5 @@
> +
> +function ManifestObtainer() {
> +    const wellKnownURI = '/.well-known/manifest.json';
> +    const manifestQuery = 'link[rel~="manifest"]';
> +    const manifestProcessor = new ManifestProcessor();

why not create it in the only place you use it?

@@ +60,5 @@
> +
> +    function fetchManifest(manifestURL, contentWindow, useCreds) {
> +        return new Promise((resolve, reject) => {
> +            var xhr = new contentWindow.XMLHttpRequest();
> +            xhr.withCredentials = useCreds; 

You also likely want xhr.mozBackgroundRequest = true;

@@ +64,5 @@
> +            xhr.withCredentials = useCreds; 
> +            xhr.open('GET', manifestURL);
> +            xhr.onload = () => {
> +                let promiseResult = {
> +                    text: xhr.responseText,

Since you are only interested in json manifests, I would just get set the xhr.responseType to 'json' and s/text/manifest : xhr.response

@@ +68,5 @@
> +                    text: xhr.responseText,
> +                    manifestURL: manifestURL,
> +                    docLocation: new URL(contentWindow.location)
> +                };
> +                if (xhr.status.toString().startsWith('2')) {

we usually rather do >=200 && < 300

@@ +86,5 @@
> +    }
> +
> +    function processManifest(obj) {
> +        return new Promise((resolve) => {
> +            let manifest = manifestProcessor.process(obj.text, obj.manifestURL, obj.docLocation);

why not just pass obj to process() ?

@@ +87,5 @@
> +
> +    function processManifest(obj) {
> +        return new Promise((resolve) => {
> +            let manifest = manifestProcessor.process(obj.text, obj.manifestURL, obj.docLocation);
> +            resolve(manifest);

the error management around process() is unclear

::: dom/manifest/ManifestProcessor.jsm
@@ +55,5 @@
> +    processedManifest.start_url = this.processStartURLMember(manifest, manifestURL, docURL);
> +    processedManifest.display_mode = this.processDisplayMember(manifest);
> +    processedManifest.orientation = this.processOrientationMember(manifest);
> +    processedManifest.name = this.processNameMember(manifest);
> +    processedManifest.icons = this.processIconsMember(manifest, manifestURL);

in other places you do:
processedManifest = {
  start_url: ...
  ...
}

@@ +129,5 @@
> +    targetURI =  toNsIURI(docURL);
> +    referrerURI = toNsIURI(potentialResult);
> +    sameOrigin = sm.checkSameOriginURI(referrerURI, targetURI, false);
> +    if(!sameOrigin){
> +        issueDeveloperWarning();

is that just a warning, not an error?

	Marcos Caceres [:marcosc]
	Comment 10 • 10 years ago

Thanks Fabrice for the feedback. I'm about to go on vacation for a couple of weeks but will make the changes as soon as I'm back.

	Ben Francis [:benfrancis]
	Comment 11 • 10 years ago

Hi Marcos, just wondered about the progress on this as I'm starting to play with the Gaia side.

Should I just polyfill this for the time being and do the XHR content side, or is this likely to land some time soon?

Thanks

	Marcos Caceres [:marcosc]
	Comment 12 • 10 years ago

I'm happy for you to handle the integration into Gaia. I already did most of the work on the processing side, and I'm happy to collaborate on that.

	Marcos Caceres [:marcosc]
	Comment 13 • 10 years ago

Restarting work on this.

	Binyamin
	Comment 14 • 9 years ago

After done, don't forget to update https://developer.mozilla.org/en-US/Apps/Build/Manifest documentation.

	Marcos Caceres [:marcosc]
	Comment 15 • 9 years ago

(In reply to Binyamin from comment #14)
> After done, don't forget to update
> https://developer.mozilla.org/en-US/Apps/Build/Manifest documentation.

That's a separate (Mozilla proprietary) format, which is not compatible with this standardized format. 

We will need to create a new set of documents for this format.

	Florian Bender
	Comment 16 • 9 years ago

(In reply to Marcos Caceres [:marcosc] from comment #15)
> (In reply to Binyamin from comment #14)
> > After done, don't forget to update
> > https://developer.mozilla.org/en-US/Apps/Build/Manifest documentation.
> 
> That's a separate (Mozilla proprietary) format, which is not compatible with
> this standardized format. 
> 
> We will need to create a new set of documents for this format.

The current Docs also point to the (new) Manifest spec. This is now totally confusing. These MDN pages need to be revamped and warnings added that the feature is not to be confused with the W3C manifest spec. Though I don't know how. Best would probably be renaming the pages and links from/to this site to something like "Open Web Apps Manifest" and add a big fat warning box on this page and the subordinate pages. 

It's also unclear how big the difference between the (old) manifests and the W3C manifests are – another section explaining the different backgrounds and implementations would be nice even if (and especially when) the MDN pages for the W3C manifests are not written yet. Google claims[1] that Firefox implements a proprietary API. We should clear that up.

Can you guys please provide some input (or get to the action)?

[1] https://www.chromestatus.com/feature/6488656873259008

	marcos
	Comment 17 • 9 years ago

(In reply to Florian Bender from comment #16)
> (In reply to Marcos Caceres [:marcosc] from comment #15)
> > (In reply to Binyamin from comment #14)
> > > After done, don't forget to update
> > > https://developer.mozilla.org/en-US/Apps/Build/Manifest documentation.
> > 
> > That's a separate (Mozilla proprietary) format, which is not compatible with
> > this standardized format. 
> > 
> > We will need to create a new set of documents for this format.
> 
> The current Docs also point to the (new) Manifest spec. This is now totally
> confusing. These MDN pages need to be revamped and warnings added that the
> feature is not to be confused with the W3C manifest spec. Though I don't
> know how. Best would probably be renaming the pages and links from/to this
> site to something like "Open Web Apps Manifest" and add a big fat warning
> box on this page and the subordinate pages. 

We should start by stopping the "Open Web App" nonsense: it's deceptive and disingenuous at best - and just and outright lie at worst. It makes us look stupid and it's just embarrassing to anyone involved with actually standardizing true open web stuff through the W3C or WHATWG.

Let's please start calling the other format "Firefox OS's Manifest Format", as that is what it is. 

> It's also unclear how big the difference between the (old) manifests and the
> W3C manifests are

Apples and oranges: the only commonality is the use of JSON. 

>  – another section explaining the different backgrounds and
> implementations would be nice even if (and especially when) the MDN pages
> for the W3C manifests are not written yet. Google claims[1] that Firefox
> implements a proprietary API. We should clear that up.

Well, we don't currently support the w3c format in any shipping products. 

> Can you guys please provide some input (or get to the action)?
> 
> [1] https://www.chromestatus.com/feature/6488656873259008

	[:fabrice] Fabrice Desré
	Comment 18 • 9 years ago

(In reply to marcos from comment #17)

> We should start by stopping the "Open Web App" nonsense: it's deceptive and
> disingenuous at best - and just and outright lie at worst. It makes us look
> stupid and it's just embarrassing to anyone involved with actually
> standardizing true open web stuff through the W3C or WHATWG.

Please step down from your horse for a bit, while you update all the references to "Open Web App" to your liking. This is an historical naming, there's nothing more to read into it.

> Let's please start calling the other format "Firefox OS's Manifest Format",
> as that is what it is. 

Since you're unlikely to reuse "Open Web App manifest" for the new "Web Manifest" that seems totally useless. Can we stop wasting energy on that kind of things?

	marcos
	Comment 19 • 9 years ago
notechnicalvalue

```
                   ,%%%,
                 ,%%%` %==--
                ,%%`( '|
               ,%%@ /\_/
     ,%.-"""--%%% "@@__  
    %%/             |__`\       
   .%'\     |   \   /  //
   ,%' >   .'----\ |  [/
      < <<`       ||      
       `\\\       ||        
         )\\      )\
 ^^^^^^^^"""^^^^^^""^^^^^^^^
```

	Jean-Yves Perrier [:teoli]
	Comment 20 • 9 years ago

Ok, let's start by checking if I've understood things right :-)

1. Firefox OS apps have a proprietary manifest format, that *was* documented on the page [1].
2. The latest revision of this page with the proprietary manifest is [2].
3. W3C and browser vendors are developing and implementing a new manifest format. That's the purpose of this bug and the URL field has the link to it.
4. Over the months, since revision [2], the Firefox OS manifest document on MDN has been morphed to describe the W3C manifest.
5. The Firefox OS manifest will stay (or there is no concrete plan to get rid of it now).
6. Both manifests are incompatible (comment 15) and have different use cases.

Are these assertions correct? (If so, we will be able to move to a plan to clean things :-) )

[1] https://developer.mozilla.org/en-US/Apps/Build/Manifest 
[2] https://developer.mozilla.org/en-US/Apps/Build/Manifest$revision/800879

	marcos
	Comment 21 • 9 years ago

(In reply to Jean-Yves Perrier [:teoli] from comment #20)
> Ok, let's start by checking if I've understood things right :-)
> 
> 1. Firefox OS apps have a proprietary manifest format, that *was* documented
> on the page [1].

Correct.

> 2. The latest revision of this page with the proprietary manifest is [2].

Correct. 

> 3. W3C and browser vendors are developing and implementing a new manifest
> format. That's the purpose of this bug and the URL field has the link to it.

Correct.

> 4. Over the months, since revision [2], the Firefox OS manifest document on
> MDN has been morphed to describe the W3C manifest.

I gave it a quick scan, but can't see any overlap so hopefully incorrect.  

> 5. The Firefox OS manifest will stay (or there is no concrete plan to get
> rid of it now).

Correct.

> 6. Both manifests are incompatible (comment 15) and have different use cases.

Correct. 

> Are these assertions correct? (If so, we will be able to move to a plan to
> clean things :-) )

Seems good. Like I said above, it would be helpful if we made it more clear that this is a Mozilla proprietary format. 

> [1] https://developer.mozilla.org/en-US/Apps/Build/Manifest 
> [2] https://developer.mozilla.org/en-US/Apps/Build/Manifest$revision/800879

	Jean-Yves Perrier [:teoli]
	Comment 22 • 9 years ago

Ok, so it looks less grim that I originally thought. I've opening a doc bug to specifically address the cleaning of the page: bug 1208966

	Marcos Caceres [:marcosc]
	Comment 23 • 6 years ago

not working on this, so...