885679 - B2G MMS: Add 'subject' to {Thread} object.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Borja Salguero [:borjasalguero]]

For showing in the thread-list of MMS App the subject we need to have this info available in 'Thread' object when requesting 'getThreads'.

Comment 1

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

Please clearly define what should thread::subject be in following cases:

1. thread messages are all SMS,
2. some are SMS and some are MMS,
3. thread messages are all MMS but with different subjects

Comment 2

[Alex Keybl [:akeybl]]

Over to dietrich to make the call on blocking, given this will require late functionality/UX. Sounds like a blocker, but we want to make sure we _need_ it.

Comment 3

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #1)
> Please clearly define what should thread::subject be in following cases:
> 
> 1. thread messages are all SMS,
> 2. some are SMS and some are MMS,
> 3. thread messages are all MMS but with different subjects

I think Borja is talking about the subject of the last message in the thread. Right?

Comment 4

[Borja Salguero [:borjasalguero]]

Yep. But I think that this is not a blocker right now, but we should handle the subject in the future (currently the composer has no subject for example, that's why I think it's not a blocker). Dietrich, could you confirm if this is our roadmap for v1.2? Thanks!

Comment 5

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

Attachment: part 1/2: interface changes

Comment 6

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

Attachment: part 2/2: dom & backend changes

Still need detailed information about what should a thread subject be under various conditions as described in my comment 1.  Will only request for review after that being addressed.

Comment 7

[Dietrich Ayala (:dietrich)]

Vicamo, who do you need that info from? UX?

Comment 8

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

(In reply to Dietrich Ayala (:dietrich) from comment #7)
> Vicamo, who do you need that info from? UX?

Let's ask the reporter.

Comment 9

[Borja Salguero [:borjasalguero]]

In this case we need input from Product & UX Team. Subject is handled in several ways depending on the platform, and we need to agree how to make this happen in our Firefox OS :)

Comment 10

[Julien Wajsberg [:julienw]]

Let me try.

The subject should reflect the last message in the thread, just like the timestamp. Therefore:

1. last message is a SMS
-> thread.subject is null

2. last message is a MMS without a subject
-> thread.subject is null

3. last message is a MMS with a subject
-> thread.subject = lastMessage.subject


Generally, this is it: thread.subject = lastMessage.subject.

Ayman, can you confirm this (so that Gecko dev can move forward) ?

Comment 11

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> Let me try.
> 
> The subject should reflect the last message in the thread, just like the
> timestamp. Therefore:
> 
> 1. last message is a SMS
> -> thread.subject is null
> 
> 2. last message is a MMS without a subject
> -> thread.subject is null
> 
> 3. last message is a MMS with a subject
> -> thread.subject = lastMessage.subject
> 
> 
> Generally, this is it: thread.subject = lastMessage.subject.
> 
> Ayman, can you confirm this (so that Gecko dev can move forward) ?

I agree, this seems a pragmatic approach to me.

Comment 12

[Gene Lian (I already quit Mozilla)]

Comment on attachment 770655 [details] [diff] [review]
part 1/2: interface changes

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

::: dom/mobilemessage/interfaces/nsIDOMMozMobileMessageThread.idl
@@ +10,5 @@
>    // Unique identity of the thread.
>    readonly attribute unsigned long long id;
>  
> +  // Message subject.
> +  readonly attribute DOMString          subject;

I'd prefer s/subject/lastMessageSubject, just like lastMessageType.

Comment 13

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

Attachment: patch 1/3: interfaces

s/subject/lastMessageSubject/

Comment 14

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

Attachment: patch 2/3: dom & backend changes : WIP

Test cases to come.

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 812984 [details] [diff] [review]
patch 2/3: dom & backend changes : WIP

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

