847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

+++ 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.

Comment 1

[Gene Lian (I already quit Mozilla)]

(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

[Joe Cheng [:jcheng] (please needinfo)]

this is required to support MMS user stories. Leo+

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

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.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

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);

Comment 5

[Mounir Lamouri (:mounir)]

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(...);

Comment 6

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

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!

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2.1

Fixed a nit. Please see comment #6 for the patch summary.

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

once bug 844431 lands, +1 day to land this bug

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 723062 [details] [diff] [review]
Patch, V2.1

Review of attachment 723062 [details] [diff] [review]:
-----------------------------------------------------------------

sr=me on the .idl changes

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 11

[Gene Lian (I already quit Mozilla)]

(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.

Comment 12

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3 (checked-in)

Ready to land but the inbound is closed now.

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3.1 (checked-in)

Make up the commit message. Ready to land but the inbound is still cosed now.

r=vicamo sr=sicking a=leo+

Comment 14

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a59af50c07e2

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a59af50c07e2

Comment 16

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18] Patch

Comment 17

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0440cee0c8a4

Comment 18

[Gene Lian (I already quit Mozilla)]

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.

Comment 19

[Gene Lian (I already quit Mozilla)]

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.

Comment 20

[Gene Lian (I already quit Mozilla)]

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.

Comment 21

[Gene Lian (I already quit Mozilla)]

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.