Bug 796277 adds voicemail object in |RadioInterFace.RILContext|, but its interface is not exposed.

nom-ing because this change is needed to commercialize v1, patch coming up shortly.

chuck, nsarkar created a patch to address this issue. We tested and it works for us. Please find the patch at https://www.codeaurora.org/gitweb/quic/b2g/?p=b2g/build.git;a=blob;f=patch/all/gecko/0001-Added-interface-nsIDOMMozVoicemailInfo-for-voicemail.patch;h=69f9589ac4c8b8840ec649a4b1e313103302cf60;hb=76d27ae0f366b851e7db09db75a8f53d3b612183.

(In reply to Anshul from comment #2) > chuck, nsarkar created a patch to address this issue. We tested and it works > for us. > > Please find the patch at > https://www.codeaurora.org/gitweb/quic/b2g/?p=b2g/build.git;a=blob;f=patch/ > all/gecko/0001-Added-interface-nsIDOMMozVoicemailInfo-for-voicemail.patch; > h=69f9589ac4c8b8840ec649a4b1e313103302cf60; > hb=76d27ae0f366b851e7db09db75a8f53d3b612183. Hi Anshul, Thanks for your patch, though I am not very sure about what kind of issue you are trying to resolve. Per my understanding, this issue was filed originally to expose voicemail info to the internal interface nsIRadioInterfaceLayer.idl, nothing to do with WebAPI. So maybe you could help clarify your issue again. :) Besides, I don't think we need to add voicemail info to nsIDOMMobileConnection.idl because we already exposed voicemail info, such as number, displayName, in WebAPI 'nsIDOMVoicemail.idl'. We probably don't want to have duplicated APIs doing the same thing. Please see http://mxr.mozilla.org/mozilla-central/source/dom/telephony/nsIDOMVoicemail.idl If the patch is ready for review, then you could simply add the attachment and ask for review.

Hsin-Yi, Moz's RIL has voicemail information added to the RilContext in RadioInterfaceLayer.js at "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#207". I am not an expert at JS or DOM/XPCOM but not sure how that works without exposing voicemail in nsIIRadioInterfaceLayer.idl unlike other radioState,cardState,voice,data and other members of RilContext being specified at "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIRadioInterfaceLayer.idl#267" However in commercial RIL we can not simply add voicemail information to RilContext as our code is not in JS but in C++. We need this voicemail interface to be exposed via nsIIRadioInterfaceLayer.idl or else we can't set the voice mail in rilcontext. Without this pressing 1 keys doesn't connect to voicemail in commercial RIL. Perhaps adding voicemail info to nsIDOMMobileConnection.idl is incorrect and we are looking forward for your input on how to address the issue.

(In reply to Anshul from comment #4) > Hsin-Yi, Moz's RIL has voicemail information added to the RilContext in > RadioInterfaceLayer.js at > "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > RadioInterfaceLayer.js#207". I am not an expert at JS or DOM/XPCOM but not > sure how that works without exposing voicemail in > nsIIRadioInterfaceLayer.idl unlike other radioState,cardState,voice,data and > other members of RilContext being specified at > "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > nsIRadioInterfaceLayer.idl#267" > > However in commercial RIL we can not simply add voicemail information to > RilContext as our code is not in JS but in C++. We need this voicemail > interface to be exposed via nsIIRadioInterfaceLayer.idl or else we can't set > the voice mail in rilcontext. Without this pressing 1 keys doesn't connect > to voicemail in commercial RIL. > > Perhaps adding voicemail info to nsIDOMMobileConnection.idl is incorrect and > we are looking forward for your input on how to address the issue. Hi Anshul, Thanks for clarifying :) I understood your problem better now. Yes, you need voicemail info exposed in nsIRadioInterfaceLayer.idl that is definitely this issue's aim, and no more needed. We don't need to modify nsIDOMMobileConnection.idl in this case. :)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5) > (In reply to Anshul from comment #4) > > Hsin-Yi, Moz's RIL has voicemail information added to the RilContext in > > RadioInterfaceLayer.js at > > "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > > RadioInterfaceLayer.js#207". I am not an expert at JS or DOM/XPCOM but not > > sure how that works without exposing voicemail in > > nsIIRadioInterfaceLayer.idl unlike other radioState,cardState,voice,data and > > other members of RilContext being specified at > > "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > > nsIRadioInterfaceLayer.idl#267" > > Here, we use ipc mechanism 'sendSyncMessage' to get rilContext from RadioInterfaceLayer, not via nsIRadioInterfaceLayer.idl. That's why that works ;-)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5) > (In reply to Anshul from comment #4) > > Hsin-Yi, Moz's RIL has voicemail information added to the RilContext in > > RadioInterfaceLayer.js at > > "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > > RadioInterfaceLayer.js#207". I am not an expert at JS or DOM/XPCOM but not > > sure how that works without exposing voicemail in > > nsIIRadioInterfaceLayer.idl unlike other radioState,cardState,voice,data and > > other members of RilContext being specified at > > "http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ > > nsIRadioInterfaceLayer.idl#267" > > > > However in commercial RIL we can not simply add voicemail information to > > RilContext as our code is not in JS but in C++. We need this voicemail > > interface to be exposed via nsIIRadioInterfaceLayer.idl or else we can't set > > the voice mail in rilcontext. Without this pressing 1 keys doesn't connect > > to voicemail in commercial RIL. > > > > Perhaps adding voicemail info to nsIDOMMobileConnection.idl is incorrect and > > we are looking forward for your input on how to address the issue. > > Hi Anshul, > Thanks for clarifying :) I understood your problem better now. > > Yes, you need voicemail info exposed in nsIRadioInterfaceLayer.idl that is > definitely this issue's aim, and no more needed. We don't need to modify > nsIDOMMobileConnection.idl in this case. :) Hsin-Yi, without modifying nsIDOMMObileCOnnecion.idl we were getting compiler errors. Please let us know when would you guys resolve this issue.

1. Add interface definition for voicemail 2. Add attribute for voicemail in RilContext value of getInterface(Ci.nsIRadioInterfaceLayer).voicemail before patch : undefined after patch : {"number":"", "displayName":""}

Rename interface from nsIVoicemail to nsIVoicemailInfo

Comment on attachment 695410 [details] [diff] [review] 0001.Add voicemail information interface and expose it in nsIRilContext Review of attachment 695410 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Thank you, Chuck!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10) > Comment on attachment 695410 [details] [diff] [review] > 0001.Add voicemail information interface and expose it in nsIRilContext > > Review of attachment 695410 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks great. Thank you, Chuck! Hi Anshul, This is our solution. Could you please help to see whether it solves your issue? Thanks.

Check if |RadioInterfaceLayer.rilContext.voicemail.number| and |RadioInterfaceLayer.rilContext.voicemail.displayName| exist

Comment on attachment 695410 [details] [diff] [review] 0001.Add voicemail information interface and expose it in nsIRilContext Review of attachment 695410 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I realized that we shouldn't put voicemailinfo into rilContext due to permission differences.

Extract voicemail from rilContext and expose in RadioInterfaceLayer because they are different permission

Permission of voicemail information is different from rilContext. To make suer RILContentHelper has only permission on voicemail but mobileconnection can get voicemail information, request voicemail information via voicemail permission IPC message.

Get voicemail information while getter is called. This fixes app without voicemail permission is killed while using RILContentHelper.

Comment on attachment 695581 [details] [diff] [review] 0003.Test case - test if voicemailInfo is exposed. Review of attachment 695581 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/marionette/manifest.ini @@ +5,4 @@ > > [test_geolocation.js] > disabled = Bug 808783 > +[test_bug_820762.js] I would prefer 'test_get_voicemailInfo.js' or something like that. ::: dom/system/gonk/tests/marionette/test_bug_820762.js @@ +6,5 @@ > +let Cc = SpecialPowers.Cc; > +let Ci = SpecialPowers.Ci; > + > +// Get telephony permission > +SpecialPowers.addPermission("voicemail", true, document); We don't need this. @@ +8,5 @@ > + > +// Get telephony permission > +SpecialPowers.addPermission("voicemail", true, document); > + > +// Get telephony object Get system worker manager, not telephony object. @@ +14,5 @@ > +ok(telephony); > + > +// Get RadioIntefaceLayer interface > +let RIL = telephony.getService(Ci.nsIInterfaceRequestor). > + getInterface(Ci.nsIRadioInterfaceLayer); Bad coding style here. Please make 'get' align. @@ +21,5 @@ > +// Check voicemail information accessible > +ok(RIL.voicemailInfo); > +ok(RIL.voicemailInfo.number); > +ok(RIL.voicemailInfo.displayName); > + Please also add the following: // These are the emulator's hard coded voicemail number and alphaId. is(RIL.voicemailInfo.number, "+15552175049"); is(RIL.voicemailInfo.displayName, "Voicemail"); @@ +23,5 @@ > +ok(RIL.voicemailInfo.number); > +ok(RIL.voicemailInfo.displayName); > + > +// Remove telephony permission > +SpecialPowers.removePermission("voicemail", document); We don't need this.

Check value of exposed interface

Comment on attachment 695712 [details] [diff] [review] 0003.Test case - test if voicemailInfo is exposed currectly Review of attachment 695712 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/marionette/test_get_voicemailInfo.js @@ +7,5 @@ > +let Ci = SpecialPowers.Ci; > + > +// Get system worker manager of telephony > +let telephony = Cc["@mozilla.org/telephony/system-worker-manager;1"]; > +ok(telephony); Oops... Sorry, I didn't mention it explicitly in my last review... Please rename 'telephony', thanks!

Comment on attachment 695628 [details] [diff] [review] 0002.RILContentHelper Access voicmail info via correct permission, v2 Review of attachment 695628 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +138,3 @@ > > function MobileVoicemailInfo() {} > MobileVoicemailInfo.prototype = { Let's rename it VoicemailInfo. @@ +138,5 @@ > > function MobileVoicemailInfo() {} > MobileVoicemailInfo.prototype = { > + number: "", > + displayName: "" Let's just keep them null as before. @@ +668,2 @@ > return this.voicemailInfo.displayName; > }, How about this? getVoicemailInfo: function getVoicemailInfo() { // Send this message only once for synchronization. let voicemailInfo = cpmm.sendSyncMessage("RIL:GetVoicemailInfo")[0]; if (voicemailInfo) { this.updateVoicemailInfo(voicemailInfo, this.voicemailInfo); } this.getVoicemailInfo = function getVoicemailInfo() { return this.voicemailInfo; }; return this.voicemailInfo; }, get voicemailNumber() { return this.getVoicemailInfo().number; }, get voicemailDisplayName() { return this.getVoicemailInfo().displayName; }, ::: dom/system/gonk/RadioInterfaceLayer.js @@ +230,5 @@ > > + this.voicemailInfo = { > + number: "", > + displayName: "" > + }; Keep them 'null' @@ +484,5 @@ > this.registerMessageTarget("cellbroadcast", msg.target); > break; > + case "RIL:GetVoicemailInfo": > + return this.voicemailInfo; > + break; Add a comment "This message is sync." and remove "break;".

Address modifications in comment 21

Address modification in comment 20

Comment on attachment 695727 [details] [diff] [review] 0002.RILContentHelper Access voicmail info via correct permission, v2.1 Review of attachment 695727 [details] [diff] [review]: ----------------------------------------------------------------- Good job! Thank you :D

Comment on attachment 695728 [details] [diff] [review] 0003.Test case - test if voicemailInfo is exposed currectly, v1.1 Review of attachment 695728 [details] [diff] [review]: ----------------------------------------------------------------- I like it! Thanks for conducting this.

try server : https://tbpl.mozilla.org/?tree=Try&rev=4d54c843a3c5

https://hg.mozilla.org/integration/mozilla-inbound/rev/f634045890a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/63e1e0f1405d https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5c6ddebb85

https://hg.mozilla.org/mozilla-central/rev/f634045890a1 https://hg.mozilla.org/mozilla-central/rev/63e1e0f1405d https://hg.mozilla.org/mozilla-central/rev/1f5c6ddebb85

https://hg.mozilla.org/releases/mozilla-aurora/rev/7be7367725c4 https://hg.mozilla.org/releases/mozilla-aurora/rev/ade1fdec8d2b https://hg.mozilla.org/releases/mozilla-aurora/rev/2e27c6ee30ac https://hg.mozilla.org/releases/mozilla-b2g18/rev/0dbc21893122 https://hg.mozilla.org/releases/mozilla-b2g18/rev/af804b78360d https://hg.mozilla.org/releases/mozilla-b2g18/rev/14b70d57c924