926169 - [DSDS][Gaia] MobileConnection webapi will be changed for DSDS, so all relevant Apps has to be modified accordingly.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Naming changes: the way to get mobileconnection object differs.

Before:
var mobileconn = navigator.mozMobileConnection;
var voiceinfo = mobileconn.voiceInfo;

After:
var mobileconn = navigator.mozMobileConnections[0];
// The same below ...
var voiceinfo =mobileconn.voiceInfo;

Comment 1

[Ken Chang[:kenkai]]

arthur, can you take this bug?

Comment 2

[Kai-Chih Hu [:khu]]

Looks like a 1.3+ bug.

Comment 3

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

We need to figure out a good way to address changes brought by bug 814629 so that Gaia could handle both old and new APIs well. Otherwise, we will be getting complaints about breaking the tests and functionalities...

See https://bugzilla.mozilla.org/show_bug.cgi?id=887699#c77 for some discussion.

Comment 4

[Kai-Chih Hu [:khu]]

Arthur, may I know what's the target milestone in your mind for this bug? Thanks.

Comment 5

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

Some notes :

Because there are some overlapping part that Arthur is currently working on, this patch has to be landed after Bug 928325.

Comment 6

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

Attachment: WIP 1

This is just a quick preview of changes to make sure we are on the right direction.

Comment 7

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

Comment on attachment 823886 [details] [review]
WIP 1

Hi Arthur, Fernando and Borja, 

this is a WIP patch focusing on Gecko API change. Can you guys give me some feedbacks about code change in your app so that I can make sure I am on the right direction :)

(Settings app for Arthur, dialer app for Fernando and FTU for Borja :D)

Comment 8

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

Comment on attachment 823886 [details] [review]
WIP 1

Looks good to me.

Comment 9

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

Comment on attachment 823886 [details] [review]
WIP 1

Hi Cristian and Dave, can you give me some feedbacks about related changes in your apps ? thanksss !

Comment 10

[Cristian Rodriguez (:crdlc)]

Comment on attachment 823886 [details] [review]
WIP 1

Please Ran take a look to give your feedback because this patch is touching an evme file js

Comment 11

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

Comment on attachment 823886 [details] [review]
WIP 1

