901457 - [sms][mms] We need a property for the "sent" timestamp

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

In Bug 901453 we'd like to show the timestamp for when a message was sent.

Gene, would this be something difficult to do or do already have the information somewhere in Gecko ?


The property could be "sentTimestamp" for example.

Comment 1

[Gene Lian (I already quit Mozilla)]

To clarify, after discussing with Julien, the user story is: the receiver end needs to know the time point when the message was sent.

-----
Our current *timestamp* in the Moz{SMS,MMS}Message means:

* For sender: the time point when sending message *on the device*.
* For receiver: the time point when receiving message *on the device*.

-----
To solve this issue, I'd suggest adding a new *serverTimestamp* which represents the time points of:

         |             SMS              |              MMS
------------------------------------------------------------------------------
sender   | TP-SCTS in SMS-SUBMIT-REPORT | when device receives M-Send.conf [1]
------------------------------------------------------------------------------
receiver | TP-SCTS in SMS-DELIVER       | Date in M-Retrieve.conf


[1] Unfortunately, M-Send.conf doesn't carry server timestamp. That's the only thing we cannot know so the time point tentatively means the time point when the device receives M-Send.conf.

Comment 2

[Gene Lian (I already quit Mozilla)]

Hi Vicamo, I'd like to double check with you to see if this solution really makes sense.

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

You need a solution for MMS too ;)

Comment 4

[Gene Lian (I already quit Mozilla)]

Solving the receiver row at comment #2 is enough for this issue. However, I hope the |serverTimestamp| can also have some meanings on the sender end.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gene Lian [:gene] from comment #1)
> To clarify, after discussing with Julien, the user story is: the receiver
> end needs to know the time point when the message was sent.
> 
> -----
> Our current *timestamp* in the Moz{SMS,MMS}Message means:
> 
> * For sender: the time point when sending message *on the device*.
> * For receiver: the time point when receiving message *on the device*.
> 
> -----
> To solve this issue, I'd suggest adding a new *serverTimestamp* which
> represents the time points of:
> 
>          |             SMS              |              MMS
> ---------------------------------------------------------------------------
> sender   | TP-SCTS in SMS-SUBMIT-REPORT | when device receives M-Send.conf [1]

I think we can just use device timestamp for the two because we adopted device timestamp for the messages in bug 840051.  Using TP-SCTS may result in some strange cases like messages are delivered before they're sent.

> ----------------------------------------------------------------------
> receiver | TP-SCTS in SMS-DELIVER       | Date in M-Retrieve.conf

Comment 6

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
> > 
> >          |             SMS              |              MMS
> > ---------------------------------------------------------------------------
> > sender   | TP-SCTS in SMS-SUBMIT-REPORT | when device receives M-Send.conf [1]
> 
> I think we can just use device timestamp for the two because we adopted
> device timestamp for the messages in bug 840051.  Using TP-SCTS may result
> in some strange cases like messages are delivered before they're sent.

Sounds good. In this way, I'd still use |sentTimestamp| instead of |serverTimestamp|, because it doesn't always imply the server timestamp.

> 
> > ----------------------------------------------------------------------
> > receiver | TP-SCTS in SMS-DELIVER       | Date in M-Retrieve.conf

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, IDL changes

Hi Mounir,

Please see comment #1 for the solution. We're hoping to add a |sentTimestamp| to specify when an message was sent, either for the sender or the receiver. Thanks for the super-review!

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation and test cases (WIP)

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 787596 [details] [diff] [review]
Part 2, implementation and test cases (WIP)

Review of attachment 787596 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take a more close look on the test cases tomorrow.

::: dom/mobilemessage/src/SmsMessage.cpp
@@ +57,5 @@
> + *         The integer value to return.
> + * @return NS_OK if the convertion succeeds.
> + */
> +static nsresult
> +convertTimeToInt(JSContext* aCx, const JS::Value& aTime, uint64_t& aReturn)

Please move this function out of namespace mozilla::dom into an anonymous one.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1264,3 @@
>      savable.timestamp = Date.now();
> +    let sentDate = intermediate.headers["Date"];
> +    savable.sentTimestamp = sentDate ? Date.parse(sentDate) : 0;

"date", not capital "Date", and it's a mandatory field in a M-Retrieve.conf PDU[1].  Besides, it should already be an javascript Date object[2], so you don't need any further parse.

