Closed
Bug 860660
Opened 9 years ago
Closed 9 years ago
[Buri][TEF check]PIN & PUK Code:Should show the counter of trials remain.
Categories
(Firefox OS Graveyard :: Gaia, defect, P1)
Tracking
(blocking-b2g:tef+, 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
|
m1
:
review+
|
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.
Comment 2•9 years ago
|
||
Comment 3•9 years ago
|
||
This looks like it might require new strings
Comment 4•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
(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.
Updated•9 years ago
|
blocking-b2g: --- → tef?
Updated•9 years ago
|
Assignee: nobody → ferjmoreno
Updated•9 years ago
|
Assignee: ferjmoreno → fernando.campo
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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]
Assignee | ||
Comment 11•9 years ago
|
||
Please let us know what message we should send to the content process? An extension to 'cardstatechange'?
Comment 12•9 years ago
|
||
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?
Assignee | ||
Updated•9 years ago
|
Whiteboard: [status: no patch, needs ETA] → [status: no patch, needs ETA] [BTG-1397]
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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 :)
Assignee | ||
Comment 16•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
As I'm not sure who should review the part for system, I'm putting Etienne as a proxy
Attachment #738455 -
Flags: review?(etienne)
Comment 21•9 years ago
|
||
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 22•9 years ago
|
||
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)
Comment 23•9 years ago
|
||
Attachment #738573 -
Flags: review?(francisco.jordano)
Comment 24•9 years ago
|
||
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+
Comment 25•9 years ago
|
||
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
Comment 26•9 years ago
|
||
Comment 27•9 years ago
|
||
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)
Assignee | ||
Comment 28•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Attachment #739104 -
Flags: feedback?
Comment 29•9 years ago
|
||
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+
Comment 30•9 years ago
|
||
Attachment #739163 -
Flags: review?(kaze)
Comment 31•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #738573 -
Flags: review?(kaze) → review+
Updated•9 years ago
|
Attachment #739094 -
Flags: review?(kaze) → review+
Updated•9 years ago
|
Attachment #739163 -
Flags: review?(kaze) → review+
Comment 32•9 years ago
|
||
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•9 years ago
|
||
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]
Comment 34•9 years ago
|
||
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)
Comment 35•9 years ago
|
||
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.
Assignee | ||
Comment 36•9 years ago
|
||
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•9 years ago
|
||
Merged settings patch: https://github.com/mozilla-b2g/gaia/commit/4cd2d491c28e7312a2c070ea4bbaf76442161794
Comment 38•9 years ago
|
||
Merged system patch: https://github.com/mozilla-b2g/gaia/commit/e5b866b2c0118a00319ced69c8adfd158fdc158f
Comment 39•9 years ago
|
||
Merged ftu patch: https://github.com/mozilla-b2g/gaia/commit/7faefd0273f4dce24563cda57450bfb1c7416b45
Comment 40•9 years ago
|
||
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 41•9 years ago
|
||
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+
Comment 42•9 years ago
|
||
Merged system patch: https://github.com/mozilla-b2g/gaia/commit/b565d874e9dba9d182c9952500869956365dbba4 Forget comment 38, as that one is not valid anymore
Updated•9 years ago
|
Whiteboard: [status: needs UX feedback] [BTG-1397] → [status: has r+, needs to land] [BTG-1397]
Updated•9 years ago
|
status-b2g18:
--- → affected
status-b2g18-v1.0.1:
--- → affected
Comment 43•9 years ago
|
||
Attachment #740732 -
Flags: review?(etienne)
Comment 44•9 years ago
|
||
Comment on attachment 740732 [details]
[dialer] Pull Request
Comment on github, otherwise it looks good :)
Attachment #740732 -
Flags: review?(etienne)
Comment 45•9 years ago
|
||
Comment on attachment 740732 [details]
[dialer] Pull Request
Fixed the private variable addressed in github comment
Attachment #740732 -
Flags: review?(etienne)
Assignee | ||
Comment 46•9 years ago
|
||
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 47•9 years ago
|
||
Comment on attachment 740732 [details]
[dialer] Pull Request
Sweet and simple :)
Attachment #740732 -
Flags: review?(etienne) → review+
Comment 48•9 years ago
|
||
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•9 years ago
|
||
So all the gaia patches are ready, only need the approval for Michael's backend one :)
Comment 50•9 years ago
|
||
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 51•9 years ago
|
||
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)
Assignee | ||
Comment 52•9 years ago
|
||
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•9 years ago
|
||
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)
Comment 54•9 years ago
|
||
(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)
Comment 55•9 years ago
|
||
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.
Assignee | ||
Comment 56•9 years ago
|
||
(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
Assignee | ||
Comment 58•9 years ago
|
||
(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•9 years ago
|
||
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)
Assignee | ||
Comment 61•9 years ago
|
||
(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
Assignee | ||
Comment 63•9 years ago
|
||
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
Assignee | ||
Comment 65•9 years ago
|
||
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.
Comment 67•9 years ago
|
||
(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. :)
Comment 69•9 years ago
|
||
(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)
Updated•9 years ago
|
Whiteboard: [status: has r+, needs to land] [BTG-1397] → [status: needs updated gecko patch, gaia work already landed] [BTG-1397]
Assignee | ||
Comment 70•9 years ago
|
||
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
Assignee | ||
Comment 72•9 years ago
|
||
(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 | ||
Updated•9 years ago
|
Assignee: fernando.campo → mvines
Assignee | ||
Comment 73•9 years ago
|
||
Attachment #745486 -
Flags: review?(fernando.campo)
Assignee | ||
Comment 74•9 years ago
|
||
Gecko content process attempt #2, complete with nsIRilContext goodness.
Attachment #739104 -
Attachment is obsolete: true
Attachment #739104 -
Flags: review?
Attachment #745487 -
Flags: review?(vyang)
Assignee | ||
Comment 75•9 years ago
|
||
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 76•9 years ago
|
||
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+
Assignee | ||
Comment 77•9 years ago
|
||
Addressed requested review changes, and rebased on bug 866272. Carrying r+ forward.
Attachment #745487 -
Attachment is obsolete: true
Attachment #746031 -
Flags: review+
Assignee | ||
Comment 78•9 years ago
|
||
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]
Assignee | ||
Comment 79•9 years ago
|
||
Attachment #745486 -
Attachment is obsolete: true
Attachment #745486 -
Flags: review?(fernando.campo)
Attachment #746079 -
Flags: review?(fernando.campo)
Comment 80•9 years ago
|
||
(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 81•9 years ago
|
||
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 82•9 years ago
|
||
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.
Assignee | ||
Comment 83•9 years ago
|
||
Want me to land a follow-up Fabrice?
Assignee | ||
Comment 84•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Whiteboard: [status: gecko patch landed, gaia needs work] [fixed-in-birch] [KEEP_OPEN] → [status: gaia patch needs r] [fixed-in-birch] [KEEP_OPEN]
Comment 85•9 years ago
|
||
(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•9 years ago
|
||
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 87•9 years ago
|
||
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+
Comment 88•9 years ago
|
||
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]
Assignee | ||
Comment 89•9 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/28a2c050414ddecbf8deb64a2b8943f94d353941
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [status: has r+'d gecko and gaia patches] [fixed-in-birch] [leave open]
Assignee | ||
Comment 90•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
(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)
Assignee | ||
Comment 93•9 years ago
|
||
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)
Comment 95•9 years ago
|
||
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•9 years ago
|
||
Wow, this bug alone is 75 new strings :-(
Assignee | ||
Comment 97•9 years ago
|
||
(Gecko patches haven't landed yet so fixed->affected for v1.0.1/v1.1)
Assignee | ||
Comment 98•9 years ago
|
||
Rebased https://hg.mozilla.org/mozilla-central/rev/390c431a2082 that should apply to v1-train/v1.0.1
Assignee | ||
Comment 99•9 years ago
|
||
Rebased https://hg.mozilla.org/projects/birch/raw-rev/b85b1800cf3f that should apply to v1-train/v1.0.1
Assignee | ||
Comment 100•9 years ago
|
||
RyanVM -- lmk if these two gecko patches don't stick
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 101•9 years ago
|
||
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)
Comment 102•9 years ago
|
||
Sure, I'm taking care of it. Thanks for the heads up
Flags: needinfo?(fernando.campo)
Comment 103•9 years ago
|
||
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 105•9 years ago
|
||
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
Assignee | ||
Comment 106•9 years ago
|
||
(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•9 years ago
|
||
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
status-b2g18-v1.0.0:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → wontfix
status-firefox23:
--- → fixed
Comment 108•9 years ago
|
||
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•9 years ago
|
||
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)
Comment 110•9 years ago
|
||
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.
Assignee | ||
Comment 111•9 years ago
|
||
Let's create a new bug for comment 109 please
Comment 112•9 years ago
|
||
Comment 113•9 years ago
|
||
there should be a retryCount alert in the picture too.
Comment 115•9 years ago
|
||
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
Reporter | ||
Comment 116•9 years ago
|
||
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•9 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•