(In reply to EJ Chen [:eragonj] from comment #9)
> Comment on attachment 823886 [details] [review]
> WIP 1
> 
> Hi Cristian and Dave, can you give me some feedbacks about related changes
> in your apps ? thanksss !

I think you may have the wrong Dave, I don't work on any Gaia apps.

Comment 12

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

(In reply to Cristian Rodriguez (:crdlc) PTO (10-24 to 10-27) from comment #10)
> Comment on attachment 823886 [details] [review]
> WIP 1
> 
> Please Ran take a look to give your feedback because this patch is touching
> an evme file js

Thanks Cristian !

Comment 13

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

(In reply to Dave Hunt (:davehunt) from comment #11)
> Comment on attachment 823886 [details] [review]
> WIP 1
> 
> (In reply to EJ Chen [:eragonj] from comment #9)
> > Comment on attachment 823886 [details] [review]
> > WIP 1
> > 
> > Hi Cristian and Dave, can you give me some feedbacks about related changes
> > in your apps ? thanksss !
> 
> I think you may have the wrong Dave, I don't work on any Gaia apps.

Dave, I blamed gaia/tests and I found you have committed a lot there. 

Should I find the others to give me some feedbacks for this part ?

Comment 14

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

Comment on attachment 823886 [details] [review]
WIP 1

Ah, I thought you were asking me to review app changes. Looking through the test changes, these look good to me.

Comment 15

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

Thanks all, it seems this change is OK to most of you. I will request r? instead. Thanks :P

Comment 16

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

Comment on attachment 823886 [details] [review]
WIP 1

Hi all, 

because this bug fix will affect lots of files using mozMobileConnection, I need your review for your own apps / files.

In the PR, I have divided my code change into several part to help you find your related part quickly ( and also help you review my code easily haha )

Thanks all :)

Comment 17

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

I dislike we do this everywhere in the code.
Could we have a helper like ICC to help us on this?

Comment 18

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

(In reply to Alive Kuo [:alive][11/4-11/8 at SFO ww.] from comment #17)
> I dislike we do this everywhere in the code.
> Could we have a helper like ICC to help us on this?

Hi Alive, 

this is a temporary patch and will be totally removed after using latest Web API. 

The reason why we need ICC helper before is because the wrong API design so that we have to add more check for backward compatibilities. After landing Gecko's patch, we will all use newest API and update old codes.

And there is also a discussion on dev-gaia talking about the shared/helpers problems. If we need helpers so shortly after implementing new features (maybe new API), it may reveal something about our problematic API design.

After some discussions, we make a conclusion that we will all use newest API and discard backward compatibility check to make code base more clean and focus on the future. 

Hope this comment helps and kindly review this patch again. Thanks :)

Comment 19

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

> Hope this comment helps and kindly review this patch again. Thanks :)
Typo : you can kindly .. *

Comment 20

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

Comment on attachment 823886 [details] [review]
WIP 1

r=me for the settings part.

Comment 21

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

Why do we need to put us in an intermediate state? The thing in the patch doesn't obey what you say about discarding backward compatibility.

(In reply to EJ Chen [:eragonj] from comment #18)
> (In reply to Alive Kuo [:alive][11/4-11/8 at SFO ww.] from comment #17)
> > I dislike we do this everywhere in the code.
> > Could we have a helper like ICC to help us on this?
> 
> Hi Alive, 
> 
> this is a temporary patch and will be totally removed after using latest Web
> API. 
> 
> The reason why we need ICC helper before is because the wrong API design so
> that we have to add more check for backward compatibilities. After landing
> Gecko's patch, we will all use newest API and update old codes.
> 
> And there is also a discussion on dev-gaia talking about the shared/helpers
> problems. If we need helpers so shortly after implementing new features
> (maybe new API), it may reveal something about our problematic API design.
> 
> After some discussions, we make a conclusion that we will all use newest API
> and discard backward compatibility check to make code base more clean and
> focus on the future. 
> 
> Hope this comment helps and kindly review this patch again. Thanks :)

Comment 22

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

(In reply to Alive Kuo [:alive][11/4-11/8 at SFO ww.] from comment #21)
> Why do we need to put us in an intermediate state? The thing in the patch
> doesn't obey what you say about discarding backward compatibility.

Hi Alive, thanks for your ask. The reason why we need this patch is to make sure Gecko API works well and no Gaia apps are broken due to this change. And after that, this will totally be removed and use the newest API. 

This is a group decided workflow to make sure there is no API broken and no one gets hurt due to this change. 

Hope this comment will make you know more clearly about what we are doing now. 

Thanks again for your ask :)

Comment 23

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

Comment on attachment 823886 [details] [review]
WIP 1

Looks good. The Travis-CI build failed but I retriggered it and it passed. I've also been able to run tests successfully on device (Hamachi) with this patch applied.

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 823886 [details] [review]
WIP 1

Seems like it miss a 'navigator' somewhere. This is ugly but as a temp solution to not break anything it sounds good.

Comment 25

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 823886 [details] [review]
WIP 1

All works great

Comment 26

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

Comment on attachment 823886 [details] [review]
WIP 1

I already stated my concern.
you will automatically have my r+ once everyone give r+.

Comment 27

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #24)
> Comment on attachment 823886 [details] [review]
> WIP 1
> 
> Seems like it miss a 'navigator' somewhere. This is ugly but as a temp
> solution to not break anything it sounds good.

Hi Vivien, thanks for pointing out the missing part. As what I have mentioned on Github, this part will be updated by Bug 928325 and Arthur is working on it now. In this way, I will keep the file unchanged before landing this patch. THanks !!

Comment 28

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

(In reply to Alive Kuo [:alive][11/4-11/8 at SFO ww.] from comment #26)
> Comment on attachment 823886 [details] [review]
> WIP 1
> 
> I already stated my concern.
> you will automatically have my r+ once everyone give r+.

Hi Alive, thanks for your quick reply and review.

I understand your concern and maybe it is also a good time to discard some legacy code due to new API change. 

Hope this change is acceptable to all of you. Thanks :)

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 823886 [details] [review]
WIP 1

Thanks Ej!

r=me with the comments addressed, please.

Comment 30

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

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #29)
> Comment on attachment 823886 [details] [review]
> WIP 1
> 
> Thanks Ej!
> 
> r=me with the comments addressed, please.

Thanks Fernando, I have updated the code :)

Comment 31

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

Comment on attachment 823886 [details] [review]
WIP 1

Hi Tim,  

Because Kaze is missing and there is a code change in shared/js/tz_select.js and needs to be reviewed as well. 

This change focus on Gecko API change and we have to make sure there is no problem before landing Gecko's API.

After asking Arthur, he told me that you may can help me review this part. 

Can you help me review this or tell me is there anyone else I can ask for help ?

Thanks :)

Comment 32

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

Thanks all, this patch is landed on Gaia/master : d58504c98824427fa391d80724fb04ff4356a92c

Comment 33

[José Antonio Olivera Ortega [:jaoo]]

Forgot to take care of apps/settings/call.js in this patch. Once the DSDS changes in the API get landed the call setting panel in the setting panel will be broken. I'll take care of it in bug 928294.

Comment 34

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #33)
> Forgot to take care of apps/settings/call.js in this patch. Once the DSDS
> changes in the API get landed the call setting panel in the setting panel
> will be broken. I'll take care of it in bug 928294.

Ignore this comment, I had not seen the changes in apps/settings/js/utils.js file. Sorry for the noise.