Closed
Bug 839436
Opened 12 years ago
Closed 12 years ago
B2G MMS: make DB be able to save MMS messages
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: airpingu, Assigned: airpingu)
References
Details
(Whiteboard: [RIL_INTERFACE])
Attachments
(2 files, 15 obsolete files)
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
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.
Attachment #711754 -
Attachment is obsolete: true
Attachment #711755 -
Attachment is obsolete: true
Attachment #715442 -
Flags: feedback?(vyang)
Assignee | ||
Comment 4•12 years ago
|
||
Fix some nits. Please see comment #3 for the summary.
Attachment #715442 -
Attachment is obsolete: true
Attachment #715442 -
Flags: feedback?(vyang)
Attachment #715858 -
Flags: review?(vyang)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #715858 -
Attachment is obsolete: true
Attachment #715858 -
Flags: review?(vyang)
Attachment #716390 -
Flags: review?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #716390 -
Attachment is obsolete: true
Attachment #716390 -
Flags: review?(vyang)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #716391 -
Attachment is obsolete: true
Attachment #716391 -
Flags: review?(vyang)
Attachment #716394 -
Flags: review?(vyang)
Comment 8•12 years ago
|
||
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 :(
Attachment #716394 -
Flags: review?(vyang)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #716394 -
Attachment is obsolete: true
Attachment #716979 -
Flags: review?(vyang)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #716979 -
Attachment is obsolete: true
Attachment #716979 -
Flags: review?(vyang)
Attachment #716982 -
Flags: review?(vyang)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #716982 -
Attachment is obsolete: true
Attachment #716982 -
Flags: review?(vyang)
Attachment #717140 -
Flags: review?(vyang)
Assignee | ||
Comment 12•12 years ago
|
||
Something is still wrong in the RadioInterfaceLayer.js. I'll try to fix them tomorrow...
Updated•12 years ago
|
Blocks: b2g-mms-dom-api
Assignee | ||
Updated•12 years ago
|
Attachment #717140 -
Flags: review?(vyang)
Assignee | ||
Comment 13•12 years ago
|
||
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
Attachment #717140 -
Attachment is obsolete: true
Attachment #717483 -
Flags: review?(vyang)
Comment 14•12 years ago
|
||
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.
Attachment #717483 -
Flags: review?(vyang)
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #717483 -
Attachment is obsolete: true
Attachment #717820 -
Flags: review?(vyang)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #717820 -
Attachment is obsolete: true
Attachment #717820 -
Flags: review?(vyang)
Attachment #717821 -
Flags: review?(vyang)
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #717821 -
Attachment is obsolete: true
Attachment #717821 -
Flags: review?(vyang)
Attachment #717824 -
Flags: review?(vyang)
Comment 18•12 years ago
|
||
Comment on attachment 717824 [details] [diff] [review] Patch, V4.3 Review of attachment 717824 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :)
Attachment #717824 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 20•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd5d8525ace
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8bd5d8525ace
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Comment 22•12 years ago
|
||
This bug relates to MMS features and needs to be tagged as leo+ so that we can uplift it into the b2g-18 branch.
blocking-b2g: --- → leo?
Assignee | ||
Comment 24•12 years ago
|
||
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.
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8feb4680772f
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox22:
--- → fixed
Comment 26•12 years ago
|
||
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.
Comment 28•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/04732631cf91 We'll see what happens with Marionette...
Comment 29•12 years ago
|
||
(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.
Comment 30•12 years ago
|
||
(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
Comment 32•12 years ago
|
||
(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.
Comment 33•12 years ago
|
||
Re-gen with hg. Landing ...
Attachment #723968 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Whiteboard: [RIL_INTERFACE]
Assignee | ||
Updated•12 years ago
|
Whiteboard: [RIL_INTERFACE] → [NO_UPLIFT][RIL_INTERFACE]
Assignee | ||
Comment 35•12 years ago
|
||
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.
Assignee | ||
Comment 36•12 years ago
|
||
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.
Assignee | ||
Comment 37•12 years ago
|
||
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.
Whiteboard: [NO_UPLIFT][RIL_INTERFACE] → [RIL_INTERFACE]
Updated•11 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•