[DSDS] Remove IccHelper from FTE sim_manager.js

RESOLVED FIXED in 1.3 Sprint 6 - 12/6

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mikehenrty, Assigned: mikehenrty)

Tracking

unspecified
1.3 Sprint 6 - 12/6

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment)

The IccHelper is used to abstract away details of having multiple SIMs. Since we now care about those details, we need to get rid of the helper and rely on the raw API underneath.
Created attachment 8338234 [details] [review]
Remove IccHelper from SimManager

Arthur, I'm starting the process of removing this from FTE since we have dual sim support now. Will you review this first step for me?
Attachment #8338234 - Flags: review?(arthur.chen)
Comment on attachment 8338234 [details] [review]
Remove IccHelper from SimManager

Michael, the patch looks good to me but I'm afraid that I am not able to review the patch as it is FTU app. Please check my comments in github.
Attachment #8338234 - Flags: review?(arthur.chen) → feedback+
Comment on attachment 8338234 [details] [review]
Remove IccHelper from SimManager

Borja,

Could you take a look at this PR. It is to decouple the SIM Manager in FTU from the IccHelper. The next step is adding additional SIM unlocking/info screens to the FTU, but I'm doing that in bug 926347.
Attachment #8338234 - Flags: review?(borja.bugzilla)
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Comment on attachment 8338234 [details] [review]
Remove IccHelper from SimManager

Looks like EJ has done some work with FTU and DSDS changes. Perhaps he could pick up this review as well.
Attachment #8338234 - Flags: review?(ejchen)
Comment on attachment 8338234 [details] [review]
Remove IccHelper from SimManager

Forwarding the review as this is related with DSDS and Francisco was working on it!  :)
Attachment #8338234 - Flags: review?(borja.bugzilla) → review?(francisco.jordano)
Comment on attachment 8338234 [details] [review]
Remove IccHelper from SimManager

Hi Michael, 

I can just give you some feedbacks because I am not reviewer haha. 

I left a comment on Github and it all looks nice ! Thanks :D
Attachment #8338234 - Flags: review?(ejchen) → feedback+
Thanks for the comments Francisco, EJ and Arthur. I've updated the PR and responded to some of the comments inline.
Comment on attachment 8338234 [details] [review]
Remove IccHelper from SimManager

Thanks!

Please merge once travis is ok, right now is red with this PR.

Cheers!
F.
Attachment #8338234 - Flags: review?(francisco.jordano) → review+
master: https://github.com/mozilla-b2g/gaia/commit/041206149b26d40545f7788f48cced09300f19f5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
The patch succesfully eliminates IccHelper from SimManager, but the scope of the bug is to remove it completely from FTU, right?
I'm just asking cause working on other bug I keep finding references to IccHelper on the FTU code [https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/ftu/js/navigation.js#L269], and I was wondering if maybe the bug shouldn't be closed yet?
Flags: needinfo?(mhenretty)
(In reply to Fernando Campo (:fcampo) from comment #10)
> The patch succesfully eliminates IccHelper from SimManager, but the scope of
> the bug is to remove it completely from FTU, right?
> I'm just asking cause working on other bug I keep finding references to
> IccHelper on the FTU code
> [https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/ftu/js/
> navigation.js#L269], and I was wondering if maybe the bug shouldn't be
> closed yet?

Yup, you are right, this should not be closed yet.
No longer blocks: 926347
Status: RESOLVED → REOPENED
Flags: needinfo?(mhenretty)
Resolution: FIXED → ---
Depends on: 949602
After looking into this more, I think the risk/reward ratio is very low for me to remove IccHelper entirely from ftu. We use it in the operator variant stuff, which is quite complex and not something I worked with directly. My thought is that whoever updates operator variant for DSDS purposes should naturally remove the IccHelper (since IccHelper is a stop gap for changing over to dual sim).

So if no one objects, I would like to change this bug to "Remove IccHelper from sim_manager.js", and mark resolved.
closing, per https://bugzilla.mozilla.org/show_bug.cgi?id=942908#c12
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Summary: [DSDS] Remove IccHelper from FTE → [DSDS] Remove IccHelper from FTE sim_manager.js
You need to log in before you can comment on or make changes to this bug.