To minimize risk, we're going to update gecko and gaia atomically for v1. That's very easy wrt gecko updater; the gaia packages are just more blobs of bits to be moved and patched. What we don't have support for currently is loading apps from multiple locations (/data/$profile/webapps and, let's call it /system/b2g/systemapps). These seems like it should be pretty straightforward, we need an extra bit or two of metadata. The packages in /system/b2g/systemapps will be read-only as well, not uninstallable, so maybe we should expose this bit to the app manager as well. I think currently we have a hard-coded list.

This patch adds support for two apps sources: - /system/b2g/webapps for "core" unremovable apps (only on b2g) - the usual apps directory The DOM Application object has now a new boolean property ("removable") which is false for all core apps. New installed apps will be installed in the usual apps directory since they are forced to be removable.

The matching PR for gaia is at https://github.com/mozilla-b2g/gaia/pull/2957

Comment on attachment 647157 [details] [diff] [review] Patch Review of attachment 647157 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/DirectoryProvider.js @@ -26,2 @@ > if (localProps.indexOf(prop) != -1) { > - prop.persistent = true; Why are you removing prop.persistent = true; ? ::: dom/apps/src/Webapps.js @@ +231,4 @@ > * mozIDOMApplication object > */ > > +function createApplicationObject(aWindow, aApp) { Thanks! ::: dom/apps/src/Webapps.jsm @@ +88,5 @@ > + if (!this.webapps[id]) { > + this.webapps[id] = aData[id]; > + } > + } > + //this.webapps = aData; leftover ? ::: netwerk/protocol/app/AppProtocolHandler.js @@ +40,3 @@ > } > > + return this._basePath[aId]; basePath -> getBasePath and add a name to the anonymous function.

(In reply to Vivien Nicolas (:vingtetun) from comment #3) > Comment on attachment 647157 [details] [diff] [review] > Patch > > Review of attachment 647157 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/components/DirectoryProvider.js > @@ -26,2 @@ > > if (localProps.indexOf(prop) != -1) { > > - prop.persistent = true; > > Why are you removing prop.persistent = true; ? Because it doesn't do anything? prop is just a string, no? > ::: dom/apps/src/Webapps.jsm > @@ +88,5 @@ > > + if (!this.webapps[id]) { > > + this.webapps[id] = aData[id]; > > + } > > + } > > + //this.webapps = aData; > > leftover ? yep! > ::: netwerk/protocol/app/AppProtocolHandler.js > @@ +40,3 @@ > > } > > > > + return this._basePath[aId]; > > basePath -> getBasePath and add a name to the anonymous function. sure

(In reply to Fabrice Desré [:fabrice] from comment #4) > (In reply to Vivien Nicolas (:vingtetun) from comment #3) > > Comment on attachment 647157 [details] [diff] [review] > > Patch > > > > Review of attachment 647157 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: b2g/components/DirectoryProvider.js > > @@ -26,2 @@ > > > if (localProps.indexOf(prop) != -1) { > > > - prop.persistent = true; > > > > Why are you removing prop.persistent = true; ? > Sigh. I think this is a typo that should be |persistent.value = true;|

updated patch to address comments, and change the way we deal with local ids since they must be stable between restarts.

Comment on attachment 649404 [details] [diff] [review] patch v2 Review of attachment 649404 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/Webapps.jsm @@ +73,5 @@ > +#ifdef MOZ_WIDGET_GONK > + dirList.push("coreAppsDir"); > +#endif > + > + dirList.push(DIRECTORY_NAME); nit: let dirList = [DIRECTORY_NAME]; #ifdef MOZ_WIDGET_GONK dirList.push("coreappsDir"); #endif @@ +79,5 @@ > + let currentId = 1; > + for (let i in dirList) { > + let dir = dirList[i]; > + let curFile = FileUtils.getFile(dir, ["webapps", "webapps.json"], true); > + if (curFile.exists()) { dirList.forEach(function(dir) { let curFile = FileUtils.getFile(dir, ["webapps", "webapps.json"], true); if (!curFile.exists()) return; ... }); @@ +87,5 @@ > + for (let id in aData) { > + if (!this.webapps[id]) { > + this.webapps[id] = aData[id]; > + } > + } Is it possible for 2 installed applications to have the same ID? If not you don't need the if. @@ +88,5 @@ > + if (!this.webapps[id]) { > + this.webapps[id] = aData[id]; > + } > + } > + //this.webapps = aData; leftover ? @@ +89,5 @@ > + this.webapps[id] = aData[id]; > + } > + } > + //this.webapps = aData; > + for (let id in this.webapps) { do you need to have a second loop here, can't you do all the logic in the first loop over aData? @@ +91,5 @@ > + } > + //this.webapps = aData; > + for (let id in this.webapps) { > + this.webapps[id].basePath = appDir.path; > + this.webapps[id].removable = dir == DIRECTORY_NAME; nit: add () surrounding your expression, it makes it hard to read otherwise @@ +98,3 @@ > #endif > + // local ids must be stable between restarts. > + // We partition the ids in two buckets: nit: extra whitespace after buckets: ::: netwerk/protocol/app/AppProtocolHandler.js @@ +34,5 @@ > > + getBasePath: function app_phGetBasePath(aId) { > + > + if (!this._basePath[aId]) { > + this._basePath[aId] = cpmm.sendSyncMessage("Webapps:GetBasePath", nit: extra whitespace after the last ','

Addressing comments. Try push at https://tbpl.mozilla.org/?tree=Try&rev=731276a76ec5

No changes, just rebasing against current m-i

Better with the last qref.

fixing a typo

Removing a forgotten debug log, and rebasing because Bug 768868 had some changes in Webapps.js

Is there anything testable here from an end-user perspective? I'm seeing new exposed properties in the patch.

You know, when your Try push shows xpcshell failures on all platforms, maybe you should address those before pushing to inbound... Backed out after it (shockingly) turned the xpcshell tests orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/fd879b3ab5cd https://tbpl.mozilla.org/php/getParsedLog.php?id=14819087&tree=Mozilla-Inbound TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | running test ... TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | test failed (with xpcshell return code: 0), see following log: >>>>>>> TEST-INFO | (xpcshell/head.js) | test 1 pending TEST-INFO | (xpcshell/head.js) | test 2 pending TEST-INFO | (xpcshell/head.js) | test 2 finished TEST-INFO | (xpcshell/head.js) | running event loop TEST-INFO | (xpcshell/head.js) | test 2 pending TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | Starting test_storage_install Service.AITC.Storage INFO Server check got 2 apps Service.AITC.Storage INFO Applying 2 installs to registry TEST-INFO | (xpcshell/head.js) | test 2 finished Services.Common.RESTRequest DEBUG GET http://localhost:8080/manifest.webapp 200 Services.Common.RESTRequest DEBUG GET http://localhost:8081/manifest.webapp 200 Service.AITC.Storage WARN _getManifest got invalid manifest: {"not":"a manifest","fullscreen":true} Service.AITC.Storage WARN Could not fetch manifest for undefined Service.AITC.Storage DEBUG 1/2 apps processed Service.AITC.Storage DEBUG 2/2 apps processed TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 90] true == true TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 93] null == null Service.AITC.Storage INFO Server check got 2 apps Service.AITC.Storage INFO Applying 2 installs to registry Services.Common.RESTRequest DEBUG GET http://localhost:8080/manifest.webapp 200 Services.Common.RESTRequest DEBUG GET http://localhost:8082/manifest.webapp 200 Service.AITC.Storage DEBUG 1/2 apps processed Service.AITC.Storage DEBUG 2/2 apps processed TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 101] true == true TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [null : 102] true == true TEST-INFO | (xpcshell/head.js) | test 2 pending TEST-INFO | (xpcshell/head.js) | test 2 finished TEST-INFO | (xpcshell/head.js) | test 2 pending TEST-INFO | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | Starting test_storage_uninstall Ensure explicit uninstalls through hidden are honored. TEST-PASS | /Users/cltbld/talos-slave/test/build/xpcshell/tests/services/aitc/tests/unit/test_storage_registry.js | [test_storage_uninstall : 110] {aea0e1bc-015d-2247-a6dd-1619dfe1abb5} != null Service.AITC.Storage INFO Server check got 1 apps Service.AITC.Storage INFO Uninstalling app: {aea0e1bc-015d-2247-a6dd-1619dfe1abb5} Service.AITC.Storage INFO Uninstalling app: {e3b334d6-01c7-6044-a906-85ea74edfd6b} TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/head.js | TypeError: this.webapps[aId] is undefined - See following stack: JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 451 JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: _run_next_test :: line 891 JS frame :: /Users/cltbld/talos-slave/test/build/xpcshell/head.js :: <TOP_LEVEL> :: line 418 TEST-INFO | (xpcshell/head.js) | exiting test TEST-INFO | (xpcshell/head.js) | test 2 finished !!! error running onStopped callback: TypeError: callback is not a function !!! error running onStopped callback: TypeError: callback is not a function !!! error running onStopped callback: TypeError: callback is not a function !!! error running onStopped callback: TypeError: callback is not a function <<<<<<< This was also causing mochitest-other failures. https://tbpl.mozilla.org/php/getParsedLog.php?id=14819142&tree=Mozilla-Inbound 217 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 222 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 227 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 232 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 237 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 282 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 285 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1 291 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 294 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1 300 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 303 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1 313 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1 316 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | childPrincipal and parent principal should not have the same appStatus - didn't expect 0, but got it 319 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | this should be an installed app - got 0, expected 1 324 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | childPrincipal and parent principal should not have the same appStatus - didn't expect 0, but got it 331 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | child should be an installed app - got 0, expected 1 334 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/caps/tests/mochitest/test_principal_extendedorigin_appid_appstatus.html | childPrincipal and parent principal should not have the same appStatus - didn't expect 0, but got it

I fixed the test failures and the try run looks green enough (I retriggered as many failing jobs I could). https://tbpl.mozilla.org/?tree=Try&rev=17912a1b633e

Sorry for not chiming in earlier, I'm concerned that with how this currently stands there is no room for OEM differentiation in the core UI assuming they want to receive Mozilla's Gecko updates. At minimum we should consider adding support for a 2nd 'core app' location, say /system/vendor/b2g/webapps, for the non-Mozilla controlled non-removable apps. Maybe the OEM wants to bundle another app or two. Additionally I'm sure there will interest in a facility to optionally override the Mozilla controlled apps, say they want to do something a little different with the Homescreen/Lockscreen/Dialer/whatever. Although maybe this safer as all-or-nothing. As for updating the 2nd 'core app' location, the OEM could use FOTA although that's a pretty big hammer. Maybe we can generalize the Gecko update downloader such that all the OEM needs to do is host their core app update package somewhere. Most of this is probably out of scope for v1 but I see this being a rather serious limitation shortly thereafter.

Can you file a followup bug on that. I agree it's an important issue, but I'm not convinced that it's a basecamp blocker.

It's a v1 shortcut. For v2+ we should not jointly update gecko and gaia and add OEM paths for this.