951586 - [DSDS][STK] Suport multiple cards in system STK code

Status: VERIFIED FIXED

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

Description

[PabloYglesias]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

To solve the bug 934404, we have divided into three parts.

In the first, we need Gaia to work with multiple cards, rewriting the code that handles icc, and it depends on a IccId.

message object will be formed by command and ICCID to change handleSTKCommand.


Actual results:

No dual SIM management.


Expected results:

Prepare Gaia to work with dual SIM.

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Added STK DSDS support on system app

Hi,

This patch implements STK DSDS support into System app (settings app is still pending work)

I tested with all commands but since it's a big change, I would like to ask you (:cyang) for pass all STK conformance test again.

Thanks in advance.

Comment 3

[Nivi]

Comment on attachment 8361612 [details] [review]
Added STK DSDS support on system app

Verified.

Comment 4

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nivi from comment #3)
> Comment on attachment 8361612 [details] [review]
> Added STK DSDS support on system app
> 
> Verified.

Thank you !

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8361612 [details] [review]
Added STK DSDS support on system app

Could you move this into an app? We have talked about this before.

I don't think it makes any sense to land more code into System app for STK.

Comment 6

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #5)
> Comment on attachment 8361612 [details] [review]
> Added STK DSDS support on system app
> 
> Could you move this into an app? We have talked about this before.
> 
> I don't think it makes any sense to land more code into System app for STK.

Hi Tim,

As we commented here https://bugzilla.mozilla.org/show_bug.cgi?id=875679#c13 this code should live in system.

Adding to the commented advantages above, we shall add the certification process which is very strict, for example, we shall display messages over any app when the SIM card requires it (also over the dialer) or the app should be permanent opened in case SIM card use events.

Moreover, this patch is no "new code", it just covers the required changes to support DSDS.

We can consider some enhancements in a near future to lazy load required modules, for example, if an Operator SIM Card doesn't use events, the events module won't be loaded; also we can lazy load per STK command and load in memory only operator required ones. Anyway it would be another different bug.

Regards.

Comment 7

[Noemí Freire (:noemi)]

Hi Tim,

Could you please confirm if we can move forward with the current proposal?.Thanks!.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Noemí Freire (:noemi) from comment #7)
> Hi Tim,
> 
> Could you please confirm if we can move forward with the current
> proposal?.Thanks!.

Bug 875679 comment 13 is almost 8 months ago. I am very concerned, since there were no action done nor even any bug filed.

Given the fact this patch is relatively small, compare to the entire STK code, and this bug is sit on the 1.4 target milestone, I am going to let it slide one last time. However for next releases I want to see effort being made on your side to address this.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8361612 [details] [review]
Added STK DSDS support on system app

Please correct the l10n IDs before landing.

Comment 10

[Daniel Coloma:dcoloma]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #8)

Hey Tim,

 Thanks for clarifying what the issue in here: you want STK to be an independent app, and I agree. However, let me try to clarify something:

 - We did not request STK to be part of settings instead of a separated app, this was something Mozilla UX decided.
 - We developed the STK implementation for 1.0 as it was requested by Mozilla because of the Mozilla lack of resources.
 - We did not like original UX or the technical approach either, but both decisions were not taken by us: The first was a Mozilla UX decision, the second one due to platform limitations.
 - We have adapted STK to support dual SIM because it was a request from other partner, not because this is interesting to us. To be clear: we are not selling any device with dual SIM.

Don't get me wrong, I just want to be clear as I have the impression you feel this is how we want this feature to be, and that is not the case.

> Given the fact this patch is relatively small, compare to the entire STK
> code, and this bug is sit on the 1.4 target milestone, I am going to let it
> slide one last time. However for next releases I want to see effort being
> made on your side to address this.

OK, I think this is going to be the last time you need to let slide it: now it is an excellent time Mozilla can do the refactor of this code as we all always wanted to. You have a very good starting point: a working implementation, supporting dual SIM and that passes certification/IOT tests.

My last contribution to this STK thread is one recommendation and one request:

 - Recommendation: Align your independent app approach early with to your UX team to validate it. 
 - Request: Ensure no certification requirements are broken because of the re-factor, otherwise this will cause problems (and delays) during IOT.

Thanks!

Comment 11

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #9)
> Comment on attachment 8361612 [details] [review]
> Added STK DSDS support on system app
> 
> Please correct the l10n IDs before landing.

Fixed and landed:

https://github.com/mozilla-b2g/gaia/commit/78a6be16d018148eaed880ddae51a86d933acbdc

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Daniel Coloma:dcoloma from comment #10)

Thanks. I will be bringing these comments to relevant people.

Comment 13

[Axel Hecht [:Pike]]

Can you attach a screenshot of this code in action? I'm wondering if the length of the strings is a concern.

Comment 14

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: screenshot.png

Comment 15

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Axel Hecht [:Pike] from comment #13)
> Can you attach a screenshot of this code in action? I'm wondering if the
> length of the strings is a concern.

attached in comment #14

Comment 16

[Axel Hecht [:Pike]]

Thanks. I hope that's gonna work out with the length of localized strings.

Comment 18

[Eric Chang [:ericcc] [:echang]]

Works okay on Fugu
GAIA_REV=730670951e40b2317a167fcd07c398bb662d6e87
GECKO_REV=ce6783cbe8ffeaea447ac7a5b9b11c2c4881f194
BUILD_TAG=jenkins-B2G.v1.4.0.fugu-4
BuildID=20140324140010

Comment 19

[James Zhang (Spreadtrum)]

Is it possible to cherry-pick it to v1.3t?

Comment 20

[Axel Hecht [:Pike]]

That would add new strings to 1.3(t), we shouldn't do that.