Please refer to Bug 914060. We have to use the similar |matchPhoneNumbers(...)| to match the receiver to update the delivery status for the incoming delivery report. Need to improve the logic in |updateMessageDeliveryById(...)|. Otherwise, we would potentially update the delivery status for the wrong receiver.

Also, Ctai found out another defect in the updateMessageDeliveryId(...). We should match |messageRecord.deliveryInfo[j].receiver| to update its delivery status instead of relying on the messageRecord.receivers[i]. We may fix that as well in this bug, since it would cause the wrong match for updating the delivery status.

Directly assign this one to you if you don't mind. :)

Sure. I am glad to handle this.

According to the patch review in Bug 921918, both setMessageReadReportByEnvelopeId() & setMessageDeliveryByEnvelopeId() will apply the same logic for matching Phone number. mark as duplicated to Bug 921918. Bevis

Oh! I originally thought we can fix that in advance because we don't know how long it will take to finish Bug 921918. Especially, we still have an obvious bug at comment #1.

How about if we can't resolved Bug 921918 before tomorrow, then we reopen this bug?

Since this one would affect the function of MMS delivery report, which is a important feature, that would be nice if we can fix that before settling down MMS read report. That sounds good if we can solve that as well at bug 921918 in a short term. Thanks for the update!

I think this patch can fix this bug. https://bugzilla.mozilla.org/attachment.cgi?id=829180

Hi, Bevis, Since I can't land bug 921918 right now. Would you mind reopen this bug and take patch https://bug921918.bugzilla.mozilla.org/attachment.cgi?id=829180 for this bug? That patch should be able fix this bug. Thanks

Since the solution will not be included into bug 921918 now, reopen this bug to track.

Hi Vicamo, Because Gene/Ctai are in the business trip, I would like to ask for your help to review this patch. By ctai's comment, since the patch:part2 in Bug 921918 will not be landed on time due to his business trip, I would like to adopt and refine his patch to fix this bug first: 1. Use unique method matchPhoneNumbers() for the phone number matching. 2. Always compare the receiver info from message.deliveryInfo[] to update the delivery status accordingly. Regards, Bevis Tseng

Comment on attachment 830086 [details] [diff] [review] 934931_v1.patch, Part 1 to apply matchPhoneNumbers() in updateMessageDeliveryById(). Review of attachment 830086 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +1584,5 @@ > return aMessageRecord.id; > }, > > + findReceiversFromDeliveryInfo: function findReceiversFromDeliveryInfo( > + receiver, deliveryInfo, modifyFn){ The function is named 'find...', so I'm expecting a function that takes two parameters only. Something like: let entry = findDeliveryInfoEntryByReceiver(deliveryInfo, receiver); if (entry) { .... } Don't entangle a function with numerous purposes. The thing we want in a 'find...' function is literally find something out, isn't it?

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12) > Comment on attachment 830086 [details] [diff] [review] > 934931_v1.patch, Part 1 to apply matchPhoneNumbers() in > updateMessageDeliveryById(). > > Review of attachment 830086 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js > @@ +1584,5 @@ > > return aMessageRecord.id; > > }, > > > > + findReceiversFromDeliveryInfo: function findReceiversFromDeliveryInfo( > > + receiver, deliveryInfo, modifyFn){ > > The function is named 'find...', so I'm expecting a function that takes two > parameters only. Something like: > > let entry = findDeliveryInfoEntryByReceiver(deliveryInfo, receiver); > if (entry) { > .... > } > > Don't entangle a function with numerous purposes. The thing we want in a > 'find...' function is literally find something out, isn't it? Thanks for your suggestion. You are right! I should write these more clearly. :) Bevis Tseng

wrap the update logic of mms delivery report into the method of |updateDeliveryStatusIntoDeliveryInfo()| for better understanding.

Comment on attachment 830661 [details] [diff] [review] 934931_v2.patch, Part 1: to apply matchPhoneNumbers() in updateMessageDeliveryById(). r=vyang Review of attachment 830661 [details] [diff] [review]: ----------------------------------------------------------------- “I'm expecting a function that takes two parameters only.“ And I meant it.

It seems we still have one deliveryInfo entry in a outgoing MmsMessage for each receiver after bug 928821, which follows we could have duplicated entry in |MmsMessage.deliveryInfo|. So, I have a forEach func instead and have an additional parameter for callback. Gene, could you confirm whether that's expected? Can't find details on http://messaging.sysapps.org/#mmsmessage-interface

Comment on attachment 830729 [details] [diff] [review] patch Review of attachment 830729 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +1584,5 @@ > return aMessageRecord.id; > }, > > + forEachMatchedMmsDeliveryInfo: > + function forEachMatchedMmsDeliveryInfo(aDeliveryInfos, aNeedle, aCallback) { If we have no duplicated entries, we can rename it back to |findMmsDeliveryInfo| and remove the third parameter. @@ +1696,5 @@ > > isRecordUpdated = true; > } > } else if (messageRecord.type == "mms") { > + let update = function(aEelement) { typo: aElement.

1) fix typo 2) bail-out early in |function update| and avoid updating record if unnecessary.

