As a BU I want home screen to be configured with Core and all 3rd party (Common and Local) applications in case of power the device on with a SIM card for the first time Acceptance criteria: * Core and Common 3rd party apps are stored in the build the same way as before * Local 3rd party apps are stored in the build in a separate location that can be reused * Build will include a grid configuration per BU (Core, Common 3rd party and Local 3rd party apps) * During first time usage: * BU grid configuration will be selected based on MCC and MNC * Local 3rd party apps for the selected BU will be installed together with the other apps * Home screen grid will be set up according to BU configuration * Storage of Local 3rd party apps not belonging to selected BU will be removed to save space * No data network connectivity will be needed during the whole process * After first time usage, no other reconfiguration will be done * Later SIM change or removal imply no change in home screen grid * Local 3rd party apps can be updated later on the same way as other apps * Factory reset will keep Local 3rd party apps already installed

Fabrice, do you mind to take a look to the patch? what it does is to wait for the mcc-mnc and then it install a set of apps using the normal procedure as if the app was downloaded from the web

(In reply to Carmen Jimenez Cabezas from comment #1) > Created attachment 783918 [details] [diff] [review] > WIP. Mostly done, except the homescreen configuration > > Fabrice, do you mind to take a look to the patch? what it does is to wait > for the mcc-mnc and then it install a set of apps using the normal procedure > as if the app was downloaded from the web There's a lot of B2G-specific ifdef logic in that patch. Can we figure out ways to make this less platform dependent?

All the code of this patch is going to be executed only when it is run on a device that has a SIM card... and that's why I locked all of it behind a MOZ_WIDGET_GONK ifdef. I guess I could use ifdef only on the actual calls to get the MCC-MNC, but that would leave a lot of code that doesn't really do anything. I don't have any problem doing that if that's the way to go, though :)

Noemi Freire changed story state to started in Pivotal Tracker

Comment on attachment 783918 [details] [diff] [review] WIP. Mostly done, except the homescreen configuration Review of attachment 783918 [details] [diff] [review]: ----------------------------------------------------------------- I'd like this functionality to not be implemented with so deep changes to the core code. That's very b2g specific, and I want Webapps.jsm to have clear hooks for runtime specific functionality. One way to go forward here, I think could be: - trigger an observer notification around https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#508 - move your code to your own jsm that is loaded by shell.js and listen for the observer notification. - you should be able to install your apps without doing changes to the current confirmInstall() and download code. See what the remote debugging actor is doing for instance.

Thanks for the feedback, Fabrice. I can actually move out most of the code out of Webapps.jsm, although at first it looked better to put it there to keep all the app-installing code together. But changing confirmInstall looked like the cleaner (as in non duplicating code) way to do a 'real' installation of the apps. Real installation meaning that those apps must be updateable from the network and must behave in all ways like if they had actually been installed from the network. So I thought the easiest way was to actually install them using the same code :). As for the debugger-installing code I did take a look at it before doing this change, but unless I'm very much mistaken apps installed that way won't get network updates (nor will get the ids.json processed if they have one...). So I the end as I said it looked like either I should change to confirmInstall or I would have to copy and paste most if it. If you prefer the second option I'll leave webapps.jsm as is and just copy the functionality I need on a new module.

FWIW (little as it might be :) ) I agree with Carmen's initial approach on confirmInstall. If anything, I think that what should be done there is to not make those changes specific for the operators single variant, but rather do it generic to allow installing local apps as if they were network apps. That way it would be useable also by the webapps actor. I mean, instead of having the app installing code duplicated at the actor in [1], the actor could just call confirmInstall(aData) with aData being something like: let appData = { app: { installOrigin: aMetadata.installOrigin, origin: aMetadata.origin, manifestURL: aMetadata.manifestURL, operatorAppId: aId, updateManifest: aMinimanifest }, isPackage: true, appId: undefined, isBrowser: false }; [1] http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#182

Agh sorry about that. Editing code directly on the comment box isn't that good idea, somehow I managed to press submit. As I was saying, aData would be something like: let aData = { app: { installOrigin: someOrigin, origin: someOrigin, manifestURL: aManifestURL, localPath: aDir + "/application.zip!manifest.webapp", updateManifest: aMinimanifest }, isPackage: true, appId: undefined, isBrowser: false }; (quick dirty and won't work as is, but I hope the idea is clear). Then Carmen's code would drop operatorAppId and instead pass the localpath of the manifest (which is what the operatorAppId is used anyway). And thus the change in confirmInstall will not be specific to B2G anymore, but instead it will allow installing apps from a local source and not just from the network. Note that as localPath we could pass directly a uri of file:// type also. TL; DR. My proposal is: * Tune Carmen's change of confirmInstall a little bit so it doesn't have any reference to the operator SV but keep essentially the same added functionality (install apps from a file uri). * Change the webapps.js actor to use the modified confirmInstall instead of having code duplicated there (on a new bug) * And move the b2g SV specific code out of Webapps.jsm to another file as Fabrice said. WDYT?

