Closed Bug 860660 Opened 7 years ago Closed 7 years ago

[Buri][TEF check]PIN & PUK Code:Should show the counter of trials remain.

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 verified)

VERIFIED FIXED
1.0.1 IOT1 (10may)
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- verified
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- verified

People

(Reporter: sync-1, Assigned: m1)

References

Details

(Keywords: late-l10n)

Attachments

(13 files, 8 obsolete files)

72.66 KB, image/png
Details
296 bytes, text/html
arcturus
: review+
kaze
: review+
Details
151.58 KB, image/jpeg
Details
296 bytes, text/html
kaze
: review+
Details
465.22 KB, image/jpeg
Details
296 bytes, text/html
arcturus
: review+
Details
296 bytes, text/html
etienne
: review+
Details
7.62 KB, patch
Details | Diff | Splinter Review
6.19 KB, patch
fcampo
: review+
Details | Diff | Splinter Review
4.33 KB, patch
Details | Diff | Splinter Review
2.97 KB, patch
Details | Diff | Splinter Review
410.16 KB, text/plain
Details
15.24 KB, image/png
Details
AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.19.059
 Firefox os  v1.0.1
 Mozilla build ID:20130331070206
 
 +++ This bug was initially created as a clone of Bug #437027 +++
 
 Created an attachment (id=384914)
 Screenshot
 
 DEFECT DESCRIPTION:
 
  PIN & PUK Code:Should show the counter of trials remain.
 
  REPRODUCING PROCEDURES:
 
  1.Power on with SIM card,the SIM PIN code is open->access enter PIN code screen->don't show the counter of trials remain==>KO1
  2.Power on with SIM card,the SIM is PUK locked->access enter PUK code screen->don't show the counter of trials remain==>KO2
  3.Launch dial->input **04*WrongOldPIN1*NewPIN1*NewPIN1#->send->don't show the counter of trials remain==>KO3
  4.Launch dial->input **05*WrongPUK1*NewPIN1*NewPIN1#->send->don't show the counter of trials remain==>KO4
 
  EXPECTED BEHAVIOUR:
  KO1,KO2,KO3,KO4==>Should show the counter of trials remain.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:SW121+TEF
 
  USER IMPACT:Medium
 
  REPRODUCING RATE:5/5
 
  For FT PR, Please list reference mobile's behavior:
 
 ++++++++++ end of initial bug #437027 description ++++++++++
 
 
 
 CONTACT INFO (Name,Phone number):
 
  DEFECT DESCRIPTION:
 
  REPRODUCING PROCEDURES:
 
  EXPECTED BEHAVIOUR:
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:
 Just like the Iphone and some Android phone, the phone should always show the remain trials when and where the pin/puk code is needed, but not only after the user submitted a wrong pin/puk code.  Therefore the user will always know how many times left, so avoid locking the sim card accidentally.
 
 By the way, in the dialer, if the user send **04*WrongOldPIN*NewPIN*NewPIN# or **05*WrongPUK*NewPIN*NewPIN#, which mean to change the pin from the dialer, after failure, the phone should show how many trials remain to the user but not just incorreckPassword.
 
 Further more, if user submit the correct pin/puk code, the remain trials should update to the total times (default value) we can try.
 
 Last, I think the remain trials should synchronized among dialer , settings and system, but this is a little complicated in my opinion.
