Closed Bug 887159 Opened 11 years ago Closed 11 years ago

B2G SMS: Handle message delivered timestamp for delivery report

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 4 - 11/8
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: steveck, Assigned: airpingu)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 4 obsolete files)

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.
blocking-b2g: --- → koi?
Blocks: b2g-mms
Blocks: b2g-sms
No longer blocks: b2g-mms
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Version: Trunk → unspecified
Steve, Any progress made on this bug?
Flags: needinfo?(schung)
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?
Flags: needinfo?(schung) → needinfo?(aymanmaat)
(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
Flags: needinfo?(aymanmaat)
Hi Gene, we will need the delievery success timestamp for message object. Will you or other RIL members take this task? Thanks.
Flags: needinfo?(gene.lian)
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?
Assignee: nobody → gene.lian
Blocks: 919977
Flags: needinfo?(gene.lian) → needinfo?(schung)
blocking-b2g: koi? → 1.3?
(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.
Flags: needinfo?(schung)
Depends on: 928821
blocking a committed 1.3 user story. 1.3+
blocking-b2g: 1.3? → 1.3+
Set milestone. If it isn't reasonable for you, please reassign it.
Target Milestone: --- → 1.3 Sprint 6 - 12/6
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 Sprint 4 - 11/8
Summary: B2G SMS : Handle message delivered timestamp for delivery report → B2G SMS & MMS : Handle message delivered timestamp for delivery report
We handle the MMS delivery timestamp at bug 927711. This bug is for SMS.
Summary: B2G SMS & MMS : Handle message delivered timestamp for delivery report → B2G SMS: Handle message delivered timestamp for delivery report
No longer depends on: 928821
Keywords: dev-doc-needed
Attached patch Part 1, DOM API IDL, V1 (obsolete) (deleted) — Splinter Review
Attached patch Part 2, implementation and test cases, V1 (obsolete) (deleted) — Splinter Review
Attachment #824624 - Flags: review?(vyang)
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
Attachment #824623 - Flags: superreview?(jonas)
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.
Attached patch Part 2, implementation and test cases, V2 (obsolete) (deleted) — Splinter Review
Attachment #824624 - Attachment is obsolete: true
Attachment #824624 - Flags: review?(vyang)
Attachment #825063 - Flags: review?(vyang)
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.
Attachment #825063 - Flags: review?(vyang)
Attached patch Part 2, implementation and test cases, V3 (obsolete) (deleted) — Splinter Review
Attachment #825063 - Attachment is obsolete: true
Attachment #825120 - Flags: review?(vyang)
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. :)
Attachment #825120 - Flags: review?(vyang) → review+
Blocks: 933131
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.
Attachment #824623 - Flags: superreview?(jonas) → superreview?(htsai)
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!
Attachment #824623 - Flags: superreview?(htsai) → superreview+
Attached patch Part 1, DOM API IDL, V1.1 (deleted) — Splinter Review
r=hsinyi
Attachment #824623 - Attachment is obsolete: true
Attachment #825850 - Flags: superreview+
Rebased and carry on r=vicamo.
Attachment #825120 - Attachment is obsolete: true
Attachment #825851 - Flags: review+
Depends on: 960741
No longer depends on: 960741
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: