893800 - [User Story] Home screen based on operator apps in case of first time usage with a SIM card

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Marcelino Veiga Tuimil [:sonmarce]]

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

Comment 1

[Carmen Jimenez Cabezas]

Attachment: 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

Comment 2

[Jason Smith [:jsmith]]

(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?

Comment 3

[Carmen Jimenez Cabezas]

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 :)

Comment 4

[Candice Serran (:cserran)]

Noemi Freire changed story state to started in Pivotal Tracker

Comment 5

[[:fabrice] Fabrice Desré]

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.

Comment 6

[Carmen Jimenez Cabezas]

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.

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

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

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

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?

Comment 9

[[:fabrice] Fabrice Desré]

(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.

Comment 10

[Carmen Jimenez Cabezas]

Attachment: Gaia patch. Add the ability to Homescreen to insert an icon on a predetermined position

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.

Comment 11

[Carmen Jimenez Cabezas]

Attachment: Sample JSON file with operator apps for the homescreen app

Comment 12

[Cristian Rodriguez (:crdlc)]

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

Comment 13

[Carmen Jimenez Cabezas]

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

Comment 14

[Cristian Rodriguez (:crdlc)]

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 15

[Cristian Rodriguez (:crdlc)]

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

Comment 16

[Cristian Rodriguez (:crdlc)]

Merged into master

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

Comment 17

[Jason Smith [:jsmith]]

(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.

Comment 18

[Carmen Jimenez Cabezas]

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

Comment 19

[Carmen Jimenez Cabezas]

Attachment: WIP v2, as described in comment 9

Comment 20

[[:fabrice] Fabrice Desré]

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

Comment 21

[Julien Wajsberg [:julienw]]

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.

Comment 22

[Carmen Jimenez Cabezas]

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

Comment 23

[[:fabrice] Fabrice Desré]

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.

Comment 24

[Carmen Jimenez Cabezas]

Attachment: Gaia patch V2. Fixes the problem with PINless cards and adds unit tests

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

Comment 25

[Cristian Rodriguez (:crdlc)]

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

Comment 26

[Carmen Jimenez Cabezas]

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

Comment 27

[Carmen Jimenez Cabezas]

Attachment: P1: Gecko patch, part 1. Single Variant Apps installation.

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

Comment 28

[Carmen Jimenez Cabezas]

Attachment: P2: Gecko patch, part 2. Unit tests for part 1.

Comment 29

[Carmen Jimenez Cabezas]

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

Comment 30

[[:fabrice] Fabrice Desré]

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.

Comment 31

[Carmen Jimenez Cabezas]

Attachment: 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

>> 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.

Comment 32

[Carmen Jimenez Cabezas]

Attachment: P2: V2 Gecko patch, part 2. Unit tests for part 1.

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.

Comment 33

[Carmen Jimenez Cabezas]

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)

Comment 34

[Marco Castelluccio [:marco]]

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 35

[Marco Castelluccio [:marco]]

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.

Comment 36

[Marcelino Veiga Tuimil [:sonmarce]]

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

Comment 37

[[:fabrice] Fabrice Desré]

(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

Comment 38

[Marco Castelluccio [:marco]]

(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.

Comment 39

[Carmen Jimenez Cabezas]

(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

Comment 40

[Carmen Jimenez Cabezas]

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

Comment 41

[Marcelino Veiga Tuimil [:sonmarce]]

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 42

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 43

[Carmen Jimenez Cabezas]

Attachment: P2: V3 Gecko patch, part 2. Unit tests for part 1.

r=ferjm

Comment 44

[[:fabrice] Fabrice Desré]

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 = ...

Comment 45

[Antonio Manuel Amaya Calvo (:amac)]

(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 :)).

Comment 46

[Carmen Jimenez Cabezas]

Attachment: P1: V3 Gecko patch, part 1. Single Variant Apps installation.

(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.

Comment 47

[Carmen Jimenez Cabezas]

Attachment: P1: V3 Gecko patch, part 1. Single Variant Apps installation.

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 48

[[:fabrice] Fabrice Desré]

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 ;)

Comment 49

[Marco Castelluccio [:marco]]

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).

Comment 50

[Carmen Jimenez Cabezas]

Attachment: P1: V4 Gecko patch, part 1. Single Variant Apps installation.

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

Comment 51

[[:fabrice] Fabrice Desré]

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 52

[Carmen Jimenez Cabezas]

Attachment: P1: V4.1 Gecko patch, part 1. Single Variant Apps installation.

Comment 53

[Carmen Jimenez Cabezas]

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

r=fabrice

Comment 54

[Antonio Manuel Amaya Calvo (:amac)]

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

Comment 55

[Antonio Manuel Amaya Calvo (:amac)]

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)

Comment 56

[Candice Serran (:cserran)]

Carmen Jimenez Cabeza changed story state to finished in Pivotal Tracker

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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

Comment 58

[Ryan VanderMeulen [:RyanVM]]

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

Comment 59

[Antonio Manuel Amaya Calvo (:amac)]

(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.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 61

[[:fabrice] Fabrice Desré]

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

Comment 62

[Carmen Jimenez Cabezas]

Attachment: P2: V3.1 Gecko patch, part 2. Unit tests for part 1.

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

Comment 63

[[:fabrice] Fabrice Desré]

Carmen, push to try before landing please.

Comment 64

[Carmen Jimenez Cabezas]

Attachment: P2: V3.2 Gecko patch, part 2. Unit tests for part 1.

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?

Comment 65

[Myk Melez [:myk] [@mykmelez]]

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

Comment 66

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 67

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

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

Comment 69

[Ryan VanderMeulen [:RyanVM]]

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

Comment 70

[Candice Serran (:cserran)]

Marcelino Veiga Tuimil changed story state to accepted in Pivotal Tracker

Comment 71

[Candice Serran (:cserran)]

Marcelino Veiga Tuimil changed story state to accepted in Pivotal Tracker

Comment 72

[Marco Castelluccio [:marco]]

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 73

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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.

Comment 74

[Jason Smith [:jsmith]]

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2506