Closed
Bug 745069
Opened 13 years ago
Closed 13 years ago
DOMApplicationRegistry needs some changes to support AITC
Categories
(Core Graveyard :: DOM: Apps, defect)
Core Graveyard
DOM: Apps
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla15
People
(Reporter: anant, Assigned: anant)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
Assignee | ||
Comment 1•13 years ago
|
||
Add getAllWithoutManifests() and makeAppId() methods to DOMApplicationRegistry
Remove installDone (undefined variable) in webappsUI.jsm
Some whitespace changes to Webapps.jsm
Attachment #614672 -
Flags: review?(fabrice)
Comment 2•13 years ago
|
||
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.
Attachment #614672 -
Flags: review?(fabrice) → feedback+
Assignee | ||
Comment 3•13 years ago
|
||
(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.
Depends on: 745065
Assignee | ||
Comment 4•13 years ago
|
||
Comment on attachment 614672 [details] [diff] [review]
Changes to DOMApplicationRegistry to support AITC - v1
Asking for review per clarification above, and pushing to try.
Attachment #614672 -
Flags: review?(fabrice)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland-try]
Updated•13 years ago
|
Attachment #614672 -
Flags: review?(fabrice)
Attachment #614672 -
Flags: review+
Attachment #614672 -
Flags: feedback+
Updated•13 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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
Updated•13 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Comment 7•13 years ago
|
||
There was 1 orange in the try build (in a test for places code). Resubmitting.
Whiteboard: [autoland-try]
Updated•13 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Comment 8•13 years ago
|
||
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•13 years ago
|
||
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
Updated•13 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Comment 10•13 years ago
|
||
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!
Whiteboard: [autoland-in-queue]
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Comment 11•13 years ago
|
||
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.
Attachment #614672 -
Attachment is obsolete: true
Attachment #615018 -
Flags: review?(fabrice)
Assignee | ||
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 13•13 years ago
|
||
(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•13 years ago
|
||
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).
Attachment #615018 -
Flags: review?(fabrice) → review-
Assignee | ||
Comment 15•13 years ago
|
||
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.
Assignee | ||
Comment 16•13 years ago
|
||
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.
Attachment #615018 -
Attachment is obsolete: true
Attachment #618743 -
Flags: review?(fabrice)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland:-b do -p all -u reftest,reftest-ipc,reftest-no-accel,crashtest,crashtest-ipc,xpcshell,jsreftest,jetpack,mozmill-all,opengl,peptest,mochitests -t none]
Comment 17•13 years ago
|
||
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.
Attachment #618743 -
Flags: review?(fabrice) → review-
Assignee | ||
Comment 18•13 years ago
|
||
Nice catch! We don't have tests yet, so unfortunately a try success doesn't mean much?
Attachment #618743 -
Attachment is obsolete: true
Attachment #618750 -
Flags: review?(fabrice)
Comment 19•13 years ago
|
||
(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!
Updated•13 years ago
|
Attachment #618750 -
Flags: review?(fabrice) → review+
Assignee | ||
Comment 20•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Whiteboard: [autoland:-b do -p all -u reftest,reftest-ipc,reftest-no-accel,crashtest,crashtest-ipc,xpcshell,jsreftest,jetpack,mozmill-all,opengl,peptest,mochitests -t none] → [autoland-try:-b do -p all -u reftest,reftest-ipc,reftest-no-accel,crashtest,crashtest-ipc,xpcshell,jsreftest,jetpack,mozmill-all,opengl,peptest,mochitests -t none]
Updated•13 years ago
|
Whiteboard: [autoland-try:-b do -p all -u reftest,reftest-ipc,reftest-no-accel,crashtest,crashtest-ipc,xpcshell,jsreftest,jetpack,mozmill-all,opengl,peptest,mochitests -t none] → [autoland-in-queue]
Assignee | ||
Comment 21•13 years ago
|
||
Whiteboard: [autoland-in-queue] → [fixed in services]
Comment 22•13 years ago
|
||
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
Updated•13 years ago
|
Whiteboard: [fixed in services] → [fixed in services][qa-]
Comment 23•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla15
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM: Apps
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•