Closed
Bug 720415
Opened 11 years ago
Closed 11 years ago
update navigator.mozApps m-c implementation
Categories
(Web Apps :: General, defect)
Web Apps
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: fabrice, Assigned: fabrice)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 9 obsolete files)
5.56 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
35.90 KB,
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
> - 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•11 years ago
|
||
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•11 years ago
|
||
(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
Comment 4•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
attaching the file instead of a diff...
Attachment #591955 -
Attachment is obsolete: true
Comment 7•11 years ago
|
||
In a comment towards the bottom it refers to "receipt:" instead of "receipts:" Besides that minor nit it looks good to me.
Comment 8•11 years ago
|
||
Another error: NOT_INSTALLED, to be returned when calling app.uninstall() twice.
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #591967 -
Attachment is obsolete: true
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Summary: update navigator.mozApps IDL → update navigator.mozApps m-c implementation
Assignee | ||
Comment 13•11 years ago
|
||
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•11 years ago
|
||
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)
Comment 15•11 years ago
|
||
>+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•11 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 17•11 years ago
|
||
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•11 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•11 years ago
|
||
Addressing review comments.
Attachment #602962 -
Attachment is obsolete: true
Attachment #602962 -
Flags: review?(anygregor)
Attachment #603137 -
Flags: review?(anygregor)
Comment 20•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
Hopefully the right patch now...
Attachment #603285 -
Attachment is obsolete: true
Attachment #603285 -
Flags: review?(anygregor)
Attachment #603343 -
Flags: review?(anygregor)
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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 | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d91de454670d https://hg.mozilla.org/integration/mozilla-inbound/rev/233d97f1d0c4
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/233d97f1d0c4 https://hg.mozilla.org/mozilla-central/rev/d91de454670d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 28•11 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•11 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
You need to log in
before you can comment on or make changes to this bug.
Description
•