978785 - Single variant: Enable customization during first time to insert a SIM card

Status: RESOLVED FIXED

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

Description

[Marcelino Veiga Tuimil [:sonmarce]]

We need to implement a general mechanism to allow customization for each country only the first time a SIM card is inserted, this way can reuse it for all single variant customizations with this behaviour

Comment 1

[Carmen Jimenez Cabezas]

Attachment: WIP proposed patch v1 - gecko

Comment 2

[Carmen Jimenez Cabezas]

Attachment: WIP proposed patch v1 - gaia

Comment 3

[Carmen Jimenez Cabezas]

Currently the single variant customization is linked to the FTU, when what we really want to do is to execute it the first time a SIM is used. With the current implementation we could start the phone without a SIM, pass the FTU, and no customization will be done. Also, adding new configurations increases the FTU size unnecessarily.

For this reason, I've moved all the single variant related customization to an independent application which only functionality will be to run the single variant customization (once it finishes its jobs it can even uninstall itself, and it'll be automatically reinstated on a hard reset)

Currently, Gecko detects the first time a SIM is used, and launches the single variant apps installation. With the change proposed it will also notify Gaia of this situation (first time a SIM is used) launching a System Message. The previously mentioned configuration app will register for this message, catch if when it's launched and execute the needed configurations.

The proposed patch does all this work. In gecko a new system message is defined (variant-customization). The Operator App component in Gecko will launch this message when it detects a first time usage of a SIM. There's a new Gaia app that register for this message and when it's received it'll execute all the configured customizations. There's no need to have IccHelper on this app (nor to have it running) since the System Message will launch it when needed, and Gecko will pass the mcc-mnc through the message (this also makes it easier to make the needed change if the behavior for DSDS is changed since it'll only have to be changed at one place)

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8387995 [details] [diff] [review]
WIP proposed patch v1 - gecko

Review of attachment 8387995 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/OperatorApps.jsm
@@ +61,5 @@
> +
> +function launchSingleVariantConf(aMcc, aMnc) {
> +  debug("Broadcast message variant-customization " + aMcc + "-" + aMnc);
> +  let messenger = Cc["@mozilla.org/system-message-internal;1"]
> +        .getService(Ci.nsISystemMessagesInternal);

nit: align .getService wit ["@mozilla...."]

@@ +62,5 @@
> +function launchSingleVariantConf(aMcc, aMnc) {
> +  debug("Broadcast message variant-customization " + aMcc + "-" + aMnc);
> +  let messenger = Cc["@mozilla.org/system-message-internal;1"]
> +        .getService(Ci.nsISystemMessagesInternal);
> +  messenger.broadcastMessage('variant-customization', { mcc: aMcc, mnc: aMnc });

nit: double quotes in this file.

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +115,5 @@
>    },
>    "wifip2p-pairing-request": { },
> +  "variant-customization": {
> +    "settings": ["read", "write"]
> +  }

1) The system message should not be named "variant-customization": this is the action you will do, but the trigger is more "new-sim-inserted" or something like that.

2) Why does that requires the settings permission? The only benefit I can see is that it prevent non certified apps to get access to the mcc/mnc.

Comment 5

[Carmen Jimenez Cabezas]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Comment on attachment 8387995 [details] [diff] [review]
> WIP proposed patch v1 - gecko
> 
> Review of attachment 8387995 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/messages/SystemMessagePermissionsChecker.jsm
> @@ +115,5 @@
> >    },
> >    "wifip2p-pairing-request": { },
> > +  "variant-customization": {
> > +    "settings": ["read", "write"]
> > +  }
> 
> 2) Why does that requires the settings permission? The only benefit I can
> see is that it prevent non certified apps to get access to the mcc/mnc.

Exactly that, it avoids non certified app to get the mcc/mnc. There's other reason to do this (and it makes more sense with the current name of the message ;)) which is that the normal processing of that message will require changes of settings.
I don't really want to add a new permission for this, so I want to link this to a certified only permission. Settings seems like the natural choice for this

Comment 6

[Carmen Jimenez Cabezas]

Attachment: WIP Proposed patch v2 - gecko

Comment 7

[Carmen Jimenez Cabezas]

Comment on attachment 8388002 [details]
WIP proposed patch v1 - gaia

>https://github.com/mcjimenez/gaia/commit/41f2e1041a8c02a33dca82fc49111c48a750e152

Comment 8

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v2 - gaia

Comment 9

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8389217 [details] [diff] [review]
WIP Proposed patch v2 - gecko

Review of attachment 8389217 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/src/OperatorApps.jsm
@@ +86,5 @@
> +      debug("Broadcast message first-run-with-sim");
> +      let messenger = Cc["@mozilla.org/system-message-internal;1"]
> +                        .getService(Ci.nsISystemMessagesInternal);
> +      messenger.broadcastMessage("first-run-with-sim", { mcc: mcc,
> +                                                         mnc: mnc });

