942908 - [DSDS] Remove IccHelper from FTE sim_manager.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

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

Comment 2

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 4

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 5

[Borja Salguero [:borjasalguero]]

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

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Comment 7

[Michael Henretty [:mikehenrty][:mhenretty]]

Thanks for the comments Francisco, EJ and Arthur. I've updated the PR and responded to some of the comments inline.

Comment 8

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

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.

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

master: https://github.com/mozilla-b2g/gaia/commit/041206149b26d40545f7788f48cced09300f19f5

Comment 10

[Fernando Campo (:fcampo)]

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?

Comment 11

[Michael Henretty [:mikehenrty][: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.

Comment 12

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

closing, per https://bugzilla.mozilla.org/show_bug.cgi?id=942908#c12