736710 - Voicemail API based on SMS Message Waiting

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

According to 3GPP 23.040 section 9.2.3.24.2, there are three levels of "Message Waiting" indication:

1) TP-PID set to 0x5F for Return Call Message along with optional text message in TP-UD;
2) DCS set to 0xCX ~xEX
3) use Special SMS Message Indication in user data header

See also 3GPP 23.038 section 4 SMS Data Coding Scheme, 3GPP 23.040 section 9.2.3.9.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

This feature seems to have no direct relation to SMS stuff but need an additional API to notify the message waiting state.

For detailed meanings of three levels indication above:

1) A boolean state indicates a (telephone) call can be established to the address specified within the TP-OA along with displayable message text that should be handled in the same way as Replace Short Message Types ones(see bug 736708).

2) DCS method allows a) indication of bi-state, yes or no, b) messages waiting of four different types: voice mail, fax, electronic mail, and other messages. This level might also order ME to discard the content after indication.

3) suppports a) whether content should be discarded after indication, b) messages waiting of types: voice, fax, electronic mail, video messages, c) Multiple Subscribe Profile, d) messages count 0..255.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo] from comment #1)
> This feature seems to have no direct relation to SMS stuff but need an
> additional API to notify the message waiting state.

Not really :(

> 1) A boolean state indicates a (telephone) call can be established to the
> address specified within the TP-OA along with displayable message text that
> should be handled in the same way as Replace Short Message Types ones(see
> bug 736708).

should depends on bug 736708

> 2) DCS method allows a) indication of bi-state, yes or no, b) messages
> waiting of four different types: voice mail, fax, electronic mail, and other
> messages, c) store or discard the content after indication.

should depends on bug 736707

> 3) suppports a) whether content should be discarded after indication, b)
> messages waiting of types: voice, fax, electronic mail, video messages, c)
> Multiple Subscribe Profile, d) messages count 0..255.

Comment 3

[Price Tseng]

Attachment: Support Message Waiting, V1

A boolean value |mwi| is a flag to record if the message is a WMI or not.
Unfortunately, there is currently no suitable mechanism to send WMI to upper layer.
There are maybe three ways to implement:
(1) Create a navigator.mozNotification object: It may be not good because we define the UI behavior in gecko.
(2) Modify the DOM API: It is required to modify the argument of |nsISmsService::CreateSmsMessage| in current implementation.
(3) Web Intent: Probably the best way, but it seems this function is still incomplete yet.

Now, just duplicate the short message and modify the string of body as "This is a MWI message" to distinguish it from normal short message.

Comment 4

[Kai-Chih Hu [:khu]]

Just help to correct some typo in comment 3: WMI should be MWI.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to Price Tseng from comment #3)
> Created attachment 619509 [details] [diff] [review]
> Support Message Waiting, V1
> 
> A boolean value |mwi| is a flag to record if the message is a WMI or not.
> Unfortunately, there is currently no suitable mechanism to send WMI to upper
> layer.
> There are maybe three ways to implement:
> (1) Create a navigator.mozNotification object: It may be not good because we
> define the UI behavior in gecko.

navigator.mozNotification already exists and means something else.

> (2) Modify the DOM API: It is required to modify the argument of
> |nsISmsService::CreateSmsMessage| in current implementation.
> (3) Web Intent: Probably the best way, but it seems this function is still
> incomplete yet.

Yes, it will probably be something like that. We will need ways for Gecko to notify the UI of system events. postMessage/WebIntents are probably going to be the mid-term solution here. Please file a follow-up and mention it in the code comment.

> Now, just duplicate the short message and modify the string of body as "This
> is a MWI message" to distinguish it from normal short message.

That's really not a useful solution.

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 619509 [details] [diff] [review]
Support Message Waiting, V1

Review of attachment 619509 [details] [diff] [review]:
-----------------------------------------------------------------

Some issues mentioned below. Asking Vicamo for feedback on SMS details in ril_worker.js.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +538,5 @@
>      Services.obs.notifyObservers(sms, kSmsReceivedObserverTopic, null);
> +
> +    // TODO:
> +    // In Bug 736710, it is required to send WMI to upper layer, 
> +    // but there is currently no suitable mechanism to do so.

What "upper layer"? It's the UI, so please use that word to increase clarity. Also please file a follow-up for this and mention the bug number in this comment rather than referring to the bug you're working on.

@@ +552,5 @@
> +                                                message.sender || null,
> +                                                message.receiver || null,
> +                                                wmiMessage,
> +                                                message.timestamp);
> +      Services.obs.notifyObservers(wmiSMS, kSmsReceivedObserverTopic, null);

This is not a very useful solution at all. Please get rid of it and defer to follow-up.

