Hi guys, I tried to send a SMS message on to number "0". On iPhone the sending message would stay for ~20 sec and then show a failed indication. But on Gaia the callback of mozSms.send() never gets called, so I cannot trigger failed UI to user. I suspect there is a timeout mechanism on iPhone. But anyhow we need that callback :P thanks.

If I understand Vicamo correctly in bug 727319 comment 21, we need to notify delivery failures for this to work.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #0) > Hi guys, > > I tried to send a SMS message on to number "0". On iPhone the sending > message would stay for ~20 sec and then show a failed indication. But on > Gaia the callback of mozSms.send() never gets called, so I cannot trigger > failed UI to user. > > I suspect there is a timeout mechanism on iPhone. But anyhow we need that > callback :P thanks. Hi, As mentioned by Philipp's comment, error notification defined in SMS specification divided into two levels. One is "sms-send-failed", which is finally implemented in gonk backend in bug 727319. This is for errors generated by the messaging entity. Possible errors could be radio-no-available, sim-not-ready, retry, etc.[1] Another is "sms-deliver-failed", which is still not available in current code base. This is for errors generated by SMSC(SMS center). Possible errors could be no-interworking-available(invalid number), validity-period-expired, service-rejected, etc.[2] The "sms-send-failed" error should be returned immediately, but "sms-deliver-failed"(or success) can take seconds to minutes even hours, depends on when does the receiving entity get that message or when does SMSC give up transmitting it. As for timeout mechanism, I don't see anything alike in the SMS spec. Besides, there is already a timeout counter in SMSC. There might be also some more complicated scenarios when device power-off is involved. Note 1: these errors can be found in http://hg.mozilla.org/mozilla-central/file/da0d07b5ca1e/dom/system/gonk/ril_consts.js#l217 . Note 1: these errors can be found in bug 736697 attachment 610446 [details] [diff] [review] (not yet merged to m-c), "ST - Status"

Remove blocking flag for b2g-sms for milestone 3 temporarily.

I just tried sending sms to an invalid number, and we will reach 'mozSms.send().onerror'. Also, 'invalid number' falls into 'sms-send-failed' error.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4) > I just tried sending sms to an invalid number, and we will reach > 'mozSms.send().onerror'. > > Also, 'invalid number' falls into 'sms-send-failed' error. That's great news! Instead of resolving this as RESOLVED/WORKSFORME, I'm going to morph this bug into writing tests for these cases.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4) > I just tried sending sms to an invalid number, and we will reach > 'mozSms.send().onerror'. > > Also, 'invalid number' falls into 'sms-send-failed' error. Some test cases and known/should behavior: [Send to "0"] Got: sms-send-failed Expect: any error will do [Send to "*147#"] Got: sms-send-failed Expect: any error will do [Send to "0999999999"] Got: sms-sent, UNSOLICITED_RESPONSE_NEW_SMS_STATUS_REPORT with error PDU_ST_2_INTERWORKING_UNAVALIABLE(0x45) but dropped without notifying sending app. Expect: sms-sent, sms-delivery-failed So, no, this issue is not yet solved. I'll rename it back.

Hi, this is a unfinished patch demonstrating what should we do in ril_worker on SMS delivery failures. The major reason that this patch remains unfinished is the lack of proper API proposal to user app. In SmsManager, we provide only positive meaning events, "onreceived", "onsent" and "ondelivered", for SMS transactions. There is no room for additional status/errorCode parameter. Besides, the SmsRequestManager destroy a request on either sms-sent or sms-send-failed. Therefore, the delivery status will never reach user app via SmsRequest. I suggest we should address this problem in new MMS API proposal and fix it later at merging SMS/MMS APIs.

The use case explained in comment 0 is handled by the API this way: var r = navigator.sms.send('0', 'test'); r.onsuccess = function() { alert('success'); } r.onerror = function() { alert('error'); }; Sending an SMS to '0' should fire an error when the send() method is called. At least, this is what the Android implementation is doing and this is what should be done. The delivery event shouldn't be involved here.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8) > Sending an SMS to '0' should fire an error when the send() method is called. > At least, this is what the Android implementation is doing and this is what > should be done. That's already what we had. > The delivery event shouldn't be involved here. Do you suggest we should still morph this bug into writing tests and open another issue for delivery-failure handling?

(In reply to Vicamo Yang [:vicamo] from comment #9) > (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8) > > Sending an SMS to '0' should fire an error when the send() method is called. > > At least, this is what the Android implementation is doing and this is what > > should be done. > > That's already what we had. Oh, my bad, I misread that. > > The delivery event shouldn't be involved here. > > Do you suggest we should still morph this bug into writing tests and open > another issue for delivery-failure handling? There should be no delivery-failure. The delivery event is only for when the message has been actually received by the recipient and the event might never be sent if the recipient doesn't send a ack for reception. The cases pointed in comment 6 should all send a send-failed, IMO.

When sending a SMS to 0999999999. iPhone would fail. iPhone does not give out information on UI to differentiate delivery fail or sent fail.

Nominate for blocking decision. Note: number stated in STR ("0") now generates fail on Otoro with a Chungwha SIM; but with number 0999999999 this bug exists still.

(In reply to Mounir Lamouri (:mounir) from comment #10) > The cases pointed in comment 6 should all send a send-failed, IMO. SMS-STATUS-REPORT is not always sent be SMSC, that's an optional. Given that's not an optional, SR could be sent minutes or even hours after the message. Besides, `sms-sent` event is always emitted immediately because it depends only on the response of REQUEST_SEND_SMS from local modem firmware. So, if you insist the third case in comment #6 should also send a `sms-sent-failed`, you may have any of following results depends on the implementation: 1. you never get 'sms-sent' or 'sms-send-failed' event at all for you never receive the omitted SR 2. you get 'sms-sent' or 'sms-send-failed' a thousand years after you sent the message 3. you get 'sms-sent' first and then 'sms-send-failed'

@mounir and @sicking, since this issue is marked as blocking-basecamp, could you help deciding what change should be made to solve it? I have a few solutions like: Method 1) 1. rename `SmsManager.ondelivered` to `ondeliver` 2. create a new interface SmsDeliverEvent, which contains a SmsMessage-typed `message` and an integer-typed `status` attributes. Method 2) 1. create an additional event `SmsManager.ondeliverfailed` 2. create a new interface SmsDeliverEvent, which contains a SmsMessage-typed `message` and an integer-typed `status` attributes. Method 3) 1. Delivery failure is sent to `SmsRequest.onerror` so we'll have to pending the original sent success event and wait for the arrival of a could-be-omitted SMS-STATUS-REPORT PDU. Please also see my comment #13 for possible consequences. Method 4) Maybe you'll have better solution than above ones.

I might be misunderstanding what we are talking about here. If I am just let me know and most of what I say probably will be wrong :) Isn't "delivery" when the receiving user receives the textmessage? That can happen literally days after a message is sent if the receiver doesn't have his device on. Delivery can fail for a number of reasons, but this failure can happen days after the message was sent. There's the unrelated error condition of something going wrong when sending a message. I.e. the SMSC not accepting the message. This can be the phone number not being valid, the user not having paid for the ability to send (more) textmessages, radio error, the network being overloaded etc. These happen generally within a few seconds of starting to attempt sending the message. We can certainly use the same UI for these types of errors. But I think it makes sense to surface the difference in the API. If for no other reason than that the app likely will have to take different actions for an error happening days later vs. one that happens immediately. When sending an SMS message we return a SmsRequest. For the second category of errors we should simply fire an "error" event on that request. For the first category we should have some sort of "deliveryfailure" event or system message which indicates which message was failed to be delivered. Similar to how we have an event/system message which indicates when an SMS message has been marked read.

