Closed Bug 928325 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+)

RESOLVED FIXED
1.3 Sprint 4 - 11/8
blocking-b2g 1.3+

People

(Reporter: kchang, Assigned: arthurcc)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(1 file)

182 bytes, text/html
arcturus
: review+
salva
: review+
crdlc
: review+
jj.evelyn
: review+
alive
: review+
davehunt
: review+
etienne
: review+
borjasalguero
: review+
julienw
: review+
julienw
: feedback+
edgar
: feedback+
Details
WebIccManager API has changed for DSDS, so contacts app has to be modified accordingly.
Joe, this bug needs help from Comms App
blocking-b2g: --- → 1.3?
Flags: needinfo?(jcheng)
Blocks: 918559
Depends on: 814637
This bug is for backward compatibility and new API detection so that bug 814637 won't break gaia.
Not only contacts app, all application that use IccManager need to to be modified accordingly.
Component: Gaia::Contacts → Gaia
Summary: [DSDS][Gaia] WebIccManager API has changed for DSDS, so contacts app has to be modified accordingly → [DSDS][Gaia] WebIccManager API will be changed for DSDS, so all relevant Apps has to be modified accordingly.
Assignee: nobody → arthur.chen
is there a good way to know which app uses IccManager? or this information should be made available to dev-gaia?

Thanks
Flags: needinfo?(jcheng) → needinfo?(echen)
We need a backward compatible API. Third party apps are using it.
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 :)
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.
Flags: needinfo?(kchang)
(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?
Flags: needinfo?(kchang)
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?
Flags: needinfo?(echen)
(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.
Flags: needinfo?(dscravaglieri)
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.
blocking-b2g: 1.3? → 1.3+
Duplicate of this bug: 921403
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Attached file 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
Attachment #823222 - Flags: feedback?(felash)
Attachment #823222 - Flags: feedback?(echen)
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 ?
Attachment #823222 - Flags: feedback?(felash) → feedback+
Edgar, does "getIccById" do a IPC call?
Flags: needinfo?(echen)
Comment on attachment 823222 [details]
WIP V1

Looks good to me. Thank you.
Attachment #823222 - Flags: feedback?(echen) → feedback+
(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.
Flags: needinfo?(echen)
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!
Attachment #823222 - Flags: review?(salva)
Attachment #823222 - Flags: review?(felash)
Attachment #823222 - Flags: review?(etienne)
Attachment #823222 - Flags: review?(ehung)
Attachment #823222 - Flags: review?(dave.hunt)
Attachment #823222 - Flags: review?(crdlc)
Attachment #823222 - Flags: review?(arcturus)
Attachment #823222 - Flags: review?(alive)
Comment on attachment 823222 [details]
WIP V1

OK for me
Attachment #823222 - Flags: review?(crdlc) → review+
Comment on attachment 823222 [details]
WIP V1

looks good to me, but how can we try the patch ? with the patches in bug 814637 ?
The patch in bug 814637 is not ready yet. We will test all patches to make sure everything works well before landing.
oki, then I'll wait that bug 814637 has patches ready to give r+ :-)
Comment on attachment 823222 [details]
WIP V1

r=me for the tiny tiny dialer part :)
Attachment #823222 - Flags: review?(etienne) → review+
Comment on attachment 823222 [details]
WIP V1

If this is the new way we check if IccHelper is enabled, go on.
Thank you.
Attachment #823222 - Flags: review?(salva) → review+
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.
Attachment #823222 - Flags: review?(dave.hunt)
Attachment #823222 - Flags: review+
Attachment #823222 - Flags: feedback?(zcampbell)
Comment on attachment 823222 [details]
WIP V1

Do file followup to remove the redundant replacement in system.
Attachment #823222 - Flags: review?(alive) → review+
The followup is bug 932134.
Whiteboard: [FT:RIL]
Comment on attachment 823222 [details]
WIP V1

looks good. Thanks.
Attachment #823222 - Flags: review?(ehung) → review+
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
Flags: needinfo?(francisco.jordano)
Flags: needinfo?(felash)
Great, will try to do this today!
Flags: needinfo?(felash)
Attachment #823222 - Flags: review?(arcturus) → review?(francisco.jordano)
Comment on attachment 823222 [details]
WIP V1

Asking Borja to review FTU
Attachment #823222 - Flags: review?(borja.bugzilla)
Sure, I just noticed the review since it was setup to my personal email.

Taking a look now :)
Flags: needinfo?(francisco.jordano)
Comment on attachment 823222 [details]
WIP V1

r+ to contacts part.

Thanks a lot for the work!
Attachment #823222 - Flags: review?(francisco.jordano) → review+
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!
Attachment #823222 - Flags: review?(borja.bugzilla) → review+
Thanks for reviewing! I'll make sure the patch not breaking any tests before merging.
Blocks: 814637
No longer depends on: 814637
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.
Attachment #823222 - Flags: review?(felash)
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
Attachment #823222 - Flags: review?(felash)
Attachment #823222 - Flags: review?(ehung)
Attachment #823222 - Flags: review?(alive)
Attachment #823222 - Flags: review+
Comment on attachment 823222 [details]
WIP V1

Works well, both with and without the gecko patch.

Please merge with a green travis.

Thanks!
Attachment #823222 - Flags: review?(felash) → review+
Comment on attachment 823222 [details]
WIP V1

looks good to me. Thanks!
Attachment #823222 - Flags: review?(ehung) → review+
Attachment #823222 - Flags: review?(alive) → review+
master: d6da558d4ff61c1189a50e5f7698eea86222ad10
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 939110
This broke a smoketest in bug 939110.

John - Can you back this out?
Flags: needinfo?(jhford)
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
(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.
Flags: needinfo?(jhford)
No longer depends on: 939110
Flags: needinfo?(dscravaglieri)
Attachment #823222 - Flags: feedback?(zcampbell)
You need to log in before you can comment on or make changes to this bug.