In bug 933133 comment 27, new system message of "sms-delivery-error" is required for gaia to be notified for the error state when delivery report is received. Because the related RIL-interface is frozen in bug 1072808, to add a new type of sms system message, the interface migration has to be taken into consideration.[1] Besides "sms-delivery-error" for SMS [2], this shall also be required for MMS. [3] Hence, file this bug to support this feature. [1] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/interfaces/nsISmsMessenger.idl#l16 [2] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/SmsService.js#l372 [3] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/MmsService.js#l1979

Note that it's not only for delivery error, but simply sending error too.

(In reply to Julien Wajsberg [:julienw] from comment #1) > Note that it's not only for delivery error, but simply sending error too. In this case, "sms-failed" is required as well. [1] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/SmsService.js#l302 [2] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/SmsService.js#l857 [3] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/SmsService.js#l898 [4] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/MmsService.js#l2241 [5] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/MmsService.js#l2258 [6] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/gonk/MmsService.js#l2285

I'm actually more interested by sms-failed. Do we have the same issue with a frozen RIL interface for this event ?

(In reply to Julien Wajsberg [:julienw] from comment #3) > I'm actually more interested by sms-failed. Do we have the same issue with a > frozen RIL interface for this event ? Unfortunately, yes, we don't expect directly access to the system messenger API by vendor's RIL after interface is frozen in 2.2 to prevent any API changes in system messenger that breaks vendor's binary release in the future. That's why we create nsISmsMessenger in [1] as a proxy to restrict the sms-related system messages requested by vendor. In this bug, what we have to do is to 1. define new nsISmsMessenger.NOTIFICATION_TYPE_XXX for "sms-failed" & "sms-delivery-error" and implement the broadcast of the corresponding system messages. 2. call nsISmsMessenger.notifySms with new types defined above in SmsService.js. (Note: MmsService is not replaced by vendor, so we can call the System Messenger APIs directly.) However, for vendor's binary release, this newly-added feature will only be completed after step 2) is done in vendor's implementation. [1] https://hg.mozilla.org/mozilla-central/file/eea6188b9b05/dom/mobilemessage/interfaces/nsISmsMessenger.idl#l16

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4) > (In reply to Julien Wajsberg [:julienw] from comment #3) > > I'm actually more interested by sms-failed. Do we have the same issue with a > > frozen RIL interface for this event ? > > Unfortunately, yes, we don't expect directly access to the system messenger > API by vendor's RIL after interface is frozen in 2.2 to prevent any API > changes in system messenger that breaks vendor's binary release in the > future. > That's why we create nsISmsMessenger in [1] as a proxy to restrict the > sms-related system messages requested by vendor. > > In this bug, what we have to do is to > 1. define new nsISmsMessenger.NOTIFICATION_TYPE_XXX for "sms-failed" & > "sms-delivery-error" and implement the broadcast of the corresponding system > messages. > 2. call nsISmsMessenger.notifySms with new types defined above in > SmsService.js. > (Note: MmsService is not replaced by vendor, so we can call the System > Messenger APIs directly.) > > However, for vendor's binary release, this newly-added feature will only be > completed after step 2) is done in vendor's implementation. > Thanks Bevis for the detailed explanation! I just want to add a remark that the situation of vendor RIL support is the same as before when we didn't freeze ril interfaces. That is, there isn't a new feature in vendor's release until it's done in vendor's implementation.

(In reply to Julien Wajsberg [:julienw] from comment #3) > I'm actually more interested by sms-failed. Do we have the same issue with a > frozen RIL interface for this event ? Hi Julien, IMO, it's more important to have "sms-delivery-error" system message because it's a standalone event that is not related to any pending DOMRequests and it provides a proactive way to notify user the status of sent messages. I am curious to know why "sms-failed" is more interesting to you when this will be notified by the callback of the DOMRequest. My understanding is that you need a notification when message app is in the background after a message is sent to allow user to launch the message app again. Is this the only reason or do your expect that there is another app requires this type of message for different purpose? Thanks!

No, it's really for the SMS application only. The main issue issue is that sending a message can take time (especially for MMS!) and the application can be killed before we get an error, and in that case we'd never have the request's onerror handler. I say it's the most important because that's basic functionality. We can still do the part that does not need the system message of course.

This is to define new system messages of "sms-failed" & "sms-delivery-error" to be applied by SmsService/MmsService. Hi Edgar, May I have your review for this change? Thanks!

This is to broadcast these new defined messages from SmsService/MmsService accordingly.

Replace inner function/bind() with arrow function: here are the regex I used to filter these inner functions: \(function\s*\w*\( bind( = function \w+\s*[^:]\s*function\s*\(

Comment on attachment 8588968 [details] [diff] [review] Part 1 v1: Define new system messages of "sms-failed" and "sms-delivery-error". Review of attachment 8588968 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you.

Comment on attachment 8588971 [details] [diff] [review] Part 4 v1: Add test coverage for "sms-failed" and "sms-delivery-error". Review of attachment 8588971 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but could you also add test for "sms-failed" in test_error_of_mms_send.js [1] and test_error_of_sms_send.js [2] to ensure the system message will be sent when failing to send sms/mms? [1] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_error_of_mms_send.js [2] https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_error_of_sms_send.js

(In reply to Edgar Chen [:edgar][:echen] from comment #13) > Comment on attachment 8588971 [details] [diff] [review] > Part 4 v1: Add test coverage for "sms-failed" and "sms-delivery-error". > > Review of attachment 8588971 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, but could you also add test for "sms-failed" in > test_error_of_mms_send.js [1] and test_error_of_sms_send.js [2] to ensure > the system message will be sent when failing to send sms/mms? > > [1] > https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/ > marionette/test_error_of_mms_send.js > [2] > https://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/ > marionette/test_error_of_sms_send.js I've tried it locally but met some problem for registering sms-failed during testing. :( Hence, bug 1153073 was created as the follow-up for testing these newly defined system-message.

Comment on attachment 8588970 [details] [diff] [review] Part 3 v1: Replace inner function/.bind(this) with arrow functions. Review of attachment 8588970 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thank you.

update try-server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=539d8487f0cf

https://hg.mozilla.org/integration/b2g-inbound/rev/e7ddf46e9438 https://hg.mozilla.org/integration/b2g-inbound/rev/b83fcc2b0018 https://hg.mozilla.org/integration/b2g-inbound/rev/81a5be7358a5 https://hg.mozilla.org/integration/b2g-inbound/rev/90908701af56

https://hg.mozilla.org/mozilla-central/rev/e7ddf46e9438 https://hg.mozilla.org/mozilla-central/rev/b83fcc2b0018 https://hg.mozilla.org/mozilla-central/rev/81a5be7358a5 https://hg.mozilla.org/mozilla-central/rev/90908701af56

Thanks so much !