Share |updateFunc| between SMS and MMS processes.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16) > Created attachment 830729 [details] [diff] [review] > patch > > It seems we still have one deliveryInfo entry in a outgoing MmsMessage for > each receiver after bug 928821, which follows we could have duplicated entry > in |MmsMessage.deliveryInfo|. So, I have a forEach func instead and have an > additional parameter for callback. Bug 928821 just attempts to add deliveryInfo. We should keep the original logic of "each receiver has one deliveryInfo" to ensure the backward-compatibility with Gaia. We can fire another bug to fix that but not for now.

Comment on attachment 831561 [details] [diff] [review] patch : v3 Review of attachment 831561 [details] [diff] [review]: ----------------------------------------------------------------- This patch won't work actually, Besides, I hope to use s/receiver/address which is more consistent findParticipantRecordByAddress(...). ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ -1129,5 @@ > threadRecord.lastMessageSubject = null; > cursor.update(threadRecord); > > cursor.continue(); > - return; How could this happen... @@ +1584,5 @@ > return aMessageRecord.id; > }, > > + forEachMatchedMmsDeliveryInfo: > + function forEachMatchedMmsDeliveryInfo(aDeliveryInfos, aNeedle, aCallback) { info is not countable noun. Let's use s/aDeliveryInfos/aDeliveryInfo to follow the same semantic in other codes. :) Also, why not s/aNeedle/aAddress/? Also, I'd prefer s/forEachMatchedMmsDeliveryInfo/updateMmsDeliveryInfoByAddress/. @@ +1586,5 @@ > > + forEachMatchedMmsDeliveryInfo: > + function forEachMatchedMmsDeliveryInfo(aDeliveryInfos, aNeedle, aCallback) { > + > + let typedAddress = MMS.Address.resolveType(aNeedle); s/aNeedle/aAddress/ @@ +1589,5 @@ > + > + let typedAddress = MMS.Address.resolveType(aNeedle); > + let normalizedAddress, parsedAddress; > + if (typedAddress.type === "PLMN") { > + normalizedAddress = PhoneNumberUtils.normalize(receiver, false); This doesn't work, baby. s/receiver/aAddress/ or s/receiver/aAddress/typedAddress.address/? I'm sure which one is correct. @@ +1590,5 @@ > + let typedAddress = MMS.Address.resolveType(aNeedle); > + let normalizedAddress, parsedAddress; > + if (typedAddress.type === "PLMN") { > + normalizedAddress = PhoneNumberUtils.normalize(receiver, false); > + parsedAddress = PhoneNumberUtils.parse(normReceiver); s/normReceiver/normalizedAddress/ @@ +1593,5 @@ > + normalizedAddress = PhoneNumberUtils.normalize(receiver, false); > + parsedAddress = PhoneNumberUtils.parse(normReceiver); > + } > + > + for (let element of aDeliveryInfos) { s/aDeliveryInfos/aDeliveryInfo/ @@ +1594,5 @@ > + parsedAddress = PhoneNumberUtils.parse(normReceiver); > + } > + > + for (let element of aDeliveryInfos) { > + let typedInfoReceiver = MMS.Address.resolveType(element.receiver); s/typedInfoReceiver/typedStoredAddress/ @@ +1595,5 @@ > + } > + > + for (let element of aDeliveryInfos) { > + let typedInfoReceiver = MMS.Address.resolveType(element.receiver); > + if (typedAddress.type !== typedInfoReceiver.type) { s/typedInfoReceiver/typedStoredAddress/ @@ +1600,5 @@ > + // Not even my type. Skip. > + continue; > + } > + > + if (typedAddress.address == typedInfoReceiver.address) { s/typedInfoReceiver/typedStoredAddress/ @@ +1611,5 @@ > + // Address type other than "PLMN" must have direct match. Or, skip. > + continue; > + } > + > + // Both are of "PLMN" type. // Both are addresses of "PLMN" type. @@ +1612,5 @@ > + continue; > + } > + > + // Both are of "PLMN" type. > + let normalizedInfoReceiver = s/normalizedInfoReceiver/normalizedStoredAddress @@ +1614,5 @@ > + > + // Both are of "PLMN" type. > + let normalizedInfoReceiver = > + PhoneNumberUtils.normalize(element.receiver, false); > + let parsedInfoReceiver = s/parsedInfoReceiver/parsedStoredAddress @@ +1615,5 @@ > + // Both are of "PLMN" type. > + let normalizedInfoReceiver = > + PhoneNumberUtils.normalize(element.receiver, false); > + let parsedInfoReceiver = > + PhoneNumberUtils.parseWithMCC(normalizedInfoReceiver, null); s/normalizedInfoReceiver/normalizedStoredAddress @@ +1617,5 @@ > + PhoneNumberUtils.normalize(element.receiver, false); > + let parsedInfoReceiver = > + PhoneNumberUtils.parseWithMCC(normalizedInfoReceiver, null); > + if (this.matchPhoneNumbers(normalizedAddress, parsedAddress, > + normalizedInfoReceiver, parsedInfoReceiver)) { s/normalizedInfoReceiver/normalizedStoredAddress s/parsedInfoReceiver/parsedStoredAddress @@ +1622,5 @@ > + aCallback(element); > + } > + } > + > + return null; Don't need to return null, I think. @@ +1682,5 @@ > messageRecord.deliveryIndex = [delivery, messageRecord.timestamp]; > isRecordUpdated = true; > } > > // Update |messageRecord.deliveryStatus| if needed. Correct this comment a bit. // Attempt to update |deliveryStatus| and |deliveryTimestamp| of: // - the |messageRecord| for SMS. // - the element(s) in |messageRecord.deliveryInfo| for MMS. @@ +1684,5 @@ > } > > // Update |messageRecord.deliveryStatus| if needed. > if (deliveryStatus) { > + let updateFunc = function(aTarget) { Could you please add some words for this callback? Like: // A callback for updating the deliveyStatus/deliveryTimestamp of each target. @@ +1708,2 @@ > } else { > + this.forEachMatchedMmsDeliveryInfo(messageRecord.deliveryInfo, Add one comment: // If the receiver is specified, we only need to update the element(s) in // deliveryInfo that match the same receiver.

