Closed Bug 820762 Opened 11 years ago Closed 11 years ago

Expose voicemail info in nsIRadioInterfaceLayer.idl

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
B2G C3 (12dec-1jan)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: chucklee, Assigned: chucklee)

Details

Attachments

(3 files, 7 obsolete files)

1.25 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
4.06 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.56 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
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.
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Target Milestone: --- → B2G C3 (12dec-1jan)
(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.
Summary: Expose voicemail in RILContext → Expose voicemail info in nsIRadioInterfaceLayer.idl
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.
Attached patch 0001.Modify IDL (obsolete) — Splinter Review
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":""}
Attachment #695406 - Flags: review?(htsai)
Rename interface from nsIVoicemail to nsIVoicemailInfo
Attachment #695406 - Attachment is obsolete: true
Attachment #695406 - Flags: review?(htsai)
Attachment #695410 - Flags: review?(htsai)
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!
Attachment #695410 - Flags: review?(htsai) → review+
(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
Attachment #695427 - Flags: review?(htsai)
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.
Attachment #695410 - Flags: review+ → review-
Extract voicemail from rilContext and expose in RadioInterfaceLayer because they are different permission
Attachment #695410 - Attachment is obsolete: true
Attachment #695579 - Flags: review?(htsai)
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.
Attachment #695580 - Flags: review?(htsai)
Attachment #695427 - Attachment is obsolete: true
Attachment #695427 - Flags: review?(htsai)
Attachment #695581 - Flags: review?(htsai)
Get voicemail information while getter is called.
This fixes app without voicemail permission is killed while using RILContentHelper.
Attachment #695580 - Attachment is obsolete: true
Attachment #695580 - Flags: review?(htsai)
Attachment #695628 - Flags: review?(htsai)
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.
Attachment #695581 - Flags: review?(htsai)
Attachment #695579 - Flags: review?(htsai) → review+
Check value of exposed interface
Attachment #695581 - Attachment is obsolete: true
Attachment #695712 - Flags: review?(htsai)
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;".
Attachment #695628 - Flags: review?(htsai)
Attachment #695712 - Flags: review?(htsai)
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
Attachment #695727 - Flags: review?(htsai) → review+
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.
Attachment #695728 - Flags: review?(htsai) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: