Closed Bug 927724 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: hsinyi, Assigned: eragonj)

References

Details

Attachments

(1 file, 1 obsolete file)

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);
This bug is for backward compatibility and new API detection so that bug 814634 won't break gaia.
Assignee: nobody → arthur.chen
Assignee: arthur.chen → ejchen
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 :)
Attachment #820189 - Flags: review?(ferjmoreno)
Attachment #820189 - Flags: review?(arthur.chen)
Attachment #820189 - Flags: review?(alive)
Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

r=me
Attachment #820189 - Flags: review?(alive) → review+
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 .
Attachment #820189 - Flags: review?(arthur.chen)
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.
Attachment #820189 - Flags: review?(arthur.chen)
Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

r=me. Thank you.
Attachment #820189 - Flags: review?(arthur.chen) → review+
Comment on attachment 820189 [details]
Pointer to Github pull request 13000.html

Thanks Ej!

Just remember to squash the commits before merging this, please.
Attachment #820189 - Flags: review?(ferjmoreno) → review+
(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 :)
Thanks all, this patch is landed on master. commit : 4d74b13f20f12996d41eb3e9a6c8775dd1717d67
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(ejchen)
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(ejchen)
Duplicate of this bug: 930555
Duplicate of this bug: 930559
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 qa-b2g@mozilla.org if there's a risky patch landing on trunk.
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
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 `qa-b2g@mozilla.org` ? 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 `qa-b2g@mozilla.org` help ?  Thanks.
(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
> `qa-b2g@mozilla.org` ? 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 `qa-b2g@mozilla.org` help ? 
> Thanks.

I would email qa-b2g@mozilla.org 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.
(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.
Attached file New updates
Attachment #820189 - Attachment is obsolete: true
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 `qa-b2g@mozilla.org`'s help. Thanks all.
Comment on attachment 823213 [details] [review]
New updates

r=me for the settings part. Thanks!
Attachment #823213 - Flags: review?(arthur.chen) → review+
Attachment #823213 - Flags: review?(alive) → review+
Attachment #823213 - Flags: review?(ferjmoreno) → review+
I did local test w/ gaia base commit d4a1705. Everything works well. Good to go.
(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.
Thanks all, this commit has been merged into gaia/master : 162cc9cd7153dcb284e5f54d5598fb27f1ab18aa.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Blocks: 814634
No longer depends on: 814634
You need to log in before you can comment on or make changes to this bug.