Attached image pics
This looks like it might require new strings
The device is already showing how many tries there are left, albeit only after the pin is entered incorrectly at least once. So no new strings should be required... for the PIN entry screens at least. Dunno about the dialer one though. The strings are there already but on the wrong app (settings and system currently).
(In reply to Antonio Manuel Amaya Calvo from comment #4)
> The device is already showing how many tries there are left, albeit only
> after the pin is entered incorrectly at least once. So no new strings should
> be required... for the PIN entry screens at least. Dunno about the dialer
> one though. The strings are there already but on the wrong app (settings and
> system currently).

The strings might be there as well, as FTE requests for PIN and it is funnily part of comms (as dialer is)
(In reply to Antonio Manuel Amaya Calvo from comment #4)
> The device is already showing how many tries there are left, albeit only
> after the pin is entered incorrectly at least once. So no new strings should
> be required... for the PIN entry screens at least. Dunno about the dialer
> one though. The strings are there already but on the wrong app (settings and
> system currently).

You are right, but did you have been experience this scene: the pin is entered incorrectly once, then the user exit the settings, then access the settings->sim security again, do something with the pin, there is nothing informing the user only 2 tries left(because the user have entered wrong pin once).
The worst thing is that the user have entered the pin incorrectly twice, then they exit the settings, some days later(maybe the user have forgotten what they have done), they launch setting and access sim security(do something need pin), there is also nothing informing the user only 1 tries left. This time, if they entered incorrectly once again, the sim Card will be locked.
So I think we should store the count of remain tries and always show the user of the tries left.
blocking-b2g: --- → tef?
Assignee: nobody → ferjmoreno
Assignee: ferjmoreno → fernando.campo
Please add late-l10n to the keywords as soon as possible if this does end up requiring new strings.

Multiple partners (TCL, TEF) now believe this should block - marking as such.
blocking-b2g: tef? → tef+
For this bug to be fixed, we'll need to make some changes on our side that will require an SR to be opened.
As far as I know, modem doesn't report the number of retry counts to RIL in RIL_REQUST_GET_SIM_STATUS,
so there's no way to know the retry count of the SIM in advance.

For other Android phones, it seems they use custom RIL_REQUEST, so those phones can get retry count beforehand. 

Michael, Anshul
Can you also let me know when you've finished the modification ?
We'd also like to do the corresponding change in Mozilla RIL.

Thanks
Fernando, can you estimate when you'll have the Gaia patch ready for this?

Michael, when will the RIL pieces be available?
Whiteboard: [status: no patch, needs ETA]
Please let us know what message we should send to the content process?   An extension to 'cardstatechange'?
Attached image proposed (obsolete) —
Right now, as the RIL doesn't gives back a number of retries until a first attempt of unlocking is made, the approach I'm taking for the bug is to store the number of retries left on a setting (or I could use async storage instead, whatever is better) and reading it when showing the pin screen. A few questions about it:
- Where to show it? On current UX it's shown as an error, but if we want to show it just as information, my current approach is to do it over the corresponding label (PIN, PUK, etc) as the attachment shows. This UX changes would probably imply some other major changes on the general UX, as if we always show the number of tries left, there's no point on showing them again on an error (unless it's the last change, where we have a specific message for it), and on some strings too.
- I can't test the patch with nck, cck or spck locks, as I don't have any simcard with those, maybe someone can provide me with one?
- And about a schedule, I can have the patch ready today (I'd say couple of hours but prefer not to get my fingers burnt), and update it later if the modifications on the RIL gives us a new function to call to have the left attempts beforehand.

So, any thoughts on this?
Whiteboard: [status: no patch, needs ETA] → [status: no patch, needs ETA] [BTG-1397]
How about adding a retryCount field to the cardstatechange message.  This field is a number > 0 and is only present when cardState == pinRequired or pukRequired?

eg:
  RIL:CardStateChanged{ rilMessageType : 'cardstatechange', cardState : 'pinRequired', retryCount : 2, }
  RIL:CardStateChanged{ rilMessageType : 'cardstatechange', cardState : 'pukRequired', retryCount : 1, }
  RIL:CardStateChanged{ rilMessageType : 'cardstatechange', cardState : 'ready', }

If so, looks like gecko/dom/system/gonk/RILContentHelper.js will need to be modified to catch this new field.
However let's also make the retryCount field optional for the pinRequired/pukRequired card states, to gracefully degrade if this information is not available
Would it be a problem for you to have that field directly accesible through the API, apart from passing it on the cardstatechange message?

Also, just as an idea, the name 'retryCount' sounds to me like the number of times that the user already tried to unlock the card, not the number of times left till blocked. But of course, whatever suits you guys better :)
Having it accessible via API would be fine too.  Happy to adapt to whatever is easiest from the pov of the content process
The name retryCount is what the API RIL:CardLockResult in content process uses to notify the exact same thing as discussed in bug so prefer to keep the same name to be consistent.
(In reply to Fernando Campo (:fcampo) from comment #15)
> Would it be a problem for you to have that field directly accesible through
> the API, apart from passing it on the cardstatechange message?
> 
> Also, just as an idea, the name 'retryCount' sounds to me like the number of
> times that the user already tried to unlock the card, not the number of
> times left till blocked. But of course, whatever suits you guys better :)

Hi, Fernando. 
There is 3 apps we need to show the info of retryCount. 
a) "Settings"
b) "System"
c) "dialer" when the user send '**04*WrongOldPIN*NewPIN*NewPIN#'.

By the way, I found that if the user enter with wrong pin code three times in the "Settings", the puk dialog(or puk page) of "System" will occur, but not the puk dialog of "Settings". And the puk dialog's style in "System" is different from "Settings". Is it a user experience problem?
(In reply to xiaokang.chen from comment #18)
> Hi, Fernando. 
> There is 3 apps we need to show the info of retryCount. 
> a) "Settings"
> b) "System"
> c) "dialer" when the user send '**04*WrongOldPIN*NewPIN*NewPIN#'.
We should add FTU too, as it also uses PIN/PUK screens of its own
I think it's gonna be better to make individual patches for each app in that case, so different drivers can review them

> By the way, I found that if the user enter with wrong pin code three times
> in the "Settings", the puk dialog(or puk page) of "System" will occur, but
> not the puk dialog of "Settings". And the puk dialog's style in "System" is
> different from "Settings". Is it a user experience problem?
Yes, it's a known issue, and a re-design of all screens to look alike is ongoing, treated in bug 852682, but as it's not a blocker, not much work on it right now
Attached file [System] Pull Request (obsolete) —
As I'm not sure who should review the part for system, I'm putting Etienne as a proxy
Attachment #738455 - Flags: review?(etienne)
Keywords: late-l10n
I'm also adding UX Sergi to the loop, as the changes affect UI and the re-design from bug 852682 (comment 19)
Flags: needinfo?(sergioa)
Comment on attachment 738455 [details]
[System] Pull Request