::: dom/system/gonk/ril_worker.js
@@ +3165,4 @@
>     */
> +  readUserDataHeader: function readUserDataHeader(msg) {
> +    /**
> +	 * A header object with properties contained in received message.

no tabs please

@@ +3389,5 @@
>      }
>      msg.epid = PDU_PID_DEFAULT;
> +
> +    // Level 1 of message waiting indication
> +    if (msg.mwi == null) {

If `msg.mwi` is meat to be a boolean, then `if (!msg.mwi)` is better here.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Unblock b2g-sms temporarily for milestone 3.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 619509 [details] [diff] [review]
> Support Message Waiting, V1
> 
> Review of attachment 619509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some issues mentioned below. Asking Vicamo for feedback on SMS details in
> ril_worker.js.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +538,5 @@
> >      Services.obs.notifyObservers(sms, kSmsReceivedObserverTopic, null);
> > +
> > +    // TODO:
> > +    // In Bug 736710, it is required to send WMI to upper layer, 
> > +    // but there is currently no suitable mechanism to do so.
> 
> What "upper layer"? It's the UI, so please use that word to increase
> clarity. Also please file a follow-up for this and mention the bug number in
> this comment rather than referring to the bug you're working on.

As mentioned in comment #3, we'll need some mechanism to inform the system or a willing app that a MWI message arrives. I think we should wait until we have a total solution for that since this bug is still not listed as gating issue.

> @@ +552,5 @@
> > +                                                message.sender || null,
> > +                                                message.receiver || null,
> > +                                                wmiMessage,
> > +                                                message.timestamp);
> > +      Services.obs.notifyObservers(wmiSMS, kSmsReceivedObserverTopic, null);
> 
> This is not a very useful solution at all. Please get rid of it and defer to
> follow-up.

Same above. We should wait until we have some conclusion in bug 757235.

> ::: dom/system/gonk/ril_worker.js
> @@ +3165,4 @@
> >     */
> > +  readUserDataHeader: function readUserDataHeader(msg) {
> > +    /**
> > +	 * A header object with properties contained in received message.
> 
> no tabs please
> 
> @@ +3389,5 @@
> >      }
> >      msg.epid = PDU_PID_DEFAULT;
> > +
> > +    // Level 1 of message waiting indication
> > +    if (msg.mwi == null) {
> 
> If `msg.mwi` is meat to be a boolean, then `if (!msg.mwi)` is better here.

Thank you.

Comment 9

[Marshall Culpepper [:marshall_law]]

Attachment: WebSmsIndicator DOM API - v1

This is my initial patch that adds the new WebSmsIndicator DOM API. This has been tested working on otoro on T-Mobile, and obviously automated testing is currently missing (but is a WIP).

Comment 10

[Marshall Culpepper [:marshall_law]]

I just posted some example usage in a gist I created here:
https://gist.github.com/2975729

Comment 11

[Marshall Culpepper [:marshall_law]]

Comment on attachment 635949 [details] [diff] [review]
WebSmsIndicator DOM API - v1

Vicamo has feedback, so flagging him

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 635949 [details] [diff] [review]
WebSmsIndicator DOM API - v1

Review of attachment 635949 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Marshall,

A few thoughts and questions:

1.) 3GPP TS 23.040 section 9.2.3.24.2 also indicates the third level MWI should be saved in SIM/USIM file EFmwis. Is there a plan for this？
2.) The "store message" Message Waiting Indication Group doesn't mean we have to store it in our sms database. Instead, it indicates the status should be saved in EFmwis. If that's not available, then ME might save it in somewhere else, and in our case, the sms database. So, it follows the message with MWI may be treated as non-sms-message. You can just have a SmsManager.onmessagewaiting event carrying needed information and skip all SMS message receiving process.

::: dom/system/gonk/ril_worker.js
@@ +4046,5 @@
> +          /*
> +           * TS 23.040 V6.8.1 Sec 9.2.3.24.2
> +           * bits 1 0 : basic message indication type
> +           * bits 4 3 2 : extended message indication type
> +           * bits 6 5 : Profile id bit 7 storage type

There should be a line break before "bit 7 storage type". Right?

@@ +4049,5 @@
> +           * bits 4 3 2 : extended message indication type
> +           * bits 6 5 : Profile id bit 7 storage type
> +           */
> +          if (msgInd == PDU_MWI_STORE_TYPE_DISCARD ||
> +              msgInd == PDU_MWI_STORE_TYPE_STORE) {

Storage type is bit 7, not the full octet.

@@ +4056,5 @@
> +            if (!mwi) {
> +              mwi = msg.mwi = {};
> +            }
> +
> +            if (msgInd == PDU_MWI_STORE_TYPE_STORE) {

ditto

@@ +4299,5 @@
> +              mwi = msg.mwi = {};
> +            }
> +
> +            // TODO: De-activate the level 1 indicator when the SMS
> +            // has been read

Were it possible to have DCS read before PID at all? There can be only one way that `readProtocolIndicator` is invoked before `readDataCodingScheme`: in a Status-Report message.

@@ +4302,5 @@
> +            // TODO: De-activate the level 1 indicator when the SMS
> +            // has been read
> +            mwi.active = true;
> +            mwi.discard = false;
> +            mwi.hasMessages = true;

`The message content(if present) gives displayable information (e.g. the number ).` So it doesn't necessarily have a message?

@@ +4367,4 @@
>      if (DEBUG) debug("PDU: message encoding is " + encoding + " bit.");
>    },
>  
> +  readMessageWaitingFromDCS: function readMessageWaitingFromDCS(msg, dcs) {

IMHO this function can perfectly be merged into readDataCodingScheme().

@@ +4381,5 @@
> +      let active = (dcs & PDU_DCS_MWI_ACTIVE_BITS) == PDU_DCS_MWI_ACTIVE_VALUE;
> +
> +      // If TP-UDH is present, these values will be overwritten
> +      switch (dcs & PDU_DCS_MWI_TYPE_BITS) {
> +        case PDU_DCS_MWI_TYPE_VOICEMAIL:

Why do we have to separate voice mail from others?

Comment 13

[Marshall Culpepper [:marshall_law]]

(In reply to Vicamo Yang [:vicamo] from comment #12)
> 1.) 3GPP TS 23.040 section 9.2.3.24.2 also indicates the third level MWI
> should be saved in SIM/USIM file EFmwis. Is there a plan for this？
> 2.) The "store message" Message Waiting Indication Group doesn't mean we
> have to store it in our sms database. Instead, it indicates the status
> should be saved in EFmwis. If that's not available, then ME might save it in
> somewhere else, and in our case, the sms database.

I'm with you up to here :) I'll make sure the message gets stored in the EFmwis if it's available, or fallback to the SMS database..

> So, it follows the
> message with MWI may be treated as non-sms-message. You can just have a
> SmsManager.onmessagewaiting event carrying needed information and skip all
> SMS message receiving process.

I'm not sure that this follows -- in other levels of MWI, the SMS message holds important information such as the return call number (the TP-OE / message.sender), and in required in level (optional in level 2) is the "Return Call Message" which is the TP-UD / message.body.

It is possible we could store this information separately as part of the indicator, though, rather than strongly tying it to the SMS message in the DOM API / Database. Maybe something like this:

> interface SmsIndicator {
>    readonly attribute boolean active;
>    readonly attribute int messageCount;
>    readonly attribute DOMString returnAddress;
>    readonly attribute DOMString returnMessage;
> };

wdyt?

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +4046,5 @@
> > +          /*
> > +           * TS 23.040 V6.8.1 Sec 9.2.3.24.2
> > +           * bits 1 0 : basic message indication type
> > +           * bits 4 3 2 : extended message indication type
> > +           * bits 6 5 : Profile id bit 7 storage type
> 
> There should be a line break before "bit 7 storage type". Right?

Yeah, good catch!

> 
> @@ +4049,5 @@
> > +           * bits 4 3 2 : extended message indication type
> > +           * bits 6 5 : Profile id bit 7 storage type
> > +           */
> > +          if (msgInd == PDU_MWI_STORE_TYPE_DISCARD ||
> > +              msgInd == PDU_MWI_STORE_TYPE_STORE) {
> 
> Storage type is bit 7, not the full octet.
> 
> @@ +4056,5 @@
> > +            if (!mwi) {
> > +              mwi = msg.mwi = {};
> > +            }
> > +
> > +            if (msgInd == PDU_MWI_STORE_TYPE_STORE) {
> 
> ditto

Thanks, I'll bitmask these out and check them the right way :)

> 
> @@ +4299,5 @@
> > +              mwi = msg.mwi = {};
> > +            }
> > +
> > +            // TODO: De-activate the level 1 indicator when the SMS
> > +            // has been read
> 
> Were it possible to have DCS read before PID at all? There can be only one
> way that `readProtocolIndicator` is invoked before `readDataCodingScheme`:
> in a Status-Report message.

Fair enough, I can remove the duplicate initialization code here.

> 
> @@ +4302,5 @@
> > +            // TODO: De-activate the level 1 indicator when the SMS
> > +            // has been read
> > +            mwi.active = true;
> > +            mwi.discard = false;
> > +            mwi.hasMessages = true;
> 
> `The message content(if present) gives displayable information (e.g. the
> number ).` So it doesn't necessarily have a message?

"hasMessages" is an indicator of whether or not the indicator should actually be active. I plan to remove it from the API because it turns out to be redundant.

> 
> @@ +4367,4 @@
> >      if (DEBUG) debug("PDU: message encoding is " + encoding + " bit.");
> >    },
> >  
> > +  readMessageWaitingFromDCS: function readMessageWaitingFromDCS(msg, dcs) {
> 
> IMHO this function can perfectly be merged into readDataCodingScheme().

Originally it was, but I moved it out to make the code easier to read. If you really strongly about moving this back into readDataCodingScheme, I can move it back in.. but it feels like bike shedding (philikon to the rescue?) :)

> 
> @@ +4381,5 @@
> > +      let active = (dcs & PDU_DCS_MWI_ACTIVE_BITS) == PDU_DCS_MWI_ACTIVE_VALUE;
> > +
> > +      // If TP-UDH is present, these values will be overwritten
> > +      switch (dcs & PDU_DCS_MWI_TYPE_BITS) {
> > +        case PDU_DCS_MWI_TYPE_VOICEMAIL:
> 
> Why do we have to separate voice mail from others?

I originally modeled this logic after the Android impl -- they completely ignore non-Voicemail indicators, which makes me wonder if any carriers actually use the other sources..

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Marshall Culpepper [:marshall_law] from comment #13)
> (In reply to Vicamo Yang [:vicamo] from comment #12)
> > 1.) 3GPP TS 23.040 section 9.2.3.24.2 also indicates the third level MWI
> > should be saved in SIM/USIM file EFmwis. Is there a plan for this？
> > 2.) The "store message" Message Waiting Indication Group doesn't mean we
> > have to store it in our sms database. Instead, it indicates the status
> > should be saved in EFmwis. If that's not available, then ME might save it in
> > somewhere else, and in our case, the sms database.
> 
> I'm with you up to here :) I'll make sure the message gets stored in the
> EFmwis if it's available, or fallback to the SMS database..

EFmwis saving could be another BIG task for we don't have it in generic message yet (see bug 736706). I would suggest just bypass USIM saving process for level 2 MWI messages for now.

> > So, it follows the
> > message with MWI may be treated as non-sms-message. You can just have a
> > SmsManager.onmessagewaiting event carrying needed information and skip all
> > SMS message receiving process.
> 
> I'm not sure that this follows -- in other levels of MWI, the SMS message
> holds important information such as the return call number (the TP-OE /
> message.sender), and in required in level (optional in level 2) is the
> "Return Call Message" which is the TP-UD / message.body.

You're right. I re-examined TS 23.038 again. In "discard message" Message Waiting Indication Group, it's the message content to be discarded, not EFmwis as I previously thought. Please ignore my "it follows ..." statement.

But just like our short discuss in the noon, I would personally prefer not to add an attribute in SmsMessage if it can be avoid. MWI is an additional function for me, not a part of every SMS message. It is carried by a sms message, so we have to store the sms message if not specified by "discard message" Message Waiting Indication Group. But can we just add an event in SmsManager and leave SmsMessage untouched?

> It is possible we could store this information separately as part of the
> indicator, though, rather than strongly tying it to the SMS message in the
> DOM API / Database. Maybe something like this:
> 
> > interface SmsIndicator {
> >    readonly attribute boolean active;
> >    readonly attribute int messageCount;
> >    readonly attribute DOMString returnAddress;
> >    readonly attribute DOMString returnMessage;
> > };
> 
> wdyt?

I would suggest replacing `returnMessage` with `body` for you'll also need same field for level 2/3 MWI. As for `active`, does a zero-valued `messageCount` automatically imply it?

> > 
> > @@ +4367,4 @@
> > >      if (DEBUG) debug("PDU: message encoding is " + encoding + " bit.");
> > >    },
> > >  
> > > +  readMessageWaitingFromDCS: function readMessageWaitingFromDCS(msg, dcs) {
> > 
> > IMHO this function can perfectly be merged into readDataCodingScheme().
> 
> Originally it was, but I moved it out to make the code easier to read. If
> you really strongly about moving this back into readDataCodingScheme, I can
> move it back in.. but it feels like bike shedding (philikon to the rescue?)
> :)

It just reminded myself there is still Message Class handling to come in `readDataCodingScheme()`. By collecting all of DCS related decoding code together, it may be easier for me to compare the code with TS 23.038.

> > 
> > @@ +4381,5 @@
> > > +      let active = (dcs & PDU_DCS_MWI_ACTIVE_BITS) == PDU_DCS_MWI_ACTIVE_VALUE;
> > > +
> > > +      // If TP-UDH is present, these values will be overwritten
> > > +      switch (dcs & PDU_DCS_MWI_TYPE_BITS) {
> > > +        case PDU_DCS_MWI_TYPE_VOICEMAIL:
> > 
> > Why do we have to separate voice mail from others?
> 
> I originally modeled this logic after the Android impl -- they completely
> ignore non-Voicemail indicators, which makes me wonder if any carriers
> actually use the other sources..

I think you're right. If we don't have a `type` field in SmsIndicator, then there should be only one type.

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 635949 [details] [diff] [review]
WebSmsIndicator DOM API - v1

Review of attachment 635949 [details] [diff] [review]:
-----------------------------------------------------------------

I see Vicamo has already given some technical feedback (thanks, Vicamo!), so I'll just stick to some general notes:

Please split up large patches that touch many different things into logical parts. E.g. (1) interfaces, (2) DOM implementation, (3) RIL backend implementation, (4) tests. Makes it easier to review, especially when different chunks should be reviewed by different people. For instance, I'm not a DOM peer, so the DOM implementation should be reviewed by e.g. mounir, while sicking needs to do superreview on the new interfaces. Vicamo is a good person to review the RIL stuff. :)

Also, things we can and should write tests for:

* SmsService::createSmsIndicator(), see https://mxr.mozilla.org/mozilla-central/source/dom/sms/tests/test_smsservice_createsmsmessage.js (this is an xpcshell test)
* the "sms-indicator" observer notification -> "onindicator" event dispatch; we don't have a test for that for messages yet, but that doesn't mean we can't write one for indicators.
* decoding MWI messages correctly in ril_worker, see https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/tests/test_ril_worker_sms.js

Lastly, Mounir is likely going to ask: What about the Android backend? You want to make sure it (and the fallback backend) (a) compiles and (b) has a follow-up bug filed for it.

::: dom/base/Navigator.cpp
@@ +1047,4 @@
>      return defaultSmsPermission;
>    }
>  
> +  return nsContentUtils::URIIsChromeOrInPref(uri, "dom.sms.whitelist");

Good, but unrelated to this bug. Please file a separate bug + patch for this.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> Lastly, Mounir is likely going to ask: What about the Android backend? You
> want to make sure it (and the fallback backend) (a) compiles and (b) has a
> follow-up bug filed for it.

Android supports only level 2 MWI. For level 1 & 3, we might have to examine per message PID/DCS/miscEltList by our own. For level 2 "discard message" MWI, `telephonyManager.listen(aPhoneStateListener, PhoneStateListener.LISTEN_MESSAGE_WAITING_INDICATOR)` may serve as a change event source, and we can then retrieve detailed information in its callback.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo] from comment #16)
> (In reply to Philipp von Weitershausen [:philikon] from comment #15)
> > Lastly, Mounir is likely going to ask: What about the Android backend? You
> > want to make sure it (and the fallback backend) (a) compiles and (b) has a
> > follow-up bug filed for it.
> 
> Android supports only level 2 MWI. For level 1 & 3, we might have to examine
> per message PID/DCS/miscEltList by our own. For level 2 "discard message"
> MWI, `telephonyManager.listen(aPhoneStateListener,
> PhoneStateListener.LISTEN_MESSAGE_WAITING_INDICATOR)` may serve as a change
> event source, and we can then retrieve detailed information in its callback.

I knew Vicamo would know. :) Let's move that discussion to the follow-up bug.

Comment 18

[Marshall Culpepper [:marshall_law]]

FYI, Bug 768529 is the SMS whitelist cleanup bug

Comment 19

[Mounir Lamouri (:mounir)]

What is that for?!

Comment 20

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:mounir) from comment #19)
> What is that for?!