Simple enough to directly give review+. :) But I think there might be some tests to fix.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +814,5 @@
> +      // actually means 'threadRecord.body'.  Swap the two values first.
> +      threadRecord.body = threadRecord.subject;
> +      delete threadRecord.subject;
> +
> +      if (threadRecord.lastMessageType != "mms") {

Add one simple comment before this line:

// Only MMS supports subject so assign null for non-MMS one.

or something you like.

@@ +825,5 @@
> +
> +      messageStore.get(threadRecord.lastMessageId).onsuccess = function(event) {
> +        let messageRecord = event.target.result;
> +        let subject = messageRecord.headers.subject;
> +        threadRecord.lastMessageSubject = subject || null;

This will convert empty string to null but should be fine... I assume you're hoping to save null as default for the thread DB.

@@ +1184,4 @@
>            let needsUpdate = false;
>  
>            if (threadRecord.lastTimestamp <= timestamp) {
> +            let messageSubject;

We might not have to use an extra variable by writing it to:

threadRecord.lastMessageSubject = (aMessageRecord.type == "mms")
                                ? aMessageRecord.headers.subject : null;

-----
After some thoughts... Maybe you're intentionally doing this to deal with empty string. OK! Your codes are good to go.

Comment 16

[Gene Lian (I already quit Mozilla)]

Ping a bit?

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

blocking a committed 1.3 user story. 1.3+

Comment 18

[Gene Lian (I already quit Mozilla)]

This is committed feature. Need to move fast. Can we finish this in a couple of days? Since we only lack test cases.

Comment 19

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

Attachment: patch 1/3: interfaces : v2

Re-gen from HG.

Comment 20

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

Attachment: patch 2/3: dom & backend changes : v2

Re-gen from HG and address comment 15.

Comment 21

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

Attachment: part 3/3: test cases

Test cases for thread subject.  Full try: https://tbpl.mozilla.org/?tree=Try&rev=f91bbb61b069

Comment 22

[Gene Lian (I already quit Mozilla)]

Comment on attachment 824617 [details] [diff] [review]
part 3/3: test cases

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

::: dom/mobilemessage/tests/marionette/head.js
@@ +27,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function sendSingleSmsWithSuccess(aReceiver, aText) {

How about s/sendSingleSmsWithSuccess/sendSmsWithSuccess/?

It's not very clear to me what does "Single" mean.

@@ +44,5 @@
> +
> +function sendMmsWithFailure(aMmsParameters) {
> +  let deferred = Promise.defer();
> +
> +  manager.onfailed = function (event) {

Looks awesome!

The only thing I don't understand is |manager.onfailed|. Where does the "onfailed" come from?

::: dom/mobilemessage/tests/marionette/test_thread_subject.js
@@ +6,5 @@
> +
> +const PHONE_NUMBER = "[phone]";
> +
> +// Have a long long subject causes the send fails, so we don't need
> +// networking here.

// What we want to check is the subject of the last message in a thread.
// We don't really need to send out a real MMS. To do so, we intentionally
// send an MMS with a long long subject, which will cause the send fails,
// so we don't need real networking here.

@@ +8,5 @@
> +
> +// Have a long long subject causes the send fails, so we don't need
> +// networking here.
> +const MMS_MAX_LENGTH_SUBJECT = 40;
> +const SUBJECT =

Please add one blank line above this var.

@@ +10,5 @@
> +// networking here.
> +const MMS_MAX_LENGTH_SUBJECT = 40;
> +const SUBJECT =
> +  "Hello" + (new Array(MMS_MAX_LENGTH_SUBJECT).join(" ")) + "World!";
> +const MMS_PARAMETER = {

Please add one blank line above this var.

@@ +21,5 @@
> +  let deferred = Promise.defer();
> +
> +  log("Testing thread subject: " + aProgressStr);
> +
> +  sendSingleSmsWithSuccess(PHONE_NUMBER, "text")

s/sendSingleSmsWithSuccess/sendSmsWithSuccess/?

@@ +23,5 @@
> +  log("Testing thread subject: " + aProgressStr);
> +
> +  sendSingleSmsWithSuccess(PHONE_NUMBER, "text")
> +    .then(function(message) {
> +      log("  SMS sent, retrieving thread of id " + message.threadId);

s/thread of id/thread by id/

@@ +44,5 @@
> +
> +  // We use a long long message subject so it will always fail.
> +  sendMmsWithFailure(MMS_PARAMETER)
> +    .then(function(message) {
> +      log("  MMS sent, retrieving thread of id " + message.threadId);

Ditto.

Comment 23

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #22)
> part 3/3: test cases
> Review of attachment 824617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/tests/marionette/head.js
> @@ +27,5 @@
> > +function sendSingleSmsWithSuccess(aReceiver, aText) {
> 
> How about s/sendSingleSmsWithSuccess/sendSmsWithSuccess/?
> It's not very clear to me what does "Single" mean.

I was to have a sendSingleSmsWithSuccess, a sendMultipleSmsWithSuccess, and a sendSmsWithSuccess that calls one of previous two depending on given parameter.  But, yes, they're removed for now because they're not referenced yet.

> @@ +44,5 @@
> > +
> > +function sendMmsWithFailure(aMmsParameters) {
> > +  let deferred = Promise.defer();
> > +
> > +  manager.onfailed = function (event) {
> 
> Looks awesome!
> 
> The only thing I don't understand is |manager.onfailed|. Where does the
> "onfailed" come from?

http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl#47
In DOMRequest.onerror events, we have only a DOMError and no the sent message object.  In order to keep the symmetry between send{Mms,Sms}WithSuccess and send{Mms,Sms}WithFailure, I listen to the onfailed event of navigator.mozMobileMessage to get that sent message object as well.

Comment 24

[Gene Lian (I already quit Mozilla)]

Oh! We forgot we have onfailed event handler. Cool!

Comment 25

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

Attachment: part 3/3: test cases : v2

Sorry, this revision changes a lot so I think another review is necessary.

1) Address previous comment, s/sendSingleSmsWithSuccess/sendSmsWithSuccess/
2) Add comments for each convenient function in head.js for future reference.
3) Ensure it really works with MOZ_B2G_RIL disabled, that is, |manager| could be undefined.  We also need a reject handler at the end.
4) Refactor getThreadById, deleteMessagesById, deleteMessages, and deleteAllMessages.  Only create new Promise whenever necessary.
5) Remove sendSmsToEmulator.  Yes, we will certainly need it when we convert other SMS/MMS test scripts to share some code, but it's not today.

Comment 26

[Gene Lian (I already quit Mozilla)]

Comment on attachment 826289 [details] [diff] [review]
part 3/3: test cases : v2

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

Nice patch!!!

::: dom/mobilemessage/tests/marionette/head.js
@@ +75,5 @@
> +/* Send a MMS message with specified parameters.  Resolve if it fails, reject
> + * otherwise.
> + *
> + * Forfill params:
> + *   error -- a DOMError.

Isn't that an MmsMessage?

@@ +86,5 @@
> + */
> +function sendMmsWithFailure(aMmsParameters) {
> +  let deferred = Promise.defer();
> +
> +  manager.onfailed = function (event) {

nit: drop the blank after "function".

::: dom/mobilemessage/tests/marionette/test_thread_subject.js
@@ +52,5 @@
> +    .then(testSms.bind(null, "SMS..MMS..SMS"))
> +    .then(deleteAllMessages)
> +    .then(testMms.bind(null, "MMS"))
> +    .then(testSms.bind(null, "MMS..SMS"))
> +    .then(testMms.bind(null, "MMS..SMS..MMS"));

Could you please add one more case for sending consecutive MMS (maybe, 2)? So that we can make sure the last MMS subject can be updated without the interference of SMS. Thank you!

Comment 27

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #26)
> part 3/3: test cases : v2
> Review of attachment 826289 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/mobilemessage/tests/marionette/head.js
> @@ +75,5 @@
> > +/* Send a MMS message with specified parameters.  Resolve if it fails, reject
> > + * otherwise.
> > + *
> > + * Forfill params:
> > + *   error -- a DOMError.
> 
> Isn't that an MmsMessage?

You're right.  The comment has to be updated.  Thanks!

Comment 28

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

Attachment: part 3/3: test cases : v3

address review comment 26:
1. update documentation for |sendMmsWithFailure()|
2. s/function (/function(/
3. add test cases for three continuous SMS messages with different text strings and for three continuous MMS messages with different subjects.

Comment 29

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

https://hg.mozilla.org/integration/b2g-inbound/rev/987aa0b99490
https://hg.mozilla.org/integration/b2g-inbound/rev/b572df9962d3
https://hg.mozilla.org/integration/b2g-inbound/rev/5b389397300d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/987aa0b99490
https://hg.mozilla.org/mozilla-central/rev/b572df9962d3
https://hg.mozilla.org/mozilla-central/rev/5b389397300d

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 824616 [details] [diff] [review]
patch 2/3: dom & backend changes : v2

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +811,5 @@
> +
> +      let threadRecord = cursor.value;
> +      // We have defined 'threadRecord.subject' in upgradeSchema7(), but it
> +      // actually means 'threadRecord.body'.  Swap the two values first.
> +      threadRecord.body = threadRecord.subject;

I don't get this: didn't have a body previously already? I think this overwrites any body that was here before.

As a matter of fact, we use a "body" in the SMS app and old databases get "undefined" now with the current code.

I'm filing a bug about this.