866938 - B2G MMS: Support email address in receiver field.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

We should support email address in sender field one day.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.0

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.1

Comment 3

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

Depends on bug 836542 for it touches database schema.  Hope we can have test cases for schema changes from now on.

Comment 4

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> Depends on bug 836542 for it touches database schema.  Hope we can have test
> cases for schema changes from now on.

Too much test cases to add in bug 836542.  Let's land this first.  Will continue the review, thank you.

Comment 5

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

Comment on attachment 803521 [details] [diff] [review]
bug-866938.patch v1.1

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ -605,5 @@
>        // retrieve the records out and insert its "senderOrReceiver" column as a
>        // new record in participantStore.
>        let number = mostRecentRecord.senderOrReceiver;
> -      self.findParticipantRecordByAddress(participantStore, number, true,
> -                                          function (participantRecord) {

Just call |findParticipantRecordByPlmnAddress|.

@@ +813,5 @@
> +  /**
> +   * Change receivers format to address and type.
> +   */
> +  upgradeSchema12: function upgradeSchema12(transaction, next) {
> +    let messageStore = transaction.objectStore(MESSAGE_STORE_NAME);

We have to correct threadId of existing MMS message records as well.

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.2

Comment 7

[lecky]

hi,ctai


sending mms to email address will be supported on V1.3?right?

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

We will try to support it on V1.3. But still need PM to approve it. :)

Comment 9

[Gene Lian (I already quit Mozilla)]

Hi Wilfred, please help mark this as V1.3+. This one is a V1.3 bug for sure.

Comment 11

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Hi, Wilfred,
This bug is for Gecko part. I reopen it for Gecko related patch landing.

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.3

Comment 13

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

blocking a committed 1.3 user story. 1.3+

Comment 14

[Ken Chang[:kenkai]]

Set 11/22 as the milestone. If you think it isn't reasonable, please reassign it.

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.4

Comment 16

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.5

Fix nit.

Comment 17

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

Comment on attachment 825210 [details] [diff] [review]
bug-866938.patch v1.5

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1766,5 @@
>              isAddrValid = false;
>            }
>            if (DEBUG) debug("createSavableFromParams: normalize phone number " +
>                             "from " + receiver + " to " + address);
> +        } else if(type == "Others") {

nit: if (

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1005,5 @@
> +
> +      let typedAddresses = [];
> +      let oldAddresses = [];
> +      let needCorrectThreadID = false;
> +      for (let receiver of messageRecord.receivers) {

But we could have email-typed sender address in a received MMS message, couldn't we?

@@ +1010,5 @@
> +        let addressType = MMS.Address.resolveType(receiver);
> +        typedAddresses.push({address: receiver,
> +                             type: addressType});
> +        oldAddresses.push({address: receiver,
> +                           type: "PLMN"});

We don't have to parse again.  Just check headers.from, headers.to, headers.cc.

@@ +1020,5 @@
> +      if (needCorrectThreadID) {
> +        let participantStore = transaction.objectStore(PARTICIPANT_STORE_NAME);
> +        let threadStore = transaction.objectStore(THREAD_STORE_NAME);
> +        let newParticipantIds;
> +        self.findParticipantIdsByTypedAddresses(participantStore, typedAddresses,

But invalid participant records are still left in the object store, so you'll certainly match the same participant record, and thus same thread record.

@@ -1097,5 @@
>                                                      expiryDate);
>      }
>    },
>  
> -  findParticipantRecordByAddress: function findParticipantRecordByAddress(

|findParticipantRecordByAddress| is called in upgradeSchema7.  So there must be a chunk for it but I can't find that chunk in this patch.

@@ +1308,5 @@
>          cursor.continue();
>        }).bind(this);
>      }).bind(this);
>    },
> +  findParticipantRecordByOtherAddress:

nit: empty line before this line

