Closed Bug 874000 (mmi-result-pin) Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
1.1 QE3 (26jun)
blocking-b2g leo+
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: fcampo, Assigned: ferjm)

References

Details

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

Attachments

(5 files, 5 obsolete files)

Followup created after bug 860660, comment 105
Assignee: nobody → fernando.campo
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?
(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)
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)
Hi Anshul, any updates on this?
Flags: needinfo?(anshulj)
Assignee: anshulj → cyang
Carol will look into this issue in a couple of days and get back.
Flags: needinfo?(anshulj)
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.
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.
Priority: -- → P2
Alias: mmi-result-pin
Summary: [Dialer] Return retryCount when changing PIN with mmi codes → [Dialer] Use MMIResult for PIN/PIN2/PUK related functionality
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
Whiteboard: ril-track, [mozilla-triage], [COM_RIL][POVB] → ril-track, [mozilla-triage], [COM_RIL][POVB], TaipeiWW
Duplicate of this bug: mmi-localization-pin
Depends on: mmi-result-imei
Target Milestone: --- → 1.1 QE3 (26jun)
Attached patch Gecko part - v1 (obsolete) — Splinter Review
Attached patch Gaia part - v1 (obsolete) — Splinter Review
I'd like to do some more testing before asking for review.
Keywords: late-l10n
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 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-
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.
This patch should address Anshul and Carol feedback.
Attachment #767809 - Attachment is obsolete: true
Attachment #768816 - Flags: review?(vyang)
Attachment #768816 - Flags: feedback?(anshulj)
Attachment #768816 - Attachment description: Gecko part - v1 → Gecko part - v2 (b2g18, needs rebase in m-c)
Attached patch Gaia part - v1 (obsolete) — Splinter Review
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)
Attached patch Gaia part - v2 (obsolete) — Splinter Review
Thanks Anthony!

I really don't know which patch I uploaded before... :\
Attachment #768824 - Attachment is obsolete: true
Attachment #769026 - Flags: review?(anthony)
Attached patch Gecko part - v2 (m-c) (obsolete) — Splinter Review
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.
Thanks Carol! Your comments are addressed in this patch.
Attachment #769030 - Attachment is obsolete: true
Attachment #769309 - Flags: review?(vyang)
Attached patch Gaia part - v3Splinter Review
Attachment #769026 - Attachment is obsolete: true
Attachment #769026 - Flags: review?(anthony)
Attachment #769310 - Flags: review?(anthony)
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)
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!
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
Closed: 7 years ago
Resolution: --- → FIXED
Thanks Vicamo! \^O^/

Is Gecko part - v2 (b2g18) ready to land to b2g18?
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.
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)
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.
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.