I assume all of this logic will only be triggered one time for the first run, because they seem to be under the check of isFirstRunWithSIM(). Right? Just double check, we don't want to fire system message to trigger the app more than one time.

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +114,5 @@
>      "settings": ["read", "write"]
>    },
>    "wifip2p-pairing-request": { },
> +  "first-run-with-sim": {
> +    "settings": ["read", "write"]

Yay, this permission makes sense to me, since "icc-stkcommand" works in a similar way.

Comment 10

[Carmen Jimenez Cabezas]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #9)
> Comment on attachment 8389217 [details] [diff] [review]
> WIP Proposed patch v2 - gecko
> 
> Review of attachment 8389217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/OperatorApps.jsm
> @@ +86,5 @@
> > +      debug("Broadcast message first-run-with-sim");
> > +      let messenger = Cc["@mozilla.org/system-message-internal;1"]
> > +                        .getService(Ci.nsISystemMessagesInternal);
> > +      messenger.broadcastMessage("first-run-with-sim", { mcc: mcc,
> > +                                                         mnc: mnc });
> 
> I assume all of this logic will only be triggered one time for the first
> run, because they seem to be under the check of isFirstRunWithSIM(). Right?
> Just double check, we don't want to fire system message to trigger the app
> more than one time.
> 

Yes, the message is sent only the first time a SIM is detected

Comment 11

[Carmen Jimenez Cabezas]

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

Comment 12

[Carmen Jimenez Cabezas]

Comment on attachment 8390642 [details]
Proposed patch v2 - gaia

https://github.com/mozilla-b2g/gaia/pull/17164

Comment 13

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v2 - gaia

Comment 14

[Carmen Jimenez Cabezas]

The try run looks good, requesting checkin

Comment 15

[Carmen Jimenez Cabezas]

Removing checkin-needed until 1.4 has been forked, this should not go into 1.4

Comment 16

[Carmen Jimenez Cabezas]

requesting checkin again

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b5f9e9baf73c

[leave open] for the Gaia patch

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5f9e9baf73c

Comment 19

[Borja Salguero [:borjasalguero]]

Comment on attachment 8392093 [details] [review]
Proposed patch v2 - gaia

Waiting to getting more info about why the customizer of Wifi is not working as expected (Seems to be a Gecko issue). Some comments in Github but the code looks great! Let me know when this is ready to review again and I'll take a look!

Comment 20

[Carmen Jimenez Cabezas]

Gecko issue is tracked at bug 986446

Comment 21

[Borja Salguero [:borjasalguero]]

Comment on attachment 8392093 [details] [review]
Proposed patch v2 - gaia

Great job Carmen!!! Working as a charm. Thanks a lot! Wait to Travis and merge when green.

Comment 22

[Carmen Jimenez Cabezas]

https://github.com/mozilla-b2g/gaia/commit/73c3a1305da4a949c1634a009c400bc2431848d2

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Reverted for Gaia unit test failures on TBPL.
https://github.com/mozilla-b2g/gaia/commit/3ca14b15e945c8e94a6b80c3aff7ea814438e6bf

https://tbpl.mozilla.org/php/getParsedLog.php?id=36618210&tree=B2g-Inbound

Comment 24

[Axel Hecht [:Pike]]

Is the manifest information in this app supposed to be localized? Then it'll need to be marked up differently, notably, you'll need a locales key like in https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/manifest.webapp#L24, the key point being a 'en-US' key in there.

Comment 25

[Carmen Jimenez Cabezas]

This app doesn't have any kind of user interaction, so it doesn't really need a locale.

Comment 26

[Carmen Jimenez Cabezas]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #23)
> Reverted for Gaia unit test failures on TBPL.
> https://github.com/mozilla-b2g/gaia/commit/
> 3ca14b15e945c8e94a6b80c3aff7ea814438e6bf
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36618210&tree=B2g-Inbound

I'm sorry about that, Travis didn't give any errors (nor did the locally ran tests). I'll check tomorrow what's making the tests fail on TBPL.

Comment 27

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v3 - gaia

Comment 28

[Carmen Jimenez Cabezas]

Attachment: Proposed patch v3 - gaia

Comment 29

[Borja Salguero [:borjasalguero]]

Some comments in GH. Please take a looK!

Comment 30

[Borja Salguero [:borjasalguero]]

Comment on attachment 8401737 [details] [review]
Proposed patch v3 - gaia

Some comments in Gaia to be fixed. We are quite close to land it! Take a look and ask me again to review this when ready!

Comment 31

[Carmen Jimenez Cabezas]

(In reply to Borja Salguero [:borjasalguero] from comment #30)
> Comment on attachment 8401737 [details] [review]
> Proposed patch v3 - gaia
> 
> Some comments in Gaia to be fixed. We are quite close to land it! Take a
> look and ask me again to review this when ready!

I've done the requested changes and update de PR. Can you take another look? Thanks

Comment 32

[Borja Salguero [:borjasalguero]]

Comment on attachment 8401737 [details] [review]
Proposed patch v3 - gaia

Great job Carmen! R+ so ready to merge.

As during the tests on the device we found the following issue in Homescreen:

E/GeckoConsole( 2191): Content JS ERROR at app://homescreen.gaiamobile.org/js/configurator.js:144 in loadSVConfFileError: Failed parsing singleVariant configuration file [js/singlevariantconf.json]:  [Exception... "File error: Not found"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: app://homescreen.gaiamobile.org/js/configurator.js :: loadFile :: line 70"  data: no]

Could you open a bug for that? Thanks!

Comment 33

[Carmen Jimenez Cabezas]

https://github.com/mozilla-b2g/gaia/commit/b66d0745a331821cb6ea172f717c78e1f732a044

Comment 34

[Carmen Jimenez Cabezas]

That is not an unexpected error, and it's not related with this patch. 
That shows when the configuration file for the singleVariant homescreen grid does not exist. 
In any case, I'm going to talk with Cristian about changing it so it's a less boisterous
log instead of an error (or we can just not log it at all).