928325 - [DSDS][Gaia] WebIccManager API will be changed for DSDS, so all relevant Apps has to be modified accordingly.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Ken Chang[:kenkai]]

WebIccManager API has changed for DSDS, so contacts app has to be modified accordingly.

Comment 1

[Ken Chang[:kenkai]]

Joe, this bug needs help from Comms App

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This bug is for backward compatibility and new API detection so that bug 814637 won't break gaia.

Comment 3

[Edgar Chen [:edgar]]

Not only contacts app, all application that use IccManager need to to be modified accordingly.

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

is there a good way to know which app uses IccManager? or this information should be made available to dev-gaia?

Thanks

Comment 5

[David Scravaglieri [:scravag]]

We need a backward compatible API. Third party apps are using it.

Comment 6

[Julien Wajsberg [:julienw]]

We can still have a new API (keeping a backward compatible API) and convert the Gaia apps to use the new API.

Some apps are just using icc_helper.js in shared/js. So, just do a wild grep and you'll have a wild guess :)

Comment 7

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

Ken, per comment 5, do we keep the original API after the new one lands? 

And it's time to remove IccHelper as we no longer depend on b2g18, lol.

Comment 8

[Ken Chang[:kenkai]]

(In reply to Arthur Chen [:arthurcc] from comment #7)
> Ken, per comment 5, do we keep the original API after the new one lands? 
I don't think so....:(. You can have more detail from bug 814637.
Edgar, would you like to comment this?

Comment 9

[Edgar Chen [:edgar]]

After grep gaia/apps, below Gaia apps use the IccHelper:
* contacts
* dialer
* ftu
* costcontrol
* email
* homescreen
* setting
* system 

In bug 814637, we do not plan to keep the original API.

My original idea is that:
It seems most of Gaia applications are using iccHelper, so we could do this backward compatible in IccHelper (detect new API or old API). After all gaia application move to new IccManager API, we can remove IccHelper.

How about this flow?

Comment 10

[Edgar Chen [:edgar]]

(In reply to David Scravaglieri [:scravag] from comment #5)
> We need a backward compatible API. Third party apps are using it.

Hi David, could you help to provide the information of third party apps? thank you.

Comment 11

[Kai-Chih Hu [:khu]]

Is it a must have item? Looks like we need to do it and just mark this one as 1.3+ for now. If you see something different, please feel free to 1.3? again.

Comment 13

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

Attachment: WIP V1

Julien, Edgar, could you have a quick look at this patch? Thanks!

The modified IccHelper is the object that has the same API on different versions of gecko (mozIccManager in the current gecko and mozIcc object after bug 814637 lands) based on the proposed changes[1]. The patch is the minimal change that we need for single sim card. The support of multiple sim cards will be introduced by other patches.

[1]: https://wiki.mozilla.org/WebAPI/WebIccManager/Multi-SIM

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 823222 [details]
WIP V1

looks good to me

a test is failing and some code can be written in a more compact way. And I have a naive question about the API that is used: if it does IPC, should it be asynchronous ?

Comment 15

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

Edgar, does "getIccById" do a IPC call?

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 823222 [details]
WIP V1

Looks good to me. Thank you.

Comment 17

[Edgar Chen [:edgar]]

(In reply to Arthur Chen [:arthurcc] from comment #15)
> Edgar, does "getIccById" do a IPC call?

No, |getIccById()| and the getter of |iccIds| will not do IPC call, we maintain these information in DOM side. Thank you.

Comment 18

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

Comment on attachment 823222 [details]
WIP V1

Hi all, could you help review the patch?

Francisco: Contacts, FTU
Etienne:   Dialer
Salvador:  Cost control
Christian: Homescreen
Evelyn:    Settings, tz_select.js in shared
Alive:     System
Julien:    icc_helper.js related in shared
dave:      UI test

Thanks a lot!

Comment 19

[Cristian Rodriguez (:crdlc)]

Comment on attachment 823222 [details]
WIP V1

OK for me

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 823222 [details]
WIP V1

looks good to me, but how can we try the patch ? with the patches in bug 814637 ?

Comment 21

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

The patch in bug 814637 is not ready yet. We will test all patches to make sure everything works well before landing.

Comment 22

[Julien Wajsberg [:julienw]]

oki, then I'll wait that bug 814637 has patches ready to give r+ :-)

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 823222 [details]
WIP V1

r=me for the tiny tiny dialer part :)

Comment 24

[Salvador de la Puente González [:salva]]

Comment on attachment 823222 [details]
WIP V1

If this is the new way we check if IccHelper is enabled, go on.
Thank you.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 823222 [details]
WIP V1

The code changes for the UI tests look good to me. Zac, could you run the relevant tests against a device with a contact on the SIM card, and provide feedback? Thanks.

Comment 26

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 823222 [details]
WIP V1

Do file followup to remove the redundant replacement in system.

Comment 27

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

The followup is bug 932134.

Comment 28

[Evelyn Hung]

Comment on attachment 823222 [details]
WIP V1

looks good. Thanks.

Comment 29

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

Francisco, are you available for reviewing the contacts and FTU parts? Thanks!

Julien, you can try the patch with the updated gecko at [1]. And you may also want check the new API[2].
[1]: http://people.mozilla.org/~echen/public/unagi_New_IccManager_API.tar.gz
[2]: https://wiki.mozilla.org/WebAPI/WebIccManager/Multi-SIM

Comment 30

[Julien Wajsberg [:julienw]]

Great, will try to do this today!

Comment 31

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

Comment on attachment 823222 [details]
WIP V1

Asking Borja to review FTU

Comment 32

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

Sure, I just noticed the review since it was setup to my personal email.

Taking a look now :)

Comment 33

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

Comment on attachment 823222 [details]
WIP V1

r+ to contacts part.

Thanks a lot for the work!

Comment 34

[Borja Salguero [:borjasalguero]]

Comment on attachment 823222 [details]
WIP V1

R=me for FTU part. Regarding Travis, do we need to land the other patch before merging this for making Travis green? Thanks!

Comment 35

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

Thanks for reviewing! I'll make sure the patch not breaking any tests before merging.

Comment 36

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

Comment on attachment 823222 [details]
WIP V1

Cancel the review at first as I missed a case. We should also cache all event listeners in case the mozIcc object becomes invalid.

Comment 37

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

Comment on attachment 823222 [details]
WIP V1

Julien, I've updated the patch to address the issue of event listeners. Since with the new API design, mozIcc objects become invalid when in airplane mode, so are all registered listeners. To keep the same behaviors, IccHelper now caches all registered listeners.

Alive, Evelyn, could you help review the files related to STK again? With the new API design, the STK constants are kept in mozIccManager and not moved to mozIcc. The changes are simply looking up the constants in mozIccManager.
system:   apps/system/js/icc.js
          apps/system/js/icc_events.js
          apps/system/js/icc_worker.js
settings: apps/settings/js/icc.js

Comment 38

[Julien Wajsberg [:julienw]]

Comment on attachment 823222 [details]
WIP V1

Works well, both with and without the gecko patch.

Please merge with a green travis.

Thanks!

Comment 39

[Evelyn Hung]

Comment on attachment 823222 [details]
WIP V1

looks good to me. Thanks!

Comment 40

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

master: d6da558d4ff61c1189a50e5f7698eea86222ad10

Comment 41

[Jason Smith [:jsmith]]

This broke a smoketest in bug 939110.

John - Can you back this out?

Comment 42

[Edgar Chen [:edgar]]

Hi Jason,

I have tried back out this locally, but it doesn't fix bug 939110.
I also try to find out who cause bug 939110, but have no idea yet.

Thanks

Comment 43

[Jason Smith [:jsmith]]

(In reply to Edgar Chen [:edgar][:echen] from comment #42)
> Hi Jason,
> 
> I have tried back out this locally, but it doesn't fix bug 939110.
> I also try to find out who cause bug 939110, but have no idea yet.
> 
> Thanks

Okay - saw your comment in the other bug then. Removing needinfo.