Closed Bug 847736 Opened 11 years ago Closed 11 years ago

B2G MMS: provide nsIDOMMobileMessageManager.delete()

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
blocking-b2g leo+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 4 obsolete files)

+++ 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.
(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.
Blocks: 847744
this is required to support MMS user stories. Leo+
blocking-b2g: leo? → leo+
Attached patch Patch (obsolete) — Splinter Review
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 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 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-
Attached patch Patch, V2 (obsolete) — Splinter Review
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)
Attached patch Patch, V2.1 (obsolete) — Splinter Review
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)
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 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+
(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.
Attached patch Patch, V3 (checked-in) (obsolete) — Splinter Review
Ready to land but the inbound is closed now.
Attachment #723062 - Attachment is obsolete: true
Attachment #725736 - Flags: superreview+
Attachment #725736 - Flags: review+
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+
https://hg.mozilla.org/mozilla-central/rev/a59af50c07e2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Keywords: dev-doc-needed
Whiteboard: [by 3/8] → [NO_UPLIFT]
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.
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.
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.
Whiteboard: [NO_UPLIFT]
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.