Closed Bug 865157 Opened 6 years ago Closed 6 years ago

B2G MMS: Receiving an MMS creates a redundant thread different from the one containing SMS (from the same sender)

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g leo+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: borjasalguero, Assigned: airpingu)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 file, 1 obsolete file)

STR:
- Receive a MMS from '+34612123123'

EXPECTED:
- 1 notification and One thread it's created with the MMS received from '+34612123123'

CURRENTLY:
- 2 notifications and 2 threads have been created with the MMS received from '+34612123123'
Assignee: nobody → vyang
Blocks: 845173
Does this duplicates bug 853752?
MMS always comes with a notification first, and we should store this notification into database so that you can retrieve it later in manual retrieval mode, so there must be two notifications.
In this case I've received one MMS twice (not a notification + MMS) :S
Just having a quick discussion with Steve. Bug 853752 only solves partial problem. This issue is happening when attempting to receive an SMS and an MMS continuously. Let me look into this.
Assignee: vyang → gene.lian
Blocks: b2g-mms
2 notifications is correct in current flow, I will discuss with UX about disabling first notification for automatic download mode. The issue in this bug should be focused on one more thread will be created after receiving a mms message, these 2 threads got different ID and contained same messages.
Re-describe the title based on the comment #5.

This bug blocks Bug 840055 which is a leo+ MMS user story (MMS Thread View). Nominating for leo+.
Blocks: 840055
blocking-b2g: --- → leo?
Summary: [MMS] Receiving a MMS creates 2 Threads, with different threads_id, with the same MMS. → B2G MMS: Receiving an MMS creates a redundant thread different from the one containing SMS (from the same sender)
blocking-b2g: leo? → leo+
Whiteboard: [NO_UPLIFT]
blocking a leo+ user story, leo+
Attached patch Patch (obsolete) — Splinter Review
Hi Mounir,

This patch touches DOM change. Need your super review. The MMS notification indication doesn't carry any receivers. Originally, we should add our phone number to receivers. However, some SIM cards don't allow us to retrieve the phone number info. Under this circumstance, we still hope to add a NULL receiver to let the Gaia know the receiver is not available (the current SMS codes do the same thing). To do so, we need to loose the restriction of checking string type. Thanks!
Attachment #742335 - Flags: superreview?(mounir)
Attachment #742335 - Flags: review?(vyang)
Doesn't seem like this bug touches any interface, at least not the current patch. Okay to uplift.
Whiteboard: [NO_UPLIFT]
Comment on attachment 742335 [details] [diff] [review]
Patch

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

r- because I do not understand why you seem to be only reading the last value of the array. I am likely missing something though.

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +175,3 @@
>        return NS_ERROR_INVALID_ARG;
>      }
> +    if (receiverJsVal.isString()) {

I'm not sure I understand what you are doing here. Isn't receiverJsVal the last value of the array?

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1008,5 @@
>        // phone number, thus setting the SMS' receiver to be null.
>        aMessage.receiver = self;
>      } else if (aMessage.type == "mms") {
>        let receivers = aMessage.receivers;
>        if (!receivers.length) {

nit: I prefer explicit == 0.
Attachment #742335 - Flags: superreview?(mounir) → superreview-
Comment on attachment 742335 [details] [diff] [review]
Patch

Keeping the request open is probably better actually.
Attachment #742335 - Flags: superreview- → superreview?(mounir)
Flags: needinfo?(gene.lian)
Comment on attachment 742335 [details] [diff] [review]
Patch

I'll answer Mounir's question and refine the patch later.
Attachment #742335 - Flags: superreview?(mounir)
Attachment #742335 - Flags: review?(vyang)
Flags: needinfo?(gene.lian)
(In reply to Mounir Lamouri (:mounir) from comment #10)
> ::: dom/mobilemessage/src/MmsMessage.cpp
> @@ +175,3 @@
> >        return NS_ERROR_INVALID_ARG;
> >      }
> > +    if (receiverJsVal.isString()) {
> 
> I'm not sure I understand what you are doing here. Isn't receiverJsVal the
> last value of the array?

No, the receiverJsVal is for each element value in this array, not just the last value. Anyway, I'm going to refine the patch with another different thought. ;)
Attached patch Patch, V2Splinter Review
Hi reviewers,

After some thoughts, I decided to loose the restriction of checking empty receivers for some reasons:

1. The MMS notification indication doesn't carry any receivers. This kind of MMS message should have no receivers in nature so I decided to expose an empty array to the Gaia end as it is, which sounds more consistent with the MMS spec. The Gaia end can recognize this kind of MMS message with |.delivery == "not-downloaded"|.

2. I originally thought to add the user's phone number into the receivers array. However, sometimes the phone number is not available for some SIM cards, which means we might need to return something like [""] or [null]. After some thoughts, I think this is not a very formal way to tell Gaia site the lack of receiver is due to a SIM card issue (or not) from the API point of view.

So let's directly return an empty array for this kind of |.delivery == "not-downloaded"| MMS message. After retrieving MMS from server later, the receivers array must not be empty for a |.delivery == "received"| MMS message.
Attachment #742335 - Attachment is obsolete: true
Attachment #742937 - Flags: superreview?(mounir)
Attachment #742937 - Flags: review?(vyang)
Comment on attachment 742937 [details] [diff] [review]
Patch, V2

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

As defined in MMS-ENC, "from" attribute in M-Notification.ind PDU is optional, so indeed we could have empty receiver array at the time.
Attachment #742937 - Flags: review?(vyang) → review+
Attachment #742937 - Flags: superreview?(mounir) → superreview+
https://hg.mozilla.org/mozilla-central/rev/d28a41f49785
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Flags: in-moztrap?
Flags: in-moztrap?
You need to log in before you can comment on or make changes to this bug.