917740 - [System][FTU][Single Variant] Update file format of customization files based on MCC/MNC

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Borja Salguero [:borjasalguero]]

After trying current implementation I've detected several issues.
When trying with my SIM Card, I got the following error:

E/GeckoConsole(  626): [JavaScript Error: "NS_ERROR_FILE_NOT_FOUND: " {file: "app://communications.gaiamobile.org/ftu/js/variant.js" line: 45}]

This is due to the file, generated following the path:
var filePath = '/ftu/js/variants/' + self.mcc_mnc + '.json';

is not generating a 404 error due to https://bugzilla.mozilla.org/show_bug.cgi?id=827243, so we would need to add a try...catch for handling this properly.

I would suggest to create a file called 'customization.json', with an array of objects with pair MCC/MNC, and with the params to customize (wallpaper, ringtone...). All resources should be *out* of this file (we need only the URI of the resource, not the resource itself in Base64), and the app will be in charge of retrieving these resources and make all the process needed. Having files embedded in Base64 in the .json, and creating one file per MNC/MCC is a more complicated solution.

I've talked with Reuben and Albert about this, so we are going to propose a new format, avoiding to load Base64 content from a JSON file, and loading resources through a different process.

Comment 1

[Borja Salguero [:borjasalguero]]

After discussing about how to deal with this customization params in a better way, we have created the following proposal http://pastebin.com/4kGCBSxX . Reuben, could you take a look? Thanks!!

Comment 3

[Albert [:albert]]

Attachment: Sample configuration

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 806599 [details]
Sample configuration

r+ with some minor changes on the sample file.

Comment 5

[Reuben Morais [:reuben]]

Guys, what exactly do you want to do here? Albert's PR doesn't match the attachment, the NS_ERROR_FILE_NOT_FOUND exception is just a minor detail, and preventing people from putting base64 encoded resources in the customization files has no relation to the way the files are laid out at all. I'm very confused as to what exactly you're objecting to.

Comment 6

[Borja Salguero [:borjasalguero]]

Hi Reuben
This patch is not complete yet, that's why it's tough to understand what we are doing here. We are working in the patch that we agreed yesterday, and we are going to try to deliver it today. The patch is going to fix the output json file with the structure we discussed yesterday, and wallpaper will be working using this new file structure.
All resources will be available in /resources folder in FTU sandbox, so everything is even more clear. Stay tuned! :)

Comment 7

[Albert [:albert]]

Attachment: Patch

Modified makefile and scripts for build proces.

Modified wallpaper setup based on mcc/mnc

Comment 8

[Gregor Wagner [:gwagner]]

We are past the feature freeze deadline here so why do you want to update the spec here?
This blocks the remaining user stories and puts the whole customization feature at risk.

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

The problem here was mainly that binary files were being included as data Uris inside of the configuration files instead of being included by reference. What this patch does is to get that data out of he configuration file. The patch is already done and ready for review so it should be ready to land tomorrow if all goes well.

Comment 10

