Closed Bug 1020209 Opened 6 years ago Closed 2 years ago

[B2G][MMS] Grouping MMS is not working if we enable it.

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: bevis, Unassigned)

References

Details

(Whiteboard: groupmms)

Attachments

(1 file, 1 obsolete file)

Attached patch mms_grouping.patch (obsolete) — Splinter Review
The code looks out-of-date and not working while reviewing the implementation of
MMS grouping to answer the question raised in comment#5 of bug 1018611.

Upload a WIP patch here.
We can include this fix once MMS Grouping is enabled.
Attachment #8434031 - Attachment is obsolete: true
Comment on attachment 8434044 [details] [diff] [review]
Patch v1: Use Array.prototype.push() to append receivers into Thread Participants.

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +2473,2 @@
>        });
> +    });

Sender information is dropped then.  This should be:

  threadParticipants =
    threadParticipants.concat(slicedReceivers.map(function(aAddress) {
      return {
        address: aAddress,
        type: MMS.Address.resolveType(aAddress)
      };
    }));
Attachment #8434044 - Flags: review?(vyang)
Comment on attachment 8434044 [details] [diff] [review]
Patch v1: Use Array.prototype.push() to append receivers into Thread Participants.

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +2473,2 @@
>        });
> +    });

Looks like I misunderstood you.  Sorry.
Attachment #8434044 - Flags: review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> Comment on attachment 8434044 [details] [diff] [review]
> Patch v1: Use Array.prototype.push() to append receivers into Thread
> Participants.
> 
> Review of attachment 8434044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
> @@ +2473,2 @@
> >        });
> > +    });
> 
> Looks like I misunderstood you.  Sorry.

Never mind. :)

As discussed offline, a test case for mms grouping is also needed before landing this fix.
Since the priority of mms grouping is low currently, I'd like to revisit this later.

Thanks again.
Note - testcase-wanted is used to track a need for a reduced test case to demonstrate a bug. It's not used for automation test tracking. If you want to request a test to be included, then flag in-testsuite? in the flags on the bug.
Keywords: testcase-wanted
Blocks: b2g-mms
Bevis, can you explain what you need here to move forward?

Thanks :)
Flags: needinfo?(btseng)
(In reply to Julien Wajsberg [:julienw] from comment #6)
> Bevis, can you explain what you need here to move forward?
> 
> Thanks :)

Hi Julien,

In current implementation, we directly rely on the availability of the MSISDN from USIM or MDN from CSIM to make this works properly.

However, these 2 fields are optional in USIM/CSIM.
Hence, somehow, we need another meta info to decide whether this function shall be enabled or not.
For example,
1. There will be 2 settings from Gaia:
   a. "Enabled Grouping MMS"
   b. "Sender's Phone Number for Grouping MMS"
2. Multi-SIM shall be taken into consideration for the settings mentioned above.
   - An array of booleans in Setting a) and
   - An array of strings (valid phone numbers) in Setting b).
3. By default, setting a) is set to Off and Setting b) is empty.
4. If user wants to enable it, check if MSISDN/MDN is available:
   - If MSISDN/MDN is available, set it into Setting b) and enable Setting a).
   - If MSISDN/MDN is not available, ask user to input a valid phone number:
     - If the provided number is valid, write this valid number into Setting b) and enable Setting a).
     - Otherwise, keep setting a) and setting b) unchanged as default values.
5. When user 'replaces' the SIM card in specific slot with 'another SIM' (take slot#1 as example):
   - We shall reset setting a) and b) of slot#1 to default.
   - That is to disable setting a) and to clear the number in setting b) of slot#1.

Then, in gecko, we could rely on this two settings to re-implement grouping mms accordingly.

Hence, we might need more input from UX/Gaia to lock down these, then we can move forward to support this when needed.
Flags: needinfo?(btseng)
Add some finding from AOSP for reference:
1. recipients in TO/CC will be taking into account if grouping mms is enabled. [1]
2. Only 2 types of PDU has to be filtered, both of them are incoming MMS to be received [1]:
   - MESSAGE_TYPE_NOTIFICATION_IND
   - MESSAGE_TYPE_RETRIEVE_CONF
