797277 - B2G SMS: expose message class info in DOM API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

The content page has to know the message class of incoming SMS messages to take additional actions, but we don't have that attribute exposed currently.

Message class of a SMS might have following possible values: unset, 0, 1, 2, 3.

See also bug 736706, 3GPP TS 23.038, 3GPP TS 23.040.

Comment 1

[Mounir Lamouri (:mounir)]

What is the message class? Why the content needs to know that? What actions could happen?

BTW, this is a feature and we are pass feature freeze, I don't know the process to push features at this point (assuming we want this).

Comment 2

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

(In reply to Mounir Lamouri (:mounir) from comment #1)
> What is the message class? Why the content needs to know that? What actions
> could happen?
> 
> BTW, this is a feature and we are pass feature freeze, I don't know the
> process to push features at this point (assuming we want this).

Message Class is a way to categorize SMS messages. All normal text messages has message class unset. Those with message class 0 should be discarded instead of sending it to the storage after being displayed in MMI, etc. Gecko supports the underlying processing in bug 736706.

The message-class-0 messages are only available in `mozSms.onreceived` event and system message for once and discarded without saving in database. When such a message arrives, user sees the notification on both status bar(because of system message) and lockscreen, but they can't find the message in message app(because message app fetches messages from database). Right now, Gaia can't have a chance to distinguish such messages and take special care on them because of the lack of message class attribute in DOM API.

Comment 3

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

Attachment: Part 1: IDL changes : WIP

Comment 4

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

Attachment: Part 2: DOM implementation : WIP

Comment 5

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

Attachment: Part 3: RIL implementation : WIP

Comment 6

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

There seems to be no other solution than this for bug 796904. Request for bb+ flag since bug 796904 is already bb+.

Comment 7

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

Attachment: Part 1: IDL changes

1. messageClass as DOMString
2. renew interface UUID as well

Comment 8

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

Attachment: Part 2: DOM implementation

1. messageClass as DOMString
2. rebase to bug 742970

Comment 9

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

Attachment: Part 3: RIL implementation

1. indexedDb version 4
2. rebase to bug 742970
3. s/FILTER_CLASS/FILTER_MESSAGE_CLASS/
4. s/CLASS_UNDEFINED/MESSAGE_CLASS_NORMAL/
5. reorder parameters in createSmsMessage()
6. use messageClass, not klass for unified naming
7. fix missed messageClass param in creating SmsMessage for ondelivery{success,error} event
8. let ril_worker return string-typed value of messageClass: "normal", "me-specific", "sim-specific", "te-specific"

Comment 10

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

Attachment: Part 4: Android implementation

SMS Message Class in Android is only available in just received message PDU carried by callback interns. SMS Message Class is assigned to "normal" in all other situations and bug 804476 is opened for further discussion.

Comment 11

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

Attachment: Part 5: test cases

update xpcshell & marionette tests

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 674195 [details] [diff] [review]
Part 3: RIL implementation

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1245,5 @@
>        return;
>      }
>  
>      let id = -1;
> +    if (message.messageClass != RIL.GECKO_SMS_MESSAGE_CLASSES[0]) {

Please keep the expression descriptive:

  RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_0]

::: dom/system/gonk/ril_consts.js
@@ +971,5 @@
>  
> +const GECKO_SMS_MESSAGE_CLASS_NORMAL = "normal";
> +const GECKO_SMS_MESSAGE_CLASSES = [
> +  "class-0", "me-specific", "sim-specific", "te-specific"
> +];

At least a comment that these correspond to PDU_DCS_MSG_CLASS_* would be nice. Maybe something like:

  const GECKO_SMS_MESSAGE_CLASSES = [
    "class-0",       // PDU_DCS_MSG_CLASS_0
    "me-specific",   // PDU_DCS_MSG_CLASS_ME_SPECIFIC
    "sim-specific",  // PDU_DCS_MSG_CLASS_SIM_SPECIFIC
    "te-specific"    // PDU_DCS_MSG_CLASS_TE_SPECIFIC
  ];

Although you could also write this out in code like in [1]:

  const GECKO_SMS_MESSAGE_CLASSES = {};
  GECKO_SMS_MESSAGE_CLASSES[PDU_DCS_MSG_CLASS_0] = "class-0";
  // etc.
  GECKO_SMS_MESSAGE_CLASSES[PDU_DCS_MSG_CLASS_UNKNOWN] = null;