The message waiting indicator functionality that's part of SMS. See e.g. http://en.wikipedia.org/wiki/Message-waiting_indicator

Comment 21

[Philipp von Weitershausen [:philikon]]

(Used by carriers for voicemail, apparently.)

Comment 22

[Mounir Lamouri (:mounir)]

This has nothing to do with the WebSMS API. If the voicemail notifications is going trough SMS's, that should be handled by the backend and we should notify the user in another way. We could for example simply firing a notification from the chrome (RIL) when such an email arrives or we could even add a VoiceMail API.

Comment 23

[Marshall Culpepper [:marshall_law]]

> We could for example simply firing a notification from the chrome (RIL) when such an email arrives or we could even add a VoiceMail API.

I prefer an API since there is more data here than a simple text message (i.e. potentially the number of new messages, a return call number which could be tied to the dialer).

A separate Voicemail API may not be a bad idea if/when we start trying to support Visual Voicemail in the future.

Comment 24

[Marshall Culpepper [:marshall_law]]

Attachment: Voicemail DOM API - v1

After talking with Mounir and Jonas today, I'm proposing this set of DOM APIs for Voicemail support.

Comment 25

[Philipp von Weitershausen [:philikon]]

Comment on attachment 637305 [details] [diff] [review]
Voicemail DOM API - v1