Quick comment on github.
Also adding :kaze to the review for the advanced l10n stuff.
Attachment #738455 - Flags: review?(kaze)
Attached file [FTU] Pull Request
Attachment #738573 - Flags: review?(francisco.jordano)
Comment on attachment 738573 [details]
[FTU] Pull Request

r+ some comments on github.

Also asking Kaze for some feedback regarding the renaming of some strings ids.
Attachment #738573 - Flags: review?(kaze)
Attachment #738573 - Flags: review?(francisco.jordano)
Attachment #738573 - Flags: review+
Attached image PIN Code entry screen
Hi guys, i've added a flow to describe the process of entering the SIM PIN, displaying errors if it's not correct, and how to show the PUK entry screen.
Attachment #737994 - Attachment is obsolete: true
Attached image PUK Entry Code Flow (obsolete) —
Flags: needinfo?(sergioa) → needinfo?(sergiov)
Attached file [system] Pull Request (obsolete) —
Attachment #738455 - Attachment is obsolete: true
Attachment #738455 - Flags: review?(kaze)
Attachment #738455 - Flags: review?(etienne)
Attachment #739094 - Flags: review?(kaze)
Attachment #739094 - Flags: review?(etienne)
Here's the content process part needed to get attachment 739094 [details] working end-to-end.   I don't think we want to keep the name 'retryCount' in the final nsIDOMMobileConnection attribute though, but that's what attachment 739094 [details] needs.   I'm on travel tomorrow so won't be able to carry this forward till Monday.

Commercial RIL support for this will be landed early next week as well
Attachment #739104 - Flags: feedback?
Comment on attachment 739094 [details]
[system] Pull Request

r=me with the small nit addressed for the non-l10n parts.
Attachment #739094 - Flags: review?(etienne) → review+
Attachment #739163 - Flags: review?(kaze)
Attached image PUK Entry Code Flow
I've updated the PUK entry flow to place the PUK+New PIN entry fields in the same screen.
Flags: needinfo?(sergiov)
Attachment #739050 - Attachment is obsolete: true
Attachment #738573 - Flags: review?(kaze) → review+
Attachment #739094 - Flags: review?(kaze) → review+
Attachment #739163 - Flags: review?(kaze) → review+
These three patches all look good to me but I think we should factorize these strings in a follow-up and make sure they please our UX team.
Josh, please provide UX input on attachment in comment 31.
Flags: needinfo?(jcarpenter)
Whiteboard: [status: no patch, needs ETA] [BTG-1397] → [status: needs UX feedback] [BTG-1397]
Overall these are looking good. Here are some low-risk copy and layout revisions

* Prompt title: change from "Enter PIN code" to "Enter PIN"
* Prompt layout: change "Ok" to "OK"
* Prompt layout: change keyboard to new numbers-only version (removes decimal key)
* Prompt layout: move "X tries left" under the input field.
* Wrong PIN prompt: change title to: "Incorrect PIN"
* Wrong PIN prompt: change body text to provide useful instructions to the user for how to resolve the situation. The copy in the PUK Code Entry prompt is a good example: "Refer to your SIM card documentation for the SIM PIN code or contact your carrier." If we cannot provide useful copy, eliminate the duplication of title and body and instead "Incorrect PIN" only.
* PUK Code Entry: move "X tries left" and instructions under the input fields.
Flags: needinfo?(jcarpenter)
Thanks for your comments Josh. Everything looks fine. 

My only concern is about moving the "X tries left" label under the input field. IMHO that's a really important message for the user, as it may lock his phone if the PIN is incorrectly typed 3 times, so i think moving it and making the font size smaller (we should do that if we move the text), may somehow "hide" the message.
Commercial RIL support for this has been landed and will be available in AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.083 and beyond.  It will also be necessary to update the underlying ril blobs as well for those without source access.
Attached file [system] Pull Request
New pull request to fix the revert done to original, due to the discovery of a bug.
Attachment #739094 - Attachment is obsolete: true
Attachment #740303 - Flags: review?(francisco.jordano)
Comment on attachment 740303 [details]
[system] Pull Request

Saw the fix working, minimun change from the previous PR, just adding the id to the element.

Thanks Fernando for fixing it so fast!
Attachment #740303 - Flags: review?(francisco.jordano) → review+
Whiteboard: [status: needs UX feedback] [BTG-1397] → [status: has r+, needs to land] [BTG-1397]
Attached file [dialer] Pull Request
Attachment #740732 - Flags: review?(etienne)
Comment on attachment 740732 [details]
[dialer] Pull Request

Comment on github, otherwise it looks good :)
Attachment #740732 - Flags: review?(etienne)
Comment on attachment 740732 [details]
[dialer] Pull Request

Fixed the private variable addressed in github comment
Attachment #740732 - Flags: review?(etienne)
Comment on attachment 739104 [details] [diff] [review]
nsIDOMMobileConnection support for 'retryCount'