(In reply to Antonio Manuel Amaya Calvo from comment #8) > > * Tune Carmen's change of confirmInstall a little bit so it doesn't have any > reference to the operator SV but keep essentially the same added > functionality (install apps from a file uri). > * Change the webapps.js actor to use the modified confirmInstall instead of > having code duplicated there (on a new bug) > * And move the b2g SV specific code out of Webapps.jsm to another file as > Fabrice said. That should work, yes.

This is totally independent from the Gecko patch. What it does is: * Reads a configuration file (I'll add an example as another file) to get the apps that are 'special'. Special apps are operator apps that have to go to a predetermined place. * When an app is being installed, if it's an special app and it's the first time it's being installed, it adds it to the predetermined place (or as close as possible). Otherwise, it adds it to the end. The "first time it's being installed" condition is added so if an user removes an operator app and then installs it again it goes to the end and not to the operator-defined place.

Carmen, I would like to talk with you before reviewing it, please contact me when you want. Thanks a lot

PR for the Gaia patch is at https://github.com/mozilla-b2g/gaia/pull/11603

Comment on attachment 791246 [details] [diff] [review] Gaia patch. Add the ability to Homescreen to insert an icon on a predetermined position Excellent work!!!! Thanks a lot for your great help

Comment on attachment 791246 [details] [diff] [review] Gaia patch. Add the ability to Homescreen to insert an icon on a predetermined position Really the r+ is for https://github.com/mozilla-b2g/gaia/pull/11603 instead of this patch

Merged into master https://github.com/mozilla-b2g/gaia/commit/418524d95143c94ea7425bc79db80329608dfda9

(In reply to Cristian Rodriguez (:crdlc) from comment #16) > Merged into master > > https://github.com/mozilla-b2g/gaia/commit/ > 418524d95143c94ea7425bc79db80329608dfda9 Don't know why this was merged without tests. For the system front end team, there needs to be tests included on every feature landing. At a minimum, this means unit tests at least.

Sorry about that. As I understood it, the story cannot be closed until the tests are done, but didn't think we couldn't land anything till then. Tests are coming anyway

backed out for making the homescreen to not load: E/GeckoConsole( 1215): [JavaScript Error: "NS_ERROR_FILE_NOT_FOUND: File error: Not found" {file: "app://homescreen.gaiamobile.org/js/configurator.js" line: 48}] (09:51:59 AM) fabrice: E/GeckoConsole( 1215): Content JS ERROR at app://homescreen.gaiamobile.org/js/configurator.js:40 in onErrorInitJSON: Failed parsing homescreen configuration file: TypeError: Configurator is undefined https://github.com/mozilla-b2g/gaia/commit/0035590eefc1ac6cfc05cfa868ed646f9f545e0c

The homescreen problem seems to happen if you don't have a sim card or if you have a simcard without the pin code security.

Thanks, it seems we hit a race condition sometimes on those cases. Will fix that and add the unit tests also.

Comment on attachment 795483 [details] [diff] [review] WIP v2, as described in comment 9 Review of attachment 795483 [details] [diff] [review]: ----------------------------------------------------------------- That's much better now, thanks for doing that. There are still a few things to improve, but that's great progress. Also, could you generate your patches with 8 lines of context? thanks! We'll need tests for that also. ::: dom/apps/src/OperatorApps.jsm @@ +6,5 @@ > + > +const Cu = Components.utils; > +const Cc = Components.classes; > +const Ci = Components.interfaces; > +const Cr = Components.results; Cr is unused, please remove it. @@ +24,5 @@ > + "@mozilla.org/ril/content-helper;1", > + "nsIMobileConnectionProvider"); > + > +function debug(aMsg) { > + dump("-*-*- OperatorApps.jsm : " + aMsg + "\n"); Please comment. @@ +59,5 @@ > + aIdsApp = [ ]; > + } > + > + try { > + svDir = FileUtils.getDir(DIRECTORY_NAME, [SINGLE_VARIANT_SOURCE_DIR], false); I'd like to not use that in new code (see bug 898314) @@ +88,5 @@ > + notifyStkCommand: function() {}, > + notifyStkSessionEnd: function() {}, > + notifyIccCardLockError: function() {}, > + notifyCardStateChange: function() {}, > + notifyIccInfoChanged: function() { Can you put a blank line between these functions? My eyes are bleeding ;) @@ +154,5 @@ > + debug("(SV) metadata:" + JSON.stringify(aMetadata)); > + if (!aMetadata) { > + return; > + } > + DOMApplicationRegistry._loadJSONAsync(aMinimanifestFile, (function (aMinimanifest) { s/aMinimanifest/aUpdateManifest ::: dom/apps/src/Webapps.jsm @@ +2213,5 @@ > > + let fullPackagePath = aManifest.fullPackagePath(); > + > + // Check if it's a local file install (we've downloaded/sideloaded the package > + // already or it did exist on the build) nit: trailing whitespace on both lines. Also add a full stop at the end of the sentence. @@ +2321,1 @@ > if (app.packageEtag) { we should guard that with && !isLocalFileInstall just in case... @@ +2499,4 @@ > let signedAppOriginsStr = > Services.prefs.getCharPref( > "dom.mozApps.signed_apps_installable_from"); > + // If it's a local install and it's signed then we asume Nit: assume @@ +2501,5 @@ > "dom.mozApps.signed_apps_installable_from"); > + // If it's a local install and it's signed then we asume > + // the app origin is a valid signer. > + let isSignedAppOrigin = (isSigned && isLocalFileInstall) || > + signedAppOriginsStr.split(",").indexOf(aApp.installOrigin) > -1; nit: reformat to fit in 80 chars. @@ +2555,5 @@ > } > > + // Local file installs can be privileged even without the signature. > + let maxStatus = isSigned || isLocalFileInstall ? > + Ci.nsIPrincipal.APP_STATUS_PRIVILEGED nit: align ? and : ::: dom/apps/src/moz.build @@ +22,4 @@ > > EXTRA_PP_JS_MODULES += [ > 'AppsUtils.jsm', > + 'OperatorApps.jsm', This file is not preprocessed, so move it to the EXTRA_JS_MODULE section. ::: toolkit/devtools/server/actors/webapps.js @@ +193,5 @@ > + app: { > + installOrigin: someOrigin, > + origin: someOrigin, > + manifestURL: aManifestURL, > + localPath: aDir + "/application.zip!manifest.webapp", In Webapps.jsm you expect localInstallPath, which is just a path to the zip.

This patch fixes the problem with PINless cards, and adds the requested unit tests.

Comment on attachment 797960 [details] [diff] [review] Gaia patch V2. Fixes the problem with PINless cards and adds unit tests Good work! Although please add some test to check when the response fails in the configurator.js

Landed on master, https://github.com/mozilla-b2g/gaia/commit/b6c5bf1d24230bfed4a8f680e625fa2175001f82

I'll open another bug to do the changes on the sideloading webapps.js when this one lands

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4caa27346fa7

Comment on attachment 802166 [details] [diff] [review] P1: Gecko patch, part 1. Single Variant Apps installation. Review of attachment 802166 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/OperatorApps.jsm @@ +15,5 @@ > +Cu.import("resource://gre/modules/Webapps.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/osfile.jsm"); > + > +#ifdef MOZ_WIDGET_GONK That will fail on tablets. I guess you want MOZ_RIL @@ +33,5 @@ > +const DIRECTORY_NAME = "webappsDir"; > + > +// Single variant utility functions and variables. > + > +// DIRECTORY_NAME/SINGLE_VARIANT_SOURCE_DIR Can you improve this comment? It doesn't tell much now. @@ +40,5 @@ > +const PREF_FIRST_RUN_WITH_SIM = "dom.webapps.firstRunWithSIM"; > +const METADATA = "metadata.json"; > +const UPDATEMANIFEST = "update.webapp"; > +const MANIFEST = "manifest.webapp"; > +const APPLICATION_ZIP = "application.zip"; Please align the '=' of all these constants. @@ +52,5 @@ > + > + return true; > +} > + > +#ifdef MOZ_WIDGET_GONK Same here, propably MOZ_RIL @@ +78,5 @@ > +this.OperatorAppsRegistry = { > + > + _fileBasePath: null, > + > + _loadJSONAsync: function (aFile, aCallback) { Nit: no space between function and (...) - here and elsewhere. @@ +92,5 @@ > + let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"] > + .createInstance(Ci.nsIScriptableUnicodeConverter); > + converter.charset = "UTF-8"; > + data = JSON.parse(converter.convertFromByteArray(rawData, > + rawData.length)); Use a TextDecoder instead. Also, bonus points if you turn that into a promise instead of using a callback. @@ +119,5 @@ > + }, > + > + init: function() { > + debug("(SV) init"); > +#ifdef MOZ_WIDGET_GONK MOZ_RIL @@ +144,5 @@ > + > + get appsDir() { > + if (!this._fileBasePath) { > + this._fileBasePath = this._getIFile([DIRECTORY_NAME, > + SINGLE_VARIANT_SOURCE_DIR]); You should use FileUtils.getFile() directly. @@ +147,5 @@ > + this._fileBasePath = this._getIFile([DIRECTORY_NAME, > + SINGLE_VARIANT_SOURCE_DIR]); > + } > + debug("appsDir GET: " + this._fileBasePath); > + return this._fileBasePath; We usually do something like: let res = this._getIFile(...); return this.appsDir = res; @@ +150,5 @@ > + debug("appsDir GET: " + this._fileBasePath); > + return this._fileBasePath; > + }, > + > + set appsDir(aDir) { You're not using this setter, so remove it. @@ +159,5 @@ > + this._fileBasePath = null; > + } > + }, > + > + _getIFile: function (aPath) { Remove this function. @@ +240,5 @@ > + debug("(SV) aId:" + aId + ". Installing as hosted app."); > + appData.app.manifest = aManifest; > + } > + if (appData.app.manifest || appData.app.updateManifest) { > + DOMApplicationRegistry.confirmInstall(appData); I'd like to get rid of the direct access to DOMApplicationRegistry but this one may be tricky... The computeHash() one can be done with AppsUtils.jsm instead. @@ +336,5 @@ > + try { > + file = this.appsDir.clone(); > + file.append(SINGLE_VARIANT_CONF_FILE); > + > + if (file && file.exists()) { Can you use os.file for all these file manipulations? @@ +347,5 @@ > + })); > + } > + } catch(e) { > + file = this.appsDir.clone(); > + file.append(SINGLE_VARIANT_CONF_FILE); You don't do anything with 'file', so I guess we don't really need that? If you do, comment why. ::: dom/apps/src/Webapps.jsm @@ +2216,5 @@ > + let fullPackagePath = aManifest.fullPackagePath(); > + > + // Check if it's a local file install (we've downloaded/sideloaded the > + // package already or it did exist on the build). > + let isLocalFileInstall = fullPackagePath.startsWith("file://"); It's a bit unfortunate to do this check like that and with a URI that we verify the path.

>> That will fail on tablets. I guess you want MOZ_RIL When I use MOZ_RIL that code isn't included on a B2G build. I used MOZ_WIDGET_GONK because it's what's used on Webapps.jsm and AppsUtils.jsm. This must be defined mostly on the same cases where MOZ_WIDGET_GONK is used on those two files >> You're not using this setter, so remove it. That method is intended to be used if the operator apps are installed at some other place, and it's currently being used by the unit tests So I'm actually using it >> Can you use os.file for all these file manipulations? I've changed most of the IO to use OS.File. The only exception is on the erase function, because OS.File.remove does not remove directories, and so I would have to make a recursive function to delete all the directories. For something that's called only once per life-time of the device, it seems a little bit overkill redoing that.

Since I changed the IO to use OS.File, now I sometimes get the oninstall event and before I can set the handlers the app is already downloaded (so the ondownloadsuccess is never called). I changed the test to take that into account and avoid intermitent tests failures.

I'm not obsoleting the old P2 patch in case you're already reviewing it. The change between that one and the new P2 one is just what I mentioned (a new way to handle the ondownloadsuccess)

We're going to need something similar to part 1 for Desktop. I think we should define a clear interface to add already existing applications to the registry, instead of modifying and making even more complicated the Webapps.jsm code.

Comment on attachment 802653 [details] [diff] [review] P1: V2 Gecko patch, part 1. Single Variant Apps installation. Review of attachment 802653 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/OperatorApps.jsm @@ +82,5 @@ > +this.OperatorAppsRegistry = { > + > + _fileBasePath: null, > + > + _loadJSONAsync: function(aFile, aCallback) { It would be clearer if this function just returned a promise and didn't have a callback, I think mixing them makes it even more difficult to read. Look at the implementation of _loadJSONAsync in bug 801610 for example. @@ +86,5 @@ > + _loadJSONAsync: function(aFile, aCallback) { > + debug("_loadJSONAsync: " + aFile); > + let deferred = Promise.defer(); > + > + OS.File.open(aFile, { read: true }).then( I think you can directly use OS.File.read here. @@ +148,5 @@ > + set appsDir(aDir) { > + debug("appsDir SET: " + aDir); > + if (aDir) { > + this._fileBasePath = Components.classes["@mozilla.org/file/local;1"] > + .createInstance(Components.interfaces.nsILocalFile); Here and throughout the file you can use the variables you defined at the top of the file (Cc, Ci, etc.). IIRC nsILocalFile is deprecated, you can use nsIFile. @@ +256,5 @@ > + (function(aId, aMetadata) { > + debug("(SV) metadata:" + JSON.stringify(aMetadata)); > + if (!aMetadata) { //In fact it is no necesary > + return; > + } Using Task.jsm here would make things clearer. @@ +286,5 @@ > + Services.prefs.setBoolPref(PREF_FIRST_RUN_WITH_SIM, false); > + }).bind(this)); > + }, > + > + _getSingleVariantApps: function(aMcc, aMnc, aNext) { If you rewrited _loadJSONAsync as I said and returned a promise here, there would be no need for yet another callback and you could use Task.jsm to wrap the entire _installOperatorApps function.

We need this for preloaded apps to be configured for each country the first time the device is powered on

(In reply to Carmen Jimenez Cabezas from comment #31) > Created attachment 802653 [details] [diff] [review] > P1: V2 Gecko patch, part 1. Single Variant Apps installation. > > >> That will fail on tablets. I guess you want MOZ_RIL > > When I use MOZ_RIL that code isn't included on a B2G build. I used > MOZ_WIDGET_GONK because it's what's used on Webapps.jsm and AppsUtils.jsm. > This must be defined mostly on the same cases where MOZ_WIDGET_GONK is used > on those two files This is MOZ_B2G_RIL, and you really want that for anything that is telephony related. File a followup to cleanup places where we use MOZ_WIDGET_GONK for that because that's wrong. > >> You're not using this setter, so remove it. > > That method is intended to be used if the operator apps are installed at > some other place, and it's currently being used by the unit tests > So I'm actually using it Can you add a comment stating that please? > I've changed most of the IO to use OS.File. The only exception is on the > erase function, because OS.File.remove does not remove directories, and so I > would have to make a recursive function to delete all the directories. For > something that's called only once per life-time of the device, it seems a > little bit overkill redoing that. Yep, that's unfortunate. I'll file a bug to add that directly in OS.File

(In reply to Fabrice Desré [:fabrice] from comment #37) > Yep, that's unfortunate. I'll file a bug to add that directly in OS.File See bug 772538. I think it's becoming more and more important, we have a lot of places in Webapps.jsm and in the platform specific installers where we'd like to remove files with OS.File.

(In reply to Marco Castelluccio [:marco] from comment #35) > Comment on attachment 802653 [details] [diff] [review] > P1: V2 Gecko patch, part 1. Single Variant Apps installation. > > Review of attachment 802653 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/apps/src/OperatorApps.jsm > @@ +82,5 @@ > > +this.OperatorAppsRegistry = { > > + > > + _fileBasePath: null, > > + > > + _loadJSONAsync: function(aFile, aCallback) { > > It would be clearer if this function just returned a promise and didn't have > a callback, I think mixing them makes it even more difficult to read. > Look at the implementation of _loadJSONAsync in bug 801610 for example. Regarding loadJSONAsync, it actually returns a promise. I forgot a qref, I'm sorry. The is not used (or should not be, a defer.reject() is missing also there. I will update that together with the comments from Fabrice's review for the next version

Ok, I changed that and it works. Waiting to upload a new version until the review, will add this change too.

This US should be P1, as it is already identified in Product Backlog as a must have for 1.2. It is a major feature for both OEMs and Operators, saving a lot of money & simplifying logistics because of having a single variant, instead of having specific builds for each country.

Comment on attachment 802657 [details] [diff] [review] P2: V2 Gecko patch, part 2. Unit tests for part 1. Review of attachment 802657 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Carmen! Looks good. Too bad that _installOperatorApps doesn't provide much information about errors or early return situations. Checking the successful path might not be enough ::: dom/apps/tests/Makefile.in @@ +17,5 @@ > > MOCHITEST_CHROME_FILES = \ > test_apps_service.xul \ > + test_operator_app_install.xul \ > + test_operator_app_install.js \ Can we just include the script within the xul file, please? ::: dom/apps/tests/test_operator_app_install.js @@ +8,5 @@ > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/OperatorApps.jsm"); > +Cu.import("resource://gre/modules/FileUtils.jsm"); > +Cu.import("resource://gre/modules/NetUtil.jsm"); > +Cu.import("resource://gre/modules/osfile.jsm"); Unused? @@ +17,5 @@ > +const PR_TRUNCATE = 0x20; > + > +SimpleTest.waitForExplicitFinish(); > + > +var gAppName = "testOperatorApp1"; Unused @@ +90,5 @@ > + Ci.nsIZipWriter.COMPRESSION_BEST, stream, false); > +} > + > +function setupDataDirs(aCb) { > + let dirNum = "tmp_" + Math.floor(Math.random() * (10000000) + 1); nit: Parenthesis this way: (Math.random() * 10000000) @@ +96,5 @@ > + true); > + let appDir = FileUtils.getDir("TmpD", [dirNum, "singlevariantapps", > + "testOperatorApp1"], true, true); > + > + singleVariantDir = tmpDir.path; nit: s/singleVariantDir/singlevariantDir in the whole patch @@ +101,5 @@ > + let singlevariantFile = tmpDir.clone(); > + singlevariantFile.append("singlevariantconf.json"); > + > + writeFile(singlevariantFile, JSON.stringify({"214-007":["testOperatorApp1"]}), > + function () { nit: indention @@ +120,5 @@ > + > + var metadataFile = appDir.clone(); > + metadataFile.append("metadata.json"); > + writeFile(metadataFile, JSON.stringify(metadataData), > + function () { nit: no need to split the line, also remove the ws after function, please @@ +263,5 @@ > + checkAppState(gApp, manifestData.version, expected, next); > + }; > + gApp.ondownloadsuccess = downloadsuccessHandler; > + if (!gApp.downloadAvailable && > + gApp.ondownloadsuccess) { // Seems we set the handler too late. nit: no need to split the condition in two lines

r=ferjm

Comment on attachment 802653 [details] [diff] [review] P1: V2 Gecko patch, part 1. Single Variant Apps installation. Review of attachment 802653 [details] [diff] [review]: ----------------------------------------------------------------- Unless I missed something, I don't understand how this can possibly work on a production device. ::: dom/apps/src/OperatorApps.jsm @@ +17,5 @@ > +Cu.import("resource://gre/modules/osfile.jsm"); > +Cu.import("resource://gre/modules/AppsUtils.jsm"); > +Cu.import("resource://gre/modules/Promise.jsm"); > + > +#ifdef MOZ_WIDGET_GONK use MOZ_B2G_RIL @@ +33,5 @@ > +} > + > +const DIRECTORY_NAME = "webappsDir"; > + > +// The files will be stored on DIRECTORY_NAME + "/" + SINGLE_VARIANT_SOURCE_DIR webappsDir points to /data/local in b2g (see https://mxr.mozilla.org/mozilla-central/source/b2g/components/DirectoryProvider.js#15). I don't see how you can preload anything from /data/local/svoperapps since we don't ship anything on this partition. You need to move that somewhere under /system/b2g/defaults @@ +39,5 @@ > +// Apps will be stored on a app per directory basis, hanging from > +// SINGLE_VARIANT_SOURCE_DIR > +const SINGLE_VARIANT_SOURCE_DIR = "svoperapps"; > +const SINGLE_VARIANT_CONF_FILE = "singlevariantconf.json"; > +const PREF_FIRST_RUN_WITH_SIM = "dom.webapps.firstRunWithSIM"; can you set this pref to 'false' in b2g/app/b2g.js in a MOZ_B2G_RIL section? @@ +51,5 @@ > + if (Services.prefs.prefHasUserValue(PREF_FIRST_RUN_WITH_SIM)) { > + return Services.prefs.getBoolPref(PREF_FIRST_RUN_WITH_SIM); > + } > + } catch(e) { > + debug ("(SV) Error getting pref. " + e); nit: Here and in other debug() calls, I would remove the (SV) part. @@ +56,5 @@ > + } > + return true; > +} > + > +#ifdef MOZ_WIDGET_GONK Use MOZ_B2G_RIL @@ +84,5 @@ > + _fileBasePath: null, > + > + _loadJSONAsync: function(aFile, aCallback) { > + debug("_loadJSONAsync: " + aFile); > + let deferred = Promise.defer(); We now have a native Promise implementation in the platform. I would prefer you to use it instead of the jsm one. Also, we should move this promise-based json loader to AppsUtils.jsm. I'm even surprised if we don't have one somewhere in toolkit/ (I haven't checked either). @@ +122,5 @@ > + }, > + > + init: function() { > + debug("(SV) init"); > +#ifdef MOZ_WIDGET_GONK MOZ_B2G_RIL @@ +152,5 @@ > + .createInstance(Components.interfaces.nsILocalFile); > + this._fileBasePath.initWithPath(aDir); > + } else { > + this._fileBasePath = null; > + } _fileBasePath is then a directory. Can you rename that to eg. _baseDirectory ? @@ +158,5 @@ > + > + get appsDir() { > + if (!this._fileBasePath) { > + return this._fileBasePath = FileUtils.getFile(DIRECTORY_NAME, > + [SINGLE_VARIANT_SOURCE_DIR]); nit: no need to return here. @@ +166,5 @@ > + > + eraseVariantAppsNotInList: function(aIdsApp) { > + let entries; > + let entry; > + let svDir; don't declare these variable so early. @@ +170,5 @@ > + let svDir; > + > + if (!aIdsApp) { > + aIdsApp = [ ]; > + } Since you check your parameters (which is great), make also sure that this is actually an array with Array.isArray(). @@ +171,5 @@ > + > + if (!aIdsApp) { > + aIdsApp = [ ]; > + } > + let svDir; @@ +184,5 @@ > + if (!svDir || !svDir.exists()) { > + return; > + } > + > + entries = svDir.directoryEntries; let entries = ... @@ +186,5 @@ > + } > + > + entries = svDir.directoryEntries; > + while (entries.hasMoreElements()) { > + entry = entries.getNext().QueryInterface(Ci.nsIFile); let entry = ... @@ +213,5 @@ > + > + if (!aManifest) { > + debug("Error: The application " + aId + " does not have a manifest"); > + return; > + } Move that block before let appData = {...} @@ +219,5 @@ > + if (isPackage) { > + debug("(SV) aId:" + aId + ". Installing as packaged app."); > + let installPack = OS.Path.join(this.appsDir.path, aId, APPLICATION_ZIP); > + OS.File.exists(installPack).then( > + function(exists) { nit: s/exists/aExists @@ +222,5 @@ > + OS.File.exists(installPack).then( > + function(exists) { > + if (!exists) { > + debug("SV " + installPack.path + " file do not exists for app " + > + aId); max length for lines is 80 chars, I don't think you need to split this one. @@ +256,5 @@ > + (function(aId, aMetadata) { > + debug("(SV) metadata:" + JSON.stringify(aMetadata)); > + if (!aMetadata) { //In fact it is no necesary > + return; > + } I agree. @@ +287,5 @@ > + }).bind(this)); > + }, > + > + _getSingleVariantApps: function(aMcc, aMnc, aNext) { > + let file = null; remove this line. @@ +299,5 @@ > + } > + > + let key = normalizeCode(aMcc) + "-" + normalizeCode(aMnc); > + > + file = OS.Path.join(this.appsDir.path, SINGLE_VARIANT_CONF_FILE); let file = ...

(In reply to Fabrice Desré [:fabrice] from comment #44) > Comment on attachment 802653 [details] [diff] [review] > P1: V2 Gecko patch, part 1. Single Variant Apps installation. > > Review of attachment 802653 [details] [diff] [review]: > ----------------------------------------------------------------- > > Unless I missed something, I don't understand how this can possibly work on > a production device. > > webappsDir points to /data/local in b2g (see > https://mxr.mozilla.org/mozilla-central/source/b2g/components/ > DirectoryProvider.js#15). I don't see how you can preload anything from > /data/local/svoperapps since we don't ship anything on this partition. You > need to move that somewhere under /system/b2g/defaults I can actually answer this one. The build process was changes on bug 899079 to add this directory if needed. The directory is added at /data/local always for two reasons: we wanted to be able to remove the unused apps (apps that were for a different operator than the one from the initial SIM) and the /data partition is usually bigger and there can potentially be a huge number of operator apps when you do a build for 5-6 operators). So the code is actually correct (and we've been testing it on production builds :)).

(In reply to Fabrice Desré [:fabrice] from comment #44) > Comment on attachment 802653 [details] [diff] [review] > P1: V2 Gecko patch, part 1. Single Variant Apps installation. > > Review of attachment 802653 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +158,5 @@ > > + > > + get appsDir() { > > + if (!this._fileBasePath) { > > + return this._fileBasePath = FileUtils.getFile(DIRECTORY_NAME, > > + [SINGLE_VARIANT_SOURCE_DIR]); > > nit: no need to return here. I would swear you told me to add this on the last review ;) > @@ +222,5 @@ > > + OS.File.exists(installPack).then( > > + function(exists) { > > + if (!exists) { > > + debug("SV " + installPack.path + " file do not exists for app " + > > + aId); > > max length for lines is 80 chars, I don't think you need to split this one. If I don't split it, it goes to 83 chars.

Ups! the latest patch had an extra file I didn't intended to add. This one removes that file. I didn't obsolete the other patch in case you're already reviewing it, the only change is that this one doesn't have the b2g-certdata.txt file on it.

Comment on attachment 803609 [details] [diff] [review] P1: V3 Gecko patch, part 1. Single Variant Apps installation. Review of attachment 803609 [details] [diff] [review]: ----------------------------------------------------------------- We're almost there! ::: b2g/chrome/content/shell.js @@ +596,5 @@ > DOMApplicationRegistry.allAppsLaunchable = true; > > this.sendEvent(window, 'ContentStart'); > > + Cu.import('resource://gre/modules/OperatorApps.jsm'); Nit:#ifdef MOZ_B2G_RIL ::: dom/apps/src/AppsUtils.jsm @@ +485,5 @@ > return true; > }, > > + // Loads a JSON file using OS.file. aFile is a string representing the path > + // of the file to be read. Add: // Returns a Promise resolved with the json payload or rejected with 'ErrorCantOpen' or 'ErrorCantRead' ::: dom/apps/src/OperatorApps.jsm @@ +108,5 @@ > + > + set appsDir(aDir) { > + debug("appsDir SET: " + aDir); > + if (aDir) { > + this._baseDirectory = Components.classes["@mozilla.org/file/local;1"] Use Cc instead of Components.classes @@ +109,5 @@ > + set appsDir(aDir) { > + debug("appsDir SET: " + aDir); > + if (aDir) { > + this._baseDirectory = Components.classes["@mozilla.org/file/local;1"] > + .createInstance(Components.interfaces.nsILocalFile); Ci.nsIFile here @@ +235,5 @@ > + debug("No metadata or updatemanifest file for " + aId + > + ". " + aError); > + } > + ); > + }, this); all this nested functions are hard to read. Can you use Task.jsm here? This will be so beautiful ;)

I think the loadJSONAsync function is overly complicated. You're creating a new promise, then opening a file, then reading it, then resolving the promise. I think the version in the patch in bug 801610 is simpler. If you don't want to use it, at least you could avoid creating a new promise and opening the file, because OS.File.read returns a promise. Look at the example in the OS.File documentation. The documentation also states that if you open a file, you should close it. Anyway, the promise constructor was recently changed in bug 911213. Also, as we add loadJSONAsync to AppsUtils.jsm we should use it in Webapps.jsm too (but probably we can do that in another bug). I still think _getSingleVariantApps shouldn't use a callback but return a promise. This would clean _installOperatorApps up even more (if you used Task.jsm).

I think this covers all the changes requested and then some. I redid everything without callbacks and using Task.jsm

Comment on attachment 804134 [details] [diff] [review] P1: V4 Gecko patch, part 1. Single Variant Apps installation. Review of attachment 804134 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the last nits addressed. ::: dom/apps/src/AppsUtils.jsm @@ +493,5 @@ > > + // Loads a JSON file using OS.file. aFile is a string representing the path > + // of the file to be read. > + // Returns a Promise resolved with the json payload or rejected with > + // 'ErrorCantOpen' or 'ErrorCantRead' Hm, is that correct now? I doubt that this is what File.open() and File.read() send. @@ +500,5 @@ > + return Task.spawn(function() { > + let file = yield OS.File.open(aFile, { read: true }); > + debug("Opened " + aFile); > + let rawData = yield file.read(); > + debug("Read " + aFile); Nit: remove these 2 debug logs before landing.

Comment on attachment 804330 [details] [diff] [review] P1: V4.1 Gecko patch, part 1. Single Variant Apps installation. r=fabrice

Try run at https://tbpl.mozilla.org/?tree=Try&rev=a33716c0365a

And try run with the proposed patch for bug 903291: https://tbpl.mozilla.org/?tree=Try&rev=432a040645ab Without this, the unit test might fail (it fails randomly)

Carmen Jimenez Cabeza changed story state to finished in Pivotal Tracker

https://hg.mozilla.org/integration/b2g-inbound/rev/2a58557adc6f https://hg.mozilla.org/integration/b2g-inbound/rev/e69381450554

Backed out (shockingly) for the exact same test_app_uninstall.html test failures that *both* of your most recent Try pushes showed. Why was this checkin-needed again? https://hg.mozilla.org/integration/b2g-inbound/rev/2aa1e16b67b0

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #58) > Backed out (shockingly) for the exact same test_app_uninstall.html test > failures that *both* of your most recent Try pushes showed. Why was this > checkin-needed again? > https://hg.mozilla.org/integration/b2g-inbound/rev/2aa1e16b67b0 That fail seems to be a known issue, bug 843649. This patch hasn't touched uninstall at all... nor localstorage. The other failures on the try run are also reported and also unrelated with this patch. Maybe they should be disabled temporarily until fixed? I will take a look at those tests to see if I can get them working but that will no be neither here nor now, I'm afraid.

Your patch changed it from an intermittent failure (and one that has only occurred on TBPL once since the beginning of August) to one that happens on every run on every platform. That's a clear regression that needs addressing before this can land. We cannot have a test suite permanently failing. Maybe Marco, Myk, or Fabrice can help you figure out what's going on.

The good news is that if it's now failing permanently that will easier to debug ;) We'll reland once this is fixed.

r=ferjm The code isn't the problem, it was a slight mistake on the unit test. I'm sorry

Carmen, push to try before landing please.

r=ferjm Besides test_app_uninstall.xul, there was another xul test failing. This new version restores the test environment so tests that require a specific environment don't fail. I can't push to try yet, Antonio, can you please push it?

It looks like my and Marco's infos are no longer needed.

Try runs: Without the patch from 903291 (might time out on the test_operator_app_install.xul): https://tbpl.mozilla.org/?tree=Try&rev=872b20d1f01e With the patch from 903291 applied: https://tbpl.mozilla.org/?tree=Try&rev=bafcaa6ca87e BTW, after taking a look to the test_app_uninstall.xul test that was failing randomly, it's quite possible that the patch from 903291 will also fix that.

Try runs finished. They're mostly green (except for 759036-1.html on https://tbpl.mozilla.org/?tree=Try&rev=872b20d1f01e, which seems to be bug 907899). Requesting checkin again. I'm going to request it also for the patch from 903291.

https://hg.mozilla.org/integration/b2g-inbound/rev/a308c1857b9e https://hg.mozilla.org/integration/b2g-inbound/rev/5b79d1417556

https://hg.mozilla.org/mozilla-central/rev/a308c1857b9e https://hg.mozilla.org/mozilla-central/rev/5b79d1417556

Marcelino Veiga Tuimil changed story state to accepted in Pivotal Tracker

Marcelino Veiga Tuimil changed story state to accepted in Pivotal Tracker

I'm still wondering why you use OS.File.open, read, close. This pattern is useful when you want to read a file in chunks, for example. In this case you're just reading the entire file, why don't you directly use OS.File.read? Anyway you're not closing the file in the catch block.

Comment on attachment 804330 [details] [diff] [review] P1: V4.1 Gecko patch, part 1. Single Variant Apps installation. Review of attachment 804330 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/OperatorApps.jsm @@ +64,5 @@ > + notifyStkSessionEnd: function() {}, > + > + notifyIccCardLockError: function() {}, > + > + notifyCardStateChange: function() {}, It should be a typo here, it should be 'notifyCardStateChanged' with ends with d. And causing Bug 921958.