update navigator.mozApps m-c implementation

RESOLVED FIXED

Status

Web Apps
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

Details

Attachments

(2 attachments, 9 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 590779 [details]
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?
> - 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.
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?
(Assignee)

Comment 3

6 years ago
Created attachment 590866 [details]
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.
Assignee: nobody → fabrice
Attachment #590779 - Attachment is obsolete: true
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 591955 [details] [diff] [review]
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
Attachment #590866 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
Created attachment 591967 [details]
wip v3

attaching the file instead of a diff...
Attachment #591955 - Attachment is obsolete: true
In a comment towards the bottom it refers to "receipt:" instead of "receipts:"

Besides that minor nit it looks good to me.
Another error: NOT_INSTALLED, to be returned when calling app.uninstall() twice.
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.
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.
Blocks: 725397
Blocks: 731538
(Assignee)

Comment 11

6 years ago
Created attachment 602492 [details] [diff] [review]
idl patch
Attachment #591967 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Created attachment 602493 [details] [diff] [review]
wip : dom + repo
(Assignee)

Updated

6 years ago
Summary: update navigator.mozApps IDL → update navigator.mozApps m-c implementation
(Assignee)

Comment 13

6 years ago
Created attachment 602961 [details] [diff] [review]
IDL

Jonas, this is the same IDL we agreed on earlier. Just formatted as a patch.
Attachment #602492 - Attachment is obsolete: true
Attachment #602961 - Flags: review?(jonas)
(Assignee)

Comment 14

6 years ago
Created attachment 602962 [details] [diff] [review]
DOM + repo

Works well in my tests, using 
http://people.mozilla.org/~fdesre/openwebapps/test.html and https://apps.mozillalabs.com/appdir/
Attachment #602493 - Attachment is obsolete: true
Attachment #602962 - Flags: review?(anygregor)
(Assignee)

Updated

6 years ago
Blocks: 725533
(Assignee)

Updated

6 years ago
Blocks: 697006
(Assignee)

Updated

6 years ago
Blocks: 702363
>+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
(Assignee)

Comment 16

6 years ago
(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 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!
(Assignee)

Comment 18

6 years ago
> >+
> >+  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.
(Assignee)

Comment 19

6 years ago
Created attachment 603137 [details] [diff] [review]
DOM + repo

Addressing review comments.
Attachment #602962 - Attachment is obsolete: true
Attachment #602962 - Flags: review?(anygregor)
Attachment #603137 - Flags: review?(anygregor)
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...)
(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.
(Assignee)

Comment 22

6 years ago
Created attachment 603285 [details] [diff] [review]
DOM + repo v2

After qref this time, including the change for exception throwing.
Attachment #603137 - Attachment is obsolete: true
Attachment #603137 - Flags: review?(anygregor)
Attachment #603285 - Flags: review?(anygregor)
(Assignee)

Comment 23

6 years ago
Created attachment 603343 [details] [diff] [review]
DOM + repo v2

Hopefully the right patch now...
Attachment #603285 - Attachment is obsolete: true
Attachment #603285 - Flags: review?(anygregor)
Attachment #603343 - Flags: review?(anygregor)
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 :)
Attachment #603343 - Flags: review?(anygregor) → review+
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.
Attachment #602961 - Flags: review?(jonas) → review+
(Assignee)

Updated

6 years ago
Depends on: 734294

Comment 28

6 years ago
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.
Keywords: dev-doc-needed

Comment 29

6 years ago
(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.
Blocks: 742802

Updated

6 years ago
Depends on: 764957
You need to log in before you can comment on or make changes to this bug.