f? -> r? 
A variant of this patch needs to land for any of the Gaia stuff that's happening in this bug to work e2e.
Attachment #739104 - Flags: feedback? → review?
Comment on attachment 740732 [details]
[dialer] Pull Request

Sweet and simple :)
Attachment #740732 - Flags: review?(etienne) → review+
And finally, merged dialer patch https://github.com/mozilla-b2g/gaia/commit/887cae7e556fc19722011f0674fdd4d4bfeacc3e


NOTE: For uplift, please be aware that we need to uplift the ftu patch BEFORE this one (https://github.com/mozilla-b2g/gaia/commit/7faefd0273f4dce24563cda57450bfb1c7416b45) as the changes in ftu will be needed for dialer to work.
So all the gaia patches are ready, only need the approval for Michael's backend one :)
Comment on attachment 739104 [details] [diff] [review]
nsIDOMMobileConnection support for 'retryCount'

Vicamo - if you're not the right person to review, please tag somebody else. Thanks!
Attachment #739104 - Flags: review?(vyang)
Comment on attachment 739104 [details] [diff] [review]
nsIDOMMobileConnection support for 'retryCount'

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

Two problems:
1) You also have to update nsIRadioInterfaceLayer.idl::nsIRilContext
2) Please move to icc manager instead. See bug 857414.
Attachment #739104 - Flags: review?(vyang)
Thanks Vicamo.  Can you help me understand why #1 needed? 

For #2, since bug 857414 has not landed, has no r+, and is not tef+ we should probably do this as a follow-up patch.
Milestoning for 5/10, since we want to get this change in the final test cycles (and give the localizers time to localize the new string).
Flags: needinfo?(vyang)
Target Milestone: --- → 1.0.1 IOT1 (10may)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #52)
> Thanks Vicamo.  Can you help me understand why #1 needed? 

rilContext you got in RILContentHelper is from RadioInterfaceLayer::rilContext, which is typed nsIRilContext. Please sync the interfaces so that we don't find an non-interface attribute read somewhere else and make us confused in the future.
Flags: needinfo?(vyang)
As 2), it's true we can delay the job and move all icc related stuff in bug 857414. So please at least fix nsIRilContext. Similar thing happened in bug 833908.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #54)
> (In reply to Michael Vines [:m1] [:evilmachines] from comment #52)
> > Thanks Vicamo.  Can you help me understand why #1 needed? 
> 
> rilContext you got in RILContentHelper is from
> RadioInterfaceLayer::rilContext, which is typed nsIRilContext. Please sync
> the interfaces so that we don't find an non-interface attribute read
> somewhere else and make us confused in the future.

Sorry for being even slower than usual, I'm still not seeing it.  AFAICT, RILContentHelper.js is implementing nsIMobileConnectionProvider (via nsIRILContentHelper) not nsIRilContext.  I'm happy to add a new retryCount field to nsIRilContext but it'll be dead code no?


BTW. Are we ok with calling the new field in nsIDOMMobileConnection 'retryCount' when we have an existing 'cardState' field.  Maybe 'cardRetryCount'?  That'll mean the Gaia patches attached here need to be refreshed of course.  LMK.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #56)
> BTW. Are we ok with calling the new field in nsIDOMMobileConnection
> 'retryCount' when we have an existing 'cardState' field.  Maybe
> 'cardRetryCount'?  That'll mean the Gaia patches attached here need to be
> refreshed of course.  LMK.

Hi Michael:
Sorry, but I'd like to ask IDL first.

From this 'retryCount',
which retry count are you referring to?
'PIN(2)' or 'PUK(2)' ? There are 4 kinds retry count.

Cannot we make it something like
getCardRetryCount(...) to make it consistent with those Card lock API?


Thanks
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #57)
> Sorry, but I'd like to ask IDL first.
> 
> From this 'retryCount',
> which retry count are you referring to?
> 'PIN(2)' or 'PUK(2)' ? There are 4 kinds retry count.
> 
> Cannot we make it something like
> getCardRetryCount(...) to make it consistent with those Card lock API?

The immediate need just for PIN1/PUK1 but I agree that we should consider the PIN2/PUK2 retry counters at the IDL level at least since we are here already.  But I not really qualified right now to have an opinion on what that IDL in the content process should look like, I haven't taken the time to upload enough of that code into my head yet.  So I welcome your suggestions!
Yoshi - do we need a separate bug on file for comment 57?
Flags: needinfo?(allstars.chh)
For interface part, I think it could be discussed if should we create a WebAPI for it to make it more complete and more consistent with other CardLock API, but it could take a while so we create another bug to address that.

But Vicamo has some questions about the implementation, comment 54. 
Why didn't Michael Vines modify nsIRilContext in nsIRadiointerfaceLayer.idl ?

Doesn't commercial RIL use this IDL?
Flags: needinfo?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #60)
> But Vicamo has some questions about the implementation, comment 54. 
> Why didn't Michael Vines modify nsIRilContext in nsIRadiointerfaceLayer.idl ?
> 
> Doesn't commercial RIL use this IDL?

Yes we implement nsIRilContext however modifying this interface was not necessary to emit a RIL:CardStateChanged message to the content process that includes the number of pin1/puk1 retries, so it's unclear to me why we would need to modify nsIRilContext.
Vicamo is PTO today, 
but I think what he refers to is currently latest m-c, not b2g18 branch.
I thought the procedure is to land on latest m-c (birch), then uplift to b2g18, am I correct?

In latest m-c, we have to sync nsIRilContext. That's what he meant in Comment 54.

So that means you need to add 'retryCount' (or cardRetryCount) in rilContext
http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#313,

and modify 
http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIRadioInterfaceLayer.idl#81
accordingly.

Vicamo, please feel free to correct me if I am wrong.

Michael, does my reply make sense to you?

Thanks
Ah, got it.  Thanks, I wasn't paying attention to what's going on @ m-c.  I'll take a look at this on m-c tomorrow.  But in the meantime, what do you want me to call the new dom API method?  I really don't care too much :)
Hi, Michael
I didn't ask you to make it a new DOM API.
I just said it seems better if we could create a DOM API for it. It could be done by Mozilla side.