@@ +1433,1 @@
>    findParticipantIdsByAddresses: function findParticipantIdsByAddresses(

To be discussed with Gene.

@@ -1467,5 @@
>                  }
>                }
>              } else {
> -              let normReceiver = PhoneNumberUtils.normalize(receiver, false);
> -              if (!normReceiver) {

To be read later.  I think we can re-use matchPhoneNumbers/matchParsedPhoneNumbers.

@@ -1570,5 @@
>      }
>      let isSuccess = false;
>      let slicedReceivers = receivers.slice();
>      if (aMessage.msisdn) {
> -      let found = slicedReceivers.indexOf(aMessage.msisdn);

Please have a rebase to master tip.

@@ +1859,5 @@
>        return;
>      }
> +    let sender = {address: aMessage.sender,
> +                  type: MMS.Address.resolveType(aMessage.sender)};
> +    let threadParticipants = [sender];

nit: just embed it.

@@ +2464,5 @@
>        let participantStore = txn.objectStore(PARTICIPANT_STORE_NAME);
> +      let typedAddresses = [];
> +      for (let number of filter.numbers) {
> +        typedAddresses.push ({address: number,
> +                              type: MMS.Address.resolveType(receiver)});

s/receiver/number/

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Set the target milestone to sprint 5 due to vyang need more thoughts about this bug.

Comment 19

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 825210 [details] [diff] [review]
bug-866938.patch v1.5

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1010,5 @@
> +        let addressType = MMS.Address.resolveType(receiver);
> +        typedAddresses.push({address: receiver,
> +                             type: addressType});
> +        oldAddresses.push({address: receiver,
> +                           type: "PLMN"});

For my understanding, the information in headers.from, headers.to, headers.cc might be wrong.

Comment 20

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.6

Comment 21

[Rick Waldron [:rwaldron]]

Is there any update on this?

Comment 22

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Vyang want to take this bug. Assign to him.

Comment 23

[Rick Waldron [:rwaldron]]

Vicamo, is there an ETA on this?

Comment 24

[Gene Lian (I already quit Mozilla)]

Bug 840515 has been moved to V1.4. I think we're not really in a hurry to work on this but we're still moving on.

Joe, please correct me if I'm wrong. Thanks!

Comment 25

[Jack Liu]

Dears,

Is there still a opportunity to has this feature done in v1.3?

Comment 26

[Jason Smith [:jsmith]]

This is not a committed feature for 1.3, so we will not block on this.

Comment 27

[Rick Waldron [:rwaldron]]

Now that 1.4 is in gear, is anyone available to work on this?

Comment 28

[Gene Lian (I already quit Mozilla)]

Hi Vicamo, it seems this patch is under way. Are you available to finish this or just pass it to Bevis or someone else?

Comment 29

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

Bevis might not be available until March for personal affairs, and I think I might have no chance to finish this as well. :(

Comment 30

[Ken Chang[:kenkai]]

Joe, noone take this bug and Bevis takes PTOs now. I wonder if we could move this to 1.5.

Comment 31

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

For test cases, please see bug 940884, especially test_mmdb_new.js[1] and/or test_thread_subject.js[2].  Expecting two test scripts:

One for testing upgradeSchema<N>:

  startTestBase(function testCaseMain() {
    initMobileMessageDB(mmdb, <unique db name>, N - 1)
    // Insert records to mmdb "manually" (DONOT call APIs)
    closeMobileMessageDB(mmdb)

    initMobileMessageDB(mmdb, <unique db name>, 0)
    // Check schema upgrade has suceeded.
    closeMobileMessageDB(mmdb)
  }

Another for testing MMDB API changes:

  startTestCommon(function testCaseMain() {
    // 1. Sent MMS messages with one or more email-based recipients,
    // 2. Check those messages are correctly grouped.
  }

[1]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_mmdb_new.js
[2]: https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_thread_subject.js

Comment 32

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

Ken, from our discussion with Wayne, we can land this in 1.5 master

Comment 33

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.7

Rebased.

Comment 34

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-866938.patch v1.8

Fix some nits.

Comment 35

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

Comment on attachment 8375332 [details] [diff] [review]
bug-866938.patch v1.8

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1516,5 @@
>      for each (let type in ["cc", "to"]) {
>        if (intermediate.headers[type]) {
> +        let addressFields = intermediate.headers[type];
> +        if (addressFields instanceof Array) {
> +          for (let field of addressFields) {

Bug 969231 is to replace for-of loops with traditional index-based for-loop.

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +454,5 @@
>    /**
>     * Create the initial database schema.
>     *
> +   * TODO need to worry about number normalization somewhere... TODO full text
> +   * search on body???

nit: I think it's time to remove the two TODOs.

@@ +501,5 @@
>      /**
>       * This mostRecentStore can be used to quickly construct a thread view of
>       * the mobile message database. Each entry looks like this:
> +     *  { senderOrReceiver: <String> (primary key), id: <Number>, timestamp:
> +     * <Date>, body: <String>, unreadCount: <Number> }

nit: it seems your editor automatically re-formats these comments and results in these unnecessary changes.

@@ +622,5 @@
>      /**
>       * This "participant" object store keeps mappings of multiple phone numbers
>       * of the same recipient to an integer participant id. Each entry looks
>       * like:
> +     *  { id: <Number> (primary key), addresses: <Array of strings> }

ditto

@@ +635,5 @@
>       * ids of the participants of that message thread. Each entry looks like:
> +     *  { id: <Number> (primary key), participantIds: <Array of participant
> +     * IDs>, participantAddresses: <Array of the first addresses of the
> +     * participants>, lastMessageId: <Number>, lastTimestamp: <Date>, subject:
> +     * <String>, unreadCount: <Number> }

ditto

@@ +1733,5 @@
> +
> +      self.findParticipantRecordByTypedAddress(aParticipantStore,
> +          aTypedAddresses[index++],
> +          aCreate,
> +          function (participantRecord) {

nit: align to aParticipantStore

Comment 36

[Bevis Tseng[:bevis][:btseng](Exited)]

take this after dicussing with Chai-hung.

Comment 37

[Bevis Tseng[:bevis][:btseng](Exited)]

update assignee to follow up.

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 866938: B2G MMS: Support email address in receiver field (v1.9)

Changes to v1.8

  - index-based iteration over header fields
  - reverted ill-formated comments
  - removed TODOs

  - pass typed addresses (address + type) to saveRecords
  - pass only plain addresses to createMmsMessage

Comment 39

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: gaia.patch

Apply this patch to Gaia to disable the address validation of the Messaging app. Otherwise you won't be able to invoke 'Send'. To generate an MMS, attach a picture to the message.

Comment 40

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 866938: B2G MMS: Support email address in receiver field (v1.10)

Changes to v1.9

  - fixed name of original author in commit message

Comment 41

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

Comment on attachment 8379002 [details] [diff] [review]
[01] Bug 866938: B2G MMS: Support email address in receiver field (v1.10)

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +1370,5 @@
> +      if (needCorrectThreadID) {
> +        let participantStore = transaction.objectStore(PARTICIPANT_STORE_NAME);
> +        let threadStore = transaction.objectStore(THREAD_STORE_NAME);
> +        let newParticipantIds;
> +        self.findParticipantIdsByTypedAddresses(participantStore, typedAddresses,

The flow here is just not correct because you'll simply get the same participantRecord if that record has not been deleted before find-or-create a new one.

The flow in my head is:

  let affectedParticipantIds = [];
  let typedAddressCache = new Map();
  participantStore.openCursor().onsuccess = function(event) {
    let cursor = event.target.result;
    if (cursor) {
      let participantRecord = cursor.value;
      // 1) iterate through |participantRecord.addresses| to populate
      //    typedAddressCache
      // 2) if there is any entry not of PLMN type and
      //    |participantRecord.addresses.length| > 1, then push
      //    |participantRecord.id| into affectedParticipantIds.

      cursor.continue();
      return;
    }

    // Done traversing participant store.
    if (!affectedParticipantIds.length) {
      // Thank God!
      next();
      return;
    }

    // Sort for easier intersect later.
    affectedParticipantIds.sort();

    // Remove affected participant records first:
    for (let id of affectedParticipantIds) {
      participantStore.delete(id);
    }

    // Find out all threads affected as well:
    let affectedThreadIds = [];
    threadStore.openCursor().onsuccess = function(event) {
    let cursor = event.target.result;
      if (cursor) {
        let threadRecord = cursor.value;
        // If |threadRecord.participantIds| intersects with 
        // |affectedParticipantIds|, then:
        // 1) resolve all typed addresses of |threadRecord.participantAddresses|
        //    if not found in typedAddressCache (should not happen)
        // 2) findParticipantIdsByTypedAddresses()
        // 3) assign new participant id array to |threadRecord.participantIds|
        // 4) add |threadRecord.id| to affectedThreadIds

        cursor.next();
        return;
      }

      // All threads are corrected, go on to fix message records.
      for (let id of affectedThreadIds) {
        messageStore.index("threadId").openCursor().onsuccess = function() {
          // Fix |messageRecord.{sender,receiver,receivers}| and
          // |messageRecord.headers.to| if it's a outgoing MMS using
          // typedAddressCache.

          // Call next() when everything is done.
        };
      }
    };
  };

Comment 42

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #41)

This is replied at 2:00 AM, so probably needs more thoughts :X

Comment 43

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

> Review of attachment 8379002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
> @@ +1370,5 @@
> > +      if (needCorrectThreadID) {
> > +        let participantStore = transaction.objectStore(PARTICIPANT_STORE_NAME);
> > +        let threadStore = transaction.objectStore(THREAD_STORE_NAME);
> > +        let newParticipantIds;
> > +        self.findParticipantIdsByTypedAddresses(participantStore, typedAddresses,
> 
> The flow here is just not correct because you'll simply get the same
> participantRecord if that record has not been deleted before find-or-create
> a new one.

I think it is possible to get a different participantRecord if the new participant has the same address as the old one, but with a different type than 'PLMN'. I don't know if this can happen in practice, though.

Comment 44

[Thomas Zimmermann [:tzimmermann] [:tdz]]

I've been looking at MobileMessageDB.jsm for hours, but I find the code is just painful to read and understand. :( Is there at least some documentation available of how the DB's records look, and how they relate to each other?

Comment 45

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8379002 [details] [diff] [review]
[01] Bug 866938: B2G MMS: Support email address in receiver field (v1.10)

Hi Vicamo,

I'm not sure what to make of your review. Have you though about the issue and could clarify? Thanks.

Comment 46

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #43)
> I think it is possible to get a different participantRecord if the new
> participant has the same address as the old one, but with a different type
> than 'PLMN'. I don't know if this can happen in practice, though.

We don't have a new field "type" here.  It's not about some rare, corner cases.  It's a fundamental flow error that 1) IndexedDB can only do full match, 2) we use full match first to match all types of addresses, 3) we do not remove existing participant entries before calling "findParticipantBy...".

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #44)
> I've been looking at MobileMessageDB.jsm for hours, but I find the code is
> just painful to read and understand. :( Is there at least some documentation
> available of how the DB's records look, and how they relate to each other?

A thousand apologies. I think that's all we get. :X

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #45)
> I'm not sure what to make of your review. Have you though about the issue
> and could clarify? Thanks.

The way to pass typed address to MMDB is almost there.  The thing is still incomplete is that upgradeSchema function.

In this bug, we have to ensure each participant record, although there is not a new 'type' field introduced, belongs to only one type.  However, the addresses field in a participant record is populated with an assumption that all input addresses are of PLMN type.  We'll want to filter EMAIL, SHORTNUM, IP/IPv6 out first.  So if any item in that addresses field doesn't fall into PLMN type, and there are more than one items in it, then this record is somehow a mixed one and has to be removed, and all thread records, message records have to be updated correspondingly.  Find out affected participant records, remove them, and then update related records -- just comment 41 in plain text.

Comment 47

[bhavana bajaj [:bajaj]]

Switching the nom to 1.5 given comment #30 and #32

Comment 48

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 866938: B2G MMS: Support email address in receiver field (v1.11)

Hi Vicamo,

Thanks for all your help with this patch! This is an updated patch that implements the algorithm you outlined.

Changes to v1.10

  - rewrote updateSchema21 according to review

Comment 49

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

Comment on attachment 8382097 [details] [diff] [review]
[01] Bug 866938: B2G MMS: Support email address in receiver field (v1.11)

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

Didn't read through the whole patch.  Maybe we can focus on that updateSchema function first. :)

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1506,5 @@
>     *        The indexedDB savable MMS message, which is going to be
>     *        merged with the extra retrieval confirmation.
>     */
>    mergeRetrievalConfirmation: function(mmsConnection, intermediate, savable) {
> +    var types = ['cc', 'to'];

Always use 'let'.

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +1355,5 @@
> +    // find all participants with non-PLMN addresses
> +    //
> +
> +    let affectedParticipantIds = [];
> +    let typedAddressCache = [];

typedAddressCache = new Map();

@@ +1364,5 @@
> +        let participantRecord = cursor.value;
> +        let participantIsAffected = false;
> +
> +        for (let address of participantRecord.addresses) {
> +          typedAddressCache.push(address);

let typedAddress = { address: address,
                     type: MMS.Address.resolveType(address) };
typedAddressCache.set(address, typedAddress);

@@ +1365,5 @@
> +        let participantIsAffected = false;
> +
> +        for (let address of participantRecord.addresses) {
> +          typedAddressCache.push(address);
> +          participantIsAffected |= (address.type != 'PLMN') &&

s/address/typedAddress/

@@ +1397,5 @@
> +        let cursor = event.target.result;
> +        if (cursor) {
> +          let threadRecord = cursor.value;
> +          if (self.arraysIntersect(threadRecord.participantIds,
> +                                   affectedParticipantIds)) {

Bail-out early:

  if (!...) {
    cursor.continue();
    return;
  }

@@ +1414,5 @@
> +              }
> +            });
> +          }
> +          cursor.continue();
> +          return;

|findParticipantIdsByTypedAddresses| is an async procedure, so calling |cursor.continue();| here may result in multiple literally-equal participant records being created.  We didn't have "unique" flag set on participantStore's "addresses" index, so this will probably succeed, but then you won't ever reach the thread record again.  So, please only call |cursor.continue();| inside |if (participantIds)| block, which should always succeed.

@@ +1429,5 @@
> +
> +        let req = messageStore.index('threadId').openCursor();
> +        req.onsuccess = function(event) {
> +          let cursor = event.target.result;
> +          if (cursor) {

Considering we should generally have only few affected messages in our database, this results in a very inefficient loop.  A better one is to loop through only messages with affected thread ID:

  (function loopAffectedThreadIds(affectedThreadId) {
    let range = IDBKeyRange.bound([affectedThreadId, 0], [affectedThreadId, ""]);
    let req = messageStore.index('threadId').openCursor(range);
    req.onsuccess = function(event) {
      let cursor = event.target.result;
      if (!cursor) {
        if (!affectedThreadIds.length) {
          next();
        } else {
          loopAffectedThreadIds(affectedThreadIds.shift());
        }
        return;
      }

      // .....
    };
  })(affectedThreadIds.shift());

@@ +1430,5 @@
> +        let req = messageStore.index('threadId').openCursor();
> +        req.onsuccess = function(event) {
> +          let cursor = event.target.result;
> +          if (cursor) {
> +            let threadId = cursor.key;

Sorry, this should be |cursor.key[0]|.

@@ +1433,5 @@
> +          if (cursor) {
> +            let threadId = cursor.key;
> +            let messageRecord = cursor.value;
> +
> +            if (affectedThreadIds.indexOf(threadId) != -1) {

Then we don't need this check.

@@ +1435,5 @@
> +            let messageRecord = cursor.value;
> +
> +            if (affectedThreadIds.indexOf(threadId) != -1) {
> +              // find typed addresses of sender
> +              messageRecord.sender = typedAddressCache.find(

typedAddressCache.get(messageRecord.sender);

@@ +1441,5 @@
> +                  return this == typedAddress.address;
> +                },
> +                messageRecord.sender);
> +              // find typed addresses of receiver
> +              messageRecord.receiver = typedAddressCache.find(

Only SMS messages has a |receiver| attribute.

@@ +1447,5 @@
> +                  return this == typedAddress.address;
> +                },
> +                messageRecord.receiver);
> +              // find typed addresses of all receivers
> +              messageRecord.receivers = typedAddressCache.filter(

Only MMS messages has a |receivers| attribute.

@@ +1455,5 @@
> +                messageRecord.receivers);
> +
> +              if (messageRecord.delivery == 'sent') {
> +                // find typed addresses of all receivers
> +                messageRecord.headers['to'] = typedAddressCache.filter(

Only MMS messages has a |headers| attribute.

@@ +1460,5 @@
> +                  function(typedAddress) {
> +                    return this.indexOf(typedAddress.address) != -1;
> +                  },
> +                  messageRecord.headers['to']);
> +              }

|messageStore.put(messageRecord)|.

Besides, sorry, I should have noticed, we also have to update |messageRecord.participantIdsIndex|, which is [[<participant id>, <message timestamp>]].  This will be very tricky because all the step we have taken here is to cover the problem that one participant record may be divided into multiple ones.  You may have a map from <affectedThreadId> to <the new participant IDs assigned to the threadRecord>, and reconstruct |messageRecord.participantIdsIndex| here back:

  let affectedThreadIds = [];
  let threadIdParticipantIdsMap = new Map();
  ...
    if (participantIds) {
      threadRecord.participantIds = participantIds;
      threadIdParticipantIdsMap.set(threadRecord.id, participantIds);
      affectedThreadIds.push(threadRecord.id);
  ...
    let participantIds = threadIdParticipantIdsMap.get(threadId);
    let participantIdsIndex = [];
    for (let participantId of participantIds) {
      participantIdsIndex.push([participantId, messageRecord.timestamp]);
    }
    messageRecord.participantIdsIndex = participantIdsIndex;

    messageStore.put(messageRecord);

@@ +1465,5 @@
> +            }
> +            cursor.continue();
> +            return;
> +          }
> +          next();

Bail-out early or people will have a hard time matching parentheses.

@@ +1469,5 @@
> +          next();
> +        };
> +        req.onerror = function() {
> +          debug('messageStore.index().openCursor failed: ' + this.error.name);
> +          next();

The transaction will be soon aborted.  Calling next() here doesn't really help.  Same below.

Comment 50

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Vicamo,

I'm completely new to this topic, and the amount of review comments clearly show that. So thanks again for your help and reviews. Just two things:

> @@ +1397,5 @@
> > +        let cursor = event.target.result;
> > +        if (cursor) {
> > +          let threadRecord = cursor.value;
> > +          if (self.arraysIntersect(threadRecord.participantIds,
> > +                                   affectedParticipantIds)) {
> 
> Bail-out early:
> 
>   if (!...) {
>     cursor.continue();
>     return;
>   }

Are you sure? Because the |!cursor| branch is basically rest of the function.

> Considering we should generally have only few affected messages in our
> database, this results in a very inefficient loop.  A better one is to loop
> through only messages with affected thread ID:
> 
>   (function loopAffectedThreadIds(affectedThreadId) {
>     let range = IDBKeyRange.bound([affectedThreadId, 0], [affectedThreadId,
> ""]);

I don't understand that line. What are these arrays for? Can't we just use 'IDBKeyRange.once(affectedThreadId)?'

Comment 51

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 866938: B2G MMS: Support email address in receiver field (v1.12)

Changes to v1.11

  - use |let| in MmsService.jsm
  - reimplemented updateSchema21 according to review for v1.11
  - added update of messageRecord.participantIdsIndex

Comment 52

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #50)
> 'IDBKeyRange.once(affectedThreadId)?'

I meant

  IDBKeyRange.only(affectedThreadId)

Comment 53

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #50)
> Hi Vicamo,
> 
> I'm completely new to this topic, and the amount of review comments clearly
> show that. So thanks again for your help and reviews. Just two things:

Actually, I'm also new to this field. (Sounds terrifying ...)

> > @@ +1397,5 @@
> > > +        let cursor = event.target.result;
> > > +        if (cursor) {
> > > +          let threadRecord = cursor.value;
> > > +          if (self.arraysIntersect(threadRecord.participantIds,
> > > +                                   affectedParticipantIds)) {
> > 
> > Bail-out early:
> > 
> >   if (!...) {
> >     cursor.continue();
> >     return;
> >   }
> 
> Are you sure? Because the |!cursor| branch is basically rest of the function.

Sorry, did not make myself clear.  I mean:

  if (!self.arraysIntersect(threadRecord.participantIds,
                            affectedParticipantIds)) {
    cursor.continue();
    return;
  }

> > Considering we should generally have only few affected messages in our
> > database, this results in a very inefficient loop.  A better one is to loop
> > through only messages with affected thread ID:
> > 
> >   (function loopAffectedThreadIds(affectedThreadId) {
> >     let range = IDBKeyRange.bound([affectedThreadId, 0], [affectedThreadId,
> > ""]);
> 
> I don't understand that line. What are these arrays for?

It's a multiEntry key.  That means evaluating the keyPath of a certain record returns an array.  IndexedDB compares key objects as a integral -- the whole array is used for comparison.  The bound() call here tries to locate all message records with a specific threadId, but that can't be easily done because we can't ask IndexedDB to compare only the first element of a multiEntry key.  Here |[affectedThreadId, 0]| is always smaller than any valid key with its first element equals to affectedThreadId under the IndexedDB key comparison rules, and |[affectedThreadId, ""]| is always greater.

We have such multiEntry key trick to make sure we always return sorted messages list by its timestamp.  Yes, the second field it the timestamp of the message record.

> Can't we just use 'IDBKeyRange.once(affectedThreadId)?'

No.

Comment 54

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 866938: B2G MMS: Support email address in receiver field (v1.13)

Changes to 1.12

  - return early in |!arraysIntersect| branch
  - use IDBKeyRange.bound()

Comment 55

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

Hi, just had two patches in bug 980354 to fix problems in populating MMDB in Marionette test cases.

Comment 56

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ping.

Hi Vicamo, could you please review the latest iteration of the patch or let me know if something's still missing. Thanks!

Comment 57

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

Attachment: [01] Bug 866938: B2G MMS: Support email address in receiver field (v1.14)

Rebase only.  Almost all test cases failed. TBD.

Comment 58

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #56)
> Hi Vicamo, could you please review the latest iteration of the patch or let
> me know if something's still missing. Thanks!

Oops! Just began to do something on this.  Basically I want to verify it locally to see if there is still anything I haven't noticed.  And, test cases, of course.

Comment 59

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Vicamo,

I did some debugging today on the failing test cases. Without the patch, the following test cases fail:

test_message_classes.js
test_mmdb_setmessagedeliverybyid_sms.js
test_replace_short_message_type.js
test_segment_info.js
test_thread_subject.js

With the patch, the following test cases fail:

test_bug814761.js
test_emulator_loopback.js
test_error_of_mms_manual_retrieval.js
test_filter_date.js
test_filter_date_notfound.js
test_filter_mixed.js
test_filter_number.js
test_filter_read.js
test_filter_received.js
test_filter_sent.js
test_filter_unread.js
test_getmessage.js
test_getmessage_notfound.js
test_getmessages.js
test_getthreads.js
test_incoming_delete.js
test_incoming.js
test_incoming_max_segments.js
test_incoming_multipart.js
test_invalid_address.js
test_mark_msg_read_error.js
test_mark_msg_read.js
test_massive_incoming_delete.js
test_message_classes.js
test_mmdb_full_storage.js
test_mmdb_setmessagedeliverybyid_sms.js
test_mmsmessage_attachments.js
test_mt_sms_concatenation.js
test_phone_number_normalization.js
test_replace_short_message_type.js
test_strict_7bit_encoding.js
test_thread_subject.js
test_update_thread_record_in_delete.js

Comment 60

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: fix.patch

This fixes aMessageRecord.headers.to being undefined near MobileMessageDB.jsm:1698. This fixes 5 test cases for me, leaving

test_bug814761.js
test_emulator_loopback.js
test_filter_date.js
test_filter_date_notfound.js
test_filter_mixed.js
test_filter_number.js
test_filter_read.js
test_filter_received.js
test_filter_sent.js
test_filter_unread.js
test_getmessage.js
test_getmessage_notfound.js
test_getmessages.js
test_getthreads.js
test_incoming_delete.js
test_incoming.js
test_incoming_max_segments.js
test_incoming_multipart.js
test_mark_msg_read_error.js
test_mark_msg_read.js
test_massive_incoming_delete.js
test_message_classes.js
test_mmdb_full_storage.js
test_mt_sms_concatenation.js
test_phone_number_normalization.js
test_replace_short_message_type.js
test_strict_7bit_encoding.js
test_update_thread_record_in_delete.js

as broken.

Comment 61

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

Comment on attachment 8398609 [details] [diff] [review]
fix.patch

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +2545,2 @@
>      } else { // SMS
>        let threadParticipants = [{address: aMessage.sender,

The root cause is this 'let'.  Remove it and we have most test cases passed.

Comment 62

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

Attachment: part 2.a/4: extract createParticipantRecord

Comment 63

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

Attachment: part 2.b/4: rename findParticipantRecordByAddress to findParticipantRecordByPlmnAddress

Comment 64

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

Attachment: part 2.c/4: rename findThreadRecordByParticipants to findThreadRecordByAddresses

Comment 65

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

Attachment: part 4/N: use typed addresses internally : WIP

So far we have all mobilemessage test cases passed, but that upgradeSchema22 is still not verified yet.

Comment 66

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

Attachment: part 5/N: remaining MmsService parts

Since we've passed all existing test cases in part 4, I have to review if we really have to change binding interfaces between MmsService & MMDB.  I'll probably work on test cases & upgradeSchema22 first.

My working branch: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/866938/mms-email

Comment 67

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Wow! Thanks, Vicamo, for all the work you do here.

Comment 68

[Ken Chang[:kenkai]]

Set target milestone. Since we need this in Madai.

Comment 69

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

Attachment: part 4/5: use typed addresses internally : WIP2

1) since we did not touch |messageRecord.sender| and |messageRecord.receivers|, we don't have to update message records in upgradeSchema22.
2) return typed address in |fillReceivedMmsThreadParticipants|
3) s/aTypedAddresses/aThreadParticipants/ in some functions, so we have better idea what is this argument for.

I'm dropping the original part 5 because we don't really need it.  However, we do need a part 5 for test cases.

Comment 70

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

Attachment: Part 5/5: marionette test cases : WIP

1) add test_mmdb_upgradeSchema_current_structure.js,
2) rewrite test_mmdb_setmessagedeliverybyid_sms.js to use mmdb_head.js,
3) rewrite test_filter_number.js to use Promise and to test email/IPv4/IPv6 as well.

Comment 71

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

Attachment: part 3/4: use typed addresses internally : WIP3

1) rewrite upgradeSchema22.  Obviously MMDB has become so complex that no one can ever easily the effect of each line.  In bug 871433 we normalize input needle before diving into participant store.  This normalization process makes almost everything a phone number.  For example, an IPv4 address "55.252.255.54" was normalized as US phone number "5525225554".  Stored participant addresses are therefore always in phone number form.  So traversal throughout participant store becomes meaningless.

In contrast, although |participantAddresses| in a thread record are reliable, the messages with different participants have been wrongly considered in the same thread because the underlying participant record are the same.  The threads we have right now are reduced, collapsed from a larger set.  Traversal through out thread store can neither bring much sense.

The last solution remains is to construct participant/thread stores back from messages one by one.

2) eliminate the use of for..let..of.

Comment 72

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

Attachment: Part 5/5: marionette test cases : WIP2

1) add test_mmdb_upgradeSchema_22.js.  In this test case we should see one thread being replaced with multiple ones after upgradeSchema22.  It's not yet completed.

2) add xpcshell test cases for "Address.resolveType".  We don't have test cases come along with bug 907140.  It's really a pity.  Still have to take care of some corner cases like the validity and completeness of those RegExp.

3) probably need some more cases in "test_getthreads.js" and/or other related topics.

Comment 73

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

Updated to my WIP branch.  Mostly done.  One IPv6 xpcshell test fails.  Still hungry for more marionette test cases.  Stay tuned.

Comment 74

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

Attachment: part 1/4: correct RegExp used to match MMS addresses

1) detect "asdf/TYPE=IPv4" at decoding
2) correct IPv4/IPv6/num RegExp.
3) respect RFC 2822 email format

Comment 75

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

Attachment: part 4.a/4: add test_mmdb_upgradeSchema_current_structure.js

Comment 76

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

Attachment: part 4.b/4: rewrite test_filter_number.js to include non-PLMN addresses tests

Comment 77

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

Attachment: part 4.c/4: rewrite test_mmdb_setmessagedeliverybyid_sms.js to use mmdb_head.js

Comment 78

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

Attachment: part 4.d/4: add test_mmdb_upgradeSchema_22.js : WIP

In this part I also change a little bit in MMDB upgrade schema process to prevent running over expected version.

Comment 79

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

Attachment: part 4.e/4: add more xpcshell test cases for MmsPduHelper::Address

Comment 80

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Unassigning myself. Vicamo deserves all the credit here. :)

Comment 81

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

Attachment: part 4.d/4: add test_mmdb_upgradeSchema_22.js

Comment 82

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #71)
> Created attachment 8401083 [details] [diff] [review]
> part 3/4: use typed addresses internally : WIP3

One thing I have still one last concern about that "anonymous" string hard-coded in mmdb.  It's for an unknown from-address of a received MMS message and we need something to represent that anonymous sender.  However, it cause some problem that |messageRecord.headers.from.address| doesn't always align to |messageRecord.sender|.  Since we have now the ability to process address formats other than PLMN, an empty string should also be easy.  Hmmm....

Comment 83

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

Attachment: part 3/4: use typed addresses internally

1) remove assignment of 'anonymous' to |messageRecord.sender| when saving a received MMS address with anonymous from address.
2) remove redundant checks in |saveSendingMessage|.

Comment 84

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

Attachment: part 4.b/4: rewrite test_filter_number.js to include non-PLMN addresses tests : v2

Check alphabetic phone numbers as well.

Comment 85

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

Attachment: part 4.e/4: add more xpcshell test cases for MmsPduHelper::Address : v2

Add one more test case for resolving empty string.

Comment 86

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

try for xpcshell & marionette: https://tbpl.mozilla.org/?tree=Try&rev=9d21db51f3c7

Comment 87

[Gene Lian (I already quit Mozilla)]

Will review on this Wed./Thr. when I'm more available. Thanks!

Comment 88

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #87)
> Will review on this Wed./Thr. when I'm more available. Thanks!

Take your time.  I'm working on other stuff. :)

Comment 89

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8403211 [details] [diff] [review]
part 3/4: use typed addresses internally

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

Overall, it's good. I need one more run to look into details. Please clarify my question first. Thank you!

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +1461,5 @@
> +      } else { // MMS
> +        if ((messageRecord.delivery == DELIVERY_RECEIVED) ||
> +            (messageRecord.delivery == DELIVERY_NOT_DOWNLOADED)) {
> +          // DISABLE_MMS_GROUPING_FOR_RECEIVING is set to true at the time, so
> +          // we consider only |messageRecord.sender|.

Can you associate the logic to DISABLE_MMS_GROUPING_FOR_RECEIVING?

Comment 90

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #89)
> Comment on attachment 8403211 [details] [diff] [review]
> part 3/4: use typed addresses internally
> 
> ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
> @@ +1461,5 @@
> > +      } else { // MMS
> > +        if ((messageRecord.delivery == DELIVERY_RECEIVED) ||
> > +            (messageRecord.delivery == DELIVERY_NOT_DOWNLOADED)) {
> > +          // DISABLE_MMS_GROUPING_FOR_RECEIVING is set to true at the time, so
> > +          // we consider only |messageRecord.sender|.
> 
> Can you associate the logic to DISABLE_MMS_GROUPING_FOR_RECEIVING?

We can't and we don't have to do that.

The stores, indice, and even referenced fields and the meanings of all above of the mobile message database have to be in a consistent state.  DISABLE_MMS_GROUPING_FOR_RECEIVING is set to true now in schema revision 23, then it has to be true for every upgrade path that involves schema revision N..23.  This also follows that we just don't have a case that DISABLE_MMS_GROUPING_FOR_RECEIVING is false inside that upgradeSchema22 function.  It just cannot be.

Someday we may want to turn DISABLE_MMS_GROUPING_FOR_RECEIVING into false, and that day we need yet another upgradeSchema<M> function to address such change.  And, inside that new upgradeSchema<M> function, DISABLE_MMS_GROUPING_FOR_RECEIVING is still __always__ considered true until the end of the upgrade function.

Comment 91

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8403211 [details] [diff] [review]
part 3/4: use typed addresses internally

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +1418,5 @@
> +  upgradeSchema22: function(transaction, next) {
> +    // Since bug 871433 (DB_VERSION 11), we normalize addresses before really
> +    // diving into participant store in findParticipantRecordByPlmnAddress.
> +    // This also follows that all addresses stored in participant store are
> +    // normalized phone numbers, althought they might not be phone numbers.

a/althought/although/

@@ +1424,5 @@
> +    //
> +    // |participantAddresses| in a thread record are reliable, but several
> +    // distinct threads can be wrongly mapped into one.  For example, an IPv4
> +    // address "55.252.255.54" was normalized as US phone number "5525225554".
> +    // So beginning with thread store is not really a good idea.

You described two drawbacks above, then could you describe the solution you want to do in this upgradeSchema22? What are you going to do with the messageStore?

A bonus: since you already clear participantStore and threadStore from now on, I think it's a good timing to outline what the participantStore/threadStore/messageStore's DB schema look like as simple comments on top of this file.

@@ -2290,5 @@
>      } else if (aMessage.type == "mms") {
> -      let receivers = aMessage.receivers
> -      if (!Array.isArray(receivers)) {
> -        if (DEBUG) {
> -          debug("Need receivers for MMS. Fail to save the sending message.");

Why do you remove this check?

Comment 92

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #91)
> Comment on attachment 8403211 [details] [diff] [review]
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
> @@ +1418,5 @@
> > +  upgradeSchema22: function(transaction, next) {
> > +    // Since bug 871433 (DB_VERSION 11), we normalize addresses before really
> > +    // diving into participant store in findParticipantRecordByPlmnAddress.
> > +    // This also follows that all addresses stored in participant store are
> > +    // normalized phone numbers, althought they might not be phone numbers.
> 
> a/althought/although/
> 
> @@ +1424,5 @@
> > +    //
> > +    // |participantAddresses| in a thread record are reliable, but several
> > +    // distinct threads can be wrongly mapped into one.  For example, an IPv4
> > +    // address "55.252.255.54" was normalized as US phone number "5525225554".
> > +    // So beginning with thread store is not really a good idea.
> 
> You described two drawbacks above, then could you describe the solution you
> want to do in this upgradeSchema22? What are you going to do with the
> messageStore?
> 
> A bonus: since you already clear participantStore and threadStore from now
> on, I think it's a good timing to outline what the
> participantStore/threadStore/messageStore's DB schema look like as simple
> comments on top of this file.

Thank you.  Will add some more descriptive comments for what's actually done in this upgrade.
 
> @@ -2290,5 @@
> >      } else if (aMessage.type == "mms") {
> > -      let receivers = aMessage.receivers
> > -      if (!Array.isArray(receivers)) {
> > -        if (DEBUG) {
> > -          debug("Need receivers for MMS. Fail to save the sending message.");
> 
> Why do you remove this check?

Because there is already a check in the beginning of saveSendingMessage(), and that makes the check here become dead code.

Comment 93

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

Attachment: part 2.c/4: rename findThreadRecordByParticipants to findThreadRecordByAddresses : v2

Rename findThreadRecordByAddresses to findThreadRecordByPlmnAddresses, findParticipantIdsByAddresses to findParticipantIdsByPlmnAddresses as well.  All PLMN only query functions now have 'Plmn' string in their function names.

Comment 94

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

Attachment: part 3/4: use typed addresses internally : v2

1. Rebase to previous name change.

2. Add a |if (DEBUG)| block in findParticipantRecordByOtherAddress() so the debug messages of findParticipantRecordByOtherAddress and findParticipantRecordByPlmnAddress look symmetrical.

3. Rewrite upgradeSchema22() to lower penalty for normal situation.  In normal case, we don't have email or recipient addresses of other types.  This method scans all messages for changed threads first and bail-out if nothing to be done.  So in normal situation, we have only N read-only requests made.  In comparison, the old method re-threads every message record, so there will be N read + N write requests.

4. Make sure we always have a valid |message.headers.to| array in MmsService.

5. Remove |isAddrValid = false| in MmsService::createSavableFromParams().

Comment 95

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

Attachment: part 4.d/4: add test_mmdb_upgradeSchema_22.js : v2

Rebase to name changes.

Comment 96

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

Try all emulator based test cases: https://tbpl.mozilla.org/?tree=Try&rev=b997886ba205

Comment 97

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8422224 [details] [diff] [review]
part 3/4: use typed addresses internally : v2

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

Awesome!

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +1432,5 @@
> +    //
> +    // The only correct way is to begin with all messages records and check if
> +    // the findThreadRecordByTypedAddresses() call using a message record's
> +    // thread participants returns the same thread record with the one it
> +    // currently belong to.

s/belong/belongs/

Comment 98

[Julien Wajsberg [:julienw]]

Hey guys, I'd like to understand how a message sent to an email will look like? Will we have the email in the sender/receiver/receivers fields?

Comment 99

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

(In reply to Julien Wajsberg [:julienw] from comment #98)
> Hey guys, I'd like to understand how a message sent to an email will look
> like? Will we have the email in the sender/receiver/receivers fields?

Exactly.  Just pass an email address instead of a phone number in the receivers field.  Actually I'm to support email/ipv4/ipv6 at once in this bug.  Mixing phone numbers, email/ipv4/ipv6 addresses are allowed.  Only MMS has additional address types.

Comment 100

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #96)
> Try all emulator based test cases:
> https://tbpl.mozilla.org/?tree=Try&rev=b997886ba205

Breaking test_invalid_address.js.  Fix on the way.

Comment 101

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

Attachment: part 1/4: correct RegExp used to match MMS addresses

Re-gen from hg only. No change.

Comment 102

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

Attachment: part 2.a/4: extract createParticipantRecord

Re-gen from hg only. No change.

Comment 103

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

Attachment: part 2.b/4: rename findParticipantRecordByAddress to findParticipantRecordByPlmnAddress

Re-gen from hg only. No change.

Comment 104

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

Attachment: part 2.c/4: rename findThreadRecordByParticipants to findThreadRecordByAddresses : v2

Re-gen from hg only. No change.

Comment 105

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

Attachment: part 3/4: use typed addresses internally : v3

1) Re-gen for hg.
2) For addresses of unknown, unrecognized types, mark isAddrValid to false.  Fixes test_invalid_address.js failure.

Comment 106

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

Attachment: part 4.a/4: add test_mmdb_upgradeSchema_current_structure.js

Re-gen from hg only. No change.

Comment 107

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

Attachment: part 4.b/4: rewrite test_filter_number.js to include non-PLMN addresses tests : v2

Re-gen from hg only. No change.

Comment 108

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

Attachment: part 4.c/4: rewrite test_mmdb_setmessagedeliverybyid_sms.js to use mmdb_head.js : v2

Rebase and re-gen from hg,

Comment 109

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

Attachment: part 4.d/4: add test_mmdb_upgradeSchema_22.js : v2

Re-gen from hg only. No change.

Comment 110

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

Attachment: part 4.e/4: add more xpcshell test cases for MmsPduHelper::Address : v2

Re-gen from hg only. No change.

Comment 111

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

https://hg.mozilla.org/integration/b2g-inbound/rev/b1ddbdcccf83
https://hg.mozilla.org/integration/b2g-inbound/rev/b37f86bedd34
https://hg.mozilla.org/integration/b2g-inbound/rev/483f24fe46d2
https://hg.mozilla.org/integration/b2g-inbound/rev/1de11659fc63
https://hg.mozilla.org/integration/b2g-inbound/rev/496a1118899d
https://hg.mozilla.org/integration/b2g-inbound/rev/2046d8172cb3
https://hg.mozilla.org/integration/b2g-inbound/rev/eaf03cf5e997
https://hg.mozilla.org/integration/b2g-inbound/rev/5f70df422e64
https://hg.mozilla.org/integration/b2g-inbound/rev/34e981624851
https://hg.mozilla.org/integration/b2g-inbound/rev/8fbfac164fad

Comment 112

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b1ddbdcccf83
https://hg.mozilla.org/mozilla-central/rev/b37f86bedd34
https://hg.mozilla.org/mozilla-central/rev/483f24fe46d2
https://hg.mozilla.org/mozilla-central/rev/1de11659fc63
https://hg.mozilla.org/mozilla-central/rev/496a1118899d
https://hg.mozilla.org/mozilla-central/rev/2046d8172cb3
https://hg.mozilla.org/mozilla-central/rev/eaf03cf5e997
https://hg.mozilla.org/mozilla-central/rev/5f70df422e64
https://hg.mozilla.org/mozilla-central/rev/34e981624851
https://hg.mozilla.org/mozilla-central/rev/8fbfac164fad