Bug 874000 (mmi-result-pin)

[MMI] Use MMIResult for PIN/PIN2/PUK related functionality

RESOLVED FIXED in Firefox 25

Status

()

P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: fcampo, Assigned: ferjm)

Tracking

({late-l10n})

unspecified
1.1 QE3 (26jun)
x86
Mac OS X
late-l10n
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: ril-track, [mozilla-triage], [COM_RIL][POVB], TaipeiWW, [LeoVB+])

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Followup created after bug 860660, comment 105
(Reporter)

Updated

6 years ago
Assignee: nobody → fernando.campo
status-b2g18: --- → affected
status-b2g18-v1.0.1: --- → affected
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.
blocking-b2g: --- → leo?

Comment 2

6 years ago
(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!
Flags: needinfo?(dcoloma)

Comment 3

6 years ago
Fernando, the PIN Change message is coming from comercial RIL. I am stealing this bug from you for now. I hope that's okay.
Assignee: fernando.campo → anshulj
(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
Flags: needinfo?(dcoloma)
status-b2g18-v1.0.1: affected → wontfix
(Reporter)

Comment 5

6 years ago
Hi Anshul, any updates on this?
Flags: needinfo?(anshulj)

Updated

6 years ago
Assignee: anshulj → cyang

Comment 6

6 years ago
Carol will look into this issue in a couple of days and get back.
Flags: needinfo?(anshulj)

Updated

6 years ago
Whiteboard: ril-track,
Mozilla triage recommends not blocking v1.1 on this bug.
Whiteboard: ril-track, → ril-track, [mozilla-triage]
Carol, any update about this bug?
Flags: needinfo?(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.
Depends on: 879032
Flags: needinfo?(cyang)
Carol, I am marking this as POVB as it seems it only requires development on your side... Please correct me I am wrong
Whiteboard: ril-track, [mozilla-triage] → ril-track, [mozilla-triage], [COM_RIL][POVB]
(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.
Just to be clear though.  This bug depends on bug 879032, which would be a change to Gecko.
(Reporter)

Comment 13

6 years ago
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.
blocking-b2g: leo? → -
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.
blocking-b2g: - → leo?
blocking-b2g: leo? → leo+
Carol, I'm stilling this and a few more bugs related to MMI as I'm gonna be working on this next week.
Assignee: cyang → ferjmoreno
Depends on: 883178
No longer depends on: 879032
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.

Updated

6 years ago
Priority: -- → P2
(Assignee)

Updated

6 years ago
Alias: mmi-result-pin
Summary: [Dialer] Return retryCount when changing PIN with mmi codes → [Dialer] Use MMIResult for PIN/PIN2/PUK related functionality
(Assignee)

Updated

6 years ago
Component: Gaia::Dialer → DOM: Device Interfaces
Product: Boot2Gecko → Core
Summary: [Dialer] Use MMIResult for PIN/PIN2/PUK related functionality → [MMI] Use MMIResult for PIN/PIN2/PUK related functionality
(Assignee)

Updated

6 years ago
Blocks: 868950
Whiteboard: ril-track, [mozilla-triage], [COM_RIL][POVB] → ril-track, [mozilla-triage], [COM_RIL][POVB], TaipeiWW
(Assignee)

Updated

6 years ago
Duplicate of this bug: 868950
(Assignee)

Updated

6 years ago
Depends on: 884349
Target Milestone: --- → 1.1 QE3 (26jun)
Created attachment 767809 [details] [diff] [review]
Gecko part - v1
Created attachment 767816 [details] [diff] [review]
Gaia part - v1

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

Updated

6 years ago
Keywords: late-l10n
(Assignee)

Updated

6 years ago
Attachment #767809 - Flags: review?(vyang)
Comment on attachment 767809 [details] [diff] [review]
Gecko part - v1

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

Thank you :)
Attachment #767809 - Flags: review?(vyang) → review+

Comment 21

6 years ago
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.
Attachment #767809 - Flags: review-

Comment 22

6 years ago
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.
(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 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.
(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.
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?
s/that I can do/that I cannot do/
(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.
Created attachment 768816 [details] [diff] [review]
Gecko part - v2 (b2g18)

This patch should address Anshul and Carol feedback.
Attachment #767809 - Attachment is obsolete: true
Attachment #768816 - Flags: review?(vyang)
Attachment #768816 - Flags: feedback?(anshulj)
(Assignee)

Updated

6 years ago
Attachment #768816 - Attachment description: Gecko part - v1 → Gecko part - v2 (b2g18, needs rebase in m-c)
Created attachment 768824 [details] [diff] [review]
Gaia part - v1
Attachment #767816 - Attachment is obsolete: true
Attachment #768824 - Flags: review?(anthony)
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.
Attachment #768824 - Flags: review?(anthony)
Created attachment 769026 [details] [diff] [review]
Gaia part - v2

Thanks Anthony!

I really don't know which patch I uploaded before... :\
Attachment #768824 - Attachment is obsolete: true
Attachment #769026 - Flags: review?(anthony)
Created attachment 769030 [details] [diff] [review]
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 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?
Attachment #768816 - Flags: feedback?(anshulj)
Hi Fernando, thanks for taking our suggestions :) Got a couple of comments but otherwise, this is exactly what we were looking for.
Created attachment 769309 [details] [diff] [review]
Gecko part - v3 (m-c)

Thanks Carol! Your comments are addressed in this patch.
Attachment #769030 - Attachment is obsolete: true
Attachment #769309 - Flags: review?(vyang)
Created attachment 769310 [details] [diff] [review]
Gaia part - v3
Attachment #769026 - Attachment is obsolete: true
Attachment #769026 - Flags: review?(anthony)
Attachment #769310 - Flags: review?(anthony)
(Assignee)

Updated

6 years ago
Attachment #768816 - Attachment description: Gecko part - v2 (b2g18, needs rebase in m-c) → Gecko part - v2 (b2g18)
Attachment #768816 - Flags: review?(vyang)
Thanks Fernando! This looks good!
Comment on attachment 769309 [details] [diff] [review]
Gecko part - v3 (m-c)

This is quite urgent, so I'll make an r? broadcast
Attachment #769309 - Flags: review?(gene.lian)
(Assignee)

Updated

6 years ago
Attachment #769310 - Flags: review?(fbsc)
The gaia patch has to be applied on top of attachment 767667 [details] [diff] [review].
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)
Attachment #769310 - Flags: review?(anthony) → review+
Actually, I have to remove that. It's useless now. Thanks Rik!
(Assignee)

Updated

6 years ago
Attachment #769310 - Flags: review?(fbsc)
(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 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?
Attachment #769309 - Flags: review?(vyang)
Attachment #769309 - Flags: review?(gene.lian)
Attachment #769309 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ac17d84c8b7c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Thanks Vicamo! \^O^/

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c6d33b73f204
status-b2g18: affected → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g-v1.1hd: --- → affected
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.
status-b2g18: fixed → affected
Re-pushed to b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/bd46fe2eda85
status-b2g18: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/bd46fe2eda85
status-b2g-v1.1hd: affected → fixed
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed
Created attachment 771014 [details]
Screenshot of PIN Change with patches

Screenshot shows that number of attempts is missing from the UI.
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}
Flags: needinfo?(ferjmoreno)
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?
Flags: needinfo?(ferjmoreno)
Created attachment 771352 [details]
Screenshot of PIN change (birch+master)
v1-train: 5e9d60e22e4c3d9090c73bbdf27126cfedfb6d0e
v1.1hd: 4a198b30f190994eeab4c025247b9735b302d079
(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.

Updated

6 years ago
Whiteboard: ril-track, [mozilla-triage], [COM_RIL][POVB], TaipeiWW → ril-track, [mozilla-triage], [COM_RIL][POVB], TaipeiWW, [LeoVB+]
v1.1.0hd: 5e9d60e22e4c3d9090c73bbdf27126cfedfb6d0e
You need to log in before you can comment on or make changes to this bug.