927724 - [DSDS][Gaia] Voicemail 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]]

Before bug 814634, we use, for example
var voicemail = navigator.mozVoicemail;
var number = voicemail.number;
var status = voicemail.status;

After,
var voicemail = navigator.mozVoicemail;
var number = voicemail.getNumber(); or voicemail.getNumber(serviceId);
var status = voicemail.getStatus(); or voicemail.getStatus(serviceId);

Comment 1

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

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

Comment 2

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

Attachment: Pointer to Github pull request 13000.html

Comment 3

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

Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

Hi Fernando, Alive and Arthur, 

This is a temporary patch for Gecko Voicemail WebAPI change for future DSDS (Dual Sim Dual Standby) support following our feature detection steps when changing Geck APIs. (you can check here for details : https://bugzilla.mozilla.org/show_bug.cgi?id=887699#c77) 

After Gecko merged the patch into codebase, we will definitely remove this part and use our brand-new API instead. 

Hope this change make sense to you all, Thanks :)

Comment 4

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

Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

r=me

Comment 5

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

Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

EJ, thanks for the patch. Please also fix the line https://github.com/EragonJ/gaia/blob/065414be66a7123283d3c916e39f123b83d49a99/apps/settings/js/call.js#L719 .

Comment 6

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

Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

Sorry Arthur, I missed one mozVoicemail when grep. Thanks for pointing out this problem.

Comment 7

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

Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

r=me. Thank you.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

Thanks Ej!

Just remember to squash the commits before merging this, please.

Comment 9

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

(In reply to Fernando Jiménez Moreno [:ferjm] (FxAccounts Work Week until Friday) from comment #8)
> Comment on attachment 820189 [details]
> Pointer to Github pull request 13000.html
> 
> Thanks Ej!
> 
> Just remember to squash the commits before merging this, please.

Yes , I will ! thanks for your reminding, Fernando :)

Comment 10

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

Thanks all, this patch is landed on master. commit : 4d74b13f20f12996d41eb3e9a6c8775dd1717d67

Comment 11

[Etienne Segonzac (:etienne)]

What happens when we don't have the voicemail number information *and* we're on an older gecko?
Looks like we're going to go through the |.getNumber()| call which will badly break the phone.

So we don't really have backward compatibility :/
We might need to back this out.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

That seems like a valid concern, we might want to check for .getNumber before calling it and fallback to empty string. No need to backout though, this can be done in a follow up.

Comment 13

[Etienne Segonzac (:etienne)]

Sorry, but this breaks the dialer completely (not able to receive or emit calls...) and Ej is probably asleep.

Backed out: https://github.com/mozilla-b2g/gaia/commit/1164cb2e8c8e4992421f58600e8b896c1882c1fa

Comment 16

[Jason Smith [:jsmith]]

In the future - make sure the existing tests in the tree work before we land a patch that risks breaking the dialer & cost control apps. Also - make sure email [e-mail] if there's a risky patch landing on trunk.

Comment 17

[Marcia Knous [:marcia]]

Even after the backout in Comment 13, the build that Rel Eng respun still show the same issues: See Bug 930804 which I just filed which has this changeset:

Gaia   2feebdb1a2583928f32407d76d798f8654621e2b
SourceStamp 19fd3388c372
BuildID 20131024154023
Version 27.0a1

Comment 18

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

Thanks Etienne for pointing out this problem. I thought we would have number info no matter in older Gecko or new one, but it seems this is wrong. 

Sorry for breaking the others' workflow.

@Jason, I landed this patch after getting green light from Travis, but it's not enough for this patch I think. What you mean emailing `[e-mail]` ? I can ask for QA's help before landing them (?)  Should I needinfo this email or just send a email with details about patches? Is there any workflow for asking `[e-mail]` help ?  Thanks.

Comment 19

[Jason Smith [:jsmith]]

(In reply to EJ Chen [:eragonj] from comment #18)
> Thanks Etienne for pointing out this problem. I thought we would have number
> info no matter in older Gecko or new one, but it seems this is wrong. 
> 
> Sorry for breaking the others' workflow.
> 
> @Jason, I landed this patch after getting green light from Travis, but it's
> not enough for this patch I think. What you mean emailing
> `[e-mail]` ? I can ask for QA's help before landing them (?) 
> Should I needinfo this email or just send a email with details about
> patches? Is there any workflow for asking `[e-mail]` help ? 
> Thanks.

I would email [e-mail] if you know a patch landing is going to have significant regression risk. That way, if we see immediate problems, we can figure out a way to land the patch without crash landing (e.g. breaking the dialer app). That might mean having someone pick up and sanity test the patch before it lands.

Comment 20

[Zac C (:zac)]

(In reply to EJ Chen [:eragonj] from comment #18)

> @Jason, I landed this patch after getting green light from Travis, but it's
> not enough for this patch I think. 

Yeah that is a bit of a misnomer because Travis cannot run any of the carrier/RIL tests! Maybe Askeing could help you to use the Tpe CI Jenkins to run the gaia-ui-tests against a custom build next time. They have a Hamachi device on it now.

Comment 21

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

Attachment: New updates

Comment 22

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

I have tested this new update from my hamachi and it seems everything is good now. After reviewed by Alive, Fernando and Arthur again, I will ask for `[e-mail]`'s help. Thanks all.

Comment 23

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

Comment on attachment 823213 [details] [review]
New updates

r=me for the settings part. Thanks!

Comment 24

[Al Tsai [:atsai]]

I did local test w/ gaia base commit d4a1705. Everything works well. Good to go.

Comment 25

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

(In reply to Al Tsai [:atsai] from comment #24)
> I did local test w/ gaia base commit d4a1705. Everything works well. Good to
> go.

Thanks for your great help, ATsai ! I will merge it into master.

Comment 26

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

Thanks all, this commit has been merged into gaia/master : 162cc9cd7153dcb284e5f54d5598fb27f1ab18aa.