879680 - (mmi-result-cf) [MMI] Use MMIResult for Call Forwarding related functionality

Status: RESOLVED FIXED

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

Description

[bov]

Steps    
- Settings, Language, Set to any language except english    
- Open Dialer App, type "*21#, and click on the green dialer button

Actual Result:
As you didn't register first, the system detects an erroneous workflow and returns an error message, but this is written in english, not in the selected language in Settings.
Tested on Unagi device. Build :Gecko-52341e4.Gaia-71c3bcd CommRIL:AU105
Result:"Call forwarding generic failure"
Tested on Unagi device. Build :Gecko-80fd2ae.Gaia-0a1cf9f CommRIL:AU115
Result:"GenericFailure"


Expected Result:
The error message should be shown in the selected language in Settings

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

These look like issues partners will care about having fixed for releasing in target markets - Michael seems related to 879032 - thoughts?

Comment 2

[David Scravaglieri [:scravag]]

Partner think it's a blocker

Comment 3

[David Scravaglieri [:scravag]]

Partner think it's a blocker

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gecko part - WIP

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gaia part - WIP

Comment 8

[Anshul]

Comment on attachment 768036 [details] [diff] [review]
Gecko part - WIP

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

::: dom/system/gonk/RILContentHelper.js
@@ +1594,5 @@
> +        if (message.success) {
> +           // MMI query call forwarding options request returns a set of rules
> +	   // that will be exposed in the form of an array of
> +	   // nsIDOMMozMobileCFInfo instances.
> +          if (message.rules) {

While we are on it, can we change ril code to send message.result instead of message.rules just like every other MMI code result to be consistent?

Comment 9

[Anshul]

Comment on attachment 768036 [details] [diff] [review]
Gecko part - WIP

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

::: dom/system/gonk/RILContentHelper.js
@@ +1600,5 @@
> +            result.statusMessage = message.rules;
> +          } else {
> +            // MMI set call forwaring option request doesn't return anything
> +            // rather than success in case of no erros.
> +            result.statusMessage = RIL.MMI_KS_SM_CALL_FORWARDING_SET_SUCCESS;

Also instead of setting a default RIL.MMI_KS_SM_CALL_FORWARDING_SET_SUCCESS message, you should read the statuMessage code sent by RIL (RIL needs to send a MMI status code such as smServiceEnabled from https://bugzilla.mozilla.org/attachment.cgi?id=765428&action=diff)

::: dom/system/gonk/ril_consts.js
@@ +2510,5 @@
>  MMI_SC_TO_CF_REASON[MMI_SC_CF_ALL] = CALL_FORWARD_REASON_ALL_CALL_FORWARDING;
>  MMI_SC_TO_CF_REASON[MMI_SC_CF_ALL_CONDITIONAL] = CALL_FORWARD_REASON_ALL_CONDITIONAL_CALL_FORWARDING;
>  
> +// MMI status message
> +this.MMI_KS_SM_CALL_FORWARDING_SET_SUCCESS = "smCallForwardingSetSuccess";

No need for this new one, you can use smServiceEnabled along with service code sent to gaia should be sufficient for user to understand that call forwarding is successfully enabled.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gecko part - v1

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gaia part - v1

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 768325 [details] [diff] [review]
Gecko part - v1

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

::: dom/system/gonk/ril_consts.js
@@ +2570,5 @@
> +this.MMI_SM_KS_SERVICE_ENABLED = "smServiceEnabled";
> +this.MMI_SM_KS_SERVICE_DISABLED = "smServiceDisabled";
> +this.MMI_SM_KS_SERVICE_REGISTERED = "smServiceRegistered";
> +this.MMI_SM_KS_SERVICE_ERASED = "smServiceErased";
> +this.MMI_SM_KS_SERVICE_INTERROGATED = "smServiceInterrogated";

Actually `smServiceInterrogated` key is not need at all. We could delete it and use `smServiceEnabled` key instead.

Comment 13

[Anshul]

Comment on attachment 768325 [details] [diff] [review]
Gecko part - v1

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

::: dom/system/gonk/RILContentHelper.js
@@ +1635,5 @@
>        case RIL.MMI_KS_SC_CALL_FORWARDING:
> +        if (message.success) {
> +          switch (message.action) {
> +            case RIL.CALL_FORWARD_ACTION_ENABLE:
> +              result.statusMessage = RIL.MMI_SM_KS_SERVICE_ENABLED; 

Antonio, it seems like for every MMI code RIL returns data in a different format which is very inconsistent and prone to bugs. What do you think about my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=874000#c22

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 768329 [details] [diff] [review]
Gaia part - v1

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

Can you follow the approach suggested by Anshul, please?

Thanks!

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gecko part - v2

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 768871 [details] [diff] [review]
Gecko part - v2

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

I guess this version address the comments from Anshul. If not, please let me know.

::: dom/system/gonk/RILContentHelper.js
@@ +1582,5 @@
> +    if (message.mmiServiceCode === RIL.MMI_KS_SC_CALL_FORWARDING &&
> +        message.rules) {
> +      message.additionalInformation =
> +        this._cfRulesToMobileCfInfo(message.additionalInformation);
> +    }

Sadly we have to so this here. Am I right?

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 768871 [details] [diff] [review]
Gecko part - v2

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

Thanks jaoo :)! This looks good to me. However I am not a RIL peer, so you'll need to get an r+ from vicamo too.

::: dom/system/gonk/ril_consts.js
@@ +260,5 @@
>  this.GECKO_ERROR_SS_MODIFIED_TO_SS = "SsModifiedToSs";
>  this.GECKO_ERROR_SUBSCRIPTION_NOT_SUPPORTED = "SubscriptionNotSupported";
>  this.GECKO_ERROR_INVALID_PARAMETER = "InvalidParameter";
>  this.GECKO_ERROR_REJECTED_BY_REMOTE = "RejectedByRemote";
> +this.GECKO_ERROR_NO_CALL_FORWARD_STATUS = "NoCallForwardStatus";

Is this an standard RIL error? Can you just return 'GenericError' in this case?

::: dom/system/gonk/ril_worker.js
@@ +4850,5 @@
>        rulesLength = Buf.readUint32();
>      }
>      if (!rulesLength) {
>        options.success = false;
> +      options.errorMsg = GECKO_ERROR_NO_CALL_FORWARD_STATUS;

GenericError?

@@ +4884,5 @@
>      }
> +    if (options.success && options.isSendMMI) {
> +      switch (options.action) {
> +        case CALL_FORWARD_ACTION_ENABLE:
> +          options.statusMessage = MMI_SM_KS_SERVICE_ENABLED; 

nit: trailing white space in all the switch cases.

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gecko part - v3

Thanks ferjm!

ferjm's comment addressed. Carrying out r=ferjm.

Requesting review at Vicamo since ferjm is not a RIL peer as he has commneted.

Anshul, your feedback is welcome.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gaia part - v2

Gaia side rebased.

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 768931 [details] [diff] [review]
Gaia part - v2

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

::: apps/communications/dialer/js/mmi.js
@@ +163,4 @@
>          }
>          break;
>        case 'scCallForwarding':
> +        if (mmiResult.statusMessage) {

I guess we always expect an statusMessage, can we have an else here showing a GenericError if no statusMessage is received, please? It's quite unlikely, but it is not good to show an empty screen.

::: apps/communications/dialer/locales/dialer.en-US.properties
@@ +100,4 @@
>  smServiceDisabled=Service has been disabled
>  smServiceRegistered=Registration was successful
>  smServiceErased=Erasure was successful
> +smServiceInterrogated=Resquest was successful

Typo: Request

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Gaia part - v3

ferjm's comments addressed.

Carrying out r=ferjm

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/7f3e1fa0204afc369e91fe3f619388781a2279fb

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

The Gecko patch depends on that of bug 874000.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

http://hg.mozilla.org/projects/birch/rev/16a6252d6d68

Comment 25

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #24)
> http://hg.mozilla.org/projects/birch/rev/16a6252d6d68

Thanks, you were faster than me! I was trying to push when I noticed you did it!

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/16a6252d6d68

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7ba32937ac32

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Backed out from b2g18 due to chrome tests failure.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2d9072f92b52

I won't be able to fix it until tomorrow as I am taking a 20 hours flight.

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Re-pushed to b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/cecdee9d3b6d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/cecdee9d3b6d

Comment 31

[Carol Yang [:cyang]]

This change also needs to be uplifted to v1-train please.

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

v1-train: f053424f88e89bdab874d074482d0eb0c2be9d39
v1.1hd: ef6c723dab955ae18d0e324a06558e78c8eb9a1b

Comment 33

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

v1.1.0hd: f053424f88e89bdab874d074482d0eb0c2be9d39