3. In PduPersister.loadRecipients(), 'myNumber' will be filtered out only if it's available from the inserted UICC. [2]

[1] http://androidxref.com/5.1.0_r1/xref/frameworks/opt/telephony/src/java/com/google/android/mms/pdu/PduPersister.java#1355
[2] http://androidxref.com/5.1.0_r1/xref/frameworks/opt/telephony/src/java/com/google/android/mms/pdu/PduPersister.java#1470
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #7)
> For example,
> 1. There will be 2 settings from Gaia:
>    a. "Enabled Grouping MMS"
>    b. "Sender's Phone Number for Grouping MMS"
> 2. Multi-SIM shall be taken into consideration for the settings mentioned
> above.
>    - An array of booleans in Setting a) and
>    - An array of strings (valid phone numbers) in Setting b).
In addition, I think we should change Setting b) as an array of { myNumber: xxxx, iccId: yyyyy }.
Then, we can ensure that this myNumber won't be applied to the other inserted UICC but we still need to ensure this will be reset if inserted UICC is changed.
[Tracking Requested - why for this release]:
Bevis, I wonder if you can leave the "myNumber" filtering to Gaia at all?

Ferjm told us about the MobileID API [1] which has been developed for Loop but that seems to fit perfectly our issue here. But I don't know if you can use it from Gecko.

Therefore maybe it's best if Gecko does not filter at all myNumber and create threads using recipients + sender without filtering.

What do you think?

