B2G SMS: expose message class info in DOM API

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

({feature})

unspecified
mozilla19
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(5 attachments, 19 obsolete attachments)

20.22 KB, patch
mounir
: review+
Details | Diff | Splinter Review
1.63 KB, patch
Details | Diff | Splinter Review
16.13 KB, patch
Details | Diff | Splinter Review
18.90 KB, patch
Details | Diff | Splinter Review
29.36 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

7 years ago
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.
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).
(Assignee)

Comment 2

7 years ago
(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.
(Assignee)

Comment 3

7 years ago
Posted patch Part 1: IDL changes : WIP (obsolete) — Splinter Review
(Assignee)

Comment 4

7 years ago
(Assignee)

Updated

7 years ago
Assignee: nobody → vyang
(Assignee)

Comment 5

7 years ago
(Assignee)

Updated

7 years ago
Keywords: testcase-wanted
(Assignee)

Updated

7 years ago
Keywords: feature
(Assignee)

Updated

7 years ago
Blocks: 804476
(Assignee)

Comment 6

7 years ago
There seems to be no other solution than this for bug 796904. Request for bb+ flag since bug 796904 is already bb+.
blocking-basecamp: --- → ?
(Assignee)

Comment 7

7 years ago
Posted patch Part 1: IDL changes (obsolete) — Splinter Review
1. messageClass as DOMString
2. renew interface UUID as well
Attachment #670573 - Attachment is obsolete: true
Attachment #674188 - Flags: superreview?(jonas)
(Assignee)

Comment 8

7 years ago
Posted patch Part 2: DOM implementation (obsolete) — Splinter Review
1. messageClass as DOMString
2. rebase to bug 742970
Attachment #670574 - Attachment is obsolete: true
Attachment #674192 - Flags: review?(mounir)
(Assignee)

Comment 9

7 years ago
Posted patch Part 3: RIL implementation (obsolete) — Splinter Review
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"
Attachment #670591 - Attachment is obsolete: true
Attachment #674195 - Flags: review?(philipp)
(Assignee)

Comment 10

7 years ago
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.
Attachment #674196 - Flags: review?(mounir)
Attachment #674196 - Flags: review?(blassey.bugs)
(Assignee)

Comment 11

7 years ago
Posted patch Part 5: test cases (obsolete) — Splinter Review
update xpcshell & marionette tests
Attachment #674199 - Flags: review?(marshall)
(Assignee)

Updated

7 years ago
Keywords: testcase-wanted
blocking-basecamp: ? → +
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.
Attachment #674195 - Flags: review?(philipp) → review+
(Assignee)

Comment 13

7 years ago
Address review comment #12. Thanks Philipp!
Attachment #674195 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
Rebase only. Original patch conflicts with V2 patch uploaded in bug 720325.
Attachment #674196 - Attachment is obsolete: true
Attachment #674196 - Flags: review?(mounir)
Attachment #674196 - Flags: review?(blassey.bugs)
Attachment #674571 - Flags: review?(mounir)
Attachment #674571 - Flags: review?(blassey.bugs)
(Assignee)

Updated

7 years ago
Attachment #674570 - Attachment description: Part 3: RIL implementation → Part 3: RIL implementation : v2
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.
Attachment #674192 - Flags: review?(mounir) → review+
(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.
(Assignee)

Comment 17

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

Oops, the original patch title is wrong!
Attachment #674571 - Attachment description: Part 3: B2G RIL implementation : v2 → Part 4: Android implementation: v2
(Assignee)

Comment 18

7 years ago
(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 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)
Attachment #674571 - Attachment description: Part 4: Android implementation: v2 → Part 3: B2G RIL implementation : v2
Attachment #674571 - Flags: review?(mounir) → review+
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...
(Assignee)

Comment 21

7 years ago
(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.
(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.?
(Assignee)

Comment 23

7 years ago
(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.
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.
(Assignee)

Comment 25

7 years ago
(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?
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.
(Assignee)

Comment 27

7 years ago
(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
(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.
(Assignee)

Comment 29

7 years ago
(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.
(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.
(Assignee)

Comment 31

7 years ago
(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.
Assignee: vyang → nobody
(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 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.
Attachment #674188 - Flags: superreview?(jonas) → superreview+
(Assignee)

Updated

7 years ago
Blocks: 805733
(Assignee)

Updated

7 years ago
Attachment #674571 - Attachment description: Part 3: B2G RIL implementation : v2 → Part 3: Android implementation : v2
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?
Attachment #674199 - Flags: review?(marshall) → review+
(Assignee)

Comment 35

7 years ago
(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.
(Assignee)

Comment 36

7 years ago
Posted patch Part 1: IDL changes : v2 (obsolete) — Splinter Review
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
Attachment #674188 - Attachment is obsolete: true
(Assignee)

Comment 37

7 years ago
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
Attachment #674192 - Attachment is obsolete: true
(Assignee)

Comment 38

7 years ago
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
Attachment #674570 - Attachment is obsolete: true
(Assignee)

Comment 39

7 years ago
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.
Attachment #674571 - Attachment is obsolete: true
Attachment #674571 - Flags: review?(blassey.bugs)
Attachment #675789 - Flags: review?(blassey.bugs)
(Assignee)

Comment 40

7 years ago
Posted patch Part 5: test cases : v2 (obsolete) — Splinter Review
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.
Attachment #674199 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Assignee: nobody → vyang
Attachment #675786 - Flags: review+
Attachment #675787 - Flags: review+
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
Attachment #675789 - Flags: review+
(Assignee)

Comment 42

7 years ago
Rebase only.
Attachment #675786 - Attachment is obsolete: true
(Assignee)

Comment 43

7 years ago
Rebase only
Attachment #675788 - Attachment is obsolete: true
(Assignee)

Comment 44

7 years ago
1. Address review comment #41.
2. Rebase
Attachment #675789 - Attachment is obsolete: true
Attachment #675789 - Flags: review?(blassey.bugs)
(Assignee)

Updated

7 years ago
Attachment #676072 - Flags: review?(blassey.bugs)
(Assignee)

Comment 45

7 years ago
Posted patch Part 5: test cases : v3 (obsolete) — Splinter Review
Rebase only
Attachment #675791 - Attachment is obsolete: true
Attachment #676072 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 46

7 years ago
Rebase only
Attachment #676071 - Attachment is obsolete: true
(Assignee)

Comment 47

7 years ago
append r=blassey in change summary
Attachment #676072 - Attachment is obsolete: true
(Assignee)

Comment 48

7 years ago
Posted patch Part 5: test cases : v4 (obsolete) — Splinter Review
append r=marshall_law in change summary
Attachment #676073 - Attachment is obsolete: true
(Assignee)

Comment 51

7 years ago
Posted patch Part 5: test cases : v5 (obsolete) — Splinter Review
fix inbound bustage: missing changes in test_message_classes.js
Try: https://tbpl.mozilla.org/?tree=Try&rev=bba7f0d6211a
Attachment #676501 - Attachment is obsolete: true
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/vyang@mozilla.com-bba7f0d6211a
(Assignee)

Comment 53

7 years ago
Yet more test cases
Attachment #676563 - Attachment is obsolete: true
Depends on: 807463
You need to log in before you can comment on or make changes to this bug.