Review of attachment 637305 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me!

::: dom/telephony/nsIDOMNavigatorVoicemail.idl
@@ +10,5 @@
> +
> +[scriptable, builtinclass, uuid(11ad8ebe-e999-4a0e-ae0a-a1e8958b6b9f)]
> +interface nsIDOMNavigatorVoicemail : nsISupports
> +{
> +  readonly attribute nsIDOMVoicemail mozVoicemail;

Could just add this to nsIDOMNavigatorTelephony...

::: dom/telephony/nsIDOMVoicemailEvent.idl
@@ +9,5 @@
> +{
> +  /**
> +   * The number of voicemails waiting is unknown (but greater than 0)
> +   */
> +  const unsigned short MESSAGE_COUNT_UNKNOWN = 0xFFFF;

I would prefer using `null` or -1.

(`null` feels more webby; you'll have to make `messageCount` a `jsval` for that, unfortunately.)

Comment 26

[Marshall Culpepper [:marshall_law]]

(In reply to Philipp von Weitershausen [:philikon] from comment #25)
> ::: dom/telephony/nsIDOMNavigatorVoicemail.idl
> @@ +10,5 @@
> > +
> > +[scriptable, builtinclass, uuid(11ad8ebe-e999-4a0e-ae0a-a1e8958b6b9f)]
> > +interface nsIDOMNavigatorVoicemail : nsISupports
> > +{
> > +  readonly attribute nsIDOMVoicemail mozVoicemail;
> 
> Could just add this to nsIDOMNavigatorTelephony... 

That's true, I'm not opposed to it. We'd want to change the event name though, something like "vmnotification"?

> ::: dom/telephony/nsIDOMVoicemailEvent.idl
> @@ +9,5 @@
> > +{
> > +  /**
> > +   * The number of voicemails waiting is unknown (but greater than 0)
> > +   */
> > +  const unsigned short MESSAGE_COUNT_UNKNOWN = 0xFFFF;
> 
> I would prefer using `null` or -1.
> 
> (`null` feels more webby; you'll have to make `messageCount` a `jsval` for
> that, unfortunately.)

The main reason this is a large number is because of the convenience of saying..

> if (event.messageCount > 0) {
>   // turn on the indicator..
>   if (event.messageCount !== MESSAGE_COUNT_UNKNOWN) {
>     // place a the count label next to the indicator
>   }
> } else {
>   // turn off the indicator
> }

Comment 27

[Marshall Culpepper [:marshall_law]]

Attachment: Voicemail DOM API - v2

I forgot to stage the changes for nsIDOMVoicemail.. There should just be one event "onnotification"

Comment 28

[Philipp von Weitershausen [:philikon]]

(In reply to Marshall Culpepper [:marshall_law] from comment #26)
> The main reason this is a large number is because of the convenience of
> saying..
> 
> > if (event.messageCount > 0) {
> >   // turn on the indicator..
> >   if (event.messageCount !== MESSAGE_COUNT_UNKNOWN) {
> >     // place a the count label next to the indicator
> >   }
> > } else {
> >   // turn off the indicator
> > }

This doesn't seem more complicated to me:

  if (event.messageCount == 0) {
    hideIndicator();
  } else {
    showIndicator();
    if (event.messageCount != null) {
      showMessageCountLabel(event.messageCount);
    }
  }

Comment 29

[Marshall Culpepper [:marshall_law]]

Attachment: Voicemail DOM API - v3

Incorporated feedback from Phil

Comment 30

[Philipp von Weitershausen [:philikon]]

Comment on attachment 637308 [details] [diff] [review]
Voicemail DOM API - v3

Review of attachment 637308 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/telephony/nsIDOMNavigatorTelephony.idl
@@ +12,4 @@
>  interface nsIDOMNavigatorTelephony : nsISupports
>  {
>    readonly attribute nsIDOMTelephony mozTelephony;
> +  readonly attribute nsIDOMVoicemail mozVoicemail;

You need to update the interface's UUID after changing it.

Comment 31

[Marshall Culpepper [:marshall_law]]

(In reply to Philipp von Weitershausen [:philikon] from comment #30)
> You need to update the interface's UUID after changing it.
D'oh! Will do once I get comments from Mounir..

Comment 32

[Mounir Lamouri (:mounir)]

Comment on attachment 637308 [details] [diff] [review]
Voicemail DOM API - v3

Review of attachment 637308 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a big fan of that API. That wouldn't provide more value than simply using System Messages.
If I understood correctly, the information we have are:
- if there are messages waiting (true/false) *or* the number of messages waiting;
- the number to call to listen to those messages;
- a text that is associated with the notification.

Can we have the list of numbers that left a message?

Also, your patches will have to be reviewed by DOM peers. That means you can ask reviews to philikon but you will need a review from at least one DOM peer to be able to land. This doesn't apply for that patch given that no real review is needed (there is no code, it's only about interface design, so sr is enough).

::: dom/telephony/nsIDOMVoicemail.idl
@@ +6,5 @@
> +
> +#include "nsIDOMEventTarget.idl"
> +
> +[scriptable, builtinclass, uuid(db5b727a-daf2-40b9-83e4-cc110eab44ca)]
> +interface nsIDOMVoicemail : nsIDOMEventTarget

This interface should be Moz prefixed.

@@ +12,5 @@
> +  /**
> +   * A voicemail notification was received from the network.
> +   */
> +  attribute nsIDOMEventListener onnotification;
> +

nit: remove the empty line.

::: dom/telephony/nsIDOMVoicemailEvent.idl
@@ +4,5 @@
> +
> +#include "nsIDOMEvent.idl"
> +
> +[scriptable, builtinclass, uuid(4642f196-1a30-4a9b-986f-7f1574d5cee1)]
> +interface nsIDOMVoicemailEvent : nsIDOMEvent

This should also be Moz prefixed.

Comment 33

[Marshall Culpepper [:marshall_law]]

(In reply to Mounir Lamouri (:mounir) from comment #32)
> I'm not a big fan of that API. That wouldn't provide more value than simply
> using System Messages.

From a consistency perspective, I think it makes more sense that Voicemail has a model that matches the other telephony / network APIs. A single event is somewhat limited for now, but there is potential for much more as we get more functionality later.

> If I understood correctly, the information we have are:
> - if there are messages waiting (true/false) *or* the number of messages
> waiting;

It's actually, if there are messages waiting (true/false) and *maybe* the number of messages waiting :)

There are basically three possible states:
- hasMessages === true && messageCount === null (we have messages waiting, but don't the network didn't tell us how many)
- hasMessages === true && messageCount > 0 (same as above, but we have an actual count)
- hasMessages === false && messageCount === 0 (no messages available, messageCount will always be 0 in this case)

Since messageCount has a tri-state, I re-added the hasMessages flag for the simple off/on check to simplify the logic a bit

> 
> Can we have the list of numbers that left a message?

Not until we get visual voicemail :) Once that happens, I plan to have a fuller querying interface for voicemails available, read state, etc..

> 
> Also, your patches will have to be reviewed by DOM peers. That means you can
> ask reviews to philikon but you will need a review from at least one DOM
> peer to be able to land. This doesn't apply for that patch given that no
> real review is needed (there is no code, it's only about interface design,
> so sr is enough).

I asked for Vicamo's feedback initially, I'll make sure he's marked r? on the new implementation patch once the IDLs are finished.

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Marshall Culpepper [:marshall_law] from comment #33)
> (In reply to Mounir Lamouri (:mounir) from comment #32)
> > Also, your patches will have to be reviewed by DOM peers. That means you can
> > ask reviews to philikon but you will need a review from at least one DOM
> > peer to be able to land. This doesn't apply for that patch given that no
> > real review is needed (there is no code, it's only about interface design,
> > so sr is enough).
> 
> I asked for Vicamo's feedback initially, I'll make sure he's marked r? on
> the new implementation patch once the IDLs are finished.

I'm not a DOM peer actually. Perhaps you should add Mounir or Jonas ... as well.
https://wiki.mozilla.org/Modules/All#Document_Object_Model

Comment 35

[Marshall Culpepper [:marshall_law]]

(In reply to Vicamo Yang [:vicamo] from comment #34)
> I'm not a DOM peer actually. Perhaps you should add Mounir or Jonas ... as
> well.
> https://wiki.mozilla.org/Modules/All#Document_Object_Model

Ahh I mistaked DOM peer for RIL peer :) I'll make sure Jonas is SR'd as well as Mounir on the next patch then

Comment 36

[Marshall Culpepper [:marshall_law]]

Attachment: Voicemail DOM API - v4

Added Moz prefixes, new uuid for nsIDOMTelephony. FYI - The majority of implementation and testing code is already done, they just needs to be updated to reflect whatever API we decide on.

Comment 37

[Marshall Culpepper [:marshall_law]]

Argh, I updated the UUID for the wrong interface :) will update again after review..

Comment 38

[Marshall Culpepper [:marshall_law]]

Attachment: Voicemail DOM API - v5

UUID fix, changing SR to Jonas per Mounir

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 637496 [details] [diff] [review]
Voicemail DOM API - v5

Review of attachment 637496 [details] [diff] [review]:
-----------------------------------------------------------------

Not the greatest design ever, but as long as the gaia folks are ok with it it's ok with me. The main problem I see is that unless an app is awake to receive the event, there is no way to get at information of what the current count is.

::: dom/telephony/nsIDOMVoicemailEvent.idl
@@ +26,5 @@
> +   * } else {
> +   *   // hide the voicemail notification
> +   * }
> +   */
> +  readonly attribute jsval messageCount;

why isn't this just a 'long' rather than a 'jsval'? Just set the count to 0 and the code example above will work. Or you can set it to -1 to indicate "unknown" if the second test is slightly modified.

Comment 40

[Marshall Culpepper [:marshall_law]]

(In reply to Jonas Sicking (:sicking) from comment #39)
> Not the greatest design ever, but as long as the gaia folks are ok with it
> it's ok with me. The main problem I see is that unless an app is awake to
> receive the event, there is no way to get at information of what the current
> count is.

I'll do some more chatting with them today to make sure I'm going down the right path. I agree that the lack of persistence in the API is limiting..

> 
> > +  readonly attribute jsval messageCount;
> 
> why isn't this just a 'long' rather than a 'jsval'? Just set the count to 0
> and the code example above will work. Or you can set it to -1 to indicate
> "unknown" if the second test is slightly modified.

Originally I had used -1 to indicate unknown, but Phil suggested that null was more webby.. I'm fine with going back to a constant value / short here (voicemail count is limited to 255).

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm fine either way, but for example Array/String.indexOf returns -1 to indicate "nothing found", so I think that's fine here too.

Comment 42

[Marshall Culpepper [:marshall_law]]

Attachment: Part 1: DOM API (IDLs) - v1

One more round of SR on this.. after talking with Jonas, Etienne, and Fabrice more late last week, I've come around to the fact that we'll need some level of persistence for the Voicemail status.

Comment 43

[Marshall Culpepper [:marshall_law]]

Attachment: Part 2: RIL impl of MWI - v1

Comment 44

[Marshall Culpepper [:marshall_law]]

Attachment: Part 3: DOM impl (C++) - v1

Comment 45

[Marshall Culpepper [:marshall_law]]

Note: I'm currently updating my xpcshell and marionette tests to use the new API, the patch for that should be coming soon.

Comment 46

[Mounir Lamouri (:mounir)]

Comment on attachment 639471 [details] [diff] [review]
Part 3: DOM impl (C++) - v1

Review of attachment 639471 [details] [diff] [review]:
-----------------------------------------------------------------

I think bent would be more appropriate to review that code.

Comment 47

[Philipp von Weitershausen [:philikon]]

Comment on attachment 639470 [details] [diff] [review]
Part 2: RIL impl of MWI - v1

Review of attachment 639470 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +840,5 @@
>                                             options.number,
>                                             options.fullBody,
>                                             timestamp,
> +                                           true,
> +                                           false);

Meaningless boolean arguments FTL. But that aside, I don't see the part where you added another parameter to nsISmsService::createSmsMessage().

(Also, please submit your patches with 8 lines of context.)

Vicamo is a better reviewer for the rest, so I'm going to remove myself from this patch.

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 639470 [details] [diff] [review]
Part 2: RIL impl of MWI - v1

Review of attachment 639470 [details] [diff] [review]:
-----------------------------------------------------------------

Much thanks for your great work! A few things have to be clarified:

::: dom/system/gonk/ril_consts.js
@@ +3,4 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // Set to true to debug all RIL layers
> +const DEBUG_ALL = true;

I think DEBUG flag is batter left unset/false by default.

::: dom/system/gonk/ril_worker.js
@@ +4082,5 @@
> +           * bits 6 5   : Profile id
> +           * bit  7     : storage type
> +           */
> +          if (storeType == PDU_MWI_STORE_TYPE_DISCARD ||
> +              storeType == PDU_MWI_STORE_TYPE_STORE) {

This conditional test is always true because storeType can only be either 0x80 or 0x00.

@@ +4099,5 @@
> +               * Check for conflict between message storage bit in TP_UDH
> +               * & DCS. The message shall be stored if either of the one
> +               * bits indicates so. TS 23.040 V6.8.1 Sec 9.2.3.24.2
> +               */
> +              let codingGroup = msg.dcs & PDU_DCS_CODING_GROUP_BITS;

I really can't convince myself it's necessary to check DCS again here. This line assumes `msg.dcs` was already fetched. For SMS-DELIVERY PDUs, DCS always comes before UDH, so this assumption is probably true. Then, if it's already fetched/decoded, you can simply refer to `msg.mwi` instead. MWI related information, if available, had been decoded before UDH decoding.

Besides, by 3GPP TS 23.040 V11.2.0 page 73, `In the event of a conflict between this setting and the seting of the Data Coding Scheme then the message shall be stored if either the DCS indicates this, or Octet 1 above indicates this.`

Comment 49

[Marshall Culpepper [:marshall_law]]

Attachment: Part 2: RIL impl of MWI - v2

Incorporated feedback from Vicamo, and a few bug fixes

Comment 50

[Marshall Culpepper [:marshall_law]]

Attachment: Part 3: DOM impl (C++) - v2

Adopt whitelist checking code from other APIs, and a DOM class info ordering fix

Comment 51

[Marshall Culpepper [:marshall_law]]

Attachment: Part 4: Tests - v1

Comment 52

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 640414 [details] [diff] [review]
Part 2: RIL impl of MWI - v2

Review of attachment 640414 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your great work!

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 640423 [details] [diff] [review]
Part 4: Tests - v1

Review of attachment 640423 [details] [diff] [review]:
-----------------------------------------------------------------

Great Job!

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 640418 [details] [diff] [review]
Part 3: DOM impl (C++) - v2

Just some nit, but I could look at still a new patch.

>+  bool changed = false;
>+  nsCOMPtr<VoicemailStatus> status = static_cast<VoicemailStatus*>(mStatus.get());
VoicemailStatus is a concrete class. You should use nsRefPtr

>+  VoicemailEvent *vmEvent = new VoicemailEvent(nsnull, nsnull);
>+  nsRefPtr<nsDOMEvent> event(vmEvent);
Why not nsRefPtr<VoicemailEvent> vwEvent = new VoicemailEvent(nsnull, nsnull);
and then no need for 'event' at all. 

>+  nsresult rv = vmEvent->Init(NS_LITERAL_STRING("statuschanged"), false, false,
>+                              mStatus);
Oh, hmm, for consistency please call the method InitVoicemailEvent


>+NS_NewVoicemail(nsPIDOMWindow* aWindow, nsIDOMMozVoicemail** aVoicemail)
>+{
>+  NS_ASSERTION(aWindow, "Null pointer!");
Rather useless assertion. We'll crash anyway immediately below.


>+  nsCOMPtr<nsIRILContentHelper> ril =
>+    do_GetService(NS_RILCONTENTHELPER_CONTRACTID);
>+  NS_ENSURE_TRUE(ril, NS_ERROR_UNEXPECTED);
I prefer NS_ENSURE_STATE(expr); for cases when NS_ERROR_UNEXPECTED is thrown

>+  nsRefPtr<Voicemail> voicemail = new Voicemail(innerWindow, ril);
>+  NS_ENSURE_TRUE(voicemail, NS_ERROR_UNEXPECTED);
'new' is infallible, so no need for the null check here.


>+public:
>+  VoicemailEvent(nsPresContext* aPresContext, nsEvent* aEvent)
>+    : nsDOMEvent(aPresContext, aEvent),
>+      mStatus(nsnull) { }
No need to initialize nsCOMPtr member variables to null. That happens automatically anyway.

>+NS_IMETHODIMP
>+VoicemailStatus::GetHasMessages(bool *aHasMessages)
Nit, bool*, not bool *

Comment 55

[Marshall Culpepper [:marshall_law]]

Attachment: Part 3: DOM impl (C++) - v3

smaug feedback updates

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 641600 [details] [diff] [review]
Part 3: DOM impl (C++) - v3


>+Voicemail::Voicemail(nsPIDOMWindow* aWindow, nsIRILContentHelper* aRIL)
>+  : mRIL(aRIL),
>+    mStatus(nsnull)
No need to initialize nsCOMPtr to nsnull


>+Voicemail::VoicemailNotification(bool aHasMessages,
>+                                 PRInt16 aMessageCount,
>+                                 const nsAString& aReturnNumber,
>+                                 const nsAString& aReturnMessage)
>+{
>+  NS_ASSERTION(aMessageCount >= 0 ||
>+               aMessageCount == nsIDOMMozVoicemailStatus::MESSAGE_COUNT_UNKNOWN,
>+               "Invalid message count");

Perhaps better to return NS_ERROR_FAILURE and not just have NS_ASSERTION

I don't know when VoicemailNotification is called.
Does it always happen at safe time so that JS (and event listeners) can run?

>+  nsRefPtr<VoicemailStatus> status = static_cast<VoicemailStatus*>(mStatus.get());
Actually, VoicemailStatus* works here fine.

Comment 57

[Marshall Culpepper [:marshall_law]]

(In reply to Olli Pettay [:smaug] from comment #56)
> I don't know when VoicemailNotification is called.
> Does it always happen at safe time so that JS (and event listeners) can run?

It should be safe, VoicemailCallback is a callback function that's called from RILContentHelper, which lives in content. FWIW, this also follows the same model seen for Telephony events / callbacks from RILContentHelper.

Comment 58

[Marshall Culpepper [:marshall_law]]

Attachment: Part 3: DOM impl (C++) - v4

More updates from smaug's review, removed an unused macro

Comment 59

[Marshall Culpepper [:marshall_law]]

Attachment: Part 1: DOM API (IDLs) - v2

Internal IDL updates to keep Voicemail data inside RILContentHelper (no DOM API updates)

Comment 60

[Marshall Culpepper [:marshall_law]]

Attachment: Part 2: RIL impl of MWI - v3

Moved VoicemailStatus impl and holder to RILContentHelper

Comment 61

[Marshall Culpepper [:marshall_law]]

Attachment: Part 3: DOM impl (C++) - v5

Moved VoicemailStatus impl to JS, updated Voicemail impl

Comment 62

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 643001 [details] [diff] [review]
Part 1: DOM API (IDLs) - v2

Review of attachment 643001 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/telephony/nsIDOMVoicemailStatus.idl
@@ +33,5 @@
> +   * } else {
> +   *   // hide the voicemail notification
> +   * }
> +   */
> +  readonly attribute short messageCount;

Might as well make this a long.

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 643006 [details] [diff] [review]
Part 2: RIL impl of MWI - v3

Review of attachment 643006 [details] [diff] [review]:
-----------------------------------------------------------------

Look good for me ;)

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 643007 [details] [diff] [review]
Part 3: DOM impl (C++) - v5


>+Voicemail::GetStatus(nsIDOMMozVoicemailStatus** aStatus)
>+{
>+  *aStatus = nsnull;
>+
>+  NS_ENSURE_TRUE(mRIL, NS_ERROR_FAILURE);
>+  return mRIL->GetVoicemailStatus(aStatus);
>+}
Could be
*aStatus = nsnull;
NS_ENSURE_STATE(mRil);
return mRIL->GetVoicemailStatus(aStatus);

>+VoicemailEvent::InitVoicemailEvent(const nsAString& aEventTypeArg,
>+                                   bool aCanBubbleArg, bool aCancelableArg,
>+                                   nsIDOMMozVoicemailStatus* aStatus)
>+{
>+  nsresult rv = nsDOMEvent::InitEvent(aEventTypeArg, aCanBubbleArg,
>+                                      aCancelableArg);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  mStatus = aStatus;
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+VoicemailEvent::GetStatus(nsIDOMMozVoicemailStatus** aStatus)
>+{
>+  NS_IF_ADDREF(*aStatus = mStatus);
>+  return NS_OK;
>+}
>+

Where is the support for event ctor?
Please add it.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

Actually, we can add support for the event ctor once the codegen for simple events has landed.

Comment 66

[Marshall Culpepper [:marshall_law]]

Attachment: Part 1: DOM API (IDLs) - v3

Update messageCount to long per Jonas

Comment 67

[Marshall Culpepper [:marshall_law]]

Attachment: Part 3: DOM impl (C++) - v6

Addressed Olli's macro nit

Comment 68

[Vicamo Yang [:vicamo][:vyang]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bf48a6be1697
http://hg.mozilla.org/integration/mozilla-inbound/rev/c7694b9bed0d
http://hg.mozilla.org/integration/mozilla-inbound/rev/c39bb0448bad
http://hg.mozilla.org/integration/mozilla-inbound/rev/2a88d2e2ca07

Comment 69

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bf48a6be1697
https://hg.mozilla.org/mozilla-central/rev/c7694b9bed0d
https://hg.mozilla.org/mozilla-central/rev/c39bb0448bad
https://hg.mozilla.org/mozilla-central/rev/2a88d2e2ca07