Closed
Bug 906223
Opened 11 years ago
Closed 10 years ago
Apps not in the registry should be added to it when they're executed
Categories
(Firefox Graveyard :: Web Apps, defect, P2)
Firefox Graveyard
Web Apps
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: marco, Assigned: marco)
References
Details
Attachments
(1 file, 3 obsolete files)
12.70 KB,
patch
|
marco
:
review+
|
Details | Diff | Splinter Review |
This makes our implementation more robust to profile failures and would allow launching apps from different OS accounts.
Assignee | ||
Comment 1•11 years ago
|
||
We probably want to store other needed info in the local webapps.json file, like the receipts and the categories (that we can't read from the manifest).
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attachment #793650 -
Flags: feedback?(myk)
Attachment #793650 -
Flags: feedback?(fabrice)
Comment 2•11 years ago
|
||
Comment on attachment 793650 [details] [diff] [review] WIP Review of attachment 793650 [details] [diff] [review]: ----------------------------------------------------------------- If this code is only for the app runtime, it should not be in Webapps.jsm : cam you the bulk of it somewhere else? I also wonder how this behaves with app updates: do we have all the information needed like the etag and hash of the manifests and packages in webapps.json once you do this kind of installation?
Attachment #793650 -
Flags: feedback?(fabrice) → feedback-
Assignee | ||
Comment 3•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #2) > Comment on attachment 793650 [details] [diff] [review] > WIP > > Review of attachment 793650 [details] [diff] [review]: > ----------------------------------------------------------------- > > If this code is only for the app runtime, it should not be in Webapps.jsm : > cam you the bulk of it somewhere else? Yes, I could move most of the code somewhere else and do |added = WebappRT.addRunningApp()| instead of |added = this.addRunningApp()|. I thought this was a good place because I'm using Webapps.jsm internal variables and functions. > I also wonder how this behaves with app updates: do we have all the > information needed like the etag and hash of the manifests and packages in > webapps.json once you do this kind of installation? I haven't looked yet at all the data needed to make everything work. I think we should store only the minimum necessary, for example the manifestHash or the packageHash can be easily recalculated. Does feedback- mean you don't want to do this at all? Or just that you want me to move the code somewhere else?
Comment 4•11 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #3) > > Does feedback- mean you don't want to do this at all? Or just that you want > me to move the code somewhere else? It means that you have to move the code and figure out the update behavior at least. I guess we want that to have getSelf() work reliably in apps?
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #4) > (In reply to Marco Castelluccio [:marco] from comment #3) > > > > > Does feedback- mean you don't want to do this at all? Or just that you want > > me to move the code somewhere else? > > It means that you have to move the code and figure out the update behavior > at least. Oh, yes, sure, as I said in comment 1 I still hadn't looked into all the data that we should store. The most important things are those that we can't recover from the manifest/package. > I guess we want that to have getSelf() work reliably in apps? Three main reasons: 1) All the mozApps functions, like getSelf, would work automatically without other needed changes (without this patch we'd need to have a different code path for the webapp runtime in a lot of functions). 2) If the user loses the profile (sometimes we even ask users to clear their profile when they experience bugs) they'd still be able to use the app. 3) In a multiuser environment where apps are installed globally (on Mac), all the users would be able to execute the apps.
Comment 6•11 years ago
|
||
Comment on attachment 793650 [details] [diff] [review] WIP Review of attachment 793650 [details] [diff] [review]: ----------------------------------------------------------------- feedback+ on the general goal! Apps installations can become out of sync with the registry on desktop, and apps will probably be installed independently of the registry on Android in the near future, so it should be possible to add an app to the registry without also installing it. But feedback- on importing WebappRT.jsm into Webapps.jsm, since WebappRT.jsm already imports Webapps.jsm, so this creates a circular dependency. Ok, it may happen to work at the moment, presumably because WebappRT.jsm loads Webapps.jsm lazily. But that's a thin layer of ice to stand on. Rather than having a special function that only gets called for the desktop runtime, I think we should tease apart the code that installs an app from the code that registers (or updates) the record for an installed app in the registry. Then platforms that don't distinguish between these steps can always perform both steps together, while platforms that *do* distinguish between them can perform only the latter step when needed to sync the registry with app installs/updates that have happened outside the registry.
Attachment #793650 -
Flags: feedback?(myk) → feedback-
Comment 7•11 years ago
|
||
(In reply to Myk Melez [:myk] [@mykmelez] from comment #6) > But feedback- on importing WebappRT.jsm into Webapps.jsm, since WebappRT.jsm > already imports Webapps.jsm Or rather it *will* import Webapps.jsm once the fix for bug 847518 lands.
Assignee | ||
Comment 8•11 years ago
|
||
Ok, I think it's impossible to fix this bug (and the bug about moving the package) in a really clean way without making a refactoring before. We can start with storing the needed data in our webapp.json file, though, so that we don't end up with installations that have the needed data and installations that don't. I've filed bug 910911 for this.
Depends on: 910911
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Comment 9•10 years ago
|
||
I've added a |addApp| function to DOMApplicationRegistry that adds an application to the registry given an object with some app properties, a manifest and an update manifest.
Attachment #793650 -
Attachment is obsolete: true
Attachment #8403597 -
Flags: feedback?(fabrice)
Comment 10•10 years ago
|
||
Comment on attachment 8403597 [details] [diff] [review] WIP Review of attachment 8403597 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +2330,5 @@ > this._writeFile(manFile, JSON.stringify(aJsonManifest)); > }, > > + // Add an app to the registry. > + addApp: Task.async(function*(aApp, aManifest, aUpdateManifest) { I'm not a fan of this function's name... a bit too generic. @@ +2335,5 @@ > + if (this.getAppLocalIdByManifestURL(aApp.manifestURL) != > + Ci.nsIScriptSecurityManager.NO_APP_ID) { > + return; > + } > + I'm not sure how much we can trust the caller, bug adding a AppsUtils. checkManifest() here would be good. @@ +2346,5 @@ > + if (aManifest.type == "privileged") { > + app.appStatus = Ci.nsIPrincipal.APP_STATUS_PRIVILEGED; > + } else { > + app.appStatus = Ci.nsIPrincipal.APP_STATUS_INSTALLED; > + } move that to a helper like https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#405 and process all the cases. @@ +2393,5 @@ > + if (aUpdateManifest) { > + this._writeManifestFile(app.id, true, aUpdateManifest); > + } > + > + this._saveApps(); you also need to notify the children by broadcasting Webapps:AddApp : this.broadcastMessage("Webapps:AddApp", { id: app.id, app: app }); ::: webapprt/Startup.jsm @@ +19,5 @@ > Cu.import('resource://gre/modules/AlarmService.jsm'); > Cu.import("resource://gre/modules/Task.jsm"); > Cu.import("resource://gre/modules/Promise.jsm"); > Cu.import("resource://gre/modules/osfile.jsm"); > +Cu.import("resource://gre/modules/WebappOSUtils.jsm"); Why do you need that?
Attachment #8403597 -
Flags: feedback?(fabrice) → feedback+
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #10) > ::: dom/apps/src/Webapps.jsm > @@ +2330,5 @@ > > this._writeFile(manFile, JSON.stringify(aJsonManifest)); > > }, > > > > + // Add an app to the registry. > > + addApp: Task.async(function*(aApp, aManifest, aUpdateManifest) { > > I'm not a fan of this function's name... a bit too generic. I've changed it to "addInstalledApp", to highlight the function works if the app is already installed. > ::: webapprt/Startup.jsm > @@ +19,5 @@ > > Cu.import('resource://gre/modules/AlarmService.jsm'); > > Cu.import("resource://gre/modules/Task.jsm"); > > Cu.import("resource://gre/modules/Promise.jsm"); > > Cu.import("resource://gre/modules/osfile.jsm"); > > +Cu.import("resource://gre/modules/WebappOSUtils.jsm"); > > Why do you need that? It was a leftover.
Attachment #8403597 -
Attachment is obsolete: true
Attachment #8404166 -
Flags: review?(fabrice)
Updated•10 years ago
|
Attachment #8404166 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Added a yield that was needed, carrying r+.
Attachment #8404166 -
Attachment is obsolete: true
Attachment #8405273 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9994446a104a
Flags: in-testsuite+
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9994446a104a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•8 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•