My points are: (It should be moved to another bug to discuss this)
1. If you add a retryCount in mozMobileConnection, but when we call unlockCardLock, it would also return a retryCount if it fails. That means
* 1a: The retryCount in the returned object of unlockCardLock is unnecessary, which doesn't make sense to me. That means user has to read the error message from the DOMRequest, then go to read retryCount from mozMobileConnection. Also when there are multiple DOMRequests, how do we make sure the synchronization is correct? For example, how do we know the value of retryCount is for 1st or 2nd DOMRequest(unlockCardLock).

2. Should we also rename oncardstatechange, because your patch makes it not only card state, but also the retry count changes. Then this goes to my argument in (1a) again,
user has to check the DOMRequest for the result (and error message, if he has to), also user has to listen oncardstatechange, which seems complicated to me.

3. For PIN2/PUK2, as I have asked before.
It seems we have to make two retryCount members for PIN/PIN2, which should double the effort as I mentioned in (1) and (2)

So it looks to me that adding a new API called getCardLockRetryCount(DOMString lockType) is easier and makes sense, also is more consistent with currently API.

Thanks
Ok, well you're more than welcome to take the content process part of this bug!  Just let me know what message you want the commercial RIL to send you instead of what I propose implicitly by attachment 739104 [details] [diff] [review].  I'm happy to poke at it tomorrow but you clearly know the code better at this point, and our goal is simply to get this tef+ bug resolved on v1.0.1 as quick as possible and with as little risk as possible.
I agree we should resolve this bug ASAP, and Vicamo already indicates you should add retryCount for nsIRilContext in your patch. Moreover I saw Gaia patches have already landed
(Although I am not quite sure how).

I would suggest you simply address Vicamo's comment.
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #66)
> [...] Moreover I saw Gaia
> patches have already landed
> (Although I am not quite sure how).
Hi Yoshi.

We were able to land the gaia patches cause they're kinda fail proof (we check for the retryCount, if there's no response, we don't show anything), so landing them would not break anything, and we'd be ready for the gecko patch.
Hope this helps
(In reply to Fernando Campo (:fcampo) from comment #67)
> We were able to land the gaia patches cause they're kinda fail proof (we
> check for the retryCount, if there's no response, we don't show anything),
> so landing them would not break anything, and we'd be ready for the gecko
> patch.
> Hope this helps

Hi, Fcampo

Thanks for explaining.
Yeah, but my point is how does Gaia show the available retry count without Gecko interface changed?

It seems to me Gaia still cannot get the retry count. (now mobileconnection.retryCount should be always undefined).

I though Gaia patches should land only when Gecko part is ready.

But anyway, I know you were in Work Week and need to land these things.

Please ignore my wondering. :)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #66)
> I agree we should resolve this bug ASAP, and Vicamo already indicates you
> should add retryCount for nsIRilContext in your patch. Moreover I saw Gaia
> 
> [...]
> 
> I would suggest you simply address Vicamo's comment.

ni?(m1)
Flags: needinfo?(mvines)
Whiteboard: [status: has r+, needs to land] [BTG-1397] → [status: needs updated gecko patch, gaia work already landed] [BTG-1397]
Yep, I'll get to the m-c patch tomorrow prolly.  Today stuck in mtgs.  But in the meantime, review of the b2g18 patch can certainly proceed since it sounds like the m-c patch is gonna be a complete rewrite anyway.
Flags: needinfo?(mvines)
Comment on attachment 739104 [details] [diff] [review]
nsIDOMMobileConnection support for 'retryCount'

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

::: dom/system/gonk/RILContentHelper.js
@@ +335,4 @@
>      return;
>    }
>    this.cardState = rilContext.cardState;
> +  this.retryCount = rilContext.retryCount;

Hi, Michael
The rilContext here is from nsIRilContext.

So even in b2g18 you still need to update nsIRilContext.

Does this make sense to you?

Thanks
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #71)
> The rilContext here is from nsIRilContext.
> 
> So even in b2g18 you still need to update nsIRilContext.
> 
> Does this make sense to you?

