745069 - DOMApplicationRegistry needs some changes to support AITC

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Anant Narayanan [:anant]]

The "Apps in the Cloud" client (AITC) being written in bug 744257 needs some changes to Webapps.jsm to support functionality. It was suggested that those changes be reviewed in a separate bug as it belongs in a different module.

Comment 1

[Anant Narayanan [:anant]]

Attachment: Changes to DOMApplicationRegistry to support AITC - v1

Add getAllWithoutManifests() and makeAppId() methods to DOMApplicationRegistry
Remove installDone (undefined variable) in webappsUI.jsm
Some whitespace changes to Webapps.jsm

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 614672 [details] [diff] [review]
Changes to DOMApplicationRegistry to support AITC - v1

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

If you remove sync related support here, we also have to remove the sync engine itself.

::: dom/base/Webapps.jsm
@@ +344,3 @@
>    },
>  
>    updateApps: function(aRecords, aCallback) {

If you don't need this method for aitc, we can also remove it. It's only used by the sync engine.

Comment 3

[Anant Narayanan [:anant]]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> If you remove sync related support here, we also have to remove the sync
> engine itself.

Removal of sync engine is being tracked in bug 745065. I'll mark that bug as a blocker for this one.

> >    updateApps: function(aRecords, aCallback) {
> 
> If you don't need this method for aitc, we can also remove it. It's only
> used by the sync engine.

updateApps is also used by the new AITC client.

Comment 4

[Anant Narayanan [:anant]]

Comment on attachment 614672 [details] [diff] [review]
Changes to DOMApplicationRegistry to support AITC - v1

Asking for review per clarification above, and pushing to try.

Comment 5

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 614672
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=0335f16c1588
Try run started, revision 0335f16c1588. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=0335f16c1588

Comment 6

[Mozilla RelEng Bot]

Try run for 0335f16c1588 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0335f16c1588
Results (out of 19 total builds):
    success: 18
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0335f16c1588

Comment 7

[Anant Narayanan [:anant]]

There was 1 orange in the try build (in a test for places code). Resubmitting.

Comment 8

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 614672
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=ae2643ab59fb
Try run started, revision ae2643ab59fb. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=ae2643ab59fb

Comment 9

[Mozilla RelEng Bot]

Try run for ae2643ab59fb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ae2643ab59fb
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ae2643ab59fb

Comment 10

[Anant Narayanan [:anant]]

Try build succeeded, Fabrice, can you check this in? This patch was originally written by "Ian Bicking <[e-mail]>", so make sure to hg commit -u with that user!

Comment 11

[Anant Narayanan [:anant]]

Attachment: Changes to DOMApplicationRegistry to support AITC - v2

Sorry! While implementing uninstall in the client, I found out two more changes that would be nice to make to the registry.

When an app is either installed or uninstalled, the observer notification only sends the ID, but this is not useful for the AitC client at all. Especially for uninstall, we cannot retrieve the app object from the ID because it has already been deleted.

This patch sends a stringified version of the app record (without the manifest) along with observer notifications for both install and uninstall.

Comment 12

[Anant Narayanan [:anant]]

We had some more discussion with the identity folks today, and we may need to expand the feature set of DOMApplicationRegistry a bit more. Particularly, we need the ability to instantiate multiple DOMApplicationRegistry(s) - each with it's own data store (webapps.js).

Currently, all apps go into the same bucket, but we need to sometimes distinguish between apps that were installed with different Personas and make sure they are separated appropriately.

Comment 13

[Anant Narayanan [:anant]]

(In reply to Anant Narayanan [:anant] from comment #12)
> Currently, all apps go into the same bucket, but we need to sometimes
> distinguish between apps that were installed with different Personas and
> make sure they are separated appropriately.

Per discussion with Ian, there are several hard issues to be tackled here that we don't feel comfortable implementing this feature for the first version of AITC. I've created Bug 746204 to track that feature.

In the meantime, let's go along with the v2 patch in the bug right now.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 615018 [details] [diff] [review]
Changes to DOMApplicationRegistry to support AITC - v2

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

The changes to webapps-sync-uninstall and webapps-sync-install break the current sync engine at http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/apps.js#130

So either remove properly the sync engine or fix it. Even if it's currently prefed off, it's still possible to use it (I do).

Comment 15

[Anant Narayanan [:anant]]

Fabrice, app sync has already been removed on services-central via bug 745065. Can we check this into services-central? Since m-c is closed anyway, I think that is our best option.

Comment 16

[Anant Narayanan [:anant]]

Attachment: Changes to DOMApplicationRegistry to support AITC - v3

In bug 745065, we backed out the removal of the classic sync engine. The current short-term plan is to support both AITC and classic sync behind a pref. This patch makes the corresponding changes needed to DOMApplicationRegistry to support both of them.

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 618743 [details] [diff] [review]
Changes to DOMApplicationRegistry to support AITC - v3

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

::: services/sync/modules/engines/apps.js
@@ +131,5 @@
>        case "webapps-sync-uninstall":
>          // ask for immediate sync. not sure if we really need this or
>          // if a lower score increment would be enough
>          this.score += SCORE_INCREMENT_XLARGE;
> +        this.addChangedID(aData.id);

aData is a json stringified object - You need to parse it.

Comment 18

[Anant Narayanan [:anant]]

Attachment: Changes to DOMApplicationRegistry to support AITC - v3.1

Nice catch! We don't have tests yet, so unfortunately a try success doesn't mean much?

Comment 19

[[:fabrice] Fabrice Desré]

(In reply to Anant Narayanan [:anant] from comment #18)
> Created attachment 618750 [details] [diff] [review]
> Changes to DOMApplicationRegistry to support AITC - v3.1
> 
> Nice catch! We don't have tests yet, so unfortunately a try success doesn't
> mean much?

bug 741549 is ready to land!

Comment 20

[Anant Narayanan [:anant]]

(In reply to Fabrice Desré [:fabrice] from comment #19)
> bug 741549 is ready to land!

They don't test the sync functionality though. If we are going to stick with the classic sync engine, we should probably have unit + TPS tests. But, I think it's a good idea to hold off on that until we resolve the AITC vs. classic issue.

Comment 21

[Anant Narayanan [:anant]]

https://hg.mozilla.org/services/services-central/rev/8e59a61ca831

Comment 22

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 618750
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=20f24d6b9d96
Try run started, revision 20f24d6b9d96. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=20f24d6b9d96

Comment 23

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/8e59a61ca831