Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Description • 10 years ago

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.

	Vicamo Yang [:vicamo][:vyang]
	Comment 2 • 10 years ago

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)
      };
    }));

	Vicamo Yang [:vicamo][:vyang]
	Comment 3 • 10 years ago

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.

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 4 • 10 years ago

(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.

	Jason Smith [:jsmith]
	Comment 5 • 10 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 6 • 10 years ago

Bevis, can you explain what you need here to move forward?

Thanks :)

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 7 • 10 years ago

(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.

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 8 • 10 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 9 • 10 years ago

(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.

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 10 • 10 years ago

[Tracking Requested - why for this release]:

	Julien Wajsberg [:julienw]
	Comment 11 • 10 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 12 • 10 years ago

(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;

	Julien Wajsberg [:julienw]
	Comment 13 • 10 years ago

(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 ?

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 14 • 10 years ago

(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. :)

	Julien Wajsberg [:julienw]
	Comment 15 • 10 years ago

(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.

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 16 • 10 years ago

(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.

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 17 • 10 years ago

(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."
> 
>

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 18 • 10 years ago

(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).

	Bevis Tseng[:bevis][:btseng](Exited) Reporter
	Comment 19 • 9 years ago

no longer following up this bug.

	BMO Automation
	Comment 20 • 7 years ago

Firefox OS is not being worked on