Closed
Bug 847736
Opened 12 years ago
Closed 12 years ago
B2G MMS: provide nsIDOMMobileMessageManager.delete()
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: airpingu, Assigned: airpingu)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
airpingu
:
review+
airpingu
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #844431 +++ After bug 844429 is done, we can have a |nsIDOMMobileMessageManager| interface to add: DOMRequest delete(in long id); and then deprecate the following one in the |nsIDOMMobileMessageManager|: nsIDOMMozSmsRequest delete(in jsval param); This function needs to be compatible with both SMS and MMS.
Assignee | ||
Comment 1•12 years ago
|
||
(In reply to Gene Lian [:gene] from comment #0) > +++ This bug was initially created as a clone of Bug #844431 +++ > > After bug 844429 is done, we can have a |nsIDOMMobileMessageManager| ^^^^^^^^^^ Sorry, I mean bug 844431.
Comment 2•12 years ago
|
||
this is required to support MMS user stories. Leo+
blocking-b2g: leo? → leo+
Assignee | ||
Comment 3•12 years ago
|
||
We'll make up the IPC structure at bug 847744. For now, we just hope to provide a non-oop version to let Gaia have some runnable APIs to use due to the MMS timeline.
Attachment #722624 -
Flags: feedback?(vyang)
Attachment #722624 -
Flags: feedback?(mounir)
Comment 4•12 years ago
|
||
Comment on attachment 722624 [details] [diff] [review] Patch Review of attachment 722624 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/MobileMessageCallback.cpp @@ +41,5 @@ > +{ > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService(DOMREQUEST_SERVICE_CONTRACTID); > + NS_ENSURE_TRUE(rs, NS_ERROR_FAILURE); > + > + rs->FireSuccess(mDOMRequest, aResult); return rs->FireSuccess(...); ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +225,3 @@ > { > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + NS_ADDREF(*aRequest = request); NS_ADDREF just before you're going to return it. Or there will be memory leakage. @@ +232,3 @@ > nsCOMPtr<nsIMobileMessageDatabaseService> mobileMessageDBService = > do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID); > NS_ENSURE_TRUE(mobileMessageDBService, NS_ERROR_FAILURE); Please move |new DOMRequest()| and |new MobileMessageCallback| after |do_GetService()|. This way, when |do_GetService| fails, we don't have to destruct instanciated but unused objects. @@ +232,5 @@ > nsCOMPtr<nsIMobileMessageDatabaseService> mobileMessageDBService = > do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID); > NS_ENSURE_TRUE(mobileMessageDBService, NS_ERROR_FAILURE); > > + mobileMessageDBService->DeleteMessage(aId, msgCallback); return mobileMessageDBService->DeleteMessage(aId, msgCallback);
Attachment #722624 -
Flags: feedback?(vyang)
Comment 5•12 years ago
|
||
Comment on attachment 722624 [details] [diff] [review] Patch Review of attachment 722624 [details] [diff] [review]: ----------------------------------------------------------------- Like for the patch is bug 847738, I do not have enough information to really judge if everything is okay. ::: dom/mobilemessage/src/MobileMessageCallback.cpp @@ +41,5 @@ > +{ > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService(DOMREQUEST_SERVICE_CONTRACTID); > + NS_ENSURE_TRUE(rs, NS_ERROR_FAILURE); > + > + rs->FireSuccess(mDOMRequest, aResult); You could also do: return rs ? rs->FireSuccess(...) : NS_ERROR_FAILURE; ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +225,3 @@ > { > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + NS_ADDREF(*aRequest = request); Use .forget() and do that at the end of the method. @@ +226,5 @@ > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + NS_ADDREF(*aRequest = request); > + > + nsCOMPtr<nsIMobileMessageCallback> msgCallback = new MobileMessageCallback(request); > + NS_ENSURE_TRUE(msgCallback, NS_ERROR_FAILURE); |new| can't fail. @@ +232,5 @@ > nsCOMPtr<nsIMobileMessageDatabaseService> mobileMessageDBService = > do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID); > NS_ENSURE_TRUE(mobileMessageDBService, NS_ERROR_FAILURE); > > + mobileMessageDBService->DeleteMessage(aId, msgCallback); return mobileMessageDBService->DeleteMessage(...);
Attachment #722624 -
Flags: feedback?(mounir) → feedback-
Assignee | ||
Comment 6•12 years ago
|
||
Hi Jonas, Need your superreview for the IDL part. Just adding a nsIDOMMobileMessageManager.delete(). Thanks! Hi Mounir, Like bug 847738, we hope to use nsIMobileMessageCallback/nsIDOMMobileMessageManager and reserve the nsISmsRequest/nsIDOMSmsManager at the same time, so that we won't break the SMS functionalities until we can really deprecate the nsISmsRequest/nsIDOMSmsManager APIs in the future. I guess letting Vicamo take this review might be appropriate? Thanks! Hi Vicamo, This patch addresses your comment #4 and Mounir's comment #5. Thanks!
Attachment #722624 -
Attachment is obsolete: true
Attachment #723061 -
Flags: superreview?(jonas)
Attachment #723061 -
Flags: review?(vyang)
Assignee | ||
Comment 7•12 years ago
|
||
Fixed a nit. Please see comment #6 for the patch summary.
Attachment #723061 -
Attachment is obsolete: true
Attachment #723061 -
Flags: superreview?(jonas)
Attachment #723061 -
Flags: review?(vyang)
Attachment #723062 -
Flags: superreview?(jonas)
Attachment #723062 -
Flags: review?(vyang)
Comment 8•12 years ago
|
||
once bug 844431 lands, +1 day to land this bug
Comment on attachment 723062 [details] [diff] [review] Patch, V2.1 Review of attachment 723062 [details] [diff] [review]: ----------------------------------------------------------------- sr=me on the .idl changes
Attachment #723062 -
Flags: superreview?(jonas) → superreview+
Comment 10•12 years ago
|
||
Comment on attachment 723062 [details] [diff] [review] Patch, V2.1 Review of attachment 723062 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/MobileMessageManager.h @@ -39,5 @@ > > - /** > - * Internal Delete() method used to delete a message. > - */ > - nsresult Delete(int32_t aId, nsIDOMMozSmsRequest** aRequest); Please keep this variant. Actually Gaia requests more variants in bug 771458, and we'll probably need it for bug 840053 and bug 840054, too.
Attachment #723062 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10) > Please keep this variant. Actually Gaia requests more variants in bug > 771458, and we'll probably need it for bug 840053 and bug 840054, too. Thanks! Done in my local build.
Assignee | ||
Comment 12•12 years ago
|
||
Ready to land but the inbound is closed now.
Attachment #723062 -
Attachment is obsolete: true
Attachment #725736 -
Flags: superreview+
Attachment #725736 -
Flags: review+
Assignee | ||
Comment 13•12 years ago
|
||
Make up the commit message. Ready to land but the inbound is still cosed now. r=vicamo sr=sicking a=leo+
Attachment #725736 -
Attachment is obsolete: true
Attachment #725772 -
Flags: superreview+
Attachment #725772 -
Flags: review+
Assignee | ||
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a59af50c07e2
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a59af50c07e2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0440cee0c8a4
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Assignee | ||
Updated•12 years ago
|
Whiteboard: [by 3/8] → [NO_UPLIFT]
Assignee | ||
Comment 18•12 years ago
|
||
Added [NO_UPLIFT] 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.
Assignee | ||
Comment 19•12 years ago
|
||
Following the previous comment, some more needs to back out: 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 20•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 21•11 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]
Updated•11 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•