Last Comment Bug 745069 - DOMApplicationRegistry needs some changes to support AITC
: DOMApplicationRegistry needs some changes to support AITC
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Anant Narayanan [:anant]
:
Mentors:
Depends on: 745065
Blocks: 744257
  Show dependency treegraph
 
Reported: 2012-04-12 20:06 PDT by Anant Narayanan [:anant]
Modified: 2012-07-28 09:23 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Changes to DOMApplicationRegistry to support AITC - v1 (9.53 KB, patch)
2012-04-12 20:56 PDT, Anant Narayanan [:anant]
fabrice: review+
Details | Diff | Splinter Review
Changes to DOMApplicationRegistry to support AITC - v2 (10.33 KB, patch)
2012-04-14 01:54 PDT, Anant Narayanan [:anant]
fabrice: review-
Details | Diff | Splinter Review
Changes to DOMApplicationRegistry to support AITC - v3 (10.05 KB, patch)
2012-04-26 11:45 PDT, Anant Narayanan [:anant]
fabrice: review-
Details | Diff | Splinter Review
Changes to DOMApplicationRegistry to support AITC - v3.1 (10.25 KB, patch)
2012-04-26 12:07 PDT, Anant Narayanan [:anant]
fabrice: review+
Details | Diff | Splinter Review

Description Anant Narayanan [:anant] 2012-04-12 20:06:53 PDT
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] 2012-04-12 20:56:28 PDT
Created attachment 614672 [details] [diff] [review]
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é 2012-04-12 21:13:02 PDT
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] 2012-04-12 21:19:55 PDT
(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] 2012-04-13 11:22:46 PDT
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 2012-04-13 11:37:39 PDT
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 2012-04-13 16:45:32 PDT
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/autolanduser@mozilla.com-0335f16c1588
Comment 7 Anant Narayanan [:anant] 2012-04-13 17:01:49 PDT
There was 1 orange in the try build (in a test for places code). Resubmitting.
Comment 8 Mozilla RelEng Bot 2012-04-13 17:06:58 PDT
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 2012-04-13 21:31:32 PDT
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/autolanduser@mozilla.com-ae2643ab59fb
Comment 10 Anant Narayanan [:anant] 2012-04-13 23:28:57 PDT
Try build succeeded, Fabrice, can you check this in? This patch was originally written by "Ian Bicking <ianb@mozilla.com>", so make sure to hg commit -u with that user!
Comment 11 Anant Narayanan [:anant] 2012-04-14 01:54:07 PDT
Created attachment 615018 [details] [diff] [review]
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] 2012-04-16 15:50:13 PDT
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] 2012-04-17 10:00:43 PDT
(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é 2012-04-18 14:02:21 PDT
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] 2012-04-18 18:06:27 PDT
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] 2012-04-26 11:45:46 PDT
Created attachment 618743 [details] [diff] [review]
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é 2012-04-26 11:56:28 PDT
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] 2012-04-26 12:07:52 PDT
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?
Comment 19 [:fabrice] Fabrice Desré 2012-04-26 12:12:00 PDT
(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] 2012-04-26 12:16:11 PDT
(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] 2012-04-28 00:10:56 PDT
https://hg.mozilla.org/services/services-central/rev/8e59a61ca831
Comment 22 Mozilla RelEng Bot 2012-05-04 08:36:54 PDT
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] 2012-05-08 10:00:34 PDT
https://hg.mozilla.org/mozilla-central/rev/8e59a61ca831

Note You need to log in before you can comment on or make changes to this bug.