Ah ha!  So I think that line should probably be removed, it was likely a blind copy'n'paste from the line above it and probably doesn't even work.  In the current implementation the retry Count is only conveyed to the content process in a RIL:CardStateChanged message.
Assignee: fernando.campo → mvines
Attachment #745486 - Flags: review?(fernando.campo)
Attached patch gecko content process stuff (obsolete) — Splinter Review
Gecko content process attempt #2, complete with nsIRilContext goodness.
Attachment #739104 - Attachment is obsolete: true
Attachment #739104 - Flags: review?
Attachment #745487 - Flags: review?(vyang)
Hey Fernando, I'm also seeing an issue in Settings wrt. # tries left, at least on gaia/master.
STR: 
1) Try to enable SIM pin.  It shows 3 tries left
2) Enter the wrong pin.
3) It still shows 3 tries left.
I can see from logcat that 2 tries left is being propagated up to Gaia, and this works fine at boot/FTU.  Want me to create a new bug for this?
Flags: needinfo?(fernando.campo)
Comment on attachment 745487 [details] [diff] [review]
gecko content process stuff

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

Please r=me with two lines in RadioInterfaceLayer.js to initialize `this.rilContext.retryCount` to zero and place a TODO comment for bug 868896 as well.

The way we have in this patch is based on proprietary implementation and we have no idea how to achieve the same thing in MOZ RIL. It's a pity we don't have a open, complete solution here that also works in MOZ RIL. So, I understand this is for tef+, but please also understand I will not grant similar things in m-c next time. We want to cover several devices as long as it's possible, so hiding retry count information internally in CARD_STATE_CHANGED status is only acceptable if that becomes a convention between all the devices. This patch is certainly not the case. We can have a more standard-compatible, supporting all PIN retry counts way for this feature as described in bug 868896.
Attachment #745487 - Flags: review?(vyang) → review+
Addressed requested review changes, and rebased on bug 866272.  Carrying r+ forward.
Attachment #745487 - Attachment is obsolete: true
Attachment #746031 - Flags: review+
attachment 746031 [details] [diff] [review]: 
http://hg.mozilla.org/projects/birch/rev/390c431a2082
Whiteboard: [status: needs updated gecko patch, gaia work already landed] [BTG-1397] → [status: gecko patch landed, gaia needs work] [fixed-in-birch] [KEEP_OPEN]
Attached file Gaia tweaks to make retryCount work (obsolete) —
Attachment #745486 - Attachment is obsolete: true
Attachment #745486 - Flags: review?(fernando.campo)
Attachment #746079 - Flags: review?(fernando.campo)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #75)
> Hey Fernando, I'm also seeing an issue in Settings wrt. # tries left, at
> least on gaia/master.
> STR: 
> 1) Try to enable SIM pin.  It shows 3 tries left
> 2) Enter the wrong pin.
> 3) It still shows 3 tries left.
> I can see from logcat that 2 tries left is being propagated up to Gaia, and
> this works fine at boot/FTU.  Want me to create a new bug for this?

Thanks for the heads up Michael, I'm taking a look asap. 
About opening different bug, if we want to close this one now, and all the patches have the r+, I don't mind new one for it (you can assign it to me directly).
Flags: needinfo?(fernando.campo)
Comment on attachment 746079 [details]
Gaia tweaks to make retryCount work

Thanks for fixing it that quick, I was afk for some days
Attachment #746079 - Flags: review?(fernando.campo) → review+
Comment on attachment 746031 [details] [diff] [review]
Gecko content process stuff

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +46,5 @@
> +   * or 'pukRequired'.  0 denotes the retry count is unavailable.
> +   *
> +   * Value is undefined for other cardState values.
> +   */
> +  readonly attribute long retryCount;

The uuid of the interface should have been updated.

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +46,5 @@
>    void registerMobileConnectionMsg(in nsIMobileConnectionListener listener);
>    void unregisterMobileConnectionMsg(in nsIMobileConnectionListener listener);
>  
>    readonly attribute DOMString cardState;
> +  readonly attribute long retryCount;

The uuid of the interface should have been updated.

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +61,2 @@
>  
>  [scriptable, uuid(f7ff9856-5c55-4ba2-9b64-bfc0ace169e7)]

The uuid of the interface should have been updated.
Want me to land a follow-up Fabrice?
Sorry for the review harassment Fernando.  A change to apps/settings/js/simcard_dialog.js was also needed to ensure that the retry count shows up for PIN enable/disable/changing.

