886238 - Use IccManager for retrieving unlock retry count

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This bug is for landing the Gaia changes for bug 875710.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Pull request: https://github.com/mozilla-b2g/gaia/pull/10561

These changes depend on bug 875710.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I couldn't really test the changes to the settings app, because the respective dialog seems broken.

Comment 3

[Fernando Campo (:fcampo)]

Comment on attachment 766577 [details]
Pull request: https://github.com/mozilla-b2g/gaia/pull/10561

I can't approve the patch till the issues with cck, nck, spck are solved, as we shouldn't return 0 retries everytime. The rest of the code looks good though, but probably needs some rebasing, or taking into account bug 884274 as the Travis is failing on all the icc tests.

Please reassign the review to me again when the problems are solved, and thanks for your work :)

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Pull request: https://github.com/mozilla-b2g/gaia/pull/10639

Hi Fernando,

I added support for the remaining locks.

I also added a wrapper to IccHelper for supporting both, mozilla-central and b2g18. I couldn't find a way to support both interfaces, so the wrapper either uses the old interface or returns the default value. From a practical point of view it's not much of a problem, because implementation currently only returns the default anyway. If you know how to wrap both interfaces in IccHelper, I'd be happy to hear about it.

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Pull request: https://github.com/mozilla-b2g/gaia/pull/10661

I was able to support both APIs by using callback functions.

Comment 6

[Fernando Campo (:fcampo)]

Hi Thomas, sorry for the delay but I was on PTO. I'm taking a look now, and before going deep and try some things, I see that you didn't follow the IccHelper current code style. Is there a particular reason for it? Is it what you talk about in comment 4?
I'd like some more info about this before getting into it

Thanks

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

No problem, I was on PTO as well! :)

At first I had problems with building an IccHelper interface that could support both, new and old, get-retries interface. I finally settled with a caller-supplied function for returning the number of retries to the app. That's what comments 4 and 5 are about.

For a cleanup, I can try to put the function getCardLockRetryCount into IccHelper, although this didn't work correctly when I tried it at first. I'm usually not a Gaia dev, so let my know if there is something else wrong with the style.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I cleaned up IccHelper and updated the pull request. Please have a look.

Comment 9

[Fernando Campo (:fcampo)]

Comment on attachment 768211 [details]
Pull request: https://github.com/mozilla-b2g/gaia/pull/10661

I finished the review and testing, and works nice. Code looks good too, except for the rebase needed (comment on github), and the failing tests.

Once you fix that, you got the r+ :)

Thanks

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

Thanks for the review.

You mean these Travis tests, right? Do you have an idea why they fail? The code seems correct and works in practice, just not in the test.

Comment 11

[Fernando Campo (:fcampo)]

HiThomas, 

yes, I mean the Travis tests. They are failing because the code of the classes involved changed (adding new function on IccHelper and calling it from different classes), but the tests are not updated with this new functions.

I guess that you need to change the mock_icc_helper.js both on system and ftu folder, and add support for the new function.

Cheers

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 768211 [details]
Pull request: https://github.com/mozilla-b2g/gaia/pull/10661

I rebased the patch set and fixed the broken tests.

Comment 13

[Fernando Campo (:fcampo)]

Comment on attachment 768211 [details]
Pull request: https://github.com/mozilla-b2g/gaia/pull/10661

The patch looks good and works perfectly, so got the r+

Anyway, I left some comments on github for better style and prepare code for future tests. You don't have to make any changes, as the patch works nicely, but it would be a nice to have if you have time for it.

Also a comment on the iccHelper (iccManager.getCardStateRetryCount.onerror) to avoid possible buggy situations.

And finally, wether you make the changes or not, don't forget to squash the commits in one, and add r=me on the final commit when merging.

Thanks for your work!

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Oh great, thanks! I'll have a look at the comments and squash the commit tomorrow. Can you merge the final patch for me? I don't think I have commit rights for Gaia.

Comment 15

[Fernando Campo (:fcampo)]

Sure no prob

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I updated the pull request according to your suggestions. I also moved the function getCardLockRetryCount into IccHelper. I actually wanted to do this in the previous iteration of the patch, but it seems as if the change got lost somehow. Please have a finally look and merge the pull request if appropriate. There is a Travis error, but it seems unrelated.

Comment 17

[Fernando Campo (:fcampo)]

Merged on master 27526f86cea54e4c6997adebfd592b9c02e15421

Many thanks Thomas