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

Status: VERIFIED FIXED

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

Description

[sync-1]

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:

Comment 1

[sync-1]

 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.

Comment 2

[xiaokang.chen]

Attachment: pics

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

This looks like it might require new strings

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

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).

Comment 5

[Daniel Coloma:dcoloma]

(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)

Comment 6

[xiaokang.chen]

(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.

Comment 7

[Alex Keybl [:akeybl]]

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.

Comment 8

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

For this bug to be fixed, we'll need to make some changes on our side that will require an SR to be opened.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 10

[Dietrich Ayala (:dietrich)]

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

Michael, when will the RIL pieces be available?

Comment 11

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

Please let us know what message we should send to the content process?   An extension to 'cardstatechange'?

Comment 12

[Fernando Campo (:fcampo)]

Attachment: proposed

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?

Comment 13

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

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.

Comment 14

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

However let's also make the retryCount field optional for the pinRequired/pukRequired card states, to gracefully degrade if this information is not available

Comment 15

[Fernando Campo (:fcampo)]

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 :)

Comment 16

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

Having it accessible via API would be fine too.  Happy to adapt to whatever is easiest from the pov of the content process

Comment 17

[Anshul]

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.

Comment 18

[xiaokang.chen]

(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?

Comment 19

[Fernando Campo (:fcampo)]

(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

Comment 20

[Fernando Campo (:fcampo)]

Attachment: [System] Pull Request

As I'm not sure who should review the part for system, I'm putting Etienne as a proxy

Comment 21

[Fernando Campo (:fcampo)]

I'm also adding UX Sergi to the loop, as the changes affect UI and the re-design from bug 852682 (comment 19)

Comment 22

[Etienne Segonzac (:etienne)]

Comment on attachment 738455 [details]
[System] Pull Request

Quick comment on github.
Also adding :kaze to the review for the advanced l10n stuff.

Comment 23

[Fernando Campo (:fcampo)]

Attachment: [FTU] Pull Request

Comment 24

[Francisco Jordano [:arcturus] [:francisco]]

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.

Comment 25

[Sergi]

Attachment: 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.

Comment 26

[Sergi]

Attachment: PUK Entry Code Flow

Comment 27

[Fernando Campo (:fcampo)]

Attachment: [system] Pull Request

Comment 28

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

Attachment: nsIDOMMobileConnection support for 'retryCount'

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

Comment 29

[Etienne Segonzac (:etienne)]

Comment on attachment 739094 [details]
[system] Pull Request

r=me with the small nit addressed for the non-l10n parts.

Comment 30

[Fernando Campo (:fcampo)]

Attachment: [settings] Pull Request

Comment 31

[Sergi]

Attachment: PUK Entry Code Flow

I've updated the PUK entry flow to place the PUK+New PIN entry fields in the same screen.

Comment 32

[Fabien Cazenave [:kaze]]

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.

Comment 33

[Andrew Overholt [:overholt]]

Josh, please provide UX input on attachment in comment 31.

Comment 34

[Josh Carpenter [:jcarpenter]]

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.

Comment 35

[Sergi]

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.

Comment 36

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

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.

Comment 37

[Fernando Campo (:fcampo)]

Merged settings patch: https://github.com/mozilla-b2g/gaia/commit/4cd2d491c28e7312a2c070ea4bbaf76442161794

Comment 38

[Fernando Campo (:fcampo)]

Merged system patch: https://github.com/mozilla-b2g/gaia/commit/e5b866b2c0118a00319ced69c8adfd158fdc158f

Comment 39

[Fernando Campo (:fcampo)]

Merged ftu patch: https://github.com/mozilla-b2g/gaia/commit/7faefd0273f4dce24563cda57450bfb1c7416b45

Comment 40

[Fernando Campo (:fcampo)]

Attachment: [system] Pull Request

New pull request to fix the revert done to original, due to the discovery of a bug.

Comment 41

[Francisco Jordano [:arcturus] [:francisco]]

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!

Comment 42

[Fernando Campo (:fcampo)]

Merged system patch: https://github.com/mozilla-b2g/gaia/commit/b565d874e9dba9d182c9952500869956365dbba4
Forget comment 38, as that one is not valid anymore

Comment 43

[Fernando Campo (:fcampo)]

Attachment: [dialer] Pull Request

Comment 44

[Etienne Segonzac (:etienne)]

Comment on attachment 740732 [details]
[dialer] Pull Request

Comment on github, otherwise it looks good :)

Comment 45

[Fernando Campo (:fcampo)]

Comment on attachment 740732 [details]
[dialer] Pull Request

Fixed the private variable addressed in github comment

Comment 46

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

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.

Comment 47

[Etienne Segonzac (:etienne)]

Comment on attachment 740732 [details]
[dialer] Pull Request

Sweet and simple :)

Comment 48

[Fernando Campo (:fcampo)]

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.

Comment 49

[Fernando Campo (:fcampo)]

So all the gaia patches are ready, only need the approval for Michael's backend one :)

Comment 50

[Alex Keybl [:akeybl]]

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!

Comment 51

