The spec for SMS delivery report in bug 883095 shows that we will need message delivered timestamps to display in the message delivered information page, but our message object doesn't store this value currently. We need to decide weather we need this feature for v1.1 because it will block bug 883095.

Steve, Any progress made on this bug?

This issue is related to message delievery report user story(bug 919977). Hi Ayman, I remember you provide a delievery report draft few months ago, and it will display the delievery information dialog(with delivered number and timestamp) when user click on the double-checked icon. Will this information dialog be preserved in the new delievery report user story?

(In reply to Steve Chung [:steveck] from comment #2) > This issue is related to message delievery report user story(bug 919977). Hi > Ayman, I remember you provide a delievery report draft few months ago, and > it will display the delievery information dialog(with delivered number and > timestamp) when user click on the double-checked icon. Will this information > dialog be preserved in the new delievery report user story? yes it will

Hi Gene, we will need the delievery success timestamp for message object. Will you or other RIL members take this task? Thanks.

OK. I'll take this. Btw, could you please block this bug to any existing user stories to make it more clear that this bug needs to be fixed? Is that bug 919977?

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #5) > OK. I'll take this. Btw, could you please block this bug to any existing > user stories to make it more clear that this bug needs to be fixed? Is that > bug 919977? Yes you're right! Thanks for tagging the bug.

blocking a committed 1.3 user story. 1.3+

Set milestone. If it isn't reasonable for you, please reassign it.

We handle the MMS delivery timestamp at bug 927711. This bug is for SMS.

Comment on attachment 824623 [details] [diff] [review] Part 1, DOM API IDL, V1 Hi Jonas, The change is trivial. Only one |deliveryTimestamp| is added, which exactly follows our agreement with W3C spec [1]. This is for sure to go. [1] http://messaging.sysapps.org/#idl-def-SmsMessage

Hi, Gene, This deliveryTimestamp is added in dictionary MmsDeliveryInfo in W3C spec [1], not MmsMessage. [1] http://messaging.sysapps.org/#idl-def-SmsMessage (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12) > Comment on attachment 824623 [details] [diff] [review] > Part 1, DOM API IDL, V1 > > Hi Jonas, > > The change is trivial. Only one |deliveryTimestamp| is added, which exactly > follows our agreement with W3C spec [1]. This is for sure to go. > > [1] http://messaging.sysapps.org/#idl-def-SmsMessage

My bad, this is for SMS, not MMS. (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #13) > Hi, Gene, > This deliveryTimestamp is added in dictionary MmsDeliveryInfo in W3C spec > [1], not MmsMessage. > [1] http://messaging.sysapps.org/#idl-def-SmsMessage > > (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #12) > > Comment on attachment 824623 [details] [diff] [review] > > Part 1, DOM API IDL, V1 > > > > Hi Jonas, > > > > The change is trivial. Only one |deliveryTimestamp| is added, which exactly > > follows our agreement with W3C spec [1]. This is for sure to go. > > > > [1] http://messaging.sysapps.org/#idl-def-SmsMessage

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #14) > My bad, this is for SMS, not MMS. No worries. :) Btw, I'm solving the MMS part at Bug 927711. Patches are under way. Will come up soon.

Comment on attachment 825063 [details] [diff] [review] Part 2, implementation and test cases, V2 Review of attachment 825063 [details] [diff] [review]: ----------------------------------------------------------------- Have to remove tests relying on delivery-report events because emulator just doesn't have it. ::: dom/mobilemessage/tests/marionette/test_getmessage.js @@ +78,5 @@ > is(sentSms.sender, EMULATOR, "sender"); > is(sentSms.messageClass, "normal", "messageClass"); > ok(sentSms.timestamp instanceof Date, "timestamp is instanceof date"); > outSmsTimeStamp = sentSms.timestamp; > + ok(sentSms.deliveryTimestamp === null, "deliveryTimestamp is null"); nit: trailing ws ::: dom/mobilemessage/tests/marionette/test_outgoing.js @@ +38,5 @@ > > ok(message.id, "message.id"); > ok(message.threadId, "message.threadId"); > + > + if (delivery == "delivered") { This line is never active due to bug 788928. Please simply place a check: // TODO: bug 788928 - add test cases for ondelivered event. ok(message.deliveryTimestamp === null, "deliveryTimestamp is null"); @@ +73,5 @@ > } > > manager.removeEventListener("sending", onSmsSending); > manager.removeEventListener("sent", onSmsSent); > + manager.removeEventListener("deliverysuccess", onDeliverySuccess); that belongs to bug 788928 as well. @@ -92,5 @@ > "the messages got from onsent event and request result must be the same"); > > opt[mark] = true; > - > - done(); keep it until 788928 is solved. @@ +167,5 @@ > + > + checkMessage(event.message, "delivered", body); > + > + done(); > + } We don't have delivery-report in emulator yet, so this function will never be executed, which follows this test case will always fail with timeout error. @@ +173,3 @@ > manager.addEventListener("sending", onSmsSending); > manager.addEventListener("sent", onSmsSent); > + manager.addEventListener("deliverysuccess", onDeliverySuccess); ditto ::: dom/mobilemessage/tests/test_smsservice_createsmsmessage.js @@ +41,5 @@ > do_check_eq(sms.sender, null); > do_check_eq(sms.body, null); > do_check_eq(sms.messageClass, "normal"); > do_check_true(sms.timestamp instanceof Date); > + do_check_true(sms.deliveryTimestamp instanceof Date); For a delivery=sent, deliveryStatus=pending SMS message, its deliveryTimestamp should be null. Please also change deliveryStatus to "success". @@ +51,5 @@ > * Verify that attributes are read-only. > */ > add_test(function test_readonly_attributes() { > let sms = newMessage(null, null, "received", "success", null, null, null, > + "normal", new Date(), new Date(), true); Same here. The deliveryTimestamp attribute should be null for received messages. You can pass 0 here without modifications to other lines. @@ +97,5 @@ > */ > add_test(function test_timestamp_number() { > let ts = Date.now(); > let sms = newMessage(42, 1, "sent", "pending", "the sender", "the receiver", > + "the body", "normal", ts, ts, true); ditto. "sent", "success" here and similar cases below.

Comment on attachment 825120 [details] [diff] [review] Part 2, implementation and test cases, V3 Review of attachment 825120 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but please provide a link to full try. Thank you. :)

Full try: https://tbpl.mozilla.org/?tree=Try&rev=e2546ed1dc35

Comment on attachment 824623 [details] [diff] [review] Part 1, DOM API IDL, V1 Jonas might be to busy to take all the reviews related to RIL. Reassign this to Hsinyi since this one is a big urgent to meet V1.3. Hsinyi, this one follows W3C spec and I believe it's safe to go. Please let me know if you have any concerns, and then I'll let Jonas take this review.

Comment on attachment 824623 [details] [diff] [review] Part 1, DOM API IDL, V1 Review of attachment 824623 [details] [diff] [review]: ----------------------------------------------------------------- Very trivial and reasonable. Thank you!

r=hsinyi

Rebased and carry on r=vicamo.

remote: https://hg.mozilla.org/integration/b2g-inbound/rev/55658db2d021 remote: https://hg.mozilla.org/integration/b2g-inbound/rev/da3b8712c20d

https://hg.mozilla.org/mozilla-central/rev/55658db2d021 https://hg.mozilla.org/mozilla-central/rev/da3b8712c20d