(In reply to Jonas Sicking (:sicking) from comment #15) > When sending an SMS message we return a SmsRequest. For the second category > of errors we should simply fire an "error" event on that request. That is already the behaviour the API requires. > For the first category we should have some sort of "deliveryfailure" event > or system message which indicates which message was failed to be delivered. > Similar to how we have an event/system message which indicates when an SMS > message has been marked read. I agree.

Is there agreement on what behavior we want here? If so, what works is remaining to make us implement that behavior?

(In reply to Jonas Sicking (:sicking) from comment #17) > Is there agreement on what behavior we want here? If so, what works is > remaining to make us implement that behavior? That's a long long comment, and did you actually mean "we should create an new event, named deliveryfailure, in SmsManager"? Like method 2 in comment #14?

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18) > (In reply to Jonas Sicking (:sicking) from comment #17) > > Is there agreement on what behavior we want here? If so, what works is > > remaining to make us implement that behavior? > > That's a long long comment, and did you actually mean "we should create an > new event, named deliveryfailure, in SmsManager"? Yes. > Like method 2 in comment #14? Yes, though I don't think we need to add a integer-type "status" on the event. We should instead add a .deliveryStatus on SMSMessage which is either "pending", "success" or "error", or something like that.

(In reply to Jonas Sicking (:sicking) from comment #19) > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #18) > > (In reply to Jonas Sicking (:sicking) from comment #17) > > Like method 2 in comment #14? > > Yes, though I don't think we need to add a integer-type "status" on the > event. We should instead add a .deliveryStatus on SMSMessage which is either > "pending", "success" or "error", or something like that. Do we still have to create a new deliveryfailure event if we move delivery status into each message object? Can we share the existing ondelivered event? Users can still distinguish successful deliveries from failed ones via the attribute.

Does the existing ondelivered message use nsIDOMMozSmsEvent? If so, reusing that sounds great!

(In reply to Jonas Sicking (:sicking) from comment #21) > Does the existing ondelivered message use nsIDOMMozSmsEvent? If so, reusing > that sounds great! Nice! I'll update patches tonight or tomorrow. Thank you :)

I do not think it is a good idea to send a "delivered" event when the delivery failed. "Delivered" means something that is opposite to a failure in the delivery process. We should have a new event or rename the current one to something more generic. Also, Vicamo, could you update the bug summary to what is actually going to be done in this bug and make it clear that it is a feature more than a bug fix.

Oh, sorry, i misunderstood. I don't think we should fire a "delivered" event here. I agree with Mounir that we should either fire a "deliveryerror" event (or some such), or rename "delivered" to something like "deliverystatuschange".

I can't run mochitest for now, something wrong. Here is try link: https://tbpl.mozilla.org/?tree=Try&rev=bef456f7cfda

Comment on attachment 671850 [details] [diff] [review] Part 2: dom implementation Review of attachment 671850 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/src/android/SmsDatabaseService.cpp @@ +42,5 @@ > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > + int32_t aDeliveryStatus) > +{ > + // FIXME > + return NS_OK; We don't need this for Android. I should change "FIXME" to other more descriptive comments here.

fix comments only

Comment on attachment 671848 [details] [diff] [review] Part 1: IDL changes Review of attachment 671848 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments applied. ::: dom/sms/interfaces/nsIDOMSmsFilter.idl @@ +24,5 @@ > attribute DOMString delivery; > > + // A DOMString that can return and be set to "success", "pending", "error" or null. > + [Null(Empty)] > + attribute DOMString deliveryStatus; Could that be [infallible] ? ::: dom/sms/interfaces/nsIDOMSmsManager.idl @@ +29,5 @@ > nsIDOMMozSmsRequest markMessageRead(in long id, in boolean aValue); > > [implicit_jscontext] attribute jsval onreceived; > [implicit_jscontext] attribute jsval onsent; > + [implicit_jscontext] attribute jsval ondeliverystatuschange; That could be [infallible] but maybe that could be done in a bug about making all this interface infallible. If appropriate. ::: dom/sms/interfaces/nsIDOMSmsMessage.idl @@ +10,5 @@ > // TODO: we should add SENT and RECEIVED DOMString constants, see bug 443316. > > readonly attribute long id; > readonly attribute DOMString delivery; // Should be "sent" or "received". > + readonly attribute DOMString deliveryStatus; // Should be "pending", "success" or "error". and that

Comment on attachment 671848 [details] [diff] [review] Part 1: IDL changes Review of attachment 671848 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/interfaces/nsIDOMSmsFilter.idl @@ +24,5 @@ > attribute DOMString delivery; > > + // A DOMString that can return and be set to "success", "pending", "error" or null. > + [Null(Empty)] > + attribute DOMString deliveryStatus; I realize setting can clearly fail here.

Comment on attachment 671848 [details] [diff] [review] Part 1: IDL changes Review of attachment 671848 [details] [diff] [review]: ----------------------------------------------------------------- Forgot to say that you should update uuid when you update an idl. ::: dom/sms/interfaces/nsIDOMSmsMessage.idl @@ +10,5 @@ > // TODO: we should add SENT and RECEIVED DOMString constants, see bug 443316. > > readonly attribute long id; > readonly attribute DOMString delivery; // Should be "sent" or "received". > + readonly attribute DOMString deliveryStatus; // Should be "pending", "success" or "error". Forget about that one.. The fact that we have to add a end guard completely breaks the type safety of enums. Let's keep it fallible for the moment.

Comment on attachment 671848 [details] [diff] [review] Part 1: IDL changes Review of attachment 671848 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/interfaces/nsIDOMSmsManager.idl @@ +29,5 @@ > nsIDOMMozSmsRequest markMessageRead(in long id, in boolean aValue); > > [implicit_jscontext] attribute jsval onreceived; > [implicit_jscontext] attribute jsval onsent; > + [implicit_jscontext] attribute jsval ondeliverystatuschange; Actually, I completely forgot that the 'delivered' event was sent on navigator.mozSms. I think 'deliverystatuschange' doesn't make sense then. I would prefer 'delivered' and 'deliveryfailed' events. Jonas, what do you think?

Comment on attachment 671853 [details] [diff] [review] Part 4: Android implementation Review of attachment 671853 [details] [diff] [review]: ----------------------------------------------------------------- r-, because of the int -> string -> int dance. Will need to look at the fixed patch. Out of curiosity, did you try that patch? ::: embedding/android/GeckoSmsManager.java @@ +928,5 @@ > MessagesListManager.getInstance().clear(); > } > + > + private String translateSmsStatusToString(int deliveryStatus) { > + if (status == kSmsDeliveryStatusNone) { I do not understand why you are trying to translate that int to a string before sending it to the C++ code then translating it to an int there. Can't you just pass the int and have some #define in the C++ code that would convert the received int to the corresponding value in our code? ::: widget/android/AndroidJNI.cpp @@ +62,5 @@ > + if (deliveryStatus == DELIVERY_STATUS_ERROR) { > + return eDeliveryStatus_Error; > + } > + > + NS_ERROR("We should not be here!"); MOZ_NOT_REACHED();

Comment on attachment 671858 [details] [diff] [review] Part 2: dom implementation : v2 Review of attachment 671858 [details] [diff] [review]: ----------------------------------------------------------------- You should update the uuid of an interface when you change it. I remember something I didn't like with having the delivery status in the API. If a phone doesn't send back delivery status (or is it the network doing that?) or if you do not ask for delivery status, the property will be set to "pending" forever. This said, our implementation always ask for delivery status for the moment. Also, what value should we have if the message has been received by the user. DeliveryStatus doesn't make any sense if the user didn't sent the message. I see that Vicamo decided to use "DELIVERY_SUCCESS" in the B2G code. I wonder if a default value shouldn't be handled by the DOM API instead. r=me but please make sure that all comments here are addressed/discussed. ::: dom/sms/src/SmsFilter.cpp @@ +245,5 @@ > + case eDeliveryStatus_Error: > + aDeliveryStatus = DELIVERY_STATUS_ERROR; > + break; > + default: > + NS_ASSERTION(false, "We shouldn't get another delivery status!"); MOZ_NOT_REACHED("We shouldn't get another delivery status!"); ::: dom/sms/src/SmsMessage.cpp @@ +31,5 @@ > + const nsString& aReceiver, > + const nsString& aBody, > + uint64_t aTimestamp, > + bool aRead) > + : mData(aId, aDelivery, aDeliveryStatus, aSender, aReceiver, aBody, aTimestamp, aRead) nit: I'm pretty sure you are beating the 80 chars limit here. @@ +153,5 @@ > + break; > + case eDeliveryStatus_Unknown: > + case eDeliveryStatus_EndGuard: > + default: > + NS_ASSERTION(true, "We shouldn't get any other delivery status!"); MOZ_NOT_REACHED("We shouldn't get any other delivery status!"); ::: dom/sms/src/android/SmsDatabaseService.cpp @@ +42,5 @@ > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > + int32_t aDeliveryStatus) > +{ > + // We don't need this in Android. > + return NS_OK; Why? It's not implemented yet but I do not think we don't want that at all.

(In reply to Mounir Lamouri (:mounir) from comment #36) > Comment on attachment 671853 [details] [diff] [review] > Part 4: Android implementation > ----------------------------------------------------------------- > r-, because of the int -> string -> int dance. Will need to look at the > fixed patch. That's because the `deliveryStatus` attribute is string-typed, so SmsMessage ctor accepts a string-typed aDeliveryStatus only. Or, we'll have to create another override ctor. I think I can fix this by change JNI interface to jint, rather than jstring. Thank you. > Out of curiosity, did you try that patch? I had applied these patches to my local Fennec devel tree and had build pass. Besides, I pushed them to try sever (https://tbpl.mozilla.org/?tree=Try&rev=bef456f7cfda) and it passed all tests. If SMS on Fennec is not enabled/tested by default, then I'll probably need more information to verify.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #38) > > Out of curiosity, did you try that patch? > > I had applied these patches to my local Fennec devel tree and had build > pass. Besides, I pushed them to try sever > (https://tbpl.mozilla.org/?tree=Try&rev=bef456f7cfda) and it passed all > tests. If SMS on Fennec is not enabled/tested by default, then I'll probably > need more information to verify. It is disabled by default. You have to add --enable-websms-backend in your mozconfig to enable it. I wouldn't be surprise if the WebSMS backend of Firefox Android doesn't even compile nowadays.

(In reply to Mounir Lamouri (:mounir) from comment #37) > Comment on attachment 671858 [details] [diff] [review] > Part 2: dom implementation : v2 > ----------------------------------------------------------------- > I remember something I didn't like with having the delivery status in the > API. If a phone doesn't send back delivery status (or is it the network > doing that?) or if you do not ask for delivery status, the property will be > set to "pending" forever. This said, our implementation always ask for > delivery status for the moment. Actually, since we don't have an interface or a Settings entry for requesting SMS-STATUS-REPORT or not, we do always request it in RadioInterfaceLayer.sendSMS(). Android has a special value NONE for messages without SR requested. > Also, what value should we have if the message has been received by the > user. DeliveryStatus doesn't make any sense if the user didn't sent the > message. I see that Vicamo decided to use "DELIVERY_SUCCESS" in the B2G > code. I wonder if a default value shouldn't be handled by the DOM API > instead. The user can't overwrite a message, at least for now. It follows that no matter what value is chosen as 'default', it just can't be overwritten. So, whether that default value is set by DOM API or not is not very meaningful. The only thing that matters is how it behaves when a SmsFilter with its deliveryStatus attribute set to some value. Should that filter match received messages as well when it's set to SUCCESS? > ::: dom/sms/src/android/SmsDatabaseService.cpp > @@ +42,5 @@ > > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > > + int32_t aDeliveryStatus) > > +{ > > + // We don't need this in Android. > > + return NS_OK; > > Why? > It's not implemented yet but I do not think we don't want that at all. You're probably right. I thought that will be handled by Android's SMS app. Need more trace.

I don't think we should worry about keeping the Android SMS API implementation up-to-date with the changes happening here. I have a feeling we'll have to make a lot of updates to it once we decide that we want to ship it.

(In reply to Jonas Sicking (:sicking) from comment #41) > I don't think we should worry about keeping the Android SMS API > implementation up-to-date with the changes happening here. I have a feeling > we'll have to make a lot of updates to it once we decide that we want to > ship it. Just want to add we still have build pass even with --enable-websms-backend.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #40) > > Also, what value should we have if the message has been received by the > > user. DeliveryStatus doesn't make any sense if the user didn't sent the > > message. I see that Vicamo decided to use "DELIVERY_SUCCESS" in the B2G > > code. I wonder if a default value shouldn't be handled by the DOM API > > instead. > > The user can't overwrite a message, at least for now. It follows that no > matter what value is chosen as 'default', it just can't be overwritten. So, > whether that default value is set by DOM API or not is not very meaningful. It is because that means all implementation of that API will be consistent. If we allow backends to decide the default value, the Android backend might end up with another default value more easily. > The only thing that matters is how it behaves when a SmsFilter with its > deliveryStatus attribute set to some value. Should that filter match > received messages as well when it's set to SUCCESS? More I think of it more I believe deliveryStatus needs an empty string value that means "not applicable". That could be used when a SMS is sent without request for delivery status and would be used for received SMS. What do you think of that? Feel free to just write another patch on top of that one if you think we should go that way. > > ::: dom/sms/src/android/SmsDatabaseService.cpp > > @@ +42,5 @@ > > > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > > > + int32_t aDeliveryStatus) > > > +{ > > > + // We don't need this in Android. > > > + return NS_OK; > > > > Why? > > It's not implemented yet but I do not think we don't want that at all. > > You're probably right. I thought that will be handled by Android's SMS app. > Need more trace. The delivery status is actually likely set by the Android SMS app indeed. So you might be right actually. However, do we get that status when we load a message? You should probably just ignore Android. Could you open a follow-up for Android and attach the patch that is for the Android backend in it? Make that bug blocking WebSMS and mention that bug number in the comment of SmsDatabaseService::SetMessageDeliveryStatus like: // TODO: might not be needed for Android but will need to be investigating whilst implementing the feature, see bug <BugNumber>.

(In reply to Mounir Lamouri (:mounir) from comment #43) > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #40) > > The only thing that matters is how it behaves when a SmsFilter with its > > deliveryStatus attribute set to some value. Should that filter match > > received messages as well when it's set to SUCCESS? > > More I think of it more I believe deliveryStatus needs an empty string value > that means "not applicable". That could be used when a SMS is sent without > request for delivery status and would be used for received SMS. > What do you think of that? Feel free to just write another patch on top of > that one if you think we should go that way. That's ok. I'll include it in next round. > > > ::: dom/sms/src/android/SmsDatabaseService.cpp > > > @@ +42,5 @@ > > > > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > > > > + int32_t aDeliveryStatus) > > > > +{ > > > > + // We don't need this in Android. > > > > + return NS_OK; > > > > > > Why? > > > It's not implemented yet but I do not think we don't want that at all. > > > > You're probably right. I thought that will be handled by Android's SMS app. > > Need more trace. > > The delivery status is actually likely set by the Android SMS app indeed. So > you might be right actually. However, do we get that status when we load a > message? Android stores the delivery status as yet another integer-typed database field named 'status'. So I can just fetch it and translate it to an appropriate value. > You should probably just ignore Android. Could you open a follow-up for > Android and attach the patch that is for the Android backend in it? Make > that bug blocking WebSMS and mention that bug number in the comment of > SmsDatabaseService::SetMessageDeliveryStatus like: > // TODO: might not be needed for Android but will need to be investigating > whilst implementing the feature, see bug <BugNumber>. I'll check if I can fix it first. If that takes too much effort, I'll do as you said.

(In reply to Mounir Lamouri (:mounir) from comment #35) > Actually, I completely forgot that the 'delivered' event was sent on > navigator.mozSms. I think 'deliverystatuschange' doesn't make sense then. I > would prefer 'delivered' and 'deliveryfailed' events. > > Jonas, what do you think? I'm fine either way. In many cases people will probably do about the same thing in both event handlers ("update delivery indicator UI for the message"). But the actions are just as likely to be different since the UI indications are probably different and for errors the app might want to immediately notify the user. Maybe using separate events is better since it's easy enough to listen to both with the same event handler.

Comment on attachment 671852 [details] [diff] [review] Part 3: B2G RIL implementation Review of attachment 671852 [details] [diff] [review]: ----------------------------------------------------------------- Changes to RadioInterfaceLayer.js / ril_worker.js look good, but I'm not familiar with the SmsDatabaseService. Would you mind requesting another review for that part?

(In reply to Marshall Culpepper [:marshall_law] from comment #46) > Comment on attachment 671852 [details] [diff] [review] > Part 3: B2G RIL implementation > > Review of attachment 671852 [details] [diff] [review]: > ----------------------------------------------------------------- > > Changes to RadioInterfaceLayer.js / ril_worker.js look good, but I'm not > familiar with the SmsDatabaseService. Would you mind requesting another > review for that part? Of course not :) But since Jonas & Mounir agreed in separating ondelivered & ondeliveryfailed events, I'll have to update these patches again, and well certainly need your review then. Thanks in advance.

Comment on attachment 671852 [details] [diff] [review] Part 3: B2G RIL implementation Review of attachment 671852 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, too. Just some comments regarding cleanliness below. Thank you, Vicamo! ::: dom/sms/src/ril/SmsDatabaseService.js @@ +425,5 @@ > + let putRequest = store.put(message); > + putRequest.onsuccess = function onsuccess(event) { > + if (DEBUG) { > + debug("Update successfully completed. Message: " + > + JSON.stringify(event.target.result)); The success handler for this request is unnecessary and only serves debugging purposes. But even if DEBUG is false we will still execute another function unnecessarily. Let's remove it. @@ +432,5 @@ > + }; > + > + txn.onerror = function onerror(event) { > + if (DEBUG) debug("Caught error on transaction ", event.target.errorCode); > + }; You don't need this, newTxn() already adds an event handler like this. Please remove. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +1208,5 @@ > delete this._sentSmsEnvelopes[message.envelopeId]; > > + let deliveryStatus = ((message.deliveryStatus >>> 5) == 0x00) > + ? DOM_SMS_DELIVERY_STATUS_SUCCESS > + : DOM_SMS_DELIVERY_STATUS_ERROR; This feels like logic that should live in ril_worker. We normally do all bit twiddling and conversion there.

Comment on attachment 671848 [details] [diff] [review] Part 1: IDL changes Review of attachment 671848 [details] [diff] [review]: ----------------------------------------------------------------- Like I said earlier, I think I'd prefer to have separate events for deliverysuccess and deliveryerror. But I can be talked out of it if someone feels strongly. sr=me with that fixed.

1. Rename ondelivered to ondeliverysuccess and add ondeliveryerror. Addressed comment #35, 45, 49 2. Update uuid. Addressed comment #37

1. Rename ondelivered to ondeliverysuccess and add ondeliveryerror. Addressed comment #35, #45, #49 2. Update uuid. Addressed comment #37 3. Add an extra parameter to nsISmsDatabaseService.saveSentMessage() to allow saving sent messages with two different delivery status: pending and non-applicable. The same reason for changes to DeliveryStatus enum, DOM value parsing in SmsMessage & SmsFilter, NotifyDelivery{Success,Error}Message in PSms IPC protocol. Addressed comment #37, #43 4. Correct parameter type of aDeliveryStatus in nsISmsDatabaseService.setMessageDeliveryStatus() to DOMString. 5. NS_ASSERTION => MOZ_NOT_REACHED. Addressed comment #37 6. Wrap long lone lines. Addressed comment #37 7. Add TODO reference in SmsDatabaseService::SetMessageDeliveryStatus() of android backend. Addressed comment #37, #43. 8. Share code between SmsChild::RecvNotify{Received,Sent,DeliverySuccess,DeliveryError}Message functions. Mounir, I still need your review because this patch contains changes other than your review comments. Besides, the value of DELIVERY_STATUS_NO_APPLICABLE is an empty string, do you feel like it in this way? I'm not quite sure because it may confuse users when it comes to resetting that attribute to unknown in a SmsFilter. Existing test cases treat empty string and null as undefined filter rules. How about change it to "non-applicable" or the like?

1. Modify observer topics for ondeliverysuccess and ondeliveryerror. Addressed comment #35, #45, #49 2. Add an extra parameter to nsISmsDatabaseService.saveSentMessage() to allow saving sent messages with two different delivery status: pending and non-applicable. Addressed comment #37, #43. 3. Remove redundant debug code. Addressed comment #48. 4. Move status conversion into ril_worker. Addressed comment #48.

1. Add deliveryStatus checks for test_getmessage.js as well. 2. Warp long lines in. Addressed comment #37.

1. Add an extra parameter to nsISmsDatabaseService.saveSentMessage() and notifySmsSent() to allow saving sent messages with two different delivery status: pending and non-applicable. Addressed comment #37, #43. 2. Respect SmsFilter.deliveryStatus in nsISmsDatabaseService.createMessageList() as well 3. Share inline exception classes and add serialVersionUID definitions to fix -Werror 4. Fix jint <--> jstring dancing. Addressed comment #36.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #44) > (In reply to Mounir Lamouri (:mounir) from comment #43) > > You should probably just ignore Android. Could you open a follow-up for > > Android and attach the patch that is for the Android backend in it? Make > > that bug blocking WebSMS and mention that bug number in the comment of > > SmsDatabaseService::SetMessageDeliveryStatus like: > > // TODO: might not be needed for Android but will need to be investigating > > whilst implementing the feature, see bug <BugNumber>. > > I'll check if I can fix it first. If that takes too much effort, I'll do as > you said. Bug 803828 created to address this issue.

I've also tried to update Gaia, but there is no any hook to the original ondelivered event at all?

remove useless header file inclusion in android/SmsDatabaseService.cpp

fix typo and verified again on Otoro

Fix an exception in handleSmsDelivery. Verified on Otoro with additional ondelivery{success,error} event listener.

Comment on attachment 673635 [details] [diff] [review] Part 2: dom implementation : v4 Review of attachment 673635 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Vicamo, the patch looks good. However, I think we could improve the internal API by putting |aDeliveryStatus| outside of |saveSentMessage|. I would like to re-review the patch with this change (and the few minor fixes mentioned below). ::: dom/sms/interfaces/nsISmsDatabaseService.idl @@ +18,5 @@ > { > // Takes some information required to save the message and returns its id. > long saveReceivedMessage(in DOMString aSender, in DOMString aBody, in unsigned long long aDate); > // Takes some information required to save the message and returns its id. > + long saveSentMessage(in DOMString aDeliveryStatus, in DOMString aReceiver, in DOMString aBody, in unsigned long long aDate); I do not think we should get |aDeliveryStatus| here if we want to set always the same delivery status to sent message. It shouldn't be up to the callers to set the status. It should always be sent to 'pending', right? ::: dom/sms/src/SmsFilter.cpp @@ +260,5 @@ > +SmsFilter::SetDeliveryStatus(const nsAString& aDeliveryStatus) > +{ > + if (aDeliveryStatus.IsEmpty()) { > + mData.deliveryStatus() = eDeliveryStatus_Unknown; > + return NS_OK; I do not think this is working as expected: aDeliveryStatus.IsEmpty() is equivalent to aDeliveryStatus.Equals(DELIVERY_STATUS_NOT_APPLICABLE) which means we will never set deliveryStatus() to eDeliveryStatus_NotApplicable. ::: dom/sms/src/android/SmsDatabaseService.cpp @@ +43,5 @@ > NS_IMETHODIMP > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > + const nsAString& aDeliveryStatus) > +{ > + // TODO: Bug 803828: update delivery status for sent messages in Android. I do not think the comment is correct here. We should call this method only to set aDeliveryStatus to 'success' or 'error'. Any other value should be managed automatically by the backend or the DOM code. Actually, aDeliveryStatus should be checked and the current message status has to be 'pending'. Ideally, those check should happen in a commen (to all backends) place but I doubt this is easily doable. At least, put a comment in the IDL saying that |setMessageDeliveryStatus| should be used that way. ::: dom/sms/src/ipc/SmsChild.cpp @@ +15,5 @@ > +namespace { > + > +using mozilla::dom::sms::SmsMessageData; > +using mozilla::dom::sms::SmsMessage; > +using mozilla::services::GetObserverService; Change this to: using mozilla; using mozilla::dom::sms; (it will require you to do services::GetObserverService() but that's fine) And also, put those |using| statements outside of the anonymous namespace. @@ +19,5 @@ > +using mozilla::services::GetObserverService; > + > +void > +notifyObserversWithMessage(const char* aEventName, > + const SmsMessageData& aMessageData) Coding style: C++ methods should beging with an uppercase. Also, could you name it: NotifyObserversWithSMSMessage. @@ +30,5 @@ > + nsCOMPtr<SmsMessage> message = new SmsMessage(aMessageData); > + obs->NotifyObservers(message, aEventName, nullptr); > +} > + > +} // End of unnamed namespace nite: } // anonymous namespace

Comment on attachment 673625 [details] [diff] [review] Part 4: Android implementation : v3 Review of attachment 673625 [details] [diff] [review]: ----------------------------------------------------------------- Isn't bug 803828 for this patch? If that's the case, could you re-attach the patch there and ask me for a review after changing the DOM implementation? I had a quick look and it seems right. The only thing I do not understand is when you set the serialId of the exception classes but my Java skills aren't great ;) This patch will also need a review from a mobile peer after mine.

(In reply to Mounir Lamouri (:mounir) from comment #61) > Comment on attachment 673625 [details] [diff] [review] > Part 4: Android implementation : v3 > > Review of attachment 673625 [details] [diff] [review]: > ----------------------------------------------------------------- > > Isn't bug 803828 for this patch? > If that's the case, could you re-attach the patch there and ask me for a > review after changing the DOM implementation? No, I've completed most delivery status related functions for Android backend in this issue. The only thing left behind is to update in-dabase delivery status at receiving a SR. Delivery status carried in the notification event is still valid. I think we still have to patch Android backend in this issue, at least don't fail Fennec build. > I had a quick look and it seems right. The only thing I do not understand is > when you set the serialId of the exception classes but my Java skills aren't > great ;) I don't know either. Without them, several warnings were generated and failed the build because of something like -Werror. > This patch will also need a review from a mobile peer after mine.

BTW, Vicamo, you can also remove the embedding/android/ code. I doubt we still care about that, nowadays.

(In reply to Mounir Lamouri (:mounir) from comment #60) > Part 2: dom implementation : v4 > Review of attachment 673635 [details] [diff] [review]: > > ::: dom/sms/interfaces/nsISmsDatabaseService.idl > @@ +18,5 @@ > > { > > // Takes some information required to save the message and returns its id. > > long saveReceivedMessage(in DOMString aSender, in DOMString aBody, > > in unsigned long long aDate); > > // Takes some information required to save the message and returns its id. > > + long saveSentMessage(in DOMString aDeliveryStatus, in DOMString aReceiver, > > in DOMString aBody, in unsigned long long aDate); > > I do not think we should get |aDeliveryStatus| here if we want to set always > the same delivery status to sent message. It shouldn't be up to the callers > to set the status. > It should always be sent to 'pending', right? In our current implementation, yes. But note that we may have a new Settings entry to enable or disable the SR requesting in the future because some carriers list it as a mandatory feature. If SR requesting is disabled, that value will be NotApplicable as you suggested in previous review comment. > ::: dom/sms/src/SmsFilter.cpp > @@ +260,5 @@ > > +SmsFilter::SetDeliveryStatus(const nsAString& aDeliveryStatus) > > +{ > > + if (aDeliveryStatus.IsEmpty()) { > > + mData.deliveryStatus() = eDeliveryStatus_Unknown; > > + return NS_OK; > > I do not think this is working as expected: > aDeliveryStatus.IsEmpty() is equivalent to > aDeliveryStatus.Equals(DELIVERY_STATUS_NOT_APPLICABLE) which means we will > never set deliveryStatus() to eDeliveryStatus_NotApplicable. That's what I meant in comment #51. I suggested let DELIVERY_STATUS_NOT_APPLICABLE be "not-applicable". > ::: dom/sms/src/android/SmsDatabaseService.cpp > @@ +43,5 @@ > > NS_IMETHODIMP > > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > > + const nsAString& aDeliveryStatus) > > +{ > > + // TODO: Bug 803828: update delivery status for sent messages in Android. > > I do not think the comment is correct here. We should call this method only > to set aDeliveryStatus to 'success' or 'error'. Any other value should be > managed automatically by the backend or the DOM code. Please note this interface method is also called by RadioInterfaceLayer as: gSmsDatabaseService.setMessageDeliveryStatus(options.id, message.deliveryStatus); Since `message.deliveryStatus` is a DOMString, then this method must accept a DOMString. IMO, that's something backend specific. We should not have defined these functions in a shared IDL at all. For example, the implementation of this function in Android backend has to be in Java, then you'll have STATUS_REPORT_RECEIVED(Java) -> setMessageDeliveryStatus(C++) -> setMessageDeliveryStatus(Java) -> STORE_IN_DATABASE(Java). > Actually, aDeliveryStatus should be checked and the current message status > has to be 'pending'. No, the initial value is always "pending" in our current implementation, but not here. When this method is invoked, it means caller is going to set the delivery status according to a newly received SR. Since the status carried by that SR can be either success or error, the aDeliveryStatus here can also be either success or error. > Ideally, those check should happen in a commen (to all > backends) place but I doubt this is easily doable. At least, put a comment > in the IDL saying that |setMessageDeliveryStatus| should be used that way.

(In reply to Mounir Lamouri (:mounir) from comment #63) > BTW, Vicamo, you can also remove the embedding/android/ code. I doubt we > still care about that, nowadays. If we're really going to remove them, what about an explicit issue for it? If not, I'd like to keep it as possible.

No need to. Just to make the patch easier. You _can_ ;)

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #64) > (In reply to Mounir Lamouri (:mounir) from comment #60) > > Part 2: dom implementation : v4 > > Review of attachment 673635 [details] [diff] [review]: > > > > ::: dom/sms/interfaces/nsISmsDatabaseService.idl > > @@ +18,5 @@ > > > { > > > // Takes some information required to save the message and returns its id. > > > long saveReceivedMessage(in DOMString aSender, in DOMString aBody, > > > in unsigned long long aDate); > > > // Takes some information required to save the message and returns its id. > > > + long saveSentMessage(in DOMString aDeliveryStatus, in DOMString aReceiver, > > > in DOMString aBody, in unsigned long long aDate); > > > > I do not think we should get |aDeliveryStatus| here if we want to set always > > the same delivery status to sent message. It shouldn't be up to the callers > > to set the status. > > It should always be sent to 'pending', right? > > In our current implementation, yes. But note that we may have a new Settings > entry to enable or disable the SR requesting in the future because some > carriers list it as a mandatory feature. If SR requesting is disabled, that > value will be NotApplicable as you suggested in previous review comment. Indeed but we are making bets on how this is going to be implemented. We could also have saveSentMessage() checking the setting. Changing this method signature doesn't have that much cost so I wouldn't design it based on possible future features. > > ::: dom/sms/src/SmsFilter.cpp > > @@ +260,5 @@ > > > +SmsFilter::SetDeliveryStatus(const nsAString& aDeliveryStatus) > > > +{ > > > + if (aDeliveryStatus.IsEmpty()) { > > > + mData.deliveryStatus() = eDeliveryStatus_Unknown; > > > + return NS_OK; > > > > I do not think this is working as expected: > > aDeliveryStatus.IsEmpty() is equivalent to > > aDeliveryStatus.Equals(DELIVERY_STATUS_NOT_APPLICABLE) which means we will > > never set deliveryStatus() to eDeliveryStatus_NotApplicable. > > That's what I meant in comment #51. I suggested let > DELIVERY_STATUS_NOT_APPLICABLE be "not-applicable". Actually, I assume you want to imitate .delivery but maybe we should change the current behaviour to have: filter.delivery = ""; <- asserts filter.delivery = null; <- reset the value And that could be the same thing for deliveryStatus. Vicamo and Jonas, what do you think? > > ::: dom/sms/src/android/SmsDatabaseService.cpp > > @@ +43,5 @@ > > > NS_IMETHODIMP > > > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > > > + const nsAString& aDeliveryStatus) > > > +{ > > > + // TODO: Bug 803828: update delivery status for sent messages in Android. > > > > I do not think the comment is correct here. We should call this method only > > to set aDeliveryStatus to 'success' or 'error'. Any other value should be > > managed automatically by the backend or the DOM code. > > Please note this interface method is also called by RadioInterfaceLayer as: > > gSmsDatabaseService.setMessageDeliveryStatus(options.id, > message.deliveryStatus); > > Since `message.deliveryStatus` is a DOMString, then this method must accept > a DOMString. I never said it shouldn't be a DOMString. > IMO, that's something backend specific. We should not have defined these > functions in a shared IDL at all. For example, the implementation of this > function in Android backend has to be in Java, then you'll have > STATUS_REPORT_RECEIVED(Java) -> setMessageDeliveryStatus(C++) -> > setMessageDeliveryStatus(Java) -> STORE_IN_DATABASE(Java). I do not thing we should have had B2G specific stuff there. We need those shared interfaces to have multiple implementations. Android is doing part of the work for us and some stuff might be useless, indeed. The code for the Android backend is written in Java but Gecko is in C++ which force us to do C++ / Java dances, this is true. However, not having interfaces would mean that backends wouldn't all follow the same rules and code inside Gecko wouldn't be able to call a method from one of those components without knowing the actual backend being implemented. > > Actually, aDeliveryStatus should be checked and the current message status > > has to be 'pending'. > > No, the initial value is always "pending" in our current implementation, but > not here. When this method is invoked, it means caller is going to set the > delivery status according to a newly received SR. Since the status carried > by that SR can be either success or error, the aDeliveryStatus here can also > be either success or error. Do we agree that this method can't be called with any other value than 'success' or 'error' and if this method is called, the message's deliveryStatus must be 'pending', right?

(In reply to Mounir Lamouri (:mounir) from comment #67) > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #64) > > (In reply to Mounir Lamouri (:mounir) from comment #60) > > > Part 2: dom implementation : v4 > > > Review of attachment 673635 [details] [diff] [review]: > > > > > > ::: dom/sms/interfaces/nsISmsDatabaseService.idl > > > @@ +18,5 @@ > > > > { > > > > // Takes some information required to save the message and returns its id. > > > > long saveReceivedMessage(in DOMString aSender, in DOMString aBody, > > > > in unsigned long long aDate); > > > > // Takes some information required to save the message and returns its id. > > > > + long saveSentMessage(in DOMString aDeliveryStatus, in DOMString aReceiver, > > > > in DOMString aBody, in unsigned long long aDate); > > > > > > I do not think we should get |aDeliveryStatus| here if we want to set always > > > the same delivery status to sent message. It shouldn't be up to the callers > > > to set the status. > > > It should always be sent to 'pending', right? > > > > In our current implementation, yes. But note that we may have a new Settings > > entry to enable or disable the SR requesting in the future because some > > carriers list it as a mandatory feature. If SR requesting is disabled, that > > value will be NotApplicable as you suggested in previous review comment. > > Indeed but we are making bets on how this is going to be implemented. We > could also have saveSentMessage() checking the setting. Changing this method > signature doesn't have that much cost so I wouldn't design it based on > possible future features. Per discussion on IRC, it's true we still have options so aDeliveryStatus here is not a must. I'll remove it. > > > ::: dom/sms/src/SmsFilter.cpp > > > @@ +260,5 @@ > > > > +SmsFilter::SetDeliveryStatus(const nsAString& aDeliveryStatus) > > > > +{ > > > > + if (aDeliveryStatus.IsEmpty()) { > > > > + mData.deliveryStatus() = eDeliveryStatus_Unknown; > > > > + return NS_OK; > > > > > > I do not think this is working as expected: > > > aDeliveryStatus.IsEmpty() is equivalent to > > > aDeliveryStatus.Equals(DELIVERY_STATUS_NOT_APPLICABLE) which means we will > > > never set deliveryStatus() to eDeliveryStatus_NotApplicable. > > > > That's what I meant in comment #51. I suggested let > > DELIVERY_STATUS_NOT_APPLICABLE be "not-applicable". > > Actually, I assume you want to imitate .delivery but maybe we should change > the current behaviour to have: > filter.delivery = ""; <- asserts > filter.delivery = null; <- reset the value > > And that could be the same thing for deliveryStatus. > > Vicamo and Jonas, what do you think? Why is searching for a sent message with no status report request forbidden? Message.deliveryStatus has NotApplicable state because some messages don't have delivery status, but searching for these messages is a different thing, isn't it? > > > ::: dom/sms/src/android/SmsDatabaseService.cpp > > > @@ +43,5 @@ > > > > NS_IMETHODIMP > > > > +SmsDatabaseService::SetMessageDeliveryStatus(int32_t aMessageId, > > > > + const nsAString& aDeliveryStatus) > > > > +{ > > > > + // TODO: Bug 803828: update delivery status for sent messages in Android. > > > > > > I do not think the comment is correct here. We should call this method only > > > to set aDeliveryStatus to 'success' or 'error'. Any other value should be > > > managed automatically by the backend or the DOM code. > > > > Please note this interface method is also called by RadioInterfaceLayer as: > > > > gSmsDatabaseService.setMessageDeliveryStatus(options.id, > > message.deliveryStatus); > > > > Since `message.deliveryStatus` is a DOMString, then this method must accept > > a DOMString. > > I never said it shouldn't be a DOMString. I misunderstood you. I've understood what you said now. > > IMO, that's something backend specific. We should not have defined these > > functions in a shared IDL at all. For example, the implementation of this > > function in Android backend has to be in Java, then you'll have > > STATUS_REPORT_RECEIVED(Java) -> setMessageDeliveryStatus(C++) -> > > setMessageDeliveryStatus(Java) -> STORE_IN_DATABASE(Java). > > I do not thing we should have had B2G specific stuff there. We need those > shared interfaces to have multiple implementations. Android is doing part of > the work for us and some stuff might be useless, indeed. The code for the > Android backend is written in Java but Gecko is in C++ which force us to do > C++ / Java dances, this is true. However, not having interfaces would mean > that backends wouldn't all follow the same rules and code inside Gecko > wouldn't be able to call a method from one of those components without > knowing the actual backend being implemented. I meant, we can have nsISmsService and nsISmsDatabaseService, but the two interfaces contain only methods that are used by SmsManager or other in-Gecko components. The two interfaces should not include methods that are only used by each other. "saveSentMessage", "saveReceivedMessage", and "setMessageDeliveryStatus" are examples of the latter kind. They are platform specific and no one other than platform specific code uses these methods. These functions should only be declared in something like nsIRilSmsDatabaseService, which extends nsISmsDatabaseService. Anyway, let's have setMessageDeliveryStatus for now and add more comments addressing this situation in the Android backend. > > > Actually, aDeliveryStatus should be checked and the current message status > > > has to be 'pending'. > > > > No, the initial value is always "pending" in our current implementation, but > > not here. When this method is invoked, it means caller is going to set the > > delivery status according to a newly received SR. Since the status carried > > by that SR can be either success or error, the aDeliveryStatus here can also > > be either success or error. > > Do we agree that this method can't be called with any other value than > 'success' or 'error' and if this method is called, the message's > deliveryStatus must be 'pending', right? Yes. I've updated my patch and will upload for your review again after some local tests.

Address review comment #60, but since we still have problem with DELIVERY_STATUS_NOT_APPLICABLE, I'm not going to request for review now.

Update for saveSentMessage interface change.

Update for saveSentMessage interface change. Because we still have problem with DELIVERY_STATUS_NOT_APPLICABLE, I'm not going to request for review now.

1. Move serialVersionUID changes to bug 720325. 2. fix exception on searching messages with DeliveryStatus unset.

Rebased for bug 801096

Randomize serialVersionUID as done in bug 720325

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #68) > > > > ::: dom/sms/src/SmsFilter.cpp > > > > @@ +260,5 @@ > > > > > +SmsFilter::SetDeliveryStatus(const nsAString& aDeliveryStatus) > > > > > +{ > > > > > + if (aDeliveryStatus.IsEmpty()) { > > > > > + mData.deliveryStatus() = eDeliveryStatus_Unknown; > > > > > + return NS_OK; > > > > > > > > I do not think this is working as expected: > > > > aDeliveryStatus.IsEmpty() is equivalent to > > > > aDeliveryStatus.Equals(DELIVERY_STATUS_NOT_APPLICABLE) which means we will > > > > never set deliveryStatus() to eDeliveryStatus_NotApplicable. > > > > > > That's what I meant in comment #51. I suggested let > > > DELIVERY_STATUS_NOT_APPLICABLE be "not-applicable". > > > > Actually, I assume you want to imitate .delivery but maybe we should change > > the current behaviour to have: > > filter.delivery = ""; <- asserts > > filter.delivery = null; <- reset the value > > > > And that could be the same thing for deliveryStatus. > > > > Vicamo and Jonas, what do you think? > > Why is searching for a sent message with no status report request forbidden? > Message.deliveryStatus has NotApplicable state because some messages don't > have delivery status, but searching for these messages is a different thing, > isn't it? Sorry, I was quite not clear here. What I meant is that we should consider the empty string as a value. For example, .delivery only allow "received" and "sent" so "" would not be allowed and will assert INVALID_ARG. Right now, we use the empty string to reset the argument. We could use |null| for that, maybe? Depends how we stringify it. Might be worth looking at the WebIDL spec... So, if we do that, deliveryStatus would allow "" as NOT_APPLICABLE.

So, seems like we could use the WebIDL "DOMString?" which, seems like, could be done by checking if the empty string you got has IsVoid() returning true. Vicamo, could you check that? If it's the case, could you implement that? ... if that sounds a good idea to you.

(In reply to Mounir Lamouri (:mounir) from comment #76) > So, seems like we could use the WebIDL "DOMString?" which, seems like, could > be done by checking if the empty string you got has IsVoid() returning true. > > Vicamo, could you check that? If it's the case, could you implement that? > ... if that sounds a good idea to you. Do you mean something like: readonly attribute DOMString? deliveryStatus; xpidl throws an IDLError("invalid syntax", location).

I'm not 100% sure that I understand the question here, so if my answer doesn't make sense, please let me know. In general, I think it's nice to avoid making a difference between "", null and undefined. Developers tend to think of all of them as "not specified". Other solutions are certainly possible, but I don't generally think that they have advantages and generally result in people writing working code less often. So I think that we should do is to allow nsIDOMMozSmsFilter.deliveryStatus to be set to either "", null or undefined to mean "no filter for deliveryStatus". This would in WebIDL be accomplished by making the property look like: attribute DOMString? deliveryStatus; and then defining that either "" or null means that no filter is applied to the property. Since this interface doesn't use WebIDL I *think* that writing the following in xpidl would work: [Null(Empty),Undefined(Empty)] attribute DOMString deliveryStatus; We ideally should make the same change in the idl for the "delivery" attribute. At least if it's just a trivial change to the idl.

(In reply to Jonas Sicking (:sicking) from comment #78) > So I think that we should do is to allow nsIDOMMozSmsFilter.deliveryStatus > to be set to either "", null or undefined to mean "no filter for > deliveryStatus". Then what should it be for nsIDOMMozSmsMessage.deliveryStatus in the NOT_APPLICABLE case? (In reply to Mounir Lamouri (:mounir) from comment #76) > So, seems like we could use the WebIDL "DOMString?" which, seems like, could > be done by checking if the empty string you got has IsVoid() returning true. > > Vicamo, could you check that? If it's the case, could you implement that? > ... if that sounds a good idea to you. I've tried again. Empty string("") doesn't have F_VOIDED flag set, so I can set an empty string for NOT_APPLICABLE case. But it seems I should do this according to Jonas's last opinion.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #79) > (In reply to Jonas Sicking (:sicking) from comment #78) > > So I think that we should do is to allow nsIDOMMozSmsFilter.deliveryStatus > > to be set to either "", null or undefined to mean "no filter for > > deliveryStatus". > > Then what should it be for nsIDOMMozSmsMessage.deliveryStatus in the > NOT_APPLICABLE case? Is the use-case here wanting to create a filter which only returns messages with deliveryStatus set to NOT_APPLICABLE? If so, then it seems ok to let: nsIDOMMozSmsFilter.deliveryStatus = ""/null/undefined -> return all messages nsIDOMMozSmsFilter.deliveryStatus = "not-applicable" -> return messages with .deliveryStatus = "not-applicable" nsIDOMMozSmsFilter.deliveryStatus = "pending" -> return messages with .deliveryStatus = "pending" nsIDOMMozSmsFilter.deliveryStatus = "success" -> return messages with .deliveryStatus = "success" nsIDOMMozSmsFilter.deliveryStatus = "error" -> return messages with .deliveryStatus = "error" Though honestly, do we have any UI which require filtering on deliveryStatus at all? If not I suggest we don't add the ability to filter on deliveryStatus right now. > (In reply to Mounir Lamouri (:mounir) from comment #76) > > So, seems like we could use the WebIDL "DOMString?" which, seems like, could > > be done by checking if the empty string you got has IsVoid() returning true. > > > > Vicamo, could you check that? If it's the case, could you implement that? > > ... if that sounds a good idea to you. > > I've tried again. Empty string("") doesn't have F_VOIDED flag set, so I can > set an empty string for NOT_APPLICABLE case. But it seems I should do this > according to Jonas's last opinion. My suggestion was to add the xpidl annotations in comment 78 and then in the C++ code check deliveryStatusFilterString.IsEmpty() and not add a filter if that returns true. Though see above about if we really need to add filtering on delivery status at all right now.

1. Update available values for SmsMessage.deliveryStatus 2. postpone SmsFilter changes to bug 805378.

Comment on attachment 674567 [details] [diff] [review] Part 4: Android implementation : v6 I'll update all following implementation patches, test cases once we have a final agreement on this issue.

So, Jonas, I prefer to make sure we are on the same page here. Between deliveryStatus values being { "", "pending", "success", "error" } or { "not-applicable", "pending", "success", "error" } you actually prefer the later?

Comment on attachment 675032 [details] [diff] [review] Part 1: IDL changes : v3 Review of attachment 675032 [details] [diff] [review]: ----------------------------------------------------------------- Ah, now I understand when not-applicable happens! With regards to received messages, I think we can do three things and I'm fine with doing any of them: * Set .deliveryStatus = "not-applicable" * Set .deliveryStatus = "" * Set .deliveryStatus = "success" Setting it to success makes a lot of sense since it was in fact successfully delivered. Setting it to "" is somewhat clean, but means that if we add filtering, you can't filter out only messages with this status since IMHO setting the filter to "" should mean "don't filter on deliveryStatus". This might be ok though since you can always filter on .delivery = "received". Setting it to "not-applicable" is somewhat verbose, but is also pretty clear. I'm fine either way, but would possibly lean towards setting it to "success".

Actually, potentially I'd lean towards using "" since we tend to use falsy values in the DOM for properties that doesn't really apply. But like I said, I'm fine either way.

(In reply to Jonas Sicking (:sicking) from comment #84) > Comment on attachment 675032 [details] [diff] [review] > ----------------------------------------------------------------- > Setting it to "" is somewhat clean, but means that if we add filtering, you > can't filter out only messages with this status since IMHO setting the > filter to "" should mean "don't filter on deliveryStatus". This might be ok > though since you can always filter on .delivery = "received". I wrote "For received ..." in the comment because we don't have a way to explicitly enable/disable SMS-STATUS-REPORT at sending messages out (see bug 805722). The empty string("") or "not-applicable" values comes from Mounir's review comment #43. We have: 1. For received messages, there is no corresponding SMS-STATUS-REPORT part, but it's certainly a "successful" delivery; 2. For sent messages with SMS-STATUS-REPORT requested, suppose we had received that requested SMS-STATUS-REPORT, the delivery status will be either success or error; 3. For sent messages without SMS-STATUS-REPORT requested, we won't know the message's final destiny. in addition, 4. The SmsFilter uses empty string("" or null) for clearing a previously set rule. Because of 4), I would rather not use empty string for 1) or 3). If you and Mounir have no *strong* opinion on it, I will use "success" for 1) and "not-applicable" for 3) and update this patch yet again.

The stronger opinion I have is about not using "success" for 3) because it would make impossible to distinguish delivered messages with messages without delivery notice. I'm not sure if that's a good idea to have different values for received and sent message when deliveryStatus isn't applicable but I don't really have a strong opinion on that. But as said Jonas in comment 85, "" is clearly the Web way to handle that situation. However, that would make the search filtering harder. I think we have to find a compromise here. Whether we disable a filter with |null| instead of "" (1), or we never allow filtering on "" (2), or we use a literal instead of "" (3). I tend to prefer solution (1). I'm not a big fan of solution (3) but it might be better than solution (2).

1) Received messages: | "" | "success" | "not-applicable" sicking | +200 | +100 | +0 mounir | +0 | +100 | +100 vicamo[1] | -3 | +2 | +1 3) Sent without SR requistion: | "" | "success" | "not-applicable" sicking | ? | ? | ? mounir | +100 | -300 | +100 vicamo | -3 | -3 | +2 [1]: Vicamo is not a DOM peer.

I think we should go with "success" for received messages and "not-applicable" for messages sent without a delivery status request. It's very much possible to tell sent messages from received messages by looking at the .delivery field. It seems better to me to try to reflect the true delivery status than to try to make it possible to filter using just the .deliveryStatus field. Especially when the use cases for filtering on "all successfully deliver messages" seem pretty weak to me.

Update comments only

1. Remove SmsFilter.deliveryStatus. Addressed review comment #80. 2. NOT_APPLICABLE as "not-applicable" rather than an empty string. Comment #89.

1. Use "success" for received messages. Comment #89. 2. NOT_APPLICABLE as "not-applicable" rather than an empty string. Comment #89. 3. Remove SmsFilter.deliveryStatus. Addressed review comment #80. 4. Share delivery status string values between RadioInterfaceLayer and ril_consts. Now this patch makes no database schema changes. All SmsFilter related functions are moved to bug 805378.

1. Uses static_cast<...>. Review comment in bug 797277 comment #19. 2. Fix misleading comments. Review comment in bug 797277 comment #19. 3. Use "success" for received messages. Comment #89. 4. Remove SmsFilter.deliveryStatus. All SmsFilter related functions are moved to bug 805378. Comment #80.

1. Use "success" for received messages. Comment #89. 2. Remove SmsFilter.deliveryStatus. All SmsFilter related functions are moved to bug 805378. Comment #80.

Comment on attachment 675779 [details] [diff] [review] Part 2: DOM implementation : v6 Review of attachment 675779 [details] [diff] [review]: ----------------------------------------------------------------- r=me Sorry it took so long. I realize we should probably just have push something and try to improve the API for v2 given that it's a certified app only api. Sorry about that :(

Comment on attachment 675783 [details] [diff] [review] Part 4: Android implementation : v7 Review of attachment 675783 [details] [diff] [review]: ----------------------------------------------------------------- All comments made for mobile/android/base/GeckoSmsManager.java apply for embedding/android/GeckoSmsManager.java. r=me with the comments applied. ::: mobile/android/base/GeckoSmsManager.java @@ +318,5 @@ > + /* > + * Keep the following status codes in sync with |DeliveryStatus| in: > + * dom/sms/src/Types.h > + */ > + private final static int kDeliveryStatusUnknown = -1; I think you can remove this one. You removed it from Types.h. @@ +562,5 @@ > values.put("address", aRecipient); > values.put("body", aBody); > values.put("date", aDate); > + // Always 'PENDING' because we always request status report. > + values.put("status", 32); If there is no public constant in the Android's API, please create constants for that (for example: kInternalDelivryStatusPending). @@ +631,3 @@ > sender = cursor.getString(cursor.getColumnIndex("address")); > } else if (type == kSmsTypeSentbox) { > + deliveryStatus = cursor.getInt(cursor.getColumnIndex("status")); I think it would be better to do: deliveryStatus = translateAndroidDeliveryStatusToGecko(cursor.getInt(cursor.getColumnIndex("status"))); Otherwise, |deliveryStatus| might contain the Android/Internal representation or the Gecko representation. It could be source of bugs. @@ +636,5 @@ > throw new InvalidTypeException(); > } > > GeckoAppShell.notifyGetSms(cursor.getInt(cursor.getColumnIndex("_id")), > + translateAndroidDeliveryStatusToGecko(deliveryStatus), You can just do: deliveryStatus, With the previous change applied. @@ +920,5 @@ > + } > + if (aDeliveryStatus >= 32) { > + return kDeliveryStatusPending; > + } > + return kDeliveryStatusSuccess; You should use constants. Also, I think it could be better to be explicit regarding the success case, like: if (aDeliveryStatus == kInternalDeliverySuccess) { return kDeliveryStatusSuccess; } You could even use a switch and throw in the |default:| case. Also, I would name the method "getGeckoDeliveryStatusFromInternal" but that's a matter of taste so keep this name if you prefer.

(In reply to Mounir Lamouri (:mounir) from comment #95) > Comment on attachment 675779 [details] [diff] [review] > ----------------------------------------------------------------- > Sorry it took so long. I realize we should probably just have push something > and try to improve the API for v2 given that it's a certified app only api. > Sorry about that :( Let's get things done. Keeping a simple, clear, and easy of use API set is DOM peer's duty, and giving as much technical details as needed to clarify the purpose of API modification is mine. As a reviewer, our name will be appended at the end of each commit summary. Being careful is understandable and necessary. Thank you :)

Address review comment #96. I use 'getGeckoDeliveryStatus' in correspondence with the latter 'getGeckoMessageClass' suggested in bug 797277 comment #41.

rebase only

Comment on attachment 676497 [details] [diff] [review] Part 4: Android implementation : v9 Sorry, this is a wrong rebase to an older commit.

https://hg.mozilla.org/integration/mozilla-inbound/rev/853062e5abc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/289f2d39289c https://hg.mozilla.org/integration/mozilla-inbound/rev/95642feb675f https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d728ec5571 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b709819750

https://hg.mozilla.org/mozilla-central/rev/853062e5abc5 https://hg.mozilla.org/mozilla-central/rev/289f2d39289c https://hg.mozilla.org/mozilla-central/rev/95642feb675f https://hg.mozilla.org/mozilla-central/rev/d6d728ec5571 https://hg.mozilla.org/mozilla-central/rev/d8b709819750

Well this is really bad timing. My patch in bug 775997 conflicts all over the place now.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ddac7cc21c7f https://hg.mozilla.org/releases/mozilla-aurora/rev/a9bacea46172 https://hg.mozilla.org/releases/mozilla-aurora/rev/923e6b945947 https://hg.mozilla.org/releases/mozilla-aurora/rev/20ab72a92b58 https://hg.mozilla.org/releases/mozilla-aurora/rev/ad9889b7a3d9

(In reply to Gregor Wagner [:gwagner] from comment #103) > Well this is really bad timing. My patch in bug 775997 conflicts all over > the place now. Gregor, you don't have to worry about rebasing your patch to mine. I've been working on this since last night and have already some results in my local branch[1]. But somehow emulator is broken yet again, b2g keeps crashing, so I have to spend extra time on it. Anyway, I'll rebase your last patch and upload one for you. [1]: https://github.com/vicamo/mozilla-central/tree/bugzilla/775997/rebase-742790

Hm I guess this patch broke my existing sms database. You have to iterate over existing entries and add the field because after the upgrade you rely on the field being set.