Closed
Bug 885679
Opened 12 years ago
Closed 11 years ago
B2G MMS: Add 'subject' to {Thread} object.
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox28 | --- | fixed |
People
(Reporter: borjasalguero, Assigned: vicamo)
References
Details
(Keywords: dev-doc-needed, Whiteboard: MMS_TEF, [u=commsapps-user c=messaging p=0] [FT:RIL])
Attachments
(3 files, 6 obsolete files)
(deleted),
patch
|
vicamo
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
For showing in the thread-list of MMS App the subject we need to have this info available in 'Thread' object when requesting 'getThreads'.
Reporter | ||
Updated•12 years ago
|
blocking-b2g: --- → leo?
Whiteboard: MMS_TEF
Reporter | ||
Updated•12 years ago
|
Whiteboard: MMS_TEF → MMS_TEF, TaipeiWW
Assignee | ||
Updated•12 years ago
|
Summary: [B2G] Add 'subject' to {Thread} object. → B2G MMS: Add 'subject' to {Thread} object.
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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.
Flags: needinfo?(dietrich)
Comment 3•11 years ago
|
||
(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?
Reporter | ||
Comment 4•11 years ago
|
||
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!
Flags: needinfo?(fbsc)
Updated•11 years ago
|
blocking-b2g: leo? → koi?
Whiteboard: MMS_TEF, TaipeiWW → MMS_TEF
Updated•11 years ago
|
Assignee: gene.lian → vyang
Assignee | ||
Comment 5•11 years ago
|
||
Assignee | ||
Comment 6•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: MMS_TEF → MMS_TEF, [u=commsapps-user c=messaging p=0]
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #7)
> Vicamo, who do you need that info from? UX?
Let's ask the reporter.
Flags: needinfo?(fbsc)
Reporter | ||
Comment 9•11 years ago
|
||
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 :)
Flags: needinfo?(fbsc)
Comment 10•11 years ago
|
||
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) ?
Flags: needinfo?(aymanmaat)
Comment 11•11 years ago
|
||
(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.
Flags: needinfo?(aymanmaat)
Comment 12•11 years ago
|
||
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.
Updated•11 years ago
|
blocking-b2g: koi? → 1.3?
Updated•11 years ago
|
Blocks: feature-mms-subject
Assignee | ||
Comment 13•11 years ago
|
||
s/subject/lastMessageSubject/
Attachment #770655 -
Attachment is obsolete: true
Attachment #812983 -
Flags: superreview?(gene.lian)
Assignee | ||
Comment 14•11 years ago
|
||
Test cases to come.
Attachment #770656 -
Attachment is obsolete: true
Attachment #812984 -
Flags: feedback?(gene.lian)
Updated•11 years ago
|
Attachment #812983 -
Flags: superreview?(gene.lian) → superreview+
Comment 15•11 years ago
|
||
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.
Attachment #812984 -
Flags: feedback?(gene.lian) → review+
Comment 16•11 years ago
|
||
Ping a bit?
Updated•11 years ago
|
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Comment 18•11 years ago
|
||
This is committed feature. Need to move fast. Can we finish this in a couple of days? Since we only lack test cases.
Flags: needinfo?(vyang)
Updated•11 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 19•11 years ago
|
||
Re-gen from HG.
Attachment #812983 -
Attachment is obsolete: true
Attachment #824614 -
Flags: superreview+
Flags: needinfo?(vyang)
Assignee | ||
Comment 20•11 years ago
|
||
Re-gen from HG and address comment 15.
Attachment #812984 -
Attachment is obsolete: true
Attachment #824616 -
Flags: review+
Assignee | ||
Comment 21•11 years ago
|
||
Test cases for thread subject. Full try: https://tbpl.mozilla.org/?tree=Try&rev=f91bbb61b069
Attachment #824617 -
Flags: review?(gene.lian)
Comment 22•11 years ago
|
||
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 = "+1234567890";
> +
> +// 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.
Attachment #824617 -
Flags: review?(gene.lian) → review+
Assignee | ||
Comment 23•11 years ago
|
||
(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•11 years ago
|
||
Oh! We forgot we have onfailed event handler. Cool!
Assignee | ||
Comment 25•11 years ago
|
||
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.
Attachment #824617 -
Attachment is obsolete: true
Attachment #826289 -
Flags: review?(gene.lian)
Updated•11 years ago
|
Whiteboard: MMS_TEF, [u=commsapps-user c=messaging p=0] → MMS_TEF, [u=commsapps-user c=messaging p=0] [FT:RIL]
Comment 26•11 years ago
|
||
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!
Attachment #826289 -
Flags: review?(gene.lian) → review+
Assignee | ||
Comment 27•11 years ago
|
||
(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!
Assignee | ||
Comment 28•11 years ago
|
||
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.
Attachment #826289 -
Attachment is obsolete: true
Attachment #827975 -
Flags: review+
Assignee | ||
Comment 29•11 years ago
|
||
Comment 30•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/987aa0b99490
https://hg.mozilla.org/mozilla-central/rev/b572df9962d3
https://hg.mozilla.org/mozilla-central/rev/5b389397300d
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 31•11 years ago
|
||
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.
Updated•11 years ago
|
status-firefox28:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•