That way you can also handle the unknown case ;).

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#236

::: dom/system/gonk/ril_worker.js
@@ +3750,5 @@
>        // 9.2.3.9
>        return PDU_FCS_OK;
>      }
>  
> +    if (message.messageClass == GECKO_SMS_MESSAGE_CLASSES[2]) {

Same as above (and everywhere else in this file): please use the const in the index.

r=me with that.

Comment 13

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

Attachment: Part 3: RIL implementation : v2

Address review comment #12. Thanks Philipp!

Comment 14

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

Attachment: Part 3: Android implementation : v2

Rebase only. Original patch conflicts with V2 patch uploaded in bug 720325.

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 674192 [details] [diff] [review]
Part 2: DOM implementation

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

r=me but that might require some changes later depending on the deliveryfailed bug outcome.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #0)
> The content page has to know the message class of incoming SMS messages to
> take additional actions, but we don't have that attribute exposed currently.
> 
> Message class of a SMS might have following possible values: unset, 0, 1, 2,
> 3.
Are the message classes always integers/constants (or unset)? If so, I'd prefer the idl to use integers rather than strings.

Comment 17

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

Comment on attachment 674571 [details] [diff] [review]
Part 3: Android implementation : v2

Oops, the original patch title is wrong!

Comment 18

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

(In reply to Brad Lassey [:blassey] from comment #16)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #0)
> > The content page has to know the message class of incoming SMS messages to
> > take additional actions, but we don't have that attribute exposed currently.
> > 
> > Message class of a SMS might have following possible values: unset, 0, 1, 2,
> > 3.
> Are the message classes always integers/constants (or unset)? If so, I'd
> prefer the idl to use integers rather than strings.

For SMS, yes! SMS has only those five message classes, but Cell Broadcast(SMSCB), another similar message type over the air, has two extra message classes: user defined 1 and user defined 2. Although cell broadcast is addressed separately in bug 788093, they share similar message interface(but still different one). I'd prefer integers for performance but strings for unified typing. Anyway, I leave the final decision to DOM peers. If Jonas/Mounir prefers the same, I'll just follow.

Comment 19

[Mounir Lamouri (:mounir)]

Comment on attachment 674571 [details] [diff] [review]
Part 3: Android implementation : v2

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

r=me

::: embedding/android/GeckoSmsManager.java
@@ +334,5 @@
>    private final static int kDeliveryStatusPending       = 2;
>    private final static int kDeliveryStatusError         = 3;
>  
> +  /*
> +   * Keep the following error codes in sync with |MessageClass| in:

nit: please update the comment... those are not "error codes".

@@ +973,5 @@
> +      return kMessageClassSIMSpecific;
> +    } else if (aMessageClass == MessageClass.CLASS_3) {
> +      return kMessageClassTESpecific;
> +    } else {
> +        return kMessageClassNormal;

nit: indentation

::: widget/android/AndroidJNI.cpp
@@ +211,5 @@
>      };
>  
>      SmsMessageData message(0, eDeliveryState_Received, eDeliveryStatus_NotApplicable,
>                             nsJNIString(aSender, jenv), EmptyString(),
> +                           nsJNIString(aBody, jenv), (MessageClass)aMessageClass,

nit: please use static_cast<MessageClass>(aMessageClass)

Comment 20

[Mounir Lamouri (:mounir)]

If we really want to support Class {0, 1, 2, 3}, I would prefer an integer too. I actually though about that but I saw you put names instead of numbers as string so I thought I was misunderstand the thing until I reviewed the Android patch which converts those class numbers to class "names". The only concern I have is if we use integers, what should be the "no class message" value. Returning null would sounds weird...

Comment 21

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

(In reply to Mounir Lamouri (:mounir) from comment #20)
> The only concern I have is if we use integers, what should be the
> "no class message" value. Returning null would sounds weird...

Besides, `null` is used to clear that attribute in SmsFilter.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> (In reply to Mounir Lamouri (:mounir) from comment #20)
> > The only concern I have is if we use integers, what should be the
> > "no class message" value. Returning null would sounds weird...
> 
> Besides, `null` is used to clear that attribute in SmsFilter.

Yes, that's quite true. So right now, unless someone comes with a good idea for the "no message class" situation, I think we should stick with strings. However, why are you using something else than "class0", "class1", etc.?

Comment 23

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

(In reply to Mounir Lamouri (:mounir) from comment #22)
> However, why are you using something else than "class0", "class1", etc.?

Here is an excerpt from 3GPP TS 23.038 section 4:

  Bit 1  Bit 0  Message Class
    0      0    Class 0
    0      1    Class 1 Default meaning: ME-specific
    1      0    Class 2 (U)SIM specific message
    1      1    Class 3 Default meaning: TE-specific

Class-0 messages do not have aliased names, but others do. I think that will be more human-readable so I used them. If you feel like the other way, I can change them. Actually, I do think Class-0/1/2/3 will be more understandable for engineers.

Comment 24

[Mounir Lamouri (:mounir)]

I'm concerned with the "default meaning" part in this list. If there are more than one meaning for "class-x", better to keep "class-x" name instead of putting the name of the default meaning and be wrong. So I would prefer having "class-x" names.

However, you are the RIL expert here so your opinion is more important than mine.

Comment 25

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

(In reply to Mounir Lamouri (:mounir) from comment #24)
> I'm concerned with the "default meaning" part in this list. If there are
> more than one meaning for "class-x", better to keep "class-x" name instead
> of putting the name of the default meaning and be wrong. So I would prefer
> having "class-x" names.
> 
> However, you are the RIL expert here so your opinion is more important than
> mine.

I'm fine with "class-x", and as I said, "class-x" is more commonly used in technical documents. Besides, no one but telephony engineers actually knows what do "ME" and "TE" mean.

By the way, do you think "normal" is a good term for "no class meaning", "no message class defined"? Most text messages you received and all that you send on your mobile phone are of this kind. Class 0/1/2/3 messages are rarely used. And "user defined" classes used only in Cell Broadcast, are "user-1" and "user-2" acceptable for you?

Comment 26

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

The classes are documented on various websites, for example in the link below. Though I can't say I feel a whole lot smarter after reading these docs.

http://library.developer.nokia.com/index.jsp?topic=/GUID-E35887BB-7E58-438C-AA27-97B2CDE7E069/GUID-CBFDD753-BAE3-5C40-B947-EB8CDA11CD23.html

What's the difference between class 1 and unset?

It sounds like class 2 can be handled entirely internally in the API implementation. No need to gaia or any other apps to do anything special here?

What is the "external device" discussed in class 3? Should these messages be displayed to the user? Should they be saved in the database?


I must admit that it sounds to me like we should save class 0 messages in the database automatically. All websites that I found that documented the classes said that it was ok to save messages of class 0 if the user requested it. Could we simply say that gaia implicitly requests that all messages are saved. It seems confusing and annoying to users if some messages can't be dismissed and then read later.

Comment 27

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

(In reply to Jonas Sicking (:sicking) from comment #26)
> The classes are documented on various websites, for example in the link
> below. Though I can't say I feel a whole lot smarter after reading these
> docs.
> 
> http://library.developer.nokia.com/index.jsp?topic=/GUID-E35887BB-7E58-438C-
> AA27-97B2CDE7E069/GUID-CBFDD753-BAE3-5C40-B947-EB8CDA11CD23.html
> 
> What's the difference between class 1 and unset?

In current Android implementation, they are the same. In the specification, there is a special case that when TP-PID set to ME-Depersonalization and Message Class set to Class-1, the message should not be displayed (but leaves undefined on whether or not should the message be stored).

> It sounds like class 2 can be handled entirely internally in the API
> implementation. No need to gaia or any other apps to do anything special
> here?

Some Class-2 messages are directly handled by modem and never escape from it. Some don't. For those reach our ril_worker, we treat them as normal ones.

> What is the "external device" discussed in class 3? Should these messages be
> displayed to the user? Should they be saved in the database?

UE[1](User Equipment): your mobile phone, contains two logical elements: UICC & ME
UICC: your SIM card
ME[2](Mobile Equipment) contains two logical elements: MT ＆ TE.
MT(Mobile Termination): for mobile network access
TE(Terminal Equipment): everything other than MT, controls MT through UICC.

So, for a TE specific message, it means that's a special message, but could be for anything on your phone. The device maker may send such messages to fix, add, control, or anything they want to do on you phone because they're able to customize their own RIL implementation. For example, Nokia has their own "Smart Message" protocol, which makes use of SMS, for device configuration. To completely support such messages, we need a `data` attribute in addition to `body` in SmsMessage.

> I must admit that it sounds to me like we should save class 0 messages in
> the database automatically. All websites that I found that documented the
> classes said that it was ok to save messages of class 0 if the user
> requested it.

"If the ME is incapable of displaying short messages or if the immediate display of the message has been disabled through MMI then the ME shall treat the short message as though there was no message class, i.e. it will ignore bits 0 and 1 in the TP-DCS and normal rules for memory capacity exceeded shall apply" ~ 3GPP TS 23.038 section 4.

We're certainly capable. Even we want to pretend the other way, there should be a preference to switch the behaviour. Besides, we've informed there is a mandatory test item for Class-0 messages in one of our partners as I've said in bug 796964 comment #19.

> Could we simply say that gaia implicitly requests that all
> messages are saved. It seems confusing and annoying to users if some
> messages can't be dismissed and then read later.

"The ME may make provision through MMI for the user to selectively prevent the message from being displayed immediately." ~ 3GPP TS 23.038 section 4.

[1]: 3GPP TS 21.905
[2]: http://books.google.com.tw/books?id=uR03kPNyx5UC&lpg=PA71&ots=5YTk12_2qf&dq=%22mobile%20termination%22%20%22mobile%20equipment%22%20%22terminal%20equipment%22&hl=zh-TW&pg=PA71#v=onepage&q=%22mobile%20termination%22%20%22mobile%20equipment%22%20%22terminal%20equipment%22&f=false

Comment 28

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #27)
> (In reply to Jonas Sicking (:sicking) from comment #26)
> > The classes are documented on various websites, for example in the link
> > below. Though I can't say I feel a whole lot smarter after reading these
> > docs.
> > 
> > http://library.developer.nokia.com/index.jsp?topic=/GUID-E35887BB-7E58-438C-
> > AA27-97B2CDE7E069/GUID-CBFDD753-BAE3-5C40-B947-EB8CDA11CD23.html
> > 
> > What's the difference between class 1 and unset?
> 
> In current Android implementation, they are the same. In the specification,
> there is a special case that when TP-PID set to ME-Depersonalization and
> Message Class set to Class-1, the message should not be displayed (but
> leaves undefined on whether or not should the message be stored).

I don't know what "TP-PID" or "ME-Depersonalization" means.

I guess the question here is, do you think we should do something different for unset message class than from message class 1?

> > What is the "external device" discussed in class 3? Should these messages be
> > displayed to the user? Should they be saved in the database?
> 
> UE[1](User Equipment): your mobile phone, contains two logical elements:
> UICC & ME
> UICC: your SIM card
> ME[2](Mobile Equipment) contains two logical elements: MT ＆ TE.
> MT(Mobile Termination): for mobile network access
> TE(Terminal Equipment): everything other than MT, controls MT through UICC.
> 
> So, for a TE specific message, it means that's a special message, but could
> be for anything on your phone. The device maker may send such messages to
> fix, add, control, or anything they want to do on you phone because they're
> able to customize their own RIL implementation. For example, Nokia has their
> own "Smart Message" protocol, which makes use of SMS, for device
> configuration. To completely support such messages, we need a `data`
> attribute in addition to `body` in SmsMessage.

So if I understand correctly, message class 3 is intended to be delivered to various software and hardware systems on the device. It's not intended to be delivered to the user?

If that is correct, it sounds to me like we shouldn't be exposing these messages through the normal SMS API.

For now it sounds to me like we shouldn't expose these messages to apps at all since we have no apps that are currently planning on making use of them, and we have said that the SMS API should only be exposed to certified apps. I.e. these messages shouldn't fire DOM events or system messages, and they should not be saved in the sms database.

In the longer term, I think it would be great to have an API very similar to the current API which allows apps to listen to these messages. But I think we should keep them separate from the user-facing messages if I understand this class correctly.

Do you agree?


By the way, is class 3 messages usually what's used to notify about pending messages in the voicemail?

> > I must admit that it sounds to me like we should save class 0 messages in
> > the database automatically. All websites that I found that documented the
> > classes said that it was ok to save messages of class 0 if the user
> > requested it.
> 
> "If the ME is incapable of displaying short messages or if the immediate
> display of the message has been disabled through MMI then the ME shall treat
> the short message as though there was no message class, i.e. it will ignore
> bits 0 and 1 in the TP-DCS and normal rules for memory capacity exceeded
> shall apply" ~ 3GPP TS 23.038 section 4.
> 
> We're certainly capable. Even we want to pretend the other way, there should
> be a preference to switch the behaviour. Besides, we've informed there is a
> mandatory test item for Class-0 messages in one of our partners as I've said
> in bug 796964 comment #19.

That bug number doesn't seem correct.

I think there are four relevant questions here:

* What does certification require.
* What does certification allow.
* What do we think makes for a good UX. I.e. what do we think is the best solution
  for users.
* What do other modern smartphones do.

If certification requires that we don't save these messages unless the user explicitly chooses to do so, then that makes things easy, we'll simply have to do that no matter if we like it or not.

However, many times specifications call for UI which over time becomes obsolete. The HTTP specification actually require that the user is informed in a bunch of situations, but all modern browsers ignore this.

I would be interested to hear what class 0 messages usually contain?

Josh: Do you have opinions on if we should always saves messages that are displayed to the user in the SMS database such that they can be viewed later?

Another thing to keep in mind here is that we regularly shut down apps due to low-memory conditions. In that case, if the message hasn't been saved then we really don't have the ability to display the message to the user. So we arguably can't always dependently display messages to the user unless we also save them in the database.

Comment 29

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

(In reply to Jonas Sicking (:sicking) from comment #28)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #27)
> > (In reply to Jonas Sicking (:sicking) from comment #26)
> I guess the question here is, do you think we should do something different
> for unset message class than from message class 1?

Yes, so that we can distinguish Class-1 messages from normal ones for further SMS standard compliant. Besides, *ALL* Class-x messages handling are listed on a mandatory features sheet.
 
> If that is correct, it sounds to me like we shouldn't be exposing these
> messages through the normal SMS API.
> Do you agree?

Really, we need all these classes to pass some certificate process.

> By the way, is class 3 messages usually what's used to notify about pending
> messages in the voicemail?

Voicemail, by specification, can be Class-x messages, but this is not the usual case. Android doesn't support it, neither does Firefox OS. I think we haven't find any Voicemail message that is not a normal one(no message class definition), but I'll leave the question to Marshall. Maybe he has more experiences in Voicemail than I do.

> That bug number doesn't seem correct.

Ah, it's bug 796904 comment #19.

> I think there are four relevant questions here:
> 
> * What does certification require.

Class-0 messages shown, but not stored in database, even after reboot.

> * What does certification allow.
> * What do we think makes for a good UX. I.e. what do we think is the best
> solution for users.

Do not save it by default, but provide options for user to save it (in the future).

> * What do other modern smartphones do.

Android: Class-0 messages shown, but not stored in database, even after reboot.

> I would be interested to hear what class 0 messages usually contain?

For example, SIM card activation in Brazil ends up with a text-based Class-0 message that notifies the user whether or not the activation is success, or pending. But its function is not limited. Class-0 messages can also serve as emergency notifications. Normal messages may be dropped because of the lack of storage space, but Class-0 messages, by definition, have no such problem. Another popular use case is advertisement.

> Another thing to keep in mind here is that we regularly shut down apps due
> to low-memory conditions. In that case, if the message hasn't been saved
> then we really don't have the ability to display the message to the user. So
> we arguably can't always dependently display messages to the user unless we
> also save them in the database.

Other platforms can also have this problem, but as said in TS 23.038, if the sms application can't display that Class-0 message immediately, then system MMI might provide an options to decide what to do.

Comment 30

[Mounir Lamouri (:mounir)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #29)
> For example, SIM card activation in Brazil ends up with a text-based Class-0
> message that notifies the user whether or not the activation is success, or
> pending. But its function is not limited. Class-0 messages can also serve as
> emergency notifications. Normal messages may be dropped because of the lack
> of storage space, but Class-0 messages, by definition, have no such problem.
> Another popular use case is advertisement.

Actually, I'm afraid those Class 0 messages exist mostly because of that. Nowadays (and in the phones with Firefox OS), storing a SMS message is not a problem.

Jonas, I definitely agree with you but I've been told (in this bug or another) that the behaviour was mandatory to be certified. I think it would worth a double-check because I think this is a useless addition to the API.
An alternative if the behaviour is mandatory would be to show those messages trough chrome so we could simply save them without notifying the SMS apps.

Comment 31

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

(In reply to Mounir Lamouri (:mounir) from comment #30)
> Jonas, I definitely agree with you but I've been told (in this bug or
> another) that the behaviour was mandatory to be certified. I think it would
> worth a double-check because I think this is a useless addition to the API.

Comment 32

[Mounir Lamouri (:mounir)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #31)
> (In reply to Mounir Lamouri (:mounir) from comment #30)
> > Jonas, I definitely agree with you but I've been told (in this bug or
> > another) that the behaviour was mandatory to be certified. I think it would
> > worth a double-check because I think this is a useless addition to the API.

Vicamo, I think there is a misunderstanding here: this bug isn't useless. We should tackle that issue and fix it if needs be. I'm sad that we have to expose this information to the API but if that's required for certification, we just have to do it...

Far from me the idea to say that your work is useless. It is actually much appreciated!

Comment 33

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

Comment on attachment 674188 [details] [diff] [review]
Part 1: IDL changes

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

It really sounds like we have to support this.

Vicamo: thanks for putting up with mine and Mounir's requests and checking for what's needed for certification. I think it's unfortunate that certification requires what it does, but I 100% agree that it's not something that we can ignore.

::: dom/sms/interfaces/nsIDOMSmsMessage.idl
@@ +19,5 @@
> +  /**
> +   * Should be "normal", "class-0", "me-specific", "sim-specific" or
> +   * "te-specific".
> +   */
> +  readonly attribute DOMString messageClass;

It might be worth simply using "class-0", "class-1", "class-2" etc as values here if that is what SMS people generally use to talk about these things. But I'll leave that up to you as I don't know what is commonly used.

Comment 34

[Marshall Culpepper [:marshall_law]]

Comment on attachment 674199 [details] [diff] [review]
Part 5: test cases

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

::: dom/system/gonk/tests/test_ril_worker_sms.js
@@ +118,5 @@
>    //   Bit 4, if set to 1, indicates that bits 1 to 0 have a message class meaning.
> +  test_dcs(0x10, PDU_DCS_MSG_CODING_7BITS_ALPHABET, "class-0");
> +  test_dcs(0x11, PDU_DCS_MSG_CODING_7BITS_ALPHABET, "me-specific");
> +  test_dcs(0x12, PDU_DCS_MSG_CODING_7BITS_ALPHABET, "sim-specific");
> +  test_dcs(0x13, PDU_DCS_MSG_CODING_7BITS_ALPHABET, "te-specific");

should we make these messageClass representation strings constants as well?

Comment 35

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

(In reply to Marshall Culpepper [:marshall_law] from comment #34)
> Review of attachment 674199 [details] [diff] [review]:
> -----------------------------------------------------------------
> should we make these messageClass representation strings constants as well?

Yes, I've updated them in my local branch, but I'll only updates patches here after bug 742790. They both introduce a new field and one must be after the other.

Comment 36

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

Attachment: Part 1: IDL changes : v2

1. Remove SmsFilter.deliveryStatus. All SmsFilter related functions are moved to bug 805733. Reviewed in bug 742790 comment #80.
2. Use "class-x" for Class-X string values. Comment #24, #33

Comment 37

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

Attachment: Part 2: DOM implementation : v2

1. Remove SmsFilter.deliveryStatus. All SmsFilter related functions are moved to bug 805733. Reviewed in bug 742790 comment #80.
2. Use "class-x" for Class-X string values. Comment #24, #33

Comment 38

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

Attachment: Part 3: RIL implementation : v3

1. Remove SmsFilter.deliveryStatus. Now this patch makes no database schema changes. All SmsFilter related functions are moved to bug 805733. Reviewed in bug 742790 comment #80.
2. Use "class-x" for Class-X string values. Comment #24, #33
3. Use more descriptive expression. Comment #12

Comment 39

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

Attachment: Part 4: Android implementation : v3

1. Uses static_cast<...>. Comment #19
2. Use "class-x" for Class-X string values. Comment #24, #33
3. Remove dangling definition of kMessageClassUnknown. I've previously removed filtering support in comment #37.

Comment 40

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

Attachment: Part 5: test cases : v2

1. Use "class-x" for Class-X string values. Comment #24, #33, #34
2. Use more descriptive expression. Comment #12
3. Use "success" for received messages. Bug 742790 comment #89.

Comment 41

[Mounir Lamouri (:mounir)]

Comment on attachment 675789 [details] [diff] [review]
Part 4: Android implementation : v3

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

r=me with the comments addressed.

::: embedding/android/GeckoSmsManager.java
@@ +334,5 @@
>    private final static int kDeliveryStatusPending       = 2;
>    private final static int kDeliveryStatusError         = 3;
>  
> +  /*
> +   * Keep the following error codes in sync with |MessageClass| in:

nit: those are not error codes.

@@ +937,5 @@
>      }
>      return kDeliveryStatusSuccess;
>    }
>  
> +  private int translateAndroidMessageClassToGecko(MessageClass aMessageClass) {

What about naming the function "getGeckoMessageClass()" given that it takes Android MessageClass object in parameter, the "fromAndroid" part is implicit.

@@ +948,5 @@
> +    } else if (aMessageClass == MessageClass.CLASS_3) {
> +      return kMessageClassClass3;
> +    } else {
> +      return kMessageClassNormal;
> +    }

MessageClass is an enum, so I think it would be better to have a switch that would add:
case MessageClass.UNKNOWN:
  return kMessageClassNormal;
default:
  // error / throwing

::: mobile/android/base/GeckoSmsManager.java
@@ +930,5 @@
>      }
>      return kDeliveryStatusSuccess;
>    }
>  
> +  private int translateAndroidMessageClassToGecko(MessageClass aMessageClass) {

What about naming the function "getGeckoMessageClass()" given that it takes Android MessageClass object in parameter, the "fromAndroid" part is implicit.

@@ +941,5 @@
> +    } else if (aMessageClass == MessageClass.CLASS_3) {
> +      return kMessageClassClass3;
> +    } else {
> +      return kMessageClassNormal;
> +    }

MessageClass is an enum, so I think it would be better to have a switch that would add:
case MessageClass.UNKNOWN:
return kMessageClassNormal;
default:
// error / throwing

Comment 42

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

Attachment: Part 1: IDL changes : v3

Rebase only.

Comment 43

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

Attachment: Part 3: RIL implementation : v4

Rebase only

Comment 44

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

Attachment: Part 4: Android implementation : v4

1. Address review comment #41.
2. Rebase

Comment 45

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

Attachment: Part 5: test cases : v3

Rebase only

Comment 46

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

Attachment: Part 3: RIL implementation : v5

Rebase only

Comment 47

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

Attachment: Part 4: Android implementation : v5

append r=blassey in change summary

Comment 48

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

Attachment: Part 5: test cases : v4

append r=marshall_law in change summary

Comment 49

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f39ba5076599
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e3e62c2b3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6834d09f8f09
https://hg.mozilla.org/integration/mozilla-inbound/rev/190933b35023
https://hg.mozilla.org/integration/mozilla-inbound/rev/95752d95cf73

Comment 50

[Ed Morley [:emorley]]

Backed out for xpcshell failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=95752d95cf73

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dceaaee52cc

Comment 51

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

Attachment: Part 5: test cases : v5

fix inbound bustage: missing changes in test_message_classes.js
Try: https://tbpl.mozilla.org/?tree=Try&rev=bba7f0d6211a

Comment 52

[Mozilla RelEng Bot]

Try run for bba7f0d6211a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bba7f0d6211a
Results (out of 97 total builds):
    success: 85
    warnings: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bba7f0d6211a

Comment 53

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

Attachment: Part 5: test cases : v6

Yet more test cases

Comment 54

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

Fix xpcshell bustage and re-commit again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2cb5f94ead6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e57c5417fb2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c02178f03fa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ce2043663d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/265f424378e8

Comment 55

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e2cb5f94ead6
https://hg.mozilla.org/mozilla-central/rev/e57c5417fb2c
https://hg.mozilla.org/mozilla-central/rev/c02178f03fa4
https://hg.mozilla.org/mozilla-central/rev/9ce2043663d8
https://hg.mozilla.org/mozilla-central/rev/265f424378e8

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff5a32f762d1
https://hg.mozilla.org/releases/mozilla-aurora/rev/a92d8fb10aee
https://hg.mozilla.org/releases/mozilla-aurora/rev/35a437b3768c
https://hg.mozilla.org/releases/mozilla-aurora/rev/499c31ed1d9c
https://hg.mozilla.org/releases/mozilla-aurora/rev/bdb1113756ca