Closed Bug 978785 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: sonmarce, Assigned: macajc)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 5 obsolete files)

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
Whiteboard: [systemsfe]
Blocks: 980301
No longer blocks: 2.0-systems-fe
Attached patch WIP proposed patch v1 - gecko (obsolete) — Splinter Review
Attached file WIP proposed patch v1 - gaia (obsolete) —
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)
Attachment #8387995 - Flags: feedback?(gene)
Attachment #8387995 - Flags: feedback?(fabrice)
Attachment #8388002 - Flags: feedback?(borja.bugzilla)
Attachment #8387995 - Flags: feedback?(gene) → feedback?(gene.lian)
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.
Attachment #8387995 - Flags: feedback?(fabrice)
(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
Attachment #8387995 - Attachment is obsolete: true
Attachment #8387995 - Flags: feedback?(gene.lian)
Assignee: nobody → cjc
Blocks: 983109
Blocks: 983124
Blocks: 983126
Blocks: 983128
Blocks: 983130
Blocks: 983132
Blocks: 983134
Attached file Proposed patch v2 - gaia (obsolete) —
Attachment #8390642 - Flags: review?(borja.bugzilla)
Attachment #8388002 - Attachment is obsolete: true
Attachment #8388002 - Flags: feedback?(borja.bugzilla)
Attachment #8389217 - Flags: review?(fabrice)
Attachment #8389217 - Flags: review?(gene.lian)
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.
Attachment #8389217 - Flags: review?(gene.lian) → review+
Attachment #8389217 - Flags: review?(fabrice) → review+
Blocks: 978783
(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
Blocks: 983668
Blocks: 983677
No longer blocks: 983130
No longer blocks: 983134
Blocks: 983679
Blocks: 983680
Blocks: 983683
Attached file Proposed patch v2 - gaia (obsolete) —
Attachment #8390642 - Attachment is obsolete: true
Attachment #8390642 - Flags: review?(borja.bugzilla)
Attachment #8392093 - Flags: review?(borja.bugzilla)
The try run looks good, requesting checkin
Keywords: checkin-needed
Removing checkin-needed until 1.4 has been forked, this should not go into 1.4
Keywords: checkin-needed
requesting checkin again
Keywords: checkin-needed
Target Milestone: --- → 1.4 S4 (28mar)
https://hg.mozilla.org/integration/b2g-inbound/rev/b5f9e9baf73c

[leave open] for the Gaia patch
Keywords: checkin-needed
Whiteboard: [systemsfe] → [systemsfe][leave open]
Blocks: 976011
Blocks: 923443
Blocks: 923444
Blocks: 923446
Blocks: 978781
Blocks: 923452
No longer blocks: 983668
No longer blocks: 983677
Depends on: 986446
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!
Attachment #8392093 - Flags: feedback+
Gecko issue is tracked at bug 986446
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.
Attachment #8392093 - Flags: review?(borja.bugzilla) → review+
https://github.com/mozilla-b2g/gaia/commit/73c3a1305da4a949c1634a009c400bc2431848d2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [systemsfe][leave open] → [systemsfe]
Target Milestone: 1.4 S4 (28mar) → 1.5 S1 (9may)
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.
This app doesn't have any kind of user interaction, so it doesn't really need a locale.
(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.
Attached file Proposed patch v3 - gaia (obsolete) —
Attachment #8392093 - Attachment is obsolete: true
Attachment #8396542 - Flags: review?(borja.bugzilla)
Attachment #8396542 - Flags: review?(borja.bugzilla)
Attachment #8396542 - Attachment is obsolete: true
Target Milestone: 1.5 S1 (9may) → 1.4 S5 (11apr)
Attachment #8401737 - Flags: review?(borja.bugzilla)
Some comments in GH. Please take a looK!
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!
Attachment #8401737 - Flags: review?(borja.bugzilla)
(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
Attachment #8401737 - Flags: review?(borja.bugzilla)
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!
Attachment #8401737 - Flags: review?(borja.bugzilla) → review+
https://github.com/mozilla-b2g/gaia/commit/b66d0745a331821cb6ea172f717c78e1f732a044
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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).
Blocks: 995900
Blocks: 996048
Flags: in-moztrap?(rafael.marquez)
See Also: → 1053826
You need to log in before you can comment on or make changes to this bug.