[Gregor Wagner [:gwagner]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #9)
> The problem here was mainly that binary files were being included as data
> Uris inside of the configuration files instead of being included by
> reference. What this patch does is to get that data out of he configuration
> file. The patch is already done and ready for review so it should be ready
> to land tomorrow if all goes well.

I understand that's a desirable cleanup but I don't think we have the right priorities here.
We should have landed the features first and let this cleanup go through the regular approval process where we decide based on benefit and risk.

Comment 11

[Aus Lacroix [:aus]]

The desire to update this file format right now is blocking me from completing bug 891724 which needs to land tomorrow!

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

Ok... I don't understand the problem. The patches on this bug haven't been reviewed nor landed yet (except for the sample customization file which is on TEF's own personalization repository as a *sample* file--- not even on the build tree). 

So... what's the problem in stalling this bug a little, finish landing the contacts and support contacts (and whatever) bugs with the old format, and then adding to the PR here the modifications needed to get the rest of the bugs on the new file format? If you have something to land with the old (current) file format then by all means do so, and we'll just adapt this patch when everything is landed.

Does that sound ok to you?

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #12)
> Ok... I don't understand the problem. The patches on this bug haven't been
> reviewed nor landed yet (except for the sample customization file which is
> on TEF's own personalization repository as a *sample* file--- not even on
> the build tree). 
> 
> So... what's the problem in stalling this bug a little, finish landing the
> contacts and support contacts (and whatever) bugs with the old format, and
> then adding to the PR here the modifications needed to get the rest of the
> bugs on the new file format? If you have something to land with the old
> (current) file format then by all means do so, and we'll just adapt this
> patch when everything is landed.
> 
> Does that sound ok to you?

Sure this sounds great. Thanks!

Comment 14

[Borja Salguero [:borjasalguero]]

Comment on attachment 807302 [details]
Patch

Reuben, this is the patch with the new file structure, and Wallpaper working with this approach. Could you take a look as well? We need to ensure that this is the structure that we want to follow in the other bugs related (ringtone, contacts...). Thanks! Gracias :)

Comment 15

[Reuben Morais [:reuben]]

(In reply to Borja Salguero [:borjasalguero] from comment #14)

I already did, see the comments on the PR. That's why I cleared the flag.

Comment 16

[Albert [:albert]]

Attachment: Patch

Comment 17

[Reuben Morais [:reuben]]

Comment on attachment 807675 [details]
Patch

Commented in the PR. f+ with a small comment.

Comment 18

[Salvador de la Puente González [:salva]]

Comment on attachment 807675 [details]
Patch

Hello guys. IMO, it is not a good idea to store all the customizations inside only one file. It scales in a much better way to have different files for MCC/MNC combinations. Note it results in shorter load time and better isolation between customizations.

In the other hand I agree it is better to have references, not base64 to the resources.

I know I was not asked but I'm was designing the original patch so here you have my feedback. Furthermore, asking :mai to contribute.

Comment 19

[Jason Smith [:jsmith]]

Renom - I want to understand more details why this is needed past feature freeze. The patch here looks really risky and probably not something we can take at this point. We need solutions that improve on the existing feature implementation in 1.2, rather than trying a partial rewrite. A rewrite needs to ride the trains.

Comment 20

[Borja Salguero [:borjasalguero]]

(In reply to Salvador de la Puente González [:salva] from comment #18)

> Note it results in shorter load time and better
> isolation between customizations.
>

In this case this field is generated for a specific carrier, which will have not a huge list of MCC/MCN pairs (Check Telefónica or Telenor... do we have more than... 30 pairs per carrier? and only 3 lines of json per MNC/MCC pair...). Actually we are following this approach in "APN.json", where the list is huge (and IMHO we should follow the same approach for all code related with MCC/MNC).

We were discussing this with Reuben, Antonio and Albert, and we agreed that this is a `nice to have`(that's why we are not blocking the other PRs). However, having all base64 content directly in the .json it's definitely not the best way to deal with this. Our new structure allow us to have all MNC/MCC combinations for a carrier in less space than the current format with only one wallpaper... (imagine with the ringtone & contacts...) :S
 
> In the other hand I agree it is better to have references, not base64 to the
> resources.
> 

Agree. This is the one of the best wins here! :)

> I know I was not asked but I'm was designing the original patch so here you
> have my feedback. Furthermore, asking :mai to contribute.

As you were in PTO that's why you are not marked as 'reviewer'! let's talk tomorrow morning about these details. Thanks for your feedback!

Comment 21

[Borja Salguero [:borjasalguero]]

(In reply to Jason Smith [:jsmith] from comment #19)
> Renom - I want to understand more details why this is needed past feature
> freeze. The patch here looks really risky and probably not something we can
> take at this point. We need solutions that improve on the existing feature
> implementation in 1.2, rather than trying a partial rewrite. A rewrite needs
> to ride the trains.

Hi Jason. I hope this https://bugzilla.mozilla.org/show_bug.cgi?id=917740#c20 will help!  :)

Comment 22

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #19)
> Renom - I want to understand more details why this is needed past feature
> freeze. The patch here looks really risky and probably not something we can
> take at this point. We need solutions that improve on the existing feature
> implementation in 1.2, rather than trying a partial rewrite. A rewrite needs
> to ride the trains.
Actually this improves the feature implementation. It's not a rewrite with the same exact inputs and outputs. It's changing the file format because the one that was originally defined isn't very manteinable. And it even comes with a full set of unit tests. Where do you see the risk? The features it modify have landed in time but I don't think they've been verified yet. Neither have this change, but that's the same risk than the original implementation. And since it's already implemented it won't impact a second the release date for Koi. 

Can you elaborate on what the risk is? The risk of not taking this are: we will end with huge json files which will make the communication package bigger, which will consume more memory (and be slower) at the first configuration time and which will require some kind of preprocessing by the operators to generate. This approach is better, and the implementation changes are really minimum related to what landed initially.

Comment 23

[Jason Smith [:jsmith]]

Let's discuss this more in Wednesday's triage.

Comment 24

[Borja Salguero [:borjasalguero]]

Comment on attachment 807675 [details]
Patch

Moving the review to Salva! :)

Comment 25

[Salvador de la Puente González [:salva]]

(In reply to Jason Smith [:jsmith] from comment #19)
> Renom - I want to understand more details why this is needed past feature
> freeze.

Feature freeze on this concern landed the last day of Oslo's working week, in a partial way and only with wallpaper customization. I mean with only one sample of customization. Extra talking was needed to clarify some other aspects once different customizations were prepared to land.

> The patch here looks really risky and probably not something we can
> take at this point.

The patch has been rewritten. Let's take a look and wait for my review.

> We need solutions that improve on the existing feature
> implementation in 1.2, rather than trying a partial rewrite. A rewrite needs
> to ride the trains.

Precisely, bug 891730 and bug 891723 provided the discussion needed to doubt about embedding base64-encoded resources. Avoiding embedding resources remove the need of having multiple configuration files by each variant we want to support. More important even, these multiple files were generated by an automatic process. Moving to a single file reduces the complexity of this automatic process.

So this bug is indeed an improvement over an already development feature. The size of the bug is not reflecting the impact or complexity: the architecture remains unaltered and no solution will land without a proper test coverage. With all in ming, please, consider restoring the koi+ flag.

Comment 26

[Jason Smith [:jsmith]]

I'll wait for Wednesday's triage to make a decision on this.

Comment 27

[Salvador de la Puente González [:salva]]

Comment on attachment 807675 [details]
Patch

Cancelling feedback given now I'm reviewer.

Comment 28

[Salvador de la Puente González [:salva]]

Comment on attachment 807675 [details]
Patch

There is a test failing on my computer:

> [communications-ftu] Default contacts customizer >
>      1) [communications-ftu]  set

Comment 29

[Salvador de la Puente González [:salva]]

Seems to be a missed dependency:

> ReferenceError: mozContact is not defined
>      at (anonymous) (http://communications.gaiamobile.org:8080/ftu/js/customizers/default_contacts_customizer.js:7)
>      at (anonymous) (http://communications.gaiamobile.org:8080/ftu/test/unit/default_contacts_test.js:32)

Comment 30

[Borja Salguero [:borjasalguero]]

Comment on attachment 807675 [details]
Patch

Updated with your comments! r?

Comment 31

[Salvador de la Puente González [:salva]]

Comment on attachment 807675 [details]
Patch

The patch looks very well: not only it's increasing the test coverage but the new file format is more machine-friendly, we removed the embedded files and simplify the Customizers architecture by gathering all the resource load functionality in a simple base class.

Thank you very much for all your efforts.

Comment 32

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 807675 [details]
Patch

it would be great if you can create a new javascript module or add functions into utils.js in GAIA_DIR/build/ and use it in webapps-zip.js to implement this feature instead of adding more features into variant.py and then we don't need to have a temp directory in distribution dir.

you can reference |utils| module in GAIA_DIR/build.

Comment 33

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

and for long term we will re-write all python build script to javascript, so implement this in javascript is better.

Comment 34

[Albert [:albert]]

(In reply to Yuren Ju [:yurenju] from comment #33)
> and for long term we will re-write all python build script to javascript, so
> implement this in javascript is better.

I started writing it in javascript, but there are a lot of problems with the XHR object downloading remote manifests and apps, so I moved to python.

Comment 35

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

so currently we have two major features in varint.py (local-apps.py):
1. preload apps from internet
2. read configuration file and copy resource files to temp folder for adding into zip file

we can keep local-apps.py and only implement (2) in javascript. what do you think for this?

Comment 36

[Albert [:albert]]

(In reply to Yuren Ju [:yurenju] from comment #35)
> so currently we have two major features in varint.py (local-apps.py):
> 1. preload apps from internet
> 2. read configuration file and copy resource files to temp folder for adding
> into zip file
> 
> we can keep local-apps.py and only implement (2) in javascript. what do you
> think for this?

Ok. The main problem with point 1 is dealing with certificates of remote sites.

Comment 37

[Albert [:albert]]

Anyway, point 1 still needs to keep a temp dir to store downloaded apps. In case there is no connection the ones in temp will be used

Comment 38

[Albert [:albert]]

Comment on attachment 807675 [details]
Patch

Changes from comment 35

Comment 39

[Jason Smith [:jsmith]]

Blocking- - it contains enhancements & is a risky patch, so we've decided this should go through approval. If it's safe, we'll consider it for uplift. If it needs to ride the trains, then the blocking issues that this bug is fixing will need to be fixed in a separate patch.

Comment 40

[Borja Salguero [:borjasalguero]]

As we discussed during the triage, we are going to wait until having all R+ from our reviewers, merge this in master and ask for approval.

Comment 41

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 807675 [details]
Patch

This is looking good guys! Great job. It also ensures that all config files are loaded from the distribution directory. And looks like a very safe patch to land also!

Comment 42

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

got build error by |make| without GAIA_DISTRIBUTION_DIR variable. 

> Exception: Error: Single variant config file /home/yurenju/src/gaia/distribution/variant.json not found
> getSingleVariantResources@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:212
> execute/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:306
> makeWebappsObject/<.forEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/utils.js:175
> makeWebappsObject/<.forEach@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/utils.js:117
> execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:250

see also bug 917783 and bug 919377

Comment 43

[Albert [:albert]]

(In reply to Yuren Ju [:yurenju] from comment #42)
> got build error by |make| without GAIA_DISTRIBUTION_DIR variable. 
> 
> > Exception: Error: Single variant config file /home/yurenju/src/gaia/distribution/variant.json not found
> > getSingleVariantResources@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:212
> > execute/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:306
> > makeWebappsObject/<.forEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/utils.js:175
> > makeWebappsObject/<.forEach@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/utils.js:117
> > execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:250
> 
> see also bug 917783 and bug 919377

The problem is that Makefile defines distributionDir but doesn't check if it exists. Patch fixed and rebased.

Comment 44

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

can't build with GAIA_DISTRIBUTION_DIR variable. steps:

1. clone firefoxos-gaia-spain and cp -r sample/* to root directory of firefoxos-gaia-spain
2. GAIA_DISTRIBUTION_DIR=<path-of-firefoxos-gaia-spain> make

> run-js-command  webapp-zip
> Exception: Error: Invalid single variant configuration: /home/yurenju/src/firefoxos-gaia-spain/resources/contacts-movistar.json not found
> getResource@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:199
> getSingleVariantResources/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:218
> getSingleVariantResources@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:213
> execute/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:304
> makeWebappsObject/<.forEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/utils.js:175
> makeWebappsObject/<.forEach@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/utils.js:117
> execute@resource://gre/modules/commonjs/toolkit/loader.js -> file:///home/yurenju/src/gaia/build/webapp-zip.js:245
> @-e:1
> 
> make: *** [webapp-zip] Error 3

Comment 45

[Salvador de la Puente González [:salva]]

(In reply to Yuren Ju [:yurenju] from comment #44)
> can't build with GAIA_DISTRIBUTION_DIR variable. steps:
> 
> 1. clone firefoxos-gaia-spain and cp -r sample/* to root directory of
> firefoxos-gaia-spain

AFAIK, this is unnecessary. 

> 2. GAIA_DISTRIBUTION_DIR=<path-of-firefoxos-gaia-spain> make

I tried with a production build:
MOZILLA_OFFICIAL=1 GAIA_DISTRIBUTION_DIR=<DIST_DIR> make production

In my case, the device did not start (system app crashed) so I had to remove power/carrier_power_on.mp4 file.

Hope it helps!

Comment 46

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

okay let me try again.

Comment 47

[Albert [:albert]]

(In reply to Salvador de la Puente González [:salva] from comment #45)
> (In reply to Yuren Ju [:yurenju] from comment #44)
> > can't build with GAIA_DISTRIBUTION_DIR variable. steps:
> > 
> > 1. clone firefoxos-gaia-spain and cp -r sample/* to root directory of
> > firefoxos-gaia-spain
> 
> AFAIK, this is unnecessary. 
> 
> > 2. GAIA_DISTRIBUTION_DIR=<path-of-firefoxos-gaia-spain> make
> 
> I tried with a production build:
> MOZILLA_OFFICIAL=1 GAIA_DISTRIBUTION_DIR=<DIST_DIR> make production
> 
> In my case, the device did not start (system app crashed) so I had to remove
> power/carrier_power_on.mp4 file.
> 
> Hope it helps!

No Salva, this is another problem.

It was a mistake in variant.json at firefox-gaia-spain, I launch a pr to fix it one week ago to fix it. Now it is landed, if you update your firefox-gaia-spain it will work.

Comment 48

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

ok success now, I'm testing on windows, if everything is ok I will give r+ for this PR :-)

Comment 49

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 807675 [details]
Patch

succuss build on windows, r=yurenju. thank you Albert for contributing to this bug! and please land this commit if you get green state on travis-ci.

https://travis-ci.org/mozilla-b2g/gaia/builds/11858720

Comment 50

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

BTW, I filed a follow up bug for rewrite variant.py in javascript - bug 921417, I'll take time to implement it.

Comment 51

[Albert [:albert]]

Great, thanks Yuren. I'll watch your progress with bug 921417.(In reply to Yuren Ju [:yurenju] from comment #50)
> BTW, I filed a follow up bug for rewrite variant.py in javascript - bug
> 921417, I'll take time to implement it.

Great, thanks Yuren. I'll watch your progress with bug 921417.

Comment 52

[Albert [:albert]]

Master: 8c7cefa4ae4d1f3d044296fcedbbadaee1cdc7dd

Comment 53

[Borja Salguero [:borjasalguero]]

Awesome job here! Thanks all for your support with this bug :). 
This patch has been reviewed & tested by a bunch of people, and it's fully covered by unit-testing, so the risk is quite low. Actually, this patch fixes a lot of problems of the previous architecture, that's why I would move forward to have a robust solution and land this for v1.2 (we don't want to deliver with a non-working single-customization variant! ;) ). That's why we are going to ask for approval!
Gregor, could you take a look about the approval process? Thanks! Gracias :)

Comment 54

[Borja Salguero [:borjasalguero]]

Comment on attachment 807675 [details]
Patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Single-variant architecture currently implemented.
[User impact] if declined: Currently some features of single-variant are not working properly (Wallpaper for example) and the architecture of the current single-variant implementation based on add base64 content to the configuration file is quite risky (actually it's not working as expected), and needs to be fixed.
[Testing completed]: This patch was reviewed & tested by bunch of peers, and it's fully covered by unit-test.
[Risk to taking this patch] (and alternatives if risky): No risk. Actually this patch is going to fix a lot of issues, and we are going to have a robust architecture which will prevent more bugs in the future.
[String changes made]:

Comment 55

[Jason Smith [:jsmith]]

(In reply to Borja Salguero [:borjasalguero] from comment #54)
> Comment on attachment 807675 [details]
> Patch
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): Single-variant architecture
> currently implemented.
> [User impact] if declined: Currently some features of single-variant are not
> working properly (Wallpaper for example) and the architecture of the current
> single-variant implementation based on add base64 content to the
> configuration file is quite risky (actually it's not working as expected),
> and needs to be fixed.
> [Testing completed]: This patch was reviewed & tested by bunch of peers, and
> it's fully covered by unit-test.

If need be, I can take a look at this on master to assess this more to determine approval.

> [Risk to taking this patch] (and alternatives if risky): No risk. Actually
> this patch is going to fix a lot of issues, and we are going to have a
> robust architecture which will prevent more bugs in the future.

This is not an accurate approval assessment. A patch like what's seen here definitely has risk if you are modifying the architecture. Please assess tradeoffs here a bit clearly to determine if approval is acceptable.

> [String changes made]:

This needs more information.

Comment 56

[Salvador de la Puente González [:salva]]

Comment on attachment 807675 [details]
Patch

We are asking for approval here since this patches introduces a lot of benefits in the current solution. See details below. Thank you.

[Bug caused by] (feature/regressing bug #): bug 891729

[User impact] if declined: -medium-
Embedded files can introduce some unexpected behaviors and are an error source. If modified by error, the application can crash in exotic ways.

Furthermore, the user has less amount of free space due to the overhead introduced by multiple configuration files and the base64 encoding.

[Testing completed]: -yes-
As Borja said, the patch was reviewed by 2 people, :yurenju and myself and :amac, :reuben and :mai have provided feedback+. :mai and me wrote the original architecture in bug 891729 with advice of :arcturus.

Moreover, test coverage has been improved including new tests for event dispatching in variant [1] and the new Resource loader [2]. The inclusion of the new class Customizer [3] increases isolation and code reuse at the same time it decreases the number of tests needed due to the separation of responsibilities [4]. Therefore, some other tests suites have lost some unnecessary tests and improve the existent ones. They suffer of testing the mockup which is useless, now they are checking the proper call are made [5], [6] and [7]

[1] https://github.com/mozilla-b2g/gaia/pull/12327/files#diff-22
[2] https://github.com/mozilla-b2g/gaia/pull/12327/files#diff-20
[3] https://github.com/mozilla-b2g/gaia/pull/12327/files#diff-3
[4] https://github.com/mozilla-b2g/gaia/pull/12327/files#diff-11
[5] https://github.com/mozilla-b2g/gaia/pull/12327/files#diff-12
[6] https://github.com/mozilla-b2g/gaia/pull/12327/files#diff-21
[7] https://github.com/mozilla-b2g/gaia/pull/12327/files#diff-23

[Risk to taking this patch] (and alternatives if risky): -low-

Despite a lot of files has been modified in this patch, the main architecture remains unchanged. Note this consists in a publisher/subscriber pattern where variant manager is in charge of loading a configuration file with customization pairs (setting, value) and to publish a message by setting. There are a set of subscribers to these events known as Customizers.

The only differences are:
 - Extract resources (formerly embedded in base64) from the config file --> with the consequent maintainability improvement.
 - Unify the configuration file -> with the consequent build process simplification [8]
 - Modify the customizers to handle references and not data -> with the improvements mentioned in the former section about avoiding embedded resources.

Without testing coverage, the risk of the patch could be -medium- as it has been exhaustively and manually checked and double checked but with the addition of unit testing, we can ensure the risk is low and a great improvement for further developments on this side.

[String changes made]: -none-

No l10n nor i18n strings have change at all.

Comment 57

[Salvador de la Puente González [:salva]]

Hello Jason, I updated the analysis of the patch in comment 56. Can you consider it again. I tried to provide well funded reasons so the explanation has end a little bit extensive.

Thank you very much.

Comment 58

[Salvador de la Puente González [:salva]]

Sorry, I missed the [8] reference. It is not seen in the patch because, without this, we still need those modifications to be added in the building stage so, consequently, without this patch, the build process will become more complicated.

Comment 59

[Jason Smith [:jsmith]]

Thanks for the update - that's a clearer approval analysis. This should help Gregor figure out if we can take this or not.

Comment 60

[Gregor Wagner [:gwagner]]

Comment on attachment 807675 [details]
Patch

Lets get it in.
Thanks everyone!

Comment 61

[James Lal [:lightsofapollo]]

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 8c7cefa4ae4d1f3d044296fcedbbadaee1cdc7dd
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 62

[Albert [:albert]]

Uplift to 1.2 fbe90e65cc922fc2966be5ec200bef6a37140ab6

Comment 63

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Albert [:albert] from comment #62)
> Uplift to 1.2 fbe90e65cc922fc2966be5ec200bef6a37140ab6

Please remember to set the status flag when uplifting.

Comment 64

[Albert [:albert]]

(In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC from comment #63)
> (In reply to Albert [:albert] from comment #62)
> > Uplift to 1.2 fbe90e65cc922fc2966be5ec200bef6a37140ab6
> 
> Please remember to set the status flag when uplifting.

Ok, sorry.