720415 - update navigator.mozApps m-c implementation

Status: RESOLVED FIXED

(Web Apps :: General, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: wip

Changes were discussed during the apps works week, as seen here:
https://etherpad.mozilla.org/apps-api-rev and in the dev.platform thread.

There are still a few points to precise:
- all the on* handlers take a DOMEvent parameter, but the event doesn't carry any useful information. Is it ok to pass just null?
- I did not add the "app-state" property to the Application object, since I'm not sure what the states could be.
- I did not add a readyState property to the Application object for the same reason. If we add it, we probably also need an onreadystatechange event?
- I'm not sure we should add the manifest URL to the public interface of the Application object. What's the use case to expose this to web content?

Comment 1

[Ian Bicking (:ianbicking)]

> - all the on* handlers take a DOMEvent parameter, but the event doesn't carry any useful information. Is it ok to pass just null?

What times does it not carry information?  It seems like it should carry the application (when there's a specific application context); e.g., when calling install() the installTime would be present on the installed app and not the app as it is being installed.  In the case of errors, there should be some error object.

> - I did not add the "app-state" property to the Application object, since I'm not sure what the states could be.

I think there is an open design question about whether we want to make a general ready state, or specific states related to specific processes (e.g., appcache, native integration, etc).  I personally would prefer to specify specific processes and let UI do the generalization.

> - I did not add a readyState property to the Application object for the same reason. If we add it, we probably also need an onreadystatechange event?

readyState and app-state seem the same to me, so I guess it's the same question.

> - I'm not sure we should add the manifest URL to the public interface of the Application object. What's the use case to expose this to web content?

There is no canonical manifest URL for an application, so if this isn't exposed then there's no way for a store or an app to figure out what URL was used, or to handle that content moving.

Comment 2

[Ian Bicking (:ianbicking)]

Re: event objects - I see now that you could do:

  getter = mozApps.getSelf();
  getter.onsuccess = function () {
    var app = this.result;
    ...
  };

thus you don't need an argument.  (I don't know if the IDL can express anything about the value of "this" for .onsuccess?)

Re: "readonly attribute DOMString receipt;"

I think this should be an array of strings, named "receipts", so I guess "readonly attribute DOMString[] receipts;"

I assume there's no particularly good way to identify the actual type of "result" (hence jsval), but my expectation is that it is mozIDOMApplication for calls to install and getSelf, mozIDOMApplication[] for calls to getInstalled and getAll, and that it may also be null.  

I am not sure if oninstall/oninstall is called with an array of apps or a single app?  Or if they are called with an event object of some sort?

"* @param receipt : An opaque string used to track payment status" - I think this something is missing here, as you don't refer to receipt after this.  Maybe you meant to define what "parameters" is?

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: wip v2

(In reply to Ian Bicking (:ianb) from comment #2)
 
> thus you don't need an argument.  (I don't know if the IDL can express
> anything about the value of "this" for .onsuccess?)

IDL can't express that. But for sure we'll bind to the request object.


I think I addressed the other comments from Ian, and added an event type for the oninstall and onuninstall events.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Fabrice Desré [:fabrice] from comment #0)
> - I did not add a readyState property to the Application object for the same
> reason. If we add it, we probably also need an onreadystatechange event?

'processing' and 'done' for example. You need that for consistency with other FooRequest objects.

BTW, DOMError is going to land very soon so I would just use it in this v2.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: wip v3

v3, in which:

- readyState added to the request object
- error is now a DOMError
- mozIDOMApplicationMgmt and mozIDOMApplicationRequest are now nsIDOMEventTarget
- launch() take an optional starting point parameter for apps that have several launch points

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: wip v3

attaching the file instead of a diff...

Comment 7

[Ian Bicking (:ianbicking)]

In a comment towards the bottom it refers to "receipt:" instead of "receipts:"

Besides that minor nit it looks good to me.

Comment 8

[Ian Bicking (:ianbicking)]

Another error: NOT_INSTALLED, to be returned when calling app.uninstall() twice.

Comment 9

[Ian Bicking (:ianbicking)]

We should define if mozIDOMApplication.receipts is null or [] when no receipts were given.  Either would be fine with me.

I think there should be another error, MANIFEST_CONTENT_TYPE_INVALID, for cases when the manifest is not served with the required mimetype (application/x-web-app-manifest+json).  It seems unclear to mark this as NETWORK_ERROR or MANIFEST_INVALID.

If we do extensive validation of the manifest (e.g., check that the icon sizes are all integers) then I think MANIFEST_INVALID will not give developers enough feedback, we'd want to include some place for an unstructured error string to provide additional information.  That could be useful for NETWORK_ERROR as well, as a difference between, for example, a network connection error and a 404 is useful to know.

Comment 10

[Ian Bicking (:ianbicking)]

It would be useful if applications could get events related to their own installation status.  Specifically if a purchase flow can happen while an application is open, this would let the application receive a notification of the new status.

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: idl patch

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: wip : dom + repo

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: IDL

Jonas, this is the same IDL we agreed on earlier. Just formatted as a patch.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: DOM + repo

Works well in my tests, using 
http://people.mozilla.org/~fdesre/openwebapps/test.html and https://apps.mozillalabs.com/appdir/

Comment 15

[:Felipe Gomes (needinfo for replies!)]

>+XPCOMUtils.defineLazyGetter(Services, "rs", function() {
>+XPCOMUtils.defineLazyGetter(Services, "ppmm", function() {
>+XPCOMUtils.defineLazyGetter(Services, "cpmm", function() {

Are these meant to be used as Services.ppmm (etc) outside of this scope? If not let's just define them in the local scope instead of |Services| to avoid exposing it elsewhere, or if yes we should add it to Services.jsm

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Felipe Gomes (:felipe) from comment #15)
> >+XPCOMUtils.defineLazyGetter(Services, "rs", function() {
> >+XPCOMUtils.defineLazyGetter(Services, "ppmm", function() {
> >+XPCOMUtils.defineLazyGetter(Services, "cpmm", function() {
> 
> Are these meant to be used as Services.ppmm (etc) outside of this scope? If
> not let's just define them in the local scope instead of |Services| to avoid
> exposing it elsewhere, or if yes we should add it to Services.jsm

I think the DOMRequest service should be added to Services, but probably not the message managers.

Comment 17

[Gregor Wagner [:gwagner]]

Comment on attachment 602962 [details] [diff] [review]
DOM + repo

>+
>+  initHelper: function(aWindow, aMessages) {
>+  	this._messages = aMessages;
>+  	this._requests = [];
>+	  this._window = aWindow;
>+	  let util = this._window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>+    this.innerWindowID = util.currentInnerWindowID;
>+    this._id = this._getRandomId();
>+    Services.obs.addObserver(this, "inner-window-destroyed", false);
>+    this._messages.forEach((function(msgName) {
>+	    Services.cpmm.addMessageListener(msgName, this);
>+	  }).bind(this));
>+	},
>+
>+	createRequest: function() {
>+		return Services.rs.createRequest(this._window);
>+	}
>+}

Indentation in the whole file is somehow off.


>@@ -181,100 +97,82 @@ WebappsRegistry.prototype = {
> 
>   // mozIDOMApplicationRegistry implementation
>   
>-  install: function(aURL, aReceipt) {
>+  install: function(aURL, aParams) {
>+    let request = this.createRequest();
>+    let requestID = this.getRequestId(request);
>     let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
>     xhr.open("GET", aURL, true);
>-
>+   

whitespace



>@@ -334,26 +277,130 @@ WebappsApplication.prototype = {
>                                     classDescription: "Webapps Application"})
> }
> 
>-function RegistryError(aCode) {
>-  this._code = aCode;
>+/**
>+  * mozIDOMApplicationMgmt object
>+  */
>+function WebappsApplicationMgmt(aWindow) {
>+  dump("XxXxX WebappsApplicationMgmt " + aWindow + "\n");

remove

>+  let principal = aWindow.document.nodePrincipal;
>+  let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].getService(Ci.nsIScriptSecurityManager);
>+
>+  let perm = principal == secMan.getSystemPrincipal() ? 
>+               Ci.nsIPermissionManager.ALLOW_ACTION : 
>+               Services.perms.testExactPermission(principal.URI, "webapps-manage");
>+
>+  //only pages with perm set can use some functions
>+  this.hasPrivileges = perm == Ci.nsIPermissionManager.ALLOW_ACTION;
>+
>+  this.initHelper(aWindow, ["Webapps:GetAll:Return:OK", "Webapps:GetAll:Return:KO",
>+                            "Webapps:Install:Return:OK", "Webapps:Uninstall:Return:OK"]);
>+
>+  this.hasPrivileges = true;

huh? Always true?


>+
>+  receiveMessage: function(aMessage) {
>+    var msg = aMessage.json;
>+    let req = this.getRequest(msg.requestID);
>+    if (!((msg.oid == this._id && req) 
>+       || aMessage.name == "Webapps:Install:Return:OK" || aMessage.name == "Webapps:Uninstall:Return:OK"))
>+      return;

Can you explain and comment this if?

otherwise looks good!

Comment 18

[[:fabrice] Fabrice Desré]

> >+
> >+  this.hasPrivileges = true;
> 
> huh? Always true?

oops, debug leftover...
 
> 
> >+
> >+  receiveMessage: function(aMessage) {
> >+    var msg = aMessage.json;
> >+    let req = this.getRequest(msg.requestID);
> >+    if (!((msg.oid == this._id && req) 
> >+       || aMessage.name == "Webapps:Install:Return:OK" || aMessage.name == "Webapps:Uninstall:Return:OK"))
> >+      return;
> 
> Can you explain and comment this if?

usually we want each object to only receive responses to messages it sent. Except for the Install and Uninstall success messages that any mgmt app can listen to globally, eg. to update a dashboard.

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: DOM + repo

Addressing review comments.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

I think you posted the same patch :)

>+      throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

This will throw just a number. When throwing it's best to throw new Error("description string") or new Components.exception("desc", Cr.NS_ERROR...)

Comment 21

[Gregor Wagner [:gwagner]]

(In reply to Fabrice Desré [:fabrice] from comment #19)
> Created attachment 603137 [details] [diff] [review]
> DOM + repo
> 
> Addressing review comments.

That looks like the same file again.

Comment 22

[[:fabrice] Fabrice Desré]

Attachment: DOM + repo v2

After qref this time, including the change for exception throwing.

Comment 23

[[:fabrice] Fabrice Desré]

Attachment: DOM + repo v2

Hopefully the right patch now...

Comment 24

[Gregor Wagner [:gwagner]]

Comment on attachment 603343 [details] [diff] [review]
DOM + repo v2

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

Please fix the search/replace Services.cpmm indentation.
I will update contacts to use the helper :)

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 602961 [details] [diff] [review]
IDL

Stealing review request, r=jst. Jonas, feel free to comment if you have thoughts here too, even if that's after this lands.

Comment 26

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d91de454670d
https://hg.mozilla.org/integration/mozilla-inbound/rev/233d97f1d0c4

Comment 27

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/233d97f1d0c4
https://hg.mozilla.org/mozilla-central/rev/d91de454670d

Comment 28

[Ed Lee :Mardak]

This bug seems to need dev-doc-needed.

Currently, the mozApps.install says the optional install_data is some arbitrary JSON object with an optional "receipt" field.

From some quick tests with the implemented API, it seems like install_data must be an object with a "receipts" field that contains an array. This array is accessible from an App object as .receipts.

E.g.,

navigator.mozApps.install(manifest_url, {
  receipts: ["data", "goes", "here"]
});

navigator.mozApps.getSelf().onsuccess = function() {
  assert(this.receipts[2] == "here");
};

There's probably other API changes with this bug that have outdated the existing docs as well.

Comment 29

[Ed Lee :Mardak]

(In reply to Edward Lee :Mardak from comment #28)
> navigator.mozApps.getSelf().onsuccess = function() {
>   assert(this.receipts[2] == "here");
Sorry, this should be this.result.receipts

> There's probably other API changes with this bug that have outdated the
> existing docs as well.
I see there's details in the etherpad doc and some of it has already been documented on MDN, e.g., pending*, App object.
https://etherpad.mozilla.org/apps-api-rev

I noticed it says receipts should only allow strings, but currently it allows objects and numbers - bug 742802.