Bug 997779 (webmanifest)

[Meta] Implement Web Manifest spec in Gecko

NEW
Unassigned

Status

()

enhancement
P3
normal
5 years ago
9 days ago

People

(Reporter: overholt, Unassigned)

Tracking

(Depends on 4 bugs, Blocks 1 bug, {dev-doc-needed, meta})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [geckoview:p2], )

Attachments

(1 attachment)

Comment 1

5 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?
Keywords: dev-doc-needed
Sounds fine to me, but what exactly will we expose to gaia? A sanitized manifest? something else?
(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.
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.
Attachment #8432815 - Flags: review?(bfrancis)
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?
Attachment #8432815 - Flags: review?(bfrancis) → review?(fabrice)
(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"
}
Flags: needinfo?(fabrice)
Fabrice, now that you are back. Can you take a look at the general direction of the attached patch?
I'll keep hacking at it and see if I can find someone else to review it.
Flags: needinfo?(fabrice)
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?
Attachment #8432815 - Flags: review?(fabrice)
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.
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
Flags: needinfo?(mcaceres)
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.
Flags: needinfo?(mcaceres)
Restarting work on this.
Depends on: 1077620
Depends on: 1079453
Depends on: 1083410
Depends on: 1089255
Depends on: 1104916
Summary: Implement W3C Manifest spec → [Meta] Implement Web Manifest spec
Depends on: 1119670
Alias: webmanifest
Summary: [Meta] Implement Web Manifest spec → [Meta] Implement Web Manifest spec in Gecko
No longer depends on: 1089255
No longer blocks: 1143879
Depends on: 1143879
Depends on: 1143898
Depends on: 1086997
Depends on: 1153958
No longer depends on: 1153958
Depends on: 1162729
Depends on: 1162808
Depends on: 1164235

Comment 14

4 years ago
After done, don't forget to update https://developer.mozilla.org/en-US/Apps/Build/Manifest documentation.
(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.
Depends on: 1166405
Depends on: 1167335
Depends on: 1167806
Depends on: 1168877
Depends on: 1089255
Depends on: 1169784
Depends on: 1171200
Depends on: 1176442
Depends on: 1178175
Depends on: 1178655
Depends on: 1180652
No longer blocks: 1187027
Depends on: 1186908
Depends on: 1195018

Comment 16

4 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
Flags: needinfo?(sebastianzartner)
Flags: needinfo?(jypenator)

Comment 17

4 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
(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?

Comment 19

4 years ago
notechnicalvalue
```
                   ,%%%,
                 ,%%%` %==--
                ,%%`( '|
               ,%%@ /\_/
     ,%.-"""--%%% "@@__  
    %%/             |__`\       
   .%'\     |   \   /  //
   ,%' >   .'----\ |  [/
      < <<`       ||      
       `\\\       ||        
         )\\      )\
 ^^^^^^^^"""^^^^^^""^^^^^^^^
```
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
Flags: needinfo?(sebastianzartner)
Flags: needinfo?(mcaceres)
Flags: needinfo?(jypenator)

Comment 21

4 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
Flags: needinfo?(mcaceres)
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
Depends on: 1237021

Updated

3 years ago
Depends on: 1239358
Blocks: 1239358
No longer depends on: 1239358
Depends on: 1240490
Depends on: 1240735
See Also: → 1246312
See Also: 1246312
Depends on: 1250048
Depends on: 1258899
Depends on: 1262739
Depends on: 1264813
Depends on: 1264816
Depends on: 1264819
Depends on: 1265279
Depends on: 1266627
Depends on: 1275160
Depends on: 1309099
No longer blocks: 1285858

Updated

2 years ago
Blocks: 1419918

Updated

11 months ago
Priority: -- → P2
not working on this, so...
Assignee: mcaceres → nobody
Priority: P2 → P3
Depends on: 1534677
Depends on: 1534756
Component: DOM → DOM: Core & HTML
Product: Core → Core
Blocks: 1522451
Whiteboard: [geckoview:p2]
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.