[1]: OMA-TS-MMS_ENC-V1_3-20110913-A Table 5 "Header fields of M-Retrieve.conf PDU".
[2]: http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/WspPduHelper.jsm#1049

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9)
> "date", not capital "Date", and it's a mandatory field in a M-Retrieve.conf
> PDU[1].  Besides, it should already be an javascript Date object[2], so you
> don't need any further parse.

Thanks for catching this! I did this by just restoring the original logic at bug 870302. If it's already a Date object then getTime() sounds good to use.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 787596 [details] [diff] [review]
Part 2, implementation and test cases (WIP)

Review of attachment 787596 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1246,5 @@
>            messageRecord.deliveryIndex = [delivery, messageRecord.timestamp];
>            isRecordUpdated = true;
> +
> +          if (delivery == DELIVERY_SENT) {
> +            messageRecord.sentTimestamp = Date.now();

We'll also have to address this in IDL, right?

::: dom/mobilemessage/tests/test_smsdatabaseservice.xul
@@ +182,5 @@
>      receiver: "[phone]",
>      body: "message 0",
>      messageClass: "normal",
> +    timestamp: 1329999861762,
> +    sentTimestamp: 1329999861762

This test case is outdated and has been disabled for a long time.  You can just skip it here.

Comment 12

[Gene Lian (I already quit Mozilla)]

It's a great opportunity to clean up some koi? feature bugs in the gap between V1.2 and V1.3. I'll start with this one first since the feedback are all positive. Will try to update the patches by the weekend.

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation and test cases, V2

Addressing comment #9 and comment #11.

Comment 14

[Gene Lian (I already quit Mozilla)]

Hi Wilfred, this is the sentTimestamp we talked about in the stand-up meeting.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 809252 [details] [diff] [review]
Part 2, implementation and test cases, V2

Review of attachment 809252 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +20,5 @@
>  using namespace mozilla::dom::mobilemessage;
>  
>  DOMCI_DATA(MozMmsMessage, mozilla::dom::MmsMessage)
>  
> +namespace {

Let's move this shared |convertTimeToInt| utility function into dom/mobilemessage/src/MobileMessageUtils.{h,cpp}, shall we?

@@ +241,5 @@
>  
> +  // Set |sentTimestamp|.
> +  uint64_t sentTimestamp;
> +  rv = convertTimeToInt(aCx, aSentTimestamp, sentTimestamp);
> +  NS_ENSURE_SUCCESS(rv, rv);

|aSentTimestamp| could be invalid in some cases.

::: dom/mobilemessage/src/SmsMessage.cpp
@@ +26,5 @@
> + *         The integer value to return.
> + * @return NS_OK if the convertion succeeds.
> + */
> +static nsresult
> +convertTimeToInt(JSContext* aCx, const JS::Value& aTime, uint64_t& aReturn)

ditto.

@@ +154,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Set |sentTimestamp|.
> +  rv = convertTimeToInt(aCx, aSentTimestamp, data.sentTimestamp());
> +  NS_ENSURE_SUCCESS(rv, rv);

|aSentTimestamp| could be invalid in some cases.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1249,5 @@
> +          // When updating an message's delivey state to 'sent', we also update
> +          // its |sentTimestamp| by the current device timestamp to represent
> +          // when the message is successfully sent.
> +          if (delivery == DELIVERY_SENT) {
> +            messageRecord.sentTimestamp = Date.now();

Please also have an database revision pump up and ensure all message records have this new field 'sentTimestamp'.

Comment 16

[Andrew Overholt [:overholt]]

(In reply to Gene Lian [:gene] (Summit + PTOs 10/3 ~ 10/14) from comment #12)
> It's a great opportunity to clean up some koi? feature bugs in the gap
> between V1.2 and V1.3.

If we make this koi+ it will hold 1.2 (and land in 1.2 when it's finished).  Is that what you intend, Gene?

Comment 17

[Gene Lian (I already quit Mozilla)]

(In reply to Andrew Overholt [:overholt] from comment #16)
> (In reply to Gene Lian [:gene] (Summit + PTOs 10/3 ~ 10/14) from comment #12)
> > It's a great opportunity to clean up some koi? feature bugs in the gap
> > between V1.2 and V1.3.
> 
> If we make this koi+ it will hold 1.2 (and land in 1.2 when it's finished). 
> Is that what you intend, Gene?

Sorry for the delayed response. Just came from a week-long vacation.

AFAIK, this one is more like a new feature and I don't think PMs are requesting this for V1.2. Maybe, we should move it to V1.3. Julien, what do you think? Should all the timestamp-related features be put in V1.2?

Comment 18

[Andrew Overholt [:overholt]]

I think Gene meant to ni(Julien) for comment 17.

Comment 19

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Can definitely be moved to 1.3, we won't do this in the app in 1.2 anyway.

Comment 20

[Gene Lian (I already quit Mozilla)]

This issue doesn't block any V1.3 user story.

Comment 21

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

Actually I think we need it in bug 933131.

see https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=822328.

Why did you remove it the block entry?

Comment 22

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

(In reply to Julien Wajsberg [:julienw] from comment #21)

> see https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=822328.

page 18

Comment 23

[Gene Lian (I already quit Mozilla)]

Hmm... I missed that bit... Thank you for bringing this to my attention.

Comment 24

[Anshul]

Hey Julie, could you please point me to the UX spec for receiver report? As per https://bugzilla.mozilla.org/show_bug.cgi?id=933131#c0 I don't see that in the UX spec. Are there any gecko bugs to support read reports?

Comment 25

[Steve Chung [:steveck]]

(In reply to Anshul from comment #24)
> Hey Julie, could you please point me to the UX spec for receiver report? As
> per https://bugzilla.mozilla.org/show_bug.cgi?id=933131#c0 I don't see that
> in the UX spec. Are there any gecko bugs to support read reports?

Hi Anshul,
Please ref the latest wireframe : https://mozilla.app.box.com/s/0u4jt353ei9ov2c150ip/1/1170795225/11918301182/1, page 25

And the gecko meta bug for read report should be bug 852863

Comment 26

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

That said, I'm perfectly open with leaving this part out of the information panel for 1.3 and moving this to 1.4.

But let's be clear here: as a dogfood user, I miss this feature every day.

Comment 27

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, IDL changes (sr=sicking), V2

Comment 28

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation (r=vicamo), V3

Comment 29

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases (r=vicamo), V3

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

Comment 30

[Gene Lian (I already quit Mozilla)]

Btw, in test_smsservice_createsmsmessage.js, I also tried to make the the combination among delivery, deliveryStatus, sentTimestamp and deliveryTimestamp more reasonable. For example, if deliveryStatus is success, then we must assign a valid deliveryTimestamp (!= 0) to the message.

Comment 31

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases (r=vicamo), V3.1

New full try: https://tbpl.mozilla.org/?tree=Try&rev=7b4fa955116a

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8343033 [details] [diff] [review]
Part 2, implementation (r=vicamo), V3

Review of attachment 8343033 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1259,5 @@
> +      }
> +
> +      let messageRecord = cursor.value;
> +      messageRecord.sentTimestamp = 0;
> +      cursor.update(messageRecord);

nit: we can still update |sentTimestamp| for received MMS messages.

@@ +2182,5 @@
>      aMessage.read = FILTER_READ_READ;
>  
> +    // If |sentTimestamp| is not specified, use 0 as default.
> +    if (aMessage.sentTimestamp == undefined) {
> +      aMessage.sentTimestamp = 0;

nit: |sentTimestamp| should be always 0 here.

Comment 33

[Julien Wajsberg [:julienw] (PTO -> Jul 24)]

move to 1.4

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8343532 [details] [diff] [review]
Part 3, test cases (r=vicamo), V3.1

Review of attachment 8343532 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/tests/test_smsservice_createsmsmessage.js
@@ -43,5 @@
>    do_check_eq(sms.receiver, null);
>    do_check_eq(sms.sender, null);
>    do_check_eq(sms.body, null);
>    do_check_eq(sms.messageClass, "normal");
> -  do_check_eq(sms.deliveryTimestamp, 0);

It seems you applied this patch on top of something else.  I get one additional test for |sms.timestamp|.

@@ -64,5 @@
>    do_check_eq(sms.receiver, null);
>    do_check_eq(sms.sender, null);
>    do_check_eq(sms.body, null);
>    do_check_eq(sms.messageClass, "normal");
> -  do_check_eq(sms.deliveryTimestamp, 0);

ditto

@@ +84,5 @@
>    sms.iccId = "987654321";
>    do_check_eq(sms.iccId, ICC_ID);
>  
> +  sms.delivery = "received";
> +  do_check_eq(sms.delivery, "sent");

nit: not a big deal, but why?

@@ +293,5 @@
>   */
>  add_test(function test_invalid_delivery_status_string() {
>    do_check_throws(function() {
>      newMessage(42, 1, ICC_ID, "sent", "this is invalid", "the sender", "the receiver",
> +               "the body", "normal", new Date(), new Date(), 0, true);

"the body", "normal", 0, 0, 0, true

@@ +304,5 @@
>   */
>  add_test(function test_invalid_delivery_status_string() {
>    do_check_throws(function() {
>      newMessage(42, 1, ICC_ID, "sent", 1, "the sender", "the receiver", "the body",
> +               "normal", new Date(), new Date(), 0, true);

ditto

@@ +315,5 @@
>   */
>  add_test(function test_invalid_message_class_string() {
>    do_check_throws(function() {
>      newMessage(42, 1, ICC_ID, "sent", "success", "the sender", "the receiver",
> +               "the body", "this is invalid", new Date(), new Date(), new Date(), true);

ditto

@@ +326,5 @@
>   */
>  add_test(function test_invalid_message_class_string() {
>    do_check_throws(function() {
>      newMessage(42, 1, ICC_ID, "sent", "success", "the sender", "the receiver",
> +               "the body", 1, new Date(), new Date(), new Date(), true);

ditto

Comment 35

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation (r=vicamo), V3.1

Fixed comment #32.

Comment 36

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #34)

> It seems you applied this patch on top of something else.  I get one
> additional test for |sms.timestamp|.

It's on top of bug 939302. I'd prefer landing that first.

> @@ +84,5 @@
> >    sms.iccId = "987654321";
> >    do_check_eq(sms.iccId, ICC_ID);
> >  
> > +  sms.delivery = "received";
> > +  do_check_eq(sms.delivery, "sent");
> 
> nit: not a big deal, but why?

Because the original combination of delivery/deliveryStatus = "received"/"success", which doesn't make sense.

> 
> @@ +293,5 @@
> >   */
> >  add_test(function test_invalid_delivery_status_string() {
> >    do_check_throws(function() {
> >      newMessage(42, 1, ICC_ID, "sent", "this is invalid", "the sender", "the receiver",
> > +               "the body", "normal", new Date(), new Date(), 0, true);
> 
> "the body", "normal", 0, 0, 0, true

If the delivery = "sent", which means timestamp/sentTimestamp should keep non-zero values.

Why I do all of this is just because I want to make the test cases more reasonable and more aligned to what's happening in the real world, instead of just testing the logic.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8343532 [details] [diff] [review]
Part 3, test cases (r=vicamo), V3.1

Review of attachment 8343532 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/tests/test_smsservice_createsmsmessage.js
@@ +282,5 @@
>   */
>  add_test(function test_invalid_delivery_string() {
>    do_check_throws(function() {
>      newMessage(42, 1, ICC_ID, 1, "pending", "the sender", "the receiver", "the body",
> +               "normal", 0, 0, 0, true);

Here.  I thought you want to zeroize all timestamps so that they don't affect the tests.  But if you want them to come more close to reality, this should be:

  "normal", new Date(), 0, 0, true

Comment 38

[Gene Lian (I already quit Mozilla)]

Thanks for the review! I'll further remove all the Date object inputs at bug 944890.

Comment 39

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, test cases (r=vicamo), V3.2

Fixed comment #37 and carry on r=vicamo.

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

Comment 40

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, implementation (r=vicamo), V3.2

RebasedPart 2, implementation (r=vicamo), V3.1

Comment 41

[Gene Lian (I already quit Mozilla)]

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

Comment 42

[Gene Lian (I already quit Mozilla)]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/e919b443a7fe
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/16a36f5bb9a8
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/6e36d9d83c3f

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e919b443a7fe
https://hg.mozilla.org/mozilla-central/rev/16a36f5bb9a8
https://hg.mozilla.org/mozilla-central/rev/6e36d9d83c3f