I've also noticed that when the PIN is blocked, the PUK screen is not displayed when "Change PIN" is selected in settings (but PUK screen is displayed when Enable/Disable PIN is selected).  That seems like a bug
Attachment #746079 - Attachment is obsolete: true
Attachment #746164 - Flags: review?(fernando.campo)
Whiteboard: [status: gecko patch landed, gaia needs work] [fixed-in-birch] [KEEP_OPEN] → [status: gaia patch needs r] [fixed-in-birch] [KEEP_OPEN]
(In reply to Michael Vines [:m1] [:evilmachines] from comment #83)
> Want me to land a follow-up Fabrice?

Just landed.
https://hg.mozilla.org/projects/birch/rev/b85b1800cf3f
https://hg.mozilla.org/mozilla-central/rev/390c431a2082
https://hg.mozilla.org/mozilla-central/rev/b85b1800cf3f
Whiteboard: [status: gaia patch needs r] [fixed-in-birch] [KEEP_OPEN] → [status: gaia patch needs r] [fixed-in-birch] [leave open]
Comment on attachment 746164 [details] [diff] [review]
Gaia tweaks to make retryCount work

(In reply to Michael Vines [:m1] [:evilmachines] from comment #84)
> Created attachment 746164 [details] [diff] [review]
> Gaia tweaks to make retryCount work
> 
> Sorry for the review harassment Fernando.  A change to
> apps/settings/js/simcard_dialog.js was also needed to ensure that the retry
> count shows up for PIN enable/disable/changing.
No problem at all, it should be me the one fixing it, so thx again
> 
> I've also noticed that when the PIN is blocked, the PUK screen is not
> displayed when "Change PIN" is selected in settings (but PUK screen is
> displayed when Enable/Disable PIN is selected).  That seems like a bug
Yeah, but seems to me like a different one, not related to this changes. Could you open a new one with steps to repro and I'll take a look?
Attachment #746164 - Flags: review?(fernando.campo) → review+
So do we just need to land the gaia patch now?
Whiteboard: [status: gaia patch needs r] [fixed-in-birch] [leave open] → [status: has r+'d gecko and gaia patches] [fixed-in-birch] [leave open]
https://github.com/mozilla-b2g/gaia/commit/28a2c050414ddecbf8deb64a2b8943f94d353941
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [status: has r+'d gecko and gaia patches] [fixed-in-birch] [leave open]
This bug was partially uplifted.

Uplifted 887cae7e556fc19722011f0674fdd4d4bfeacc3e to:
v1-train: 607690a4b3b12376e1d4ad9ea4e1d37396a7926b
v1.0.1: 69f273fb42b66174d621daaff1fd498775e1b2cc
Uplifted b565d874e9dba9d182c9952500869956365dbba4 to:
v1-train: 216300087cf21bcb9ffb2686172f93bb4ad6dae8
Uplifted 7faefd0273f4dce24563cda57450bfb1c7416b45 to:
v1-train: 78210cf0935e35935eb260ee7a4681bf6f624120
Uplifted 4cd2d491c28e7312a2c070ea4bbaf76442161794 to:
Uplifted 28a2c050414ddecbf8deb64a2b8943f94d353941 to:

Commit b565d874e9dba9d182c9952500869956365dbba4 didn't uplift to branch v1.0.1
Commit 7faefd0273f4dce24563cda57450bfb1c7416b45 didn't uplift to branch v1.0.1
Commit 4cd2d491c28e7312a2c070ea4bbaf76442161794 didn't uplift to branch v1-train
Commit 4cd2d491c28e7312a2c070ea4bbaf76442161794 didn't uplift to branch v1.0.1
Commit 28a2c050414ddecbf8deb64a2b8943f94d353941 didn't uplift to branch v1-train
Commit 28a2c050414ddecbf8deb64a2b8943f94d353941 didn't uplift to branch v1.0.1
(In reply to John Ford [:jhford] -- If you expect a reply from me, use needsinfo? instead of CC from comment #91)
> This bug was partially uplifted.
> ....

This is easier to view as:

Bug    |    Master     |    v1-train     |    v1.0.1
860660 |    887cae7    |     607690a     |    69f273f
       |    b565d87    |     2163000     |     failed
       |    7faefd0    |     78210cf     |     failed
       |    4cd2d49    |      failed     |     failed
       |    28a2c05    |      failed     |     failed

I've backed these out as:
v1-train: 60f1de43dc018757e63715f05bee867fdf527c35
v1.0.1: 30ad443fbe5363a7381c19c6bf9d6ca49228fb8b

Michael, these didn't uplift cleanly atomically so I backed them out.  Could you please handle this uplift, it looks like it's going to need a lot of developer attention.
Flags: needinfo?(mvines)
The gecko patch doesn't uplift cleanly at all either, joy.  

Fernando, I'll make you a deal.  If you rebase the gaia stuff for v1.0.1 and v1-train, I'll test e2e again on these two branches after I rebase the gecko patch.
Flags: needinfo?(mvines) → needinfo?(fernando.campo)
Ok, giving it a try (it's gonna be fun :D)
Flags: needinfo?(fernando.campo)
Ok, I think I managed to uplift the pending commits to the branches. 

commit b565d874e9dba9d182c9952500869956365dbba4	- v1.0.1 - c0b5fb5ebede4b540f07bdb9060833196a81b695
commit 7faefd0273f4dce24563cda57450bfb1c7416b45 - v1.0.1 - aa515def2e92f676e3b5d820fb57dbb89ab5b3b5
commit 4cd2d491c28e7312a2c070ea4bbaf76442161794 - v1.0.1 - c85199c03dd17a168b5bb80a4d2651caecaf4815
commit 28a2c050414ddecbf8deb64a2b8943f94d353941 - v1.0.1 - f1ac50095ba40d336fbbc7b71c59077e2321aeb2

commit 4cd2d491c28e7312a2c070ea4bbaf76442161794 - v1-train - 8c27d942e4cec036e05390b07d8f37049df5cadb
commit 28a2c050414ddecbf8deb64a2b8943f94d353941 - v1-train - 4eadbbe13043807cdb02a049076949e78d87f90d

Bug    |    Master     |    v1-train     |    v1.0.1
860660 |    887cae7    |     607690a     |    69f273f
       |    b565d87    |     2163000     |    c0b5fb5
       |    7faefd0    |     78210cf     |    aa515de
       |    4cd2d49    |     8c27d94     |    c85199c
       |    28a2c05    |     4eadbbe     |    f1ac500

Hope the gecko patch is not giving you much trouble, Michael
Wow, this bug alone is 75 new strings :-(
(Gecko patches haven't landed yet so fixed->affected for v1.0.1/v1.1)
RyanVM -- lmk if these two gecko patches don't stick
Flags: needinfo?(ryanvm)
Hey Fernardo -- on v1-train with your patches, I get a blank dialog at boot with a PIN-enable SIM where the enter pin screen should be (with or without the gecko patches in this bug).  Maybe an issue with the uplift?  I didn't have time today to debug it or check v1.0.1 though.  Would you be able to take a look?   PIN toggle/change in Settings seems ok though after a quick smoke test
Flags: needinfo?(fernando.campo)
Sure, I'm taking care of it. Thanks for the heads up
Flags: needinfo?(fernando.campo)
Got it, it was my fault. When I jhford wrote he backed out the commits, I misunderstood he was referring to the ones that created conflict, not to all of them, and I only uplifted the pending ones.

I'm backing out the ones I uplifted yesterday and doing it from scratch, sorry for the mess :(
I'm lost as to what needs doing here.
Flags: needinfo?(ryanvm)
Hope I got this finally right...

Bug    |    Master     |    v1-train     |    v1.0.1
860660 |    887cae7    |    e5789e0e     |   19cbb45
       |    b565d87    |    0cf1bf48     |   21ffab4
       |    7faefd0    |    df152873     |   2c7227c
       |    4cd2d49    |    3c81ae64     |   78176f8
       |    28a2c05    |    1542f9e6     |   9483697

@Michael, I made a quick smoke test on both v1-train and v1.0.1 and seems to be working as expected (without gecko patch landed of course)

@RyanVM, I think that Michael wanted you to take a look at the patches on comment 98 and comment 99
(In reply to Fernando Campo (:fcampo) from comment #105)
> @RyanVM, I think that Michael wanted you to take a look at the patches on
> comment 98 and comment 99

Yes please.  Those are rebased versions of the stuff in comment 86 for gecko v1-train and v1.0.1
Firstly, thanks for your hardworking;
And there's something I should tell you: there is still no retrycount after I send **04*wrongoldpin*newpin*newpin# in the dialer. Could you have a look at this? Thanks.
Flags: needinfo?(fernando.campo)
Please provide the android main logs and the radio logs for the issue
 adb logcat -b radio -b main -v time

Also let us know where do you expect to see the retry count after you entered the PIN change MMI code with incorrect PIN.
Let's create a new bug for comment 109 please
Attached file adb log
there should be a retryCount alert in the picture too.
Created 874000
Flags: needinfo?(fernando.campo)
After testing a few devices:
Unari V1.1 and Inari V1.01
Prompt error message appears only after the incorrect PUK PIN is entered
The error message disappears as soon as a user exited from the "PUK PIN menu"

Unable to test on Buri and Leo device, blocked by bug 876010

Environmental  Variables:
Unagi Build ID: 20130530070208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/09ac1fd2959c
Gaia: 1cca9324d4444ad28c6fa99875e17abf7e8230be

Environmental  Variables:
Inari Build ID: 20130530070213
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/11b55d3ada71
Gaia: ac293ce59acc3bede083fad1b973794fa8bf0253
Comment from Mozilla:After testing a few devices:
 Unari V1.1 and Inari V1.01
 Prompt error message appears only after the incorrect PUK PIN is entered
 The error message disappears as soon as a user exited from the "PUK PIN menu"
 
 Unable to test on Buri and Leo device, blocked by bug 876010
 
 Environmental  Variables:
 Unagi Build ID: 20130530070208
 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/09ac1fd2959c
 Gaia: 1cca9324d4444ad28c6fa99875e17abf7e8230be
 
 Environmental  Variables:
 Inari Build ID: 20130530070213
 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/11b55d3ada71
 Gaia: ac293ce59acc3bede083fad1b973794fa8bf0253
Verified fixed on Buri 1.0.1 Build ID: 20130621070204

Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia: 93241eb6c5d6c110710fad8da3ccd4423312b0c9
Platform Version: 18.0

Also verified fixed on Leo Build ID: 20130621070212

Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/a34f6d62cb05
Gaia: cca61224e6df8e9f7e450d77cb6a8cf91029be64
Platform Version: 18.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.