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)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 3 obsolete files)

This makes our implementation more robust to profile failures and would allow launching apps from different OS accounts.
Depends on: 907481
Attached patch WIP (obsolete) — Splinter Review
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 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-
(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?
(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?
(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 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-
(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.
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
Priority: -- → P2
Attached patch WIP (obsolete) — Splinter Review
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 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+
Attached patch Patch (obsolete) — Splinter Review
(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)
Attachment #8404166 - Flags: review?(fabrice) → review+
Attached patch PatchSplinter Review
Added a yield that was needed, carrying r+.
Attachment #8404166 - Attachment is obsolete: true
Attachment #8405273 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9994446a104a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: