We need to make MobileMessageDatabaseService.js be able to save the MMS messages and also keep the backward-compatibility for the existing SMS DB functions at the same time.

Hi Vicamo, Eventually, I didn't pass the message object into the functions because of some experiences learned from my previous reviewers: 1. It's not good to wrap all the parameters into an object, which makes us hard to know the needed inputs by looking at the API definition. 2. It's not good to save everything into the DB until we really need it, so that the DB space for saving messages can be estimated and under control.

Fix some nits. Please see comment #3 for the summary.

Comment on attachment 716394 [details] [diff] [review] Patch, V3.2 Review of attachment 716394 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +54,5 @@ > > const GLOBAL_SCOPE = this; > > +function getNumbersFromRecord(aRecord) { > + return aRecord.delivery == DELIVERY_RECEIVED ? [aRecord.sender] : aRecord.receivers; That's still something we still have problems in bug 833291 :( @@ +447,5 @@ > + aRecord.body, > + aRecord.messageClass, > + aRecord.timestamp, > + aRecord.read); > + } else if (aRecord.type == "mms") { You don't need |else| or even |if (aRecord.type == "mms")| here. We should ensure the database has only two types of messages. @@ +527,5 @@ > + // TODO: we skip the current message we got if it's not > + // a SMS one. Restores the waiting request and calls the > + // function again to get the next one when not processing. > + // Needs to find a way to handle MMS message in the future. > + aMessageList.requestWaiting = smsRequest; Reset the request back to requestWaiting may result in racing condition. The onsuccess callback is executed asynchronously. So there is a small time window between: let smsRequest = aMessageList.requestWaiting; aMessageList.requestWaiting = null; ... getRequest.onsuccess = function onsuccess(event) { let record = event.target.result; if (record.type != "sms") { aMessageList.requestWaiting = smsRequest; You should probably take smsRequest when that step is assured. But, why bother? We have no official MMS support yet. I'd rather kill the data flow in WapPushManager for now, and leave the database code here as it should be. @@ +733,5 @@ > + // Next update the other objectStore. > + let numbers = getNumbersFromRecord(aRecord); > + for (let i = 0; i < numbers.length; i++) { > + let number = numbers[i]; > + stores[1].get(number).onsuccess = function onsuccess(event) { Update mostRecenStore here relates to how do we define a thread. So, bug 833291 :( @@ +778,3 @@ > > + let receivers = aReceivers; > + if (!receivers) { Why? I can't understand :(

Something is still wrong in the RadioInterfaceLayer.js. I'll try to fix them tomorrow...

Hi Vicamo, This patch is working well without bugs. Please take a review when you have a chance. Please feel free to let me know if any details can be improved. Thanks! Run the try at the same time: https://tbpl.mozilla.org/?tree=Try&rev=422dd864ecc9

Comment on attachment 717483 [details] [diff] [review] Patch, V4 Review of attachment 717483 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RadioInterfaceLayer.js @@ +1460,5 @@ > } > > + let messageSender = message.sender || null; > + let messageReceiver = message.receiver || null; > + let messageFullBody = message.fullBody || null; Per offline discussion, the reason you have these saved variables is to fill the message object carried by system messenger. However, we should keep the objects sent via different channels the same. I mean, the original code is wrong, let's fix it here. @@ +1499,5 @@ > delivery: DOM_SMS_DELIVERY_RECEIVED, > deliveryStatus: RIL.GECKO_SMS_DELIVERY_STATUS_SUCCESS, > + sender: messageSender, > + receiver: messageReceiver, > + body: messageFullBody, ditto @@ +1510,5 @@ > > + message.type = "sms"; > + message.sender = messageSender; > + message.receiver = messageReceiver; > + message.body = messageFullBody; ditto @@ +2580,5 @@ > let timestamp = Date.now(); > let deliveryStatus = options.requestStatusReport > ? RIL.GECKO_SMS_DELIVERY_STATUS_PENDING > : RIL.GECKO_SMS_DELIVERY_STATUS_NOT_APPLICABLE; > + let id = gMobileMessageDatabaseService.saveSendingMessage({ please have |let record = {...}| instead.

Comment on attachment 717824 [details] [diff] [review] Patch, V4.3 Review of attachment 717824 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :)

Try: https://tbpl.mozilla.org/?tree=Try&rev=304eb1a15722

This bug relates to MMS features and needs to be tagged as leo+ so that we can uplift it into the b2g-18 branch.

Leo triage: leo+ for MMS blockers

Due to the conflict at Bug 830164 (s/icc/iccInfo is only applied in m-c but not b2g18), we need to prepare a b2g18-specific patch for this bug.

The entire set of clian's pushes was backed out for multiple reasons. https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=a0b06192f882 1.) Not every patch you pushed had leo+, tracking+, or approval+ to land. Every patch that lands needs these approvals whether they block another one or not. 2.) The tree rules are clear that you are not to land on top of bustage. At the time you pushed, both B2G Mn and B2G xpcshell had bustage from prior commits that hadn't been backed out yet. 3.) The tree rules are also clear that you are to watch your pushes for any bustage and handle them accordingly. mozilla-inbound is the ONLY tree where this rule does not apply. 4.) Even after the earlier bustage was backed out, something in one of your many pushes was causing further B2G Mn failures as shown in the log below. https://tbpl.mozilla.org/php/getParsedLog.php?id=20424173&tree=Mozilla-B2g18 5.) This isn't cause for backout by itself, but it is also strongly preferred to not push each commit individually as our build and testing resources are limited and doing so stretches them even thinner. Please limit your number of pushes as much as possible unless you have good reason for keeping them separate.

Ignore #1, my bad. The rest still stands.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/04732631cf91 We'll see what happens with Marionette...

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28) > https://hg.mozilla.org/releases/mozilla-b2g18/rev/04732631cf91 > > We'll see what happens with Marionette... We'll take care of it.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28) > https://hg.mozilla.org/releases/mozilla-b2g18/rev/04732631cf91 > > We'll see what happens with Marionette... Yep, looks like this is the patch that caused the Marionette failures noted in comment 26. Backed out. https://hg.mozilla.org/releases/mozilla-b2g18/rev/ad36b4b684c7 https://tbpl.mozilla.org/php/getParsedLog.php?id=20567378&tree=Mozilla-B2g18

s/iccInfo/icc/

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #31) > Created attachment 723968 [details] [diff] [review] > b2g18-specific-patch : v2 Verified locally. Will push after last tbpl commit turns (nearly) all green.

Re-gen with hg. Landing ...

Added [NO_UPLIFT][RIL_INTERFACE] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out. ------------------------------ If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible: Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643) Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments. Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741). Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix) Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix) Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead(). Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete(). Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) Bug 845643 - B2G MMS: Save retrieved MMS into database. Bug 792321 - Check max values of MMS parameters in sendRequest. Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS Bug 839436 - B2G MMS: make DB be able to save MMS messages.

Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.

Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.