[Vicamo Yang [:vicamo][: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.

Comment 52

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

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.

Comment 53

[Alex Keybl [:akeybl]]

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).

Comment 54

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

(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.

Comment 55

[Vicamo Yang [:vicamo][: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.

Comment 56

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

(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.

Comment 57

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

(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

Comment 58

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

(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!

Comment 59

[Alex Keybl [:akeybl]]

Yoshi - do we need a separate bug on file for comment 57?

Comment 60

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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?

Comment 61

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

(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.

Comment 62

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 63

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

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 :)

Comment 64

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 65

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

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.

Comment 66

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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.

Comment 67

[Fernando Campo (:fcampo)]

(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

Comment 68

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

(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. :)

Comment 69

[Andrew Overholt [:overholt]]

(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)

Comment 70

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

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.

Comment 71

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

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

Comment 72

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

(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.

Comment 73

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

Attachment: Gaia tweaks to get retryCount actually working at boot.

Comment 74

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

Attachment: gecko content process stuff

Gecko content process attempt #2, complete with nsIRilContext goodness.

Comment 75

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

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?

Comment 76

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

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.

Comment 77

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

Attachment: Gecko content process stuff

Addressed requested review changes, and rebased on bug 866272.  Carrying r+ forward.

Comment 78

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

attachment 746031 [details] [diff] [review]: 
http://hg.mozilla.org/projects/birch/rev/390c431a2082

Comment 79

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

Attachment: Gaia tweaks to make retryCount work

Comment 80

[Fernando Campo (:fcampo)]

(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).

Comment 81

[Fernando Campo (:fcampo)]

Comment on attachment 746079 [details]
Gaia tweaks to make retryCount work

Thanks for fixing it that quick, I was afk for some days

Comment 82

[[:fabrice] Fabrice Desré]

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.

Comment 83

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

Want me to land a follow-up Fabrice?

Comment 84

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

Attachment: 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.

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

Comment 85

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

(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

Comment 86

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/390c431a2082
https://hg.mozilla.org/mozilla-central/rev/b85b1800cf3f

Comment 87

[Fernando Campo (:fcampo)]

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?

Comment 88

[Andrew Overholt [:overholt]]

So do we just need to land the gaia patch now?

Comment 89

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

https://github.com/mozilla-b2g/gaia/commit/28a2c050414ddecbf8deb64a2b8943f94d353941

Comment 90

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

Dear uplifters,

For the Gaia uplifts, please cherry pick these 
https://github.com/mozilla-b2g/gaia/commit/887cae7e556fc19722011f0674fdd4d4bfeacc3e
https://github.com/mozilla-b2g/gaia/commit/b565d874e9dba9d182c9952500869956365dbba4
https://github.com/mozilla-b2g/gaia/commit/7faefd0273f4dce24563cda57450bfb1c7416b45
https://github.com/mozilla-b2g/gaia/commit/4cd2d491c28e7312a2c070ea4bbaf76442161794

and then
https://github.com/mozilla-b2g/gaia/commit/28a2c050414ddecbf8deb64a2b8943f94d353941
to avoid (worse?) merge conflicts.

The needed Gecko uplifts are at comment 86.

Comment 91

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

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

Comment 92

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

(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.

Comment 93

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

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.

Comment 94

[Fernando Campo (:fcampo)]

Ok, giving it a try (it's gonna be fun :D)

Comment 95

[Fernando Campo (:fcampo)]

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

Comment 96

[Axel Hecht [:Pike]]

Wow, this bug alone is 75 new strings :-(

Comment 97

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

(Gecko patches haven't landed yet so fixed->affected for v1.0.1/v1.1)

Comment 98

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

Attachment: v1-train/v1.0.1 version, part I

Rebased https://hg.mozilla.org/mozilla-central/rev/390c431a2082 that should apply to v1-train/v1.0.1

Comment 99

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

Attachment: v1-train/v1.0.1 version, part 2

Rebased https://hg.mozilla.org/projects/birch/raw-rev/b85b1800cf3f that should apply to v1-train/v1.0.1

Comment 100

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

RyanVM -- lmk if these two gecko patches don't stick

Comment 101

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

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

Comment 102

[Fernando Campo (:fcampo)]

Sure, I'm taking care of it. Thanks for the heads up

Comment 103

[Fernando Campo (:fcampo)]

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 :(

Comment 104

[Ryan VanderMeulen [:RyanVM]]

I'm lost as to what needs doing here.

Comment 105

[Fernando Campo (:fcampo)]

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

Comment 106

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

(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

Comment 107

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/54313cfdfa6e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/93fe79b30391

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/7f9601a3ded0
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/57fc01b93bfa

Comment 108

[Ryan VanderMeulen [:RyanVM]]

And a fix so these will actually build.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0ae052f5dec7
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0cd2aaaaf6c7

Comment 109

[xiaokang.chen]

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.

Comment 110

[Anshul]

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.

Comment 111

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

Let's create a new bug for comment 109 please

Comment 112

[xiaokang.chen]

Attachment: adb log

Comment 113

[xiaokang.chen]

Attachment: when sending incorrect old pin from the dialer

there should be a retryCount alert in the picture too.

Comment 114

[Fernando Campo (:fcampo)]

Created 874000

Comment 115

[sarsenyev]

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 116

[sync-1]

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

Comment 117

[Sarah Parsons]

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