Sorry for all the serious mistakes in previous revision. Should have a test before review.

This patches add test cases for SMS only.

I was to write test cases for MMS as well. However, it suffers the same problem we have for SMS reference work load in bug 930839 comment 7. This test script will never be finished. Just for future reference.

Full try: https://tbpl.mozilla.org/?tree=Try&rev=770d03267431

Comment on attachment 832763 [details] [diff] [review] part 1/2: v4 Review of attachment 832763 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +1584,5 @@ > return aMessageRecord.id; > }, > > + forEachMatchedMmsDeliveryInfo: > + function forEachMatchedMmsDeliveryInfo(aDeliveryInfo, aNeedle, aCallback) { OK. I understood. This function is for iterating through the elements not updating them. My previous suggestion might not be reasonable. @@ +1612,5 @@ > + continue; > + } > + > + // Both are of "PLMN" type. > + let normalizedStoredReceiver = I feel comfortable to use: s/normalizedStoredReceiver/normalizedStoredAddress to have a more consistent semantic within this file. @@ +1614,5 @@ > + > + // Both are of "PLMN" type. > + let normalizedStoredReceiver = > + PhoneNumberUtils.normalize(element.receiver, false); > + let parsedStoredReceiver = s/parsedStoredReceiver/parsedStoredAddress @@ +1615,5 @@ > + // Both are of "PLMN" type. > + let normalizedStoredReceiver = > + PhoneNumberUtils.normalize(element.receiver, false); > + let parsedStoredReceiver = > + PhoneNumberUtils.parseWithMCC(normalizedStoredReceiver, null); s/normalizedStoredReceiver/normalizedStoredAddress @@ +1617,5 @@ > + PhoneNumberUtils.normalize(element.receiver, false); > + let parsedStoredReceiver = > + PhoneNumberUtils.parseWithMCC(normalizedStoredReceiver, null); > + if (this.matchPhoneNumbers(normalizedAddress, parsedAddress, > + normalizedStoredReceiver, parsedStoredReceiver)) { s/parsedStoredReceiver/parsedStoredAddress s/normalizedStoredReceiver/normalizedStoredAddress

Comment on attachment 832765 [details] [diff] [review] part 2/2: test cases Review of attachment 832765 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Address previous nits.

https://hg.mozilla.org/integration/b2g-inbound/rev/a7aa3ff58f78 https://hg.mozilla.org/integration/b2g-inbound/rev/e255bea5f2c1

https://hg.mozilla.org/mozilla-central/rev/a7aa3ff58f78 https://hg.mozilla.org/mozilla-central/rev/e255bea5f2c1

This will be part of 1.3 since it landed in gecko 28.