[1] https://wiki.mozilla.org/WebAPI/MobileIdentity
Flags: needinfo?(btseng)
(In reply to Julien Wajsberg [:julienw] (PTO -> Apr 27) from comment #11)
> Bevis, I wonder if you can leave the "myNumber" filtering to Gaia at all?
> Ferjm told us about the MobileID API [1] which has been developed for Loop
> but that seems to fit perfectly our issue here. But I don't know if you can
> use it from Gecko.
> 
> Therefore maybe it's best if Gecko does not filter at all myNumber and
> create threads using recipients + sender without filtering.
Sorry to say that I can't understand clearly how to grouping mms messages works without filtering myNumber out that you mentioned here because the originator's address will also be presented in "To"/"Cc" of the MMS header in the replied MMS message and all the addresses in "To/Cc" are recipients of this MMS message.

Our difficulty here is not about how to get the MSISDN/MDN from SIM for MMS because IccService in RIL is handy for us and we already apply it for adding MSISDN/MDN into "From" in header if available. [1]

The only difficulty is that when there is no MSISDN/MDN, what number shall we use to identify the originator address in the replied MMS message.
In this case, we must get some input from user for the valid number if we want to enable grouping MMS.

For MobileIdentifyManager, as I know, it also relies on the MSIDN/MDN retrieved from IccService as refactored by me recently in bug 1155142 for authentication.
If not available, it will prompt a dialogue to ask user to input a valid number as well.

[1] https://hg.mozilla.org/mozilla-central/file/11077895df62/dom/mobilemessage/gonk/MmsService.js#l1196

> What do you think?
> 
> [1] https://wiki.mozilla.org/WebAPI/MobileIdentity

Regarding how to access msisdn/mdn from Gaia, I think it shall be easy to create new API from gaia/shared/js/simslot.js because it always holds an MozIcc object.
It's handy to get the msisdn/mdn from iccObj.iccInfo by 
> var myPhoneNumber = iccObj.iccInfo.msisdn || iccObj.iccInfo.mdn || null;
Flags: needinfo?(btseng)
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #12)
> (In reply to Julien Wajsberg [:julienw] (PTO -> Apr 27) from comment #11)
> > Bevis, I wonder if you can leave the "myNumber" filtering to Gaia at all?
> > Ferjm told us about the MobileID API [1] which has been developed for Loop
> > but that seems to fit perfectly our issue here. But I don't know if you can
> > use it from Gecko.
> > 
> > Therefore maybe it's best if Gecko does not filter at all myNumber and
> > create threads using recipients + sender without filtering.
> Sorry to say that I can't understand clearly how to grouping mms messages
> works without filtering myNumber out that you mentioned here because the
> originator's address will also be presented in "To"/"Cc" of the MMS header
> in the replied MMS message and all the addresses in "To/Cc" are recipients
> of this MMS message.
> 
> Our difficulty here is not about how to get the MSISDN/MDN from SIM for MMS
> because IccService in RIL is handy for us and we already apply it for adding
> MSISDN/MDN into "From" in header if available. [1]
> 
> The only difficulty is that when there is no MSISDN/MDN, what number shall
> we use to identify the originator address in the replied MMS message.
> In this case, we must get some input from user for the valid number if we
> want to enable grouping MMS.


I understand what is the difficulty, that's why I propose to remove this filtering.
Why do we need the filtering to happen at all in Gecko?

My idea is that this filtering could be done in Gaia so that we control in a better way when the phone number is asked to the user, in case it's not present in MSISDN/MDN.

> For MobileIdentifyManager, as I know, it also relies on the MSIDN/MDN
> retrieved from IccService as refactored by me recently in bug 1155142 for
> authentication.
> If not available, it will prompt a dialogue to ask user to input a valid
> number as well.

Yes :)

> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/11077895df62/dom/mobilemessage/
> gonk/MmsService.js#l1196
> 
> > What do you think?
> > 
> > [1] https://wiki.mozilla.org/WebAPI/MobileIdentity
> 
> Regarding how to access msisdn/mdn from Gaia, I think it shall be easy to
> create new API from gaia/shared/js/simslot.js because it always holds an
> MozIcc object.

I didn't know about simslot but I don't really understand the purpose of this; it looks like it's a wrapper on a Icc object that redispatch any received event to window, but I don't think we need this in the SMS app.

> It's handy to get the msisdn/mdn from iccObj.iccInfo by 
> > var myPhoneNumber = iccObj.iccInfo.msisdn || iccObj.iccInfo.mdn || null;

I don't get why we need a new API if we use MobileID ?
(In reply to Julien Wajsberg [:julienw] (PTO May 1st) from comment #13)
> I understand what is the difficulty, that's why I propose to remove this
> filtering.
> Why do we need the filtering to happen at all in Gecko?
> 
> My idea is that this filtering could be done in Gaia so that we control in a
> better way when the phone number is asked to the user, in case it's not
> present in MSISDN/MDN.
> 

The reason is that, in current design, the only way to filter all the messages from a selected thread is the saved "threadId" returned by MobileMessageManager.getThreads().

Hence, we have to identify the correct threadId of the received message before saving it into MobileMessageDB.

Otherwise, I don't know how messages of the same thread to be filtered to Gaia efficiently. :(

Or, do you mean that there will be no getThreads() method from gecko any more and all the threading will be handled by gaia instead?
Even though, Gaia still have to identify it when receiving.
Otherwise, the threading will be varied all the time if the inserted UICC is changed.
This sounds weird to me. :(

> 
> > It's handy to get the msisdn/mdn from iccObj.iccInfo by 
> > > var myPhoneNumber = iccObj.iccInfo.msisdn || iccObj.iccInfo.mdn || null;
> 
> I don't get why we need a new API if we use MobileID ?

Ok, I see your point that you would like to rely on MobileId to identify what the valid number of each slot is.
Be aware that addition verification flow to the network is required and this should be informed to the device user. :)
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> (In reply to Julien Wajsberg [:julienw] (PTO May 1st) from comment #13)
> > I understand what is the difficulty, that's why I propose to remove this
> > filtering.
> > Why do we need the filtering to happen at all in Gecko?
> > 
> > My idea is that this filtering could be done in Gaia so that we control in a
> > better way when the phone number is asked to the user, in case it's not
> > present in MSISDN/MDN.
> > 
> 
> The reason is that, in current design, the only way to filter all the
> messages from a selected thread is the saved "threadId" returned by
> MobileMessageManager.getThreads().
> 
> Hence, we have to identify the correct threadId of the received message
> before saving it into MobileMessageDB.
> 
> Otherwise, I don't know how messages of the same thread to be filtered to
> Gaia efficiently. :(
> 
> Or, do you mean that there will be no getThreads() method from gecko any
> more and all the threading will be handled by gaia instead?
> Even though, Gaia still have to identify it when receiving.
> Otherwise, the threading will be varied all the time if the inserted UICC is
> changed.
> This sounds weird to me. :(

I feel like we have trouble understanding each other :)

(tell me I'm wrong, I haven't checked the code)
A thread is uniquely identified by:
* for SMS: the normalized sender for received SMS, OR normalized recipient for sent SMS.
* for MMS:
** currently (?): normalized sender for received MMS, OR list of normalized recipients for sent MMS.
** what we need: the list of normalized recipients + normalized sender -- both for received and sent MMS.

Lists are sorted and only unique recipients are kept.


So I don't see where you want to filter the MSISDN here. The more I think of it, the more I think that the filtering should never be done by Gecko, otherwise as you say "the threading will be varied all the time if the inserted UICC is changed."


> 
> > 
> > > It's handy to get the msisdn/mdn from iccObj.iccInfo by 
> > > > var myPhoneNumber = iccObj.iccInfo.msisdn || iccObj.iccInfo.mdn || null;
> > 
> > I don't get why we need a new API if we use MobileID ?
> 
> Ok, I see your point that you would like to rely on MobileId to identify
> what the valid number of each slot is.
> Be aware that addition verification flow to the network is required and this
> should be informed to the device user. :)

I haven't tried it yet but I think this works by receiving a SMS (and likely the Internet connection could use a WiFi connection). We'll need to try it out to properly see what is said to the user.
(In reply to Julien Wajsberg [:julienw] (PTO May 1st) from comment #15)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> > (In reply to Julien Wajsberg [:julienw] (PTO May 1st) from comment #13)
> > > I understand what is the difficulty, that's why I propose to remove this
> > > filtering.
> > > Why do we need the filtering to happen at all in Gecko?
> > > 
> > > My idea is that this filtering could be done in Gaia so that we control in a
> > > better way when the phone number is asked to the user, in case it's not
> > > present in MSISDN/MDN.
> > > 
> > 
> > The reason is that, in current design, the only way to filter all the
> > messages from a selected thread is the saved "threadId" returned by
> > MobileMessageManager.getThreads().
> > 
> > Hence, we have to identify the correct threadId of the received message
> > before saving it into MobileMessageDB.
> > 
> > Otherwise, I don't know how messages of the same thread to be filtered to
> > Gaia efficiently. :(
> > 
> > Or, do you mean that there will be no getThreads() method from gecko any
> > more and all the threading will be handled by gaia instead?
> > Even though, Gaia still have to identify it when receiving.
> > Otherwise, the threading will be varied all the time if the inserted UICC is
> > changed.
> > This sounds weird to me. :(
> 
> I feel like we have trouble understanding each other :)
> 
> (tell me I'm wrong, I haven't checked the code)
> A thread is uniquely identified by:
> * for SMS: the normalized sender for received SMS, OR normalized recipient
> for sent SMS.
> * for MMS:
> ** currently (?): normalized sender for received MMS, OR list of normalized
> recipients for sent MMS.
> ** what we need: the list of normalized recipients + normalized sender --
> both for received and sent MMS.
> 
> Lists are sorted and only unique recipients are kept.
> 
> 
> So I don't see where you want to filter the MSISDN here. The more I think of
> it, the more I think that the filtering should never be done by Gecko,
> otherwise as you say "the threading will be varied all the time if the
> inserted UICC is changed."
> 
I see what you are trying to say now.
I'll take some time to see how we can achieve this in current implementation and 
to see if there is any use case that wasn't taken into account.

set NI to myself to feedback this later.
> 
> > 
> > > 
> > > > It's handy to get the msisdn/mdn from iccObj.iccInfo by 
> > > > > var myPhoneNumber = iccObj.iccInfo.msisdn || iccObj.iccInfo.mdn || null;
> > > 
> > > I don't get why we need a new API if we use MobileID ?
> > 
> > Ok, I see your point that you would like to rely on MobileId to identify
> > what the valid number of each slot is.
> > Be aware that addition verification flow to the network is required and this
> > should be informed to the device user. :)
> 
> I haven't tried it yet but I think this works by receiving a SMS (and likely
> the Internet connection could use a WiFi connection). We'll need to try it
> out to properly see what is said to the user.
Flags: needinfo?(btseng)
(In reply to Julien Wajsberg [:julienw] (PTO May 1st) from comment #15)
> 
> I feel like we have trouble understanding each other :)
> 
> (tell me I'm wrong, I haven't checked the code)
> A thread is uniquely identified by:
> * for SMS: the normalized sender for received SMS, OR normalized recipient
> for sent SMS.
> * for MMS:
> ** currently (?): normalized sender for received MMS, OR list of normalized
> recipients for sent MMS.
> ** what we need: the list of normalized recipients + normalized sender --
> both for received and sent MMS.
> 
> Lists are sorted and only unique recipients are kept.
> 
To have consistent way for message grouping, not only for MMS, we also have to normalized sender + receiver of the sent/received SMS.

However, there is a critical issue here if we choose this approach:
We can not have sender address(MyNumber) of an outgoing MMS/MMS message and the receiver address(MyNumber) all the time when we would like to save the message into data base.

With this approach, unless got a valid phone number from the user, all the messaging function will be pending, even user don't want to enable grouping mms. :(

> 
> So I don't see where you want to filter the MSISDN here. The more I think of
> it, the more I think that the filtering should never be done by Gecko,
> otherwise as you say "the threading will be varied all the time if the
> inserted UICC is changed."
> 
>
Flags: needinfo?(btseng)
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #17)
> (In reply to Julien Wajsberg [:julienw] (PTO May 1st) from comment #15)
> > 
> > I feel like we have trouble understanding each other :)
> > 
> > (tell me I'm wrong, I haven't checked the code)
> > A thread is uniquely identified by:
> > * for SMS: the normalized sender for received SMS, OR normalized recipient
> > for sent SMS.
> > * for MMS:
> > ** currently (?): normalized sender for received MMS, OR list of normalized
> > recipients for sent MMS.
> > ** what we need: the list of normalized recipients + normalized sender --
> > both for received and sent MMS.
> > 
> > Lists are sorted and only unique recipients are kept.
> > 
> To have consistent way for message grouping, not only for MMS, we also have
> to normalized sender + receiver of the sent/received SMS.
> 
> However, there is a critical issue here if we choose this approach:
> We can not have sender address(MyNumber) of an outgoing MMS/MMS message and
> the receiver address(MyNumber) all the time when we would like to save the
> message into data base.
> 
> With this approach, unless got a valid phone number from the user, all the
> messaging function will be pending, even user don't want to enable grouping
> mms. :(
> 

Besides, there are other problems with this approach:
1. From API perspective, for other messaging app developers(if we'd like to expose MobileMessageManager to apps in the future), they have to handle the filter by themselves.
2. For the users who has multiple UICCs available, the behaviour becomes strange when sending/replying a grouped mms in the following scenario:
   - Device user sends a MMS with UICC#1 to A, B, C.
   - Device user receives a MMS replied from A to all participants.
   - These 2 messages will be grouped into the same thread with 4 participants(UICC#1, A, B, C).
   - When Device user restarts the device with UICC#1 replaced to UICC#2, and sends a MMS in the same thread, A new outgoing message & a new thread will be created with 5 participants(UICC#2, UICC#1, A, B, C).
   - When device user receives a MMS replied from B, this message will be grouped into 2nd thread with 5 participants.
   However, for this device user, all these messages are expected to be grouped into the same thread instead.

Due the the reasons mentioned above, I'd prefer to have the filtering done when saving (in gecko).
Depends on: 1200564
no longer following up this bug.
Assignee: btseng → nobody
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.