Closed Bug 910911 Opened 7 years ago Closed 7 years ago

Add more application data to the webapp.json file in the installation directory

Categories

(Firefox Graveyard :: Web Apps, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch add_app_data_localinstallation (obsolete) — Splinter Review
We should add more data about the application to the webapp.json file, so that we can add applications running in the webapp runtime in the registry (bug 906223).

I think we should store only the necessary data, that is the data that we can't regenerate automatically from the manifests and from the package.

Fabrice, do we need anything else?
Attachment #797498 - Flags: review?(fabrice)
Blocks: 906223
Yes, you potentially need more than that. Basically anything that we use to clone app objects could be useful (see https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#65)
Attachment #797498 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #1)
> Yes, you potentially need more than that. Basically anything that we use to
> clone app objects could be useful (see
> https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#65)

If I remove from the list the data that can be retrieved from the manifests and from the package, the data that we can avoid storing (that is the things here: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#149 and the things that have a default value in cloneAppObject), and the data that I've added in the patch, the only things remaining are:

installTime
downloadAvailable
lastUpdateCheck
updateTime
staged

I think these shouldn't be stored as application data. Actually I think we shouldn't store also the etag and the packageEtag.
This file shouldn't be another copy of the registry that then we need to keep in sync with what we store in the registry. I think we should only store the minimal set of information needed to "reinstall" the application (that is the same information a developer would need to call the mozApps.install function).
Flags: needinfo?(fabrice)
Then you only need the manifest url, the manifest itself, installTime, receipts and categories. At this point we consider the application as installed, right? 

downloadAvailable, lastUpdateCheck, updateTime and staged are all used for updates. It's not clear to me if we expect updates to only happen in the firefox profile or also in the individual app profiles.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Then you only need the manifest url, the manifest itself, installTime,
> receipts and categories. At this point we consider the application as
> installed, right? 

And the installOrigin also, to make getInstalled work.

> downloadAvailable, lastUpdateCheck, updateTime and staged are all used for
> updates. It's not clear to me if we expect updates to only happen in the
> firefox profile or also in the individual app profiles.

I don't know if we will trigger updates from Firefox or from the webapp runtime, but the updated data will likely be stored both in the Firefox profile and in the webapp installation directory.
What you must make sure is that the DOM object has all the properties with sane values. Do we have tests for that part of the webrt yet?
(In reply to Fabrice Desré [:fabrice] from comment #5)
> What you must make sure is that the DOM object has all the properties with
> sane values. Do we have tests for that part of the webrt yet?

No, not yet. But at the moment the properties are taken from the registry.

When we'll support bug 906223 we'll need to create a test where we run an application that isn't in the registry and check if it's correctly added (it's probably going to be tricky).
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mcastelluccio
Attachment #797498 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #799675 - Flags: review?(myk)
Attachment #799675 - Flags: review?(fabrice)
Comment on attachment 799675 [details] [diff] [review]
Patch

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

::: toolkit/webapps/WebappsInstaller.jsm
@@ +175,2 @@
>      // was installed.
>      let registryFolder = Services.dirsvc.get("ProfD", Ci.nsIFile);

let webapp = aData.app;
and use webapp everywhere you used aData.app

@@ +190,5 @@
>  
> +    if (aData.app.etag) {
> +      this.webappJson.app.etag = aData.app.etag;
> +    }
> +

Don't you want also the packageEtag if available?
Attachment #799675 - Flags: review?(fabrice) → feedback+
Attached patch PatchSplinter Review
Attachment #799675 - Attachment is obsolete: true
Attachment #799675 - Flags: review?(myk)
Attachment #801677 - Flags: review?(myk)
Attachment #801677 - Flags: review?(fabrice)
Attachment #801677 - Flags: review?(fabrice) → review+
Attachment #801677 - Flags: review?(myk) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6fbeefd18043
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.