874000 - (mmi-result-pin) [MMI] Use MMIResult for PIN/PIN2/PUK related functionality

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P2)

Description

[Fernando Campo (:fcampo)]

Followup created after bug 860660, comment 105

Comment 1

[Daniel Coloma:dcoloma]

Bug 860660 was tef+ and has been partially fixed as tef agreed with fixing the critical part for 1.0.1 and left the rest for 1.1. Hence I am nominating this for 1.1 to complete the remaining work.

Comment 2

[xiaokang.chen]

(In reply to Daniel Coloma:dcoloma from comment #1)
> Bug 860660 was tef+ and has been partially fixed as tef agreed with fixing
> the critical part for 1.0.1 and left the rest for 1.1. Hence I am nominating
> this for 1.1 to complete the remaining work.
So this bug will not be fixed in v1.01? Thanks!

Comment 3

[Anshul]

Fernando, the PIN Change message is coming from comercial RIL. I am stealing this bug from you for now. I hope that's okay.

Comment 4

[Daniel Coloma:dcoloma]

(In reply to xiaokang.chen from comment #2)
> (In reply to Daniel Coloma:dcoloma from comment #1)
> > Bug 860660 was tef+ and has been partially fixed as tef agreed with fixing
> > the critical part for 1.0.1 and left the rest for 1.1. Hence I am nominating
> > this for 1.1 to complete the remaining work.
> So this bug will not be fixed in v1.01? Thanks!

Exactly

Comment 5

[Fernando Campo (:fcampo)]

Hi Anshul, any updates on this?

Comment 6

[Anshul]

Carol will look into this issue in a couple of days and get back.

Comment 7

[Alex Keybl [:akeybl]]

Mozilla triage recommends not blocking v1.1 on this bug.

Comment 8

[Daniel Coloma:dcoloma]

Carol, any update about this bug?

Comment 9

[Carol Yang [:cyang]]

Hi Daniel,

I have a fix for this on Commericial RIL, however, it is pending on a bigger change which will localize all MMI strings: https://bugzilla.mozilla.org/show_bug.cgi?id=879032.

Once that change is in, then this one will be subsequently fixed.

Comment 10

[Daniel Coloma:dcoloma]

Carol, I am marking this as POVB as it seems it only requires development on your side... Please correct me I am wrong

Comment 11

[Carol Yang [:cyang]]

(In reply to Daniel Coloma:dcoloma from comment #10)
> Carol, I am marking this as POVB as it seems it only requires development on
> your side... Please correct me I am wrong

Yup, fix will be in Commericial RIL.

Comment 12

[Michael Vines [:m1] [:evilmachines]]

Just to be clear though.  This bug depends on bug 879032, which would be a change to Gecko.

Comment 13

[Fernando Campo (:fcampo)]

Michael, I think that this could be related with bug 879032, but is not really depending on it. One thing is that the retryCount is given back on the error message, and another different thing is that the message comes back translated.
Right now we have a gaia workaround for some of the translation, and it would be nice to get rid of, but I don't consider it a blocker.

Comment 14

[Noemí Freire (:noemi)]

Hi,

Since we need this patch (retryCount info to be accesible through mmi codes) to be implemented in Commercial RIL for v1.1 we nominate this issue for that version.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Carol, I'm stilling this and a few more bugs related to MMI as I'm gonna be working on this next week.

Comment 16

[Carol Yang [:cyang]]

Yup that's fine.

Note that we still need RIL changes from our side after your change goes in to fix this in commercial RIL.

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko part - v1

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia part - v1

I'd like to do some more testing before asking for review.

Comment 20

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

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

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

Thank you :)

Comment 21

[Anshul]

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

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

::: dom/system/gonk/RILContentHelper.js
@@ +1591,3 @@
>        case RIL.MMI_KS_SC_PIN:
> +        if (success) {
> +          result.statusMessage = RIL.MMI_SM_KS_PIN_CHANGED;

This information should come from RIL and RILContentHelper should really be a pass through. The code for handing the PIN/PUK should look like the IMEI case above with the exception of retry count if the PIN MMI codes result in failures.

@@ +1621,5 @@
> +        if (message.errorMsg === RIL.GECKO_ERROR_PASSWORD_INCORRECT) {
> +          if (message.mmiServiceCode === RIL.MMI_KS_SC_PIN ||
> +              message.mmiServiceCode === RIL.MMI_KS_SC_PIN2) {
> +            result.name = RIL.MMI_ERROR_KS_BAD_PIN;
> +          } else if (message.errorMsg === RIL.MMI_KS_SC_PUK ||

message.errorMsg should be message.mmiServiceCode on this and the next line.

Comment 22

[Anshul]

Fernando, it seems like for every MMI code RIL returns data in a different format which is very inconsistent and prone to bugs. Are you guys willing to make it consistent by sending the information something like below from RIL for all MMI code. Please refer definition of MMIResult at https://bugzilla.mozilla.org/attachment.cgi?id=767638&action=diff.

serviceCode: scCode
statusMessage: (IMEI for IMEI MMI code and status message like smServiceEnabled for rest of the MMI codes)
additonalInformation: (rules for Call forwarding and retryCount for PIN commands)
errorMsg: MMI error message code like emMmiError in case of error.

This might be a slightly bigger change in RILs but at least be consistent for all MMI codes. It would also make changes in RILContentHelper much smaller then what is made in this bug and in bug 879680.

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Anshul from comment #21)
> Comment on attachment 767809 [details] [diff] [review]
> Gecko part - v1
> 
> Review of attachment 767809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1591,3 @@
> >        case RIL.MMI_KS_SC_PIN:
> > +        if (success) {
> > +          result.statusMessage = RIL.MMI_SM_KS_PIN_CHANGED;
> 
> This information should come from RIL and RILContentHelper should really be
> a pass through. The code for handing the PIN/PUK should look like the IMEI
> case above with the exception of retry count if the PIN MMI codes result in
> failures.
> 
> @@ +1621,5 @@
> > +        if (message.errorMsg === RIL.GECKO_ERROR_PASSWORD_INCORRECT) {
> > +          if (message.mmiServiceCode === RIL.MMI_KS_SC_PIN ||
> > +              message.mmiServiceCode === RIL.MMI_KS_SC_PIN2) {
> > +            result.name = RIL.MMI_ERROR_KS_BAD_PIN;
> > +          } else if (message.errorMsg === RIL.MMI_KS_SC_PUK ||
> 
> message.errorMsg should be message.mmiServiceCode on this and the next line.

Thanks Anshul! I'll take a look at this tomorrow.

Comment 24

[Carol Yang [:cyang]]

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

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

::: dom/system/gonk/RILContentHelper.js
@@ +1610,5 @@
> +        }
> +
> +        if (message.retryCount !== undefined) {
> +          if (message.retryCount <= 0) {
> +            result.name = RIL.MMI_ERROR_KS_NEEDS_PUK;

There's already logic down at commericial RIL to figure this out. Similar to Anshul's comment above, the error message to be displayed would already be in message.errorMsg so there wouldn't be any need to explicitly setting it here.

On that note, if retryCount is 0 for PUK/PUK2, your SIM card is hosed and there wouldn't be a need to prompt for PUK again. There should be a new message defined in one of these patches for that purpose.

Comment 25

[Carol Yang [:cyang]]

(In reply to Anshul from comment #22)
> additonalInformation: (rules for Call forwarding and retryCount for PIN
> commands)

To help clarify Anshul's point, I think he's proposing something like:

{
  requestId : 'id{07db922e-b856-4248-8909-165a8ddc3bc4}',
  mmiServiceCode: 'scCallBarring',
  statusMessage: 'smServiceEnabledFor',
  additionalInformation: ['serviceClassVoice', 'serviceClassData']
}

and likes wise for call forwarding:
{
  requestId : 'id{7279cc95-b3c2-440b-98bd-848e7dac39ed}',
  mmiServiceCode : 'scCallForwarding',
  statusMessage: '',
  additionalInformation: [{"active":false,"reason":0,"serviceClass":255,"toa":0,"number":null,"timeSeconds":0}]
}

This can help prevent extra "result.statusMessage = message.result;" assigns and the additionalInfo can still be processed based on the mmiServiceCode.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Hi guys, sorry for the late reply. It was 3am when I posted comment 23 :(

We can probably send from the RIL to the RILContentHelper for successful requests a consistent object like:

{ 
 serviceCode
 statusMessage
 additionalInformation
}

And for failed requests and object like:

{
 serviceCode
 errorName
 message
 additionalInformation
}

Thanks for the suggestion!

In any case, I'm not quite sure that I can do that change in time as this needs to land... today :\ We can probably do that in a follow up, as this seems only an implementation detail that doesn't affect the strings (which needs to be closed by 30th) and/or the functionality. Is that ok for you?

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

s/that I can do/that I cannot do/

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Carol Yang [:cyang] from comment #24)
> Comment on attachment 767809 [details] [diff] [review]
> Gecko part - v1
> 
> Review of attachment 767809 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1610,5 @@
> > +        }
> > +
> > +        if (message.retryCount !== undefined) {
> > +          if (message.retryCount <= 0) {
> > +            result.name = RIL.MMI_ERROR_KS_NEEDS_PUK;
> 
> There's already logic down at commericial RIL to figure this out. Similar to
> Anshul's comment above, the error message to be displayed would already be
> in message.errorMsg so there wouldn't be any need to explicitly setting it
> here.

Ok, so it seems that I was missing some important point... we still use the RILContentHelper even with the commercial RIL.

Patch on the way.

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko part - v2 (b2g18)

This patch should address Anshul and Carol feedback.

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia part - v1

Comment 31

[Anthony Ricaud (:rik)]

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

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

Logic looks good, a few things to explain or change.

Also, I couldn't apply this patch on master. Does this depend on a patch not yet landed?

::: apps/communications/dialer/js/mmi.js
@@ +178,4 @@
>            message.result = mmiResult.statusMessage;
>          }
> +        message.result = mmiResult.statusMessage ?
> +                         mmiResult.statusMessage : null;

Hum, redondant with the if above?

::: apps/communications/dialer/js/mmi_ui.js
@@ +189,2 @@
>      var error = data.error ? data.error : this._('mmi-error');
> +    if (data.message) {

Why this empty if?

::: apps/communications/dialer/locales/dialer.en-US.properties
@@ +123,4 @@
>  
>  # MMI Error message
>  emMmiError=Connection problem or invalid MMI code
> +emMmiErrorPasswordIncorrect=Incorrect Password\n

In other patches, you were inserting \n in the JS. I think we should also do this here.

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia part - v2

Thanks Anthony!

I really don't know which patch I uploaded before... :\

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko part - v2 (m-c)

This is the rebased patch for m-c, no need to ask for review again as it's exactly the same as the b2g18 one.

Comment 34

[Carol Yang [:cyang]]

Comment on attachment 768816 [details] [diff] [review]
Gecko part - v2 (b2g18)

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

::: dom/system/gonk/ril_consts.js
@@ +2240,5 @@
> +this.MMI_ERROR_KS_BAD_PUK = "emMmiErrorBadPuk";
> +this.MMI_ERROR_KS_INVALID_PIN = "emMmiErrorInvalidPin";
> +this.MMI_ERROR_KS_NEEDS_PUK = "emMmiErrorNeedsPuk";
> +this.MMI_ERROR_KS_PIN_PUK_ATTEMPTS = "emMmiErrorPinPukAttempts";
> +this.MMI_ERROR_KS_SIM_BLOCKED = "emMmiErrorSimBlocked";

I don't see this in dialer.en-US.properties

::: dom/system/gonk/ril_worker.js
@@ +4026,5 @@
> +                   mmiServiceCode === MMI_KS_SC_PUK2) {
> +          options.errorMsg = MMI_ERROR_KS_BAD_PUK;
> +        }
> +        if (options.retryCount != undefined) {
> +          options.errorMsgAdd = MMI_ERROR_KS_PIN_PUK_ATTEMPTS;

I'm curious if this needs to be sent at all? Can Gaia just append this part when it knows that this is for PIN/PIN2/PUK/PUK2 and a retryCount is sent? What do you think?

Comment 35

[Carol Yang [:cyang]]

Hi Fernando, thanks for taking our suggestions :) Got a couple of comments but otherwise, this is exactly what we were looking for.

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gecko part - v3 (m-c)

Thanks Carol! Your comments are addressed in this patch.

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia part - v3

Comment 38

[Carol Yang [:cyang]]

Thanks Fernando! This looks good!

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 769309 [details] [diff] [review]
Gecko part - v3 (m-c)

This is quite urgent, so I'll make an r? broadcast

Comment 40

[Anthony Ricaud (:rik)]

The gaia patch has to be applied on top of attachment 767667 [details] [diff] [review].

Comment 41

[Anthony Ricaud (:rik)]

Comment on attachment 769310 [details] [diff] [review]
Gaia part - v3

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

r+ if the answer to the question is yes.

::: apps/communications/dialer/js/mmi_ui.js
@@ -194,5 @@
> -          error += this._('inputCodeRetriesLeft', {n: retries});
> -        break;
> -      case 'NEW_PIN_MISMATCH':
> -        error = this._('newpinConfirmation');
> -        break;

Is this case now handled by the Gecko patches? (This was introduced in bug 863297)

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Actually, I have to remove that. It's useless now. Thanks Rik!

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Anthony Ricaud (:rik) from comment #41)
> Comment on attachment 769310 [details] [diff] [review]
> Gaia part - v3
> 
> Review of attachment 769310 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if the answer to the question is yes.
> 
> ::: apps/communications/dialer/js/mmi_ui.js
> @@ -194,5 @@
> > -          error += this._('inputCodeRetriesLeft', {n: retries});
> > -        break;
> > -      case 'NEW_PIN_MISMATCH':
> > -        error = this._('newpinConfirmation');
> > -        break;
> 
> Is this case now handled by the Gecko patches? (This was introduced in bug
> 863297)
Forget about my previous answer. Yes, it is handled by gecko.

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

https://github.com/mozilla-b2g/gaia/commit/4232b08531de197b619797c1467e49a9157963aa

Comment 45

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

Comment on attachment 769309 [details] [diff] [review]
Gecko part - v3 (m-c)

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

::: dom/system/gonk/RILContentHelper.js
@@ +1587,5 @@
>        Services.DOMRequest.fireSuccess(request, mmiResult);
>      } else {
>        let mmiError = new this._window.MMIError(result.serviceCode,
> +                                               message.errorMsg,
> +                                               message.errorMsgAdd,

Throughout RILContentHelper/RadioInterfaceLayer/ril_worker, no errorMsgAdd attribute is ever defined.  Maybe place a null/undefined/empty string here?

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

Thanks!

http://hg.mozilla.org/projects/birch/rev/ac17d84c8b7c

Comment 47

[Phil Ringnalda (:philor)]

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

Comment 48

[Gene Lian (I already quit Mozilla)]

Thanks Vicamo! \^O^/

Is Gecko part - v2 (b2g18) ready to land to b2g18?

Comment 49

[Fernando Jiménez Moreno [:ferjm]]

Yes :)

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c6d33b73f204

Comment 50

[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 51

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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

Comment 53

[Carol Yang [:cyang]]

Attachment: Screenshot of PIN Change with patches

Screenshot shows that number of attempts is missing from the UI.

Comment 54

[Carol Yang [:cyang]]

Hi Fernando, 

Got a few questions on this one:

1) I didn't see the Gaia change get uplifted to v1-train. Is this coming soon?
2) After I manually pulled in the Gaia changes, I'm noticing that the number of attempts does not show up on the UI in the error case. Please see attachment 771014 [details]. This is true for reference or commercial RIL. This was being sent:

RILContentHelper: handleSendCancelMMI {"requestId":"id{f4168a23-98eb-4a51-a114-2daf0b235cb7}","success":false,"mmiServiceCode":"scPin","errorMsg":"emMmiErrorBadPin","additionalInformation":2}

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Hi Carol,

I'll do the uplift in a few hours.

I can't reproduce the issue that you mention with birch and master. Are you sure that you applied the correct patches?

Comment 56

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Screenshot of PIN change (birch+master)

Comment 57

[Fernando Jiménez Moreno [:ferjm]]

v1-train: 5e9d60e22e4c3d9090c73bbdf27126cfedfb6d0e
v1.1hd: 4a198b30f190994eeab4c025247b9735b302d079

Comment 58

[Carol Yang [:cyang]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #55)
> Hi Carol,
> 
> I'll do the uplift in a few hours.
> 
> I can't reproduce the issue that you mention with birch and master. Are you
> sure that you applied the correct patches?

Ah, I applied the patches correctly but because I was only applying a patch and it is not committed, the change in dialer.en-US.properties was automatically reverted when I was building. Now I see the right UI.

Comment 59

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

v1.1.0hd: 5e9d60e22e4c3d9090c73bbdf27126cfedfb6d0e