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

VERIFIED FIXED in Firefox 26

Status

Core Graveyard
DOM: Apps
VERIFIED FIXED
5 years ago
4 months ago

People

(Reporter: sonmarce, Assigned: Carmen Jimenez Cabezas)

Tracking

({feature})

Trunk
mozilla26
ARM
Gonk (Firefox OS)
feature
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed)

Details

(Whiteboard: [UCID:System10, FT:systems-fe, KOI:P2][systemsfe])

Attachments

(4 attachments, 12 obsolete attachments)

935 bytes, application/json
Details
185 bytes, patch
crdlc
: review+
Details | Diff | Splinter Review
19.47 KB, patch
Carmen Jimenez Cabezas
: review+
Details | Diff | Splinter Review
10.02 KB, patch
Carmen Jimenez Cabezas
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Keywords: feature
(Reporter)

Updated

5 years ago
Blocks: 892938
(Assignee)

Updated

5 years ago
Assignee: nobody → cjc
(Assignee)

Comment 1

5 years ago
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
Attachment #783918 - Flags: feedback?(fabrice)

Updated

5 years ago
Component: Gaia::System → DOM: Apps
Product: Boot2Gecko → Core
Version: unspecified → Trunk
(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?
(Assignee)

Comment 3

5 years ago
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.
Attachment #783918 - Flags: feedback?(fabrice) → feedback-
(Assignee)

Comment 6

5 years ago
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.
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
Depends on: 903291
(Assignee)

Comment 10

5 years ago
Created attachment 791246 [details] [diff] [review]
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.
Attachment #791246 - Flags: review?(crdlc)
(Assignee)

Comment 11

5 years ago
Created attachment 791250 [details]
Sample JSON file with operator apps for the homescreen app
Carmen, I would like to talk with you before reviewing it, please contact me when you want. Thanks a lot
Status: NEW → ASSIGNED
(Assignee)

Comment 13

5 years ago
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
Attachment #791246 - Flags: review?(crdlc) → review+
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
(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.
(Assignee)

Comment 18

5 years ago
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
(Assignee)

Comment 19

5 years ago
Created attachment 795483 [details] [diff] [review]
WIP v2, as described in comment 9
Attachment #783918 - Attachment is obsolete: true
Attachment #795483 - Flags: feedback?(fabrice)
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.
(Assignee)

Comment 22

5 years ago
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.
Attachment #795483 - Flags: feedback?(fabrice) → feedback+
Whiteboard: [FT:systems-fe, KOI:P1]
(Assignee)

Comment 24

5 years ago
Created attachment 797960 [details] [diff] [review]
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.
Attachment #797960 - Flags: review?(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
Attachment #797960 - Flags: review?(crdlc) → review+
Depends on: 913222
(Assignee)

Comment 27

5 years ago
Created attachment 802166 [details] [diff] [review]
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
Attachment #795483 - Attachment is obsolete: true
Attachment #802166 - Flags: review?(fabrice)
(Assignee)

Comment 28

5 years ago
Created attachment 802173 [details] [diff] [review]
P2: Gecko patch, part 2. Unit tests for part 1.
Attachment #802173 - Flags: review?(fabrice)
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.
Attachment #802166 - Flags: review?(fabrice) → review-
(Assignee)

Comment 31

5 years ago
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

>> 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.
Attachment #802166 - Attachment is obsolete: true
Attachment #802653 - Flags: review?(fabrice)
(Assignee)

Comment 32

5 years ago
Created attachment 802657 [details] [diff] [review]
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.
Attachment #802657 - Flags: review?(fabrice)
(Assignee)

Comment 33

5 years ago
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.
(Reporter)

Comment 36

5 years ago
We need this for preloaded apps to be configured for each country the first time the device is powered on
blocking-b2g: --- → koi?
(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.
Attachment #802173 - Attachment is obsolete: true
Attachment #802173 - Flags: review?(fabrice)
(Assignee)

Comment 39

5 years ago
(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
(Assignee)

Comment 40

5 years ago
Ok, I changed that and it works. Waiting to upload a new version until the review, will add this change too.
(Reporter)

Updated

5 years ago
Blocks: 893807
Whiteboard: [FT:systems-fe, KOI:P1] → [UCID:System10, FT:systems-fe, KOI:P1]
Whiteboard: [UCID:System10, FT:systems-fe, KOI:P1] → [UCID:System10, FT:systems-fe, KOI:P2]
(Reporter)

Comment 41

5 years ago
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.
Attachment #802657 - Flags: review?(fabrice) → review?(ferjmoreno)
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
Attachment #802657 - Flags: review?(ferjmoreno) → review+
(Assignee)

Comment 43

5 years ago
Created attachment 803160 [details] [diff] [review]
P2: V3 Gecko patch, part 2. Unit tests for part 1.

r=ferjm
Attachment #802657 - Attachment is obsolete: true
Attachment #803160 - Flags: review+
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 = ...
Attachment #802653 - Flags: review?(fabrice) → review-

Updated

5 years ago
Whiteboard: [UCID:System10, FT:systems-fe, KOI:P2] → [UCID:System10, FT:systems-fe, KOI:P2][systemsfe]
(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 :)).
(Assignee)

Comment 46

5 years ago
Created attachment 803574 [details] [diff] [review]
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.
Attachment #802653 - Attachment is obsolete: true
Attachment #803574 - Flags: review?(fabrice)
(Assignee)

Comment 47

5 years ago
Created attachment 803609 [details] [diff] [review]
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.
Attachment #803609 - Flags: review?(fabrice)
(Assignee)

Updated

5 years ago
Attachment #803574 - Attachment is obsolete: true
Attachment #803574 - Flags: review?(fabrice)
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 ;)
Attachment #803609 - Flags: review?(fabrice) → feedback+
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).
(Assignee)

Comment 50

5 years ago
Created attachment 804134 [details] [diff] [review]
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
Attachment #803609 - Attachment is obsolete: true
Attachment #804134 - Flags: review?(fabrice)
Attachment #791246 - Attachment is obsolete: true
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.
Attachment #804134 - Flags: review?(fabrice) → review+
(Assignee)

Comment 52

5 years ago
Created attachment 804330 [details] [diff] [review]
P1: V4.1 Gecko patch, part 1. Single Variant Apps installation.
Attachment #804134 - Attachment is obsolete: true
Attachment #804330 - Flags: review+
(Assignee)

Comment 53

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

r=fabrice
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
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
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
Flags: in-testsuite+
(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.
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
Flags: needinfo?(myk)
Flags: needinfo?(mcastelluccio)
Flags: needinfo?(fabrice)
The good news is that if it's now failing permanently that will easier to debug ;)
We'll reland once this is fixed.
Flags: needinfo?(fabrice)
(Assignee)

Comment 62

5 years ago
Created attachment 804543 [details] [diff] [review]
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
Attachment #803160 - Attachment is obsolete: true
Attachment #804543 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Carmen, push to try before landing please.
Keywords: checkin-needed
(Assignee)

Comment 64

5 years ago
Created attachment 804576 [details] [diff] [review]
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?
Attachment #804543 - Attachment is obsolete: true
Attachment #804576 - Flags: review+
It looks like my and Marco's infos are no longer needed.
Flags: needinfo?(myk)
Flags: needinfo?(mcastelluccio)
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a308c1857b9e
https://hg.mozilla.org/mozilla-central/rev/5b79d1417556
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith
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.

Updated

5 years ago
blocking-b2g: koi? → koi+

Updated

5 years ago
QA Contact: jsmith
status-firefox26: --- → fixed
(Assignee)

Updated

5 years ago
Blocks: 918432

Updated

5 years ago
No longer blocks: 918432
No longer depends on: 903291

Updated

5 years ago
QA Contact: rafael.marquez

Updated

5 years ago
Flags: in-moztrap?(rafael.marquez)
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.

Updated

4 years ago
Depends on: 923766
(Assignee)

Updated

4 years ago
Depends on: 926219

Updated

4 years ago
Depends on: 928134

Updated

4 years ago
Depends on: 927973

Updated

4 years ago
Depends on: 927967

Updated

4 years ago
Depends on: 927959
(Assignee)

Updated

4 years ago
Depends on: 930544

Updated

4 years ago
Depends on: 931299

Updated

4 years ago
No longer depends on: 931299
Depends on: 932843
Depends on: 933752

Updated

4 years ago
Depends on: 936018

Updated

4 years ago
Depends on: 936028

Updated

4 years ago
Depends on: 934017

Updated

4 years ago
Depends on: 941755
Flags: sec-review?(ptheriault)

Updated

4 years ago
Depends on: 942787
Depends on: 943231

Updated

4 years ago
No longer depends on: 942787

Updated

4 years ago
Depends on: 944495

Updated

4 years ago
No longer depends on: 934017

Updated

4 years ago
Status: RESOLVED → VERIFIED

Updated

4 years ago
Keywords: verifyme
Depends on: 991246
Flags: sec-review?(ptheriault)

Updated

4 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.