847744 - B2G MMS: support IPC for MMS DOM API

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

Currently, all the DOM APIs don't consider the existence of multi-processes in the initiative step (due to the time line). Eventually, we must need to provide IPC structure for MMS DOM APIs to support multi-processes.

Comment 1

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

this is needed to bring Message app OOP again. Leo+

Comment 2

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

assign to Gene for now

Comment 3

[[:fabrice] Fabrice Desré]

Reassigning to Josh that will help there.

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I can directly work with Gene and have some bandwidth now. If you haven't start to work on this, I can take this bug. :)

Comment 5

[Ken Chang[:kenkai]]

Fabrice and Josh, Thanks for your help. But Patrick have worked on this topic for 2 weeks. I think it is better to let Patrick to keep working on this bug.

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Ken Chang from comment #5)
> Fabrice and Josh, Thanks for your help. But Patrick have worked on this
> topic for 2 weeks. I think it is better to let Patrick to keep working on
> this bug.

Well, ok. But reading comment #0 doesn't let people know that this is being worked on since 2 weeks.

Comment 7

[Ken Chang[:kenkai]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> (In reply to Ken Chang from comment #5)
> > Fabrice and Josh, Thanks for your help. But Patrick have worked on this
> > topic for 2 weeks. I think it is better to let Patrick to keep working on
> > this bug.
> 
> Well, ok. But reading comment #0 doesn't let people know that this is being
> worked on since 2 weeks.
Oops!! Patrick was assigned this task on 2/25. We updated this task status in mms sync-up meeting on 3/4 and then Joe filed this bug for tracking on 3/5.

Comment 8

[Josh Matthews [:jdm]]

I did have an incomplete patch, but I'm ok not being responsible for this work.

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Josh Matthews [:jdm] from comment #8)
> I did have an incomplete patch, but I'm ok not being responsible for this
> work.

Can you still attach your wip there? thanks!

Comment 10

[Josh Matthews [:jdm]]

Attachment: IPCify the MMS code WIP

Comment 11

[Gene Lian (I already quit Mozilla)]

(In reply to Josh Matthews [:jdm] from comment #10)
> Created attachment 723338 [details] [diff] [review]
> IPCify the MMS code WIP

This is awesome! Thanks Josh! This patch really helps a lot! :D

Btw, bug 844431 provides fundamental stuctures like nsIDOMMobileMessageManager, nsIMobileMessageService and nsIMobileMessageCallback for MMS DOM APIs with non-oop. I'd be better if the IPC implementation can depend on that. Patrick can help the integration.

Comment 12

[Josh Matthews [:jdm]]

Yeah, I wrote it on top of the patches from bug 844431, but when I started it I was still confused about the overlap between the SMS stuff and MobileMessageManager.

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 1 - Create SmsIPCService for content process

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 2 - IPC for MmsService

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Create SmsIPCService for content process

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: IPC MmsService.send()

Comment 17

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Create SmsIPCService for content process

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: IPC change

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: IPC for MmsService

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 725356 [details] [diff] [review]
Part 3: IPC for MmsService

>+      att.content = static_cast<BlobParent*>(element.contentParent())->GetBlob();
>+    } else {
>+      att.content = static_cast<BlobChild*>(element.contentChild())->GetBlob();

You can get rid of the aIsParent argument and just check conentParent() and contentChild() for non-null values - one will always be null.

> NS_IMPL_ADDREF(MobileMessageCallback)
> NS_IMPL_RELEASE(MobileMessageCallback)
> 
> NS_INTERFACE_MAP_BEGIN(MobileMessageCallback)
>   NS_INTERFACE_MAP_ENTRY(nsIMobileMessageCallback)
>   NS_INTERFACE_MAP_ENTRY(nsISupports)
> NS_INTERFACE_MAP_END

NS_IMPL_ISUPPORTS1(MobileMessageCallback, nsIMobileMessageCallback)

>+  nsCOMPtr<MmsMessage> message;

nsRefPtr, or nsCOMPtr<nsIMmsMessage>

>+MmsRequestParent::SendReply(const MessageReply& aReply) {
>+  nsRefPtr<MobileMessageCallback> callback;
>+  mCallback.swap(callback);

Why is this necessary? It will be released as part of the destructor that's invoked by Send__delete__.

>+  params.Init(aContext, &aParam);

Check return value.

>+  JS_GetArrayLength(aContext, &receiversObj, &len);

Check return value.

>+    JS_GetElement(aContext, &receiversObj, i, &val);

Check return value.

>+    str.init(aContext, val.toString());

Check return value.

>+  JS_GetArrayLength(aContext, &attachmentsObj, &len);

Check return value.

>+    JS_GetElement(aContext, &attachmentsObj, i, &val);

Check return value.

>+    attachment.Init(aContext, &val);

Check return value.

Comment 21

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Thanks, Josh! The comment helps a lot. I will fix them in next version. By the way, would you help to review this patch when I done?

Comment 22

[Josh Matthews [:jdm]]

Sure.

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: IPDL change

Comment 24

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: IPDL change

Comment 25

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 725729 [details] [diff] [review]
Part 2: IPDL change

Hi Vicamo, because mms service and sms service use different topics to notify 'sending', 'sent' and 'fail', I use different messages in PSms instead of making NotifySendingMessage/NotifySentMessage/NotifyFailedMessage take union of SmsMessage and MmsMessage in argument.

Comment 26

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

Comment on attachment 725729 [details] [diff] [review]
Part 2: IPDL change

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

We don't need another PMmsRequest. The "mms-*" observer topics are mistakes. It's my fault, I should have noticed. But we really really don't need another ipdl protocol here.

Comment 27

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

Comment on attachment 725354 [details] [diff] [review]
Part 1: Create SmsIPCService for content process

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

::: dom/mms/interfaces/nsIMmsService.idl
@@ +12,1 @@
>  #define RIL_MMSSERVICE_CONTRACTID "@mozilla.org/mms/rilmmsservice;1"

Please have "MMS_SERVICE_CID" & "MMSSERVICE_CONTRACTID" here instead.

::: dom/mms/src/ril/MmsService.manifest
@@ +1,3 @@
>  # MmsService.js
>  component {217ddd76-75db-4210-955d-8806cd8d87f9} MmsService.js
> +contract @mozilla.org/mms/rilmmsserviceinstance;1 {217ddd76-75db-4210-955d-8806cd8d87f9}

You don't have to do this at all.

::: layout/build/nsLayoutModule.cpp
@@ +830,5 @@
>  NS_DEFINE_NAMED_CID(NS_HAPTICFEEDBACK_CID);
>  #endif
>  #endif
>  NS_DEFINE_NAMED_CID(SMS_SERVICE_CID);
> +NS_DEFINE_NAMED_CID(RIL_MMSSERVICE_CID);

MMSSERVICE_CID

@@ +1110,5 @@
>  #endif
>    { &kTHIRDPARTYUTIL_CID, false, NULL, ThirdPartyUtilConstructor },
>    { &kNS_STRUCTUREDCLONECONTAINER_CID, false, NULL, nsStructuredCloneContainerConstructor },
>    { &kSMS_SERVICE_CID, false, NULL, nsISmsServiceConstructor },
> +  { &kRIL_MMSSERVICE_CID, false, NULL, nsIMmsServiceConstructor },

MMSSERVICE_CID

@@ +1254,5 @@
>  #endif
>    { THIRDPARTYUTIL_CONTRACTID, &kTHIRDPARTYUTIL_CID },
>    { NS_STRUCTUREDCLONECONTAINER_CONTRACTID, &kNS_STRUCTUREDCLONECONTAINER_CID },
>    { SMS_SERVICE_CONTRACTID, &kSMS_SERVICE_CID },
> +  { RIL_MMSSERVICE_CONTRACTID, &kRIL_MMSSERVICE_CID },

MMSSERVICE_CID & MMSSERVICE_CONTRACTID

Comment 28

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

Comment on attachment 725356 [details] [diff] [review]
Part 3: IPC for MmsService

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

Please don't duplicate code for MMS. Just make it into PSms as we had discussed in private.

Comment 29

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
> Please don't duplicate code for MMS. Just make it into PSms as we had
> discussed in private.

SmsRequestForwarder is removed in nsIDOMMobileMessageManager.getMessage(), while it is still used for SMS. Is there any way we can tell if request is an SmsRequestForwarder or an nsIMobileMessageCallback in the GetMessageMoz() of SmsIPCService?

Comment 30

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

(In reply to Patrick Wang [:kk1fff] from comment #29)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #28)
> > Please don't duplicate code for MMS. Just make it into PSms as we had
> > discussed in private.
> 
> SmsRequestForwarder is removed in nsIDOMMobileMessageManager.getMessage(),
> while it is still used for SMS. Is there any way we can tell if request is
> an SmsRequestForwarder or an nsIMobileMessageCallback in the GetMessageMoz()
> of SmsIPCService?

Yes, just remove SmsRequest in bug 749086 (SMS/MMS DOMRequest-ize). But since it depends on bug 838467 (SMS/MMS DOMCursor-ize) and Jonas still has problems with it, let's wait for his opinion here. Or, we can:

option 1) have MmsMessage support in MobileMessageManager only, or
option 2) duplicate every request related interface method(send, delete, markRead, etc.) to be explicitly MMS specific.

Either one kills me :(

Comment 31

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Create SmsIPCService for content process

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: IPDL change

Comment 33

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: IPC for MmsService.send()

According to comment 30, we need more info to decide what we should do for other API. Implementing send() here.

Comment 34

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: mms-* topic -> sms-* topic

Comment 35

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: IPDL change

Comment 36

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: IPC for MmsService.send()

Comment 37

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

Comment on attachment 726499 [details] [diff] [review]
Part 1: Create SmsIPCService for content process

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

::: dom/mobilemessage/src/android/MmsService.cpp
@@ +18,5 @@
> +MmsService::Send(const JS::Value& aParameters,
> +                 nsIMobileMessageCallback *aRequest)
> +{
> +  NS_NOTYETIMPLEMENTED("Implement me!");
> +  return NS_OK;

return NS_ERROR_NOTIMPLEMENTED;

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +145,5 @@
> +NS_IMETHODIMP
> +SmsIPCService::Send(const JS::Value& aParameters,
> +                    nsIMobileMessageCallback *aRequest)
> +{
> +  return NS_OK;

ditto

Comment 38

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

Comment on attachment 726583 [details] [diff] [review]
Part 2: mms-* topic -> sms-* topic

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +321,4 @@
>  
> +  nsCOMPtr<nsIDOMMozSmsMessage> smsMsg = do_QueryInterface(aMsg);
> +  nsCOMPtr<nsIDOMMozMmsMessage> mmsMsg;
> +  if (smsMsg) {

That means we should rename SmsEvent to MobileMessageEvent, too. :(
But in this patch, let's keep it and focus on IPC stuff. Bail out early:

  nsCOMPtr<nsIDOMMozSmsMessage> sms = do_QueryInterface(aMsg);
  if (sms) {
    ...
    return DispatchTrustedEvent(event);
  }

  nsCOMPtr<nsIDOMMozMmsMessage> mms = do_QueryInterface(aMsg);
  NS_ENSURE_TRUE(mms, NS_ERROR_FAILURE);
  ...
  return DispatchTrustedEvent(event);

@@ +356,1 @@
>      return NS_OK;

return DispatchTrustedEventToSelf(...);

Comment 39

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

Comment on attachment 726584 [details] [diff] [review]
Part 3: IPDL change

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

::: dom/mobilemessage/src/ipc/SmsTypes.ipdlh
@@ +59,5 @@
> +
> +union MobileMessageData {
> +  MmsMessageData;
> +  SmsMessageData;
> +};

new line, please.

Comment 40

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

(In reply to Patrick Wang [:kk1fff] from comment #36)
> Created attachment 726586 [details] [diff] [review]
> Part 4: IPC for MmsService.send()

The solution in my mind is actuall make SmsRequestParent inherit nsIMobileMessageCallback instead. And that's already in bug 749086[1].

[1]: https://github.com/vicamo/b2g_releases-mozilla-central/commit/c40c140991e185fb95ed714a2750408f3efb0867

Comment 41

[Josh Matthews [:jdm]]

Comment on attachment 726586 [details] [diff] [review]
Part 4: IPC for MmsService.send()

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

MMSParent.(cpp|h) aren't used in this patch? Are they leftover code? If so, please apply the comments I left to the same code in SmsParent. I'll want to look at this again once the changes here are applied.

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +258,5 @@
> +  // Attachement
> +  aData->attachments().SetCapacity(mAttachments.Length());
> +  for (uint32_t i = 0; i < mAttachments.Length(); i++) {
> +    MmsAttachmentData mma;
> +    MmsAttachment &element = mAttachments[i];

nit: const

::: dom/mobilemessage/src/MmsMessage.h
@@ +9,5 @@
>  #include "nsIDOMMozMmsMessage.h"
>  #include "nsString.h"
>  #include "jspubtd.h"
>  #include "mozilla/dom/mobilemessage/Types.h"
> +#include "mozilla/dom/mobilemessage/SmsTypes.h"

This isn't necessary; you can just add a
>namespace mobilemessage {
>  class MmsMessageData;
>}
block in this header instead.

::: dom/mobilemessage/src/MobileMessageCallback.cpp
@@ +39,5 @@
>  {
>  }
>  
> +nsresult
> +MobileMessageCallback::SendMessageReply(const MessageReply& aReply)

Nothing checks the return value, and it always returns NS_OK. Might as well make this void.

@@ +114,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +    MmsMessage *mmsMsg = static_cast<MmsMessage*>(mm.get());
> +    MmsMessageData mData;
> +    mmsMsg->GetMmsMessageData(mParent->GetContentParent(), &mData);

mParent->Manager()

::: dom/mobilemessage/src/ipc/MmsParent.cpp
@@ +15,5 @@
> +namespace dom {
> +namespace mobilemessage {
> +
> +JSObject *MmsAttachmentDataToJSObject(const MmsAttachmentData& aAttachment,
> +                                      JSContext* aContext) {

In the JS engine code, if a function's first argument is the JSContext, that means that it can cause JS exceptions to be thrown. Let's do the same here and flip the arguments.

@@ +24,5 @@
> +  JSString* idStr = JS_NewUCStringCopyN(aContext,
> +                                        aAttachment.id().get(),
> +                                        aAttachment.id().Length());
> +  NS_ENSURE_TRUE(idStr, nullptr);
> +  if (!JS_DefineProperty(aContext, obj, "id", STRING_TO_JSVAL(idStr),

JS::StringValue

@@ +33,5 @@
> +  JSString* locStr = JS_NewUCStringCopyN(aContext,
> +                                         aAttachment.location().get(),
> +                                         aAttachment.location().Length());
> +  NS_ENSURE_TRUE(locStr, nullptr);
> +  if (!JS_DefineProperty(aContext, obj, "location", STRING_TO_JSVAL(locStr),

JS::StringValue

@@ +56,5 @@
> +  return obj;
> +}
> +
> +bool GetParamsFromSendMmsRequest(const SendMmsRequest& aRequest,
> +                                 JSContext* aContext,

Make this be the first argument.

@@ +66,5 @@
> +  JSString* smilStr = JS_NewUCStringCopyN(aContext,
> +                                          aRequest.smil().get(),
> +                                          aRequest.smil().Length());
> +  NS_ENSURE_TRUE(smilStr, false);
> +  if(!JS_DefineProperty(aContext, paramsObj, "smil", STRING_TO_JSVAL(smilStr),

JS::StringValue

@@ +77,5 @@
> +                                             aRequest.subject().get(),
> +                                             aRequest.subject().Length());
> +  NS_ENSURE_TRUE(subjectStr, false);
> +  if(!JS_DefineProperty(aContext, paramsObj, "subject",
> +                        STRING_TO_JSVAL(subjectStr), nullptr, nullptr, 0)) {

JS::StringValue

@@ +92,5 @@
> +    JSString* jsStr = JS_NewUCStringCopyN(aContext,
> +                                          recv.get(),
> +                                          recv.Length());
> +    NS_ENSURE_TRUE(jsStr, false);
> +    jsval val = STRING_TO_JSVAL(jsStr);

JS::Value and JS::StringValue

@@ +98,5 @@
> +      return false;
> +    }
> +  }
> +  if (!JS_DefineProperty(aContext, paramsObj, "receivers",
> +                         OBJECT_TO_JSVAL(receiverArray), nullptr, nullptr, 0)) {

JS::ObjectValue

@@ +110,5 @@
> +  for (uint32_t i = 0; i < aRequest.attachments().Length(); i++) {
> +    JSObject *obj = MmsAttachmentDataToJSObject(
> +                        aRequest.attachments().ElementAt(i), aContext);
> +    NS_ENSURE_TRUE(obj, false);
> +    jsval val = OBJECT_TO_JSVAL(obj);

JS::ObjectValue

@@ +117,5 @@
> +    }
> +  }
> +
> +  if (!JS_DefineProperty(aContext, paramsObj, "attachments",
> +                         OBJECT_TO_JSVAL(attachmentArray), nullptr, nullptr, 0)) {

JS::ObjectValue

@@ +134,5 @@
> +    NS_WARNING("MmsRequestParent: Unable to get MMS service");
> +    return false;
> +  }
> +
> +  JSContext *cx = nsContentUtils::GetSafeJSContext();

This may need to use AutoJSContext instead. Definitely have Blake look at all of the JSAPI usage in this patch.

@@ +163,5 @@
> +
> +void
> +MmsRequestParent::SendReply(const MessageReply& aReply) {
> +  nsRefPtr<MobileMessageCallback> callback;
> +  mCallback.swap(callback);

Unnecessary since we're going to run the destructor when we call Send__delete__.

::: dom/mobilemessage/src/ipc/MmsParent.h
@@ +21,5 @@
> +class MobileMessageCallback;
> +
> +class MmsRequestParent: public PMmsRequestParent
> +{
> +  friend class SmsParent;

Why's this necessary?

@@ +35,5 @@
> +
> +  nsRefPtr<MobileMessageCallback> mCallback;
> +
> +  bool DoRequest(const SendMmsRequest& aRequest);
> +};

This class needs an ActorDestroy to call SetActorDied on the callback.

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +165,5 @@
> +  uint32_t len;
> +
> +  // SendMobileMessageRequest.receivers
> +  JSObject &receiversObj = params.receivers.toObject();
> +  JS_ALWAYS_TRUE(JS_GetArrayLength(cx, &receiversObj, &len));

1) JS_ALWAYS_TRUE should stay in the JS engine, use MOZ_ALWAYS_TRUE instead.
2) We want to check the return value in release builds as well. Just use the regular if checks for this function instead.

@@ +166,5 @@
> +
> +  // SendMobileMessageRequest.receivers
> +  JSObject &receiversObj = params.receivers.toObject();
> +  JS_ALWAYS_TRUE(JS_GetArrayLength(cx, &receiversObj, &len));
> +  NS_ENSURE_SUCCESS(rv, false);

Bogus.

@@ +183,5 @@
> +
> +  // SendMobileMessageRequest.attachments
> +  ContentChild* cc = ContentChild::GetSingleton();
> +
> +  JSObject &attachmentsObj = params.attachments.toObject();

We probably need to ensure that params.attachments.isObject() is true (same as the check at the start of the function).

@@ +203,5 @@
> +
> +    request.attachments().AppendElement(mmsAttachment);
> +  }
> +
> +  // SendMmsRequest.smil

This doesn't add anything.

@@ +206,5 @@
> +
> +  // SendMmsRequest.smil
> +  request.smil() = params.smil;
> +
> +  // SendMmsMessageRequest.subject

Nor this.

@@ +217,5 @@
>  SmsIPCService::Send(const JS::Value& aParameters,
>                      nsIMobileMessageCallback *aRequest)
>  {
> +  SendMmsMessageRequest req;
> +  GetSendMmsMessageRequestFromParams(aParameters, req);

Check the return value here.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +27,5 @@
>  namespace mobilemessage {
>  
> +static JSObject*
> +MmsAttachmentDataToJSObject(const MmsAttachmentData& aAttachment,
> +                            JSContext* aContext)

Swap these arguments. However, this code _really_ needs to be shared between the SMS and MMS code that currently duplicates it.

@@ +260,5 @@
> +{
> +  nsCOMPtr<nsIDOMMozMmsMessage> mmsMsg = do_QueryInterface(aMsg);
> +  if (mmsMsg) {
> +    MmsMessageData data;
> +    static_cast<MmsMessage*>(mmsMsg.get())->GetMmsMessageData(mContentParent, &data);

GetMmsMessageData(Manager(), &data);

@@ +360,5 @@
>  
>  PSmsRequestParent*
>  SmsParent::AllocPSmsRequest(const IPCSmsRequest& aRequest)
>  {
> +  return new SmsRequestParent(mContentParent);

return new SmsRequestParent(Manager());

::: dom/mobilemessage/src/ipc/SmsParent.h
@@ +58,5 @@
> +  bool
> +  GetMobileMessageDataFromMessage(nsISupports* aMsg, MobileMessageData& aData);
> +
> +private:
> +  ContentParent* mContentParent;

Unnecessary.

Comment 42

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Josh Matthews [:jdm] from comment #41)
> Comment on attachment 726586 [details] [diff] [review]
> Part 4: IPC for MmsService.send()
> MMSParent.(cpp|h) aren't used in this patch? Are they leftover code? If so,
> please apply the comments I left to the same code in SmsParent. I'll want to
> look at this again once the changes here are applied.

Sorry. I forgot to remove MMSParent.* from this patch, codes in those files are moved to SmsParent.*. I will apply the review comments to the codes in SmsParent.

Comment 43

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

Depends on bug 749086 to make life here easier.

Comment 44

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

WIP for rebasing on bug 749086 - https://github.com/kk1fff/releases-mozilla-central/tree/b/847744

Comment 45

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Create SmsIPCService for content process

Requesting review again for adding fallback/MmsService.cpp and fallback/MmsService.h.

Comment 46

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: mms-* topic -> sms-* topic

Rebasing without logical change. Keep r+ for this patch.

Comment 47

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: IPDL change

Rebase without logical change. keep r+.

Comment 48

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: IPC for MmsService.send()

Rebase and address comment 41.

Comment 49

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: IPC for MmsService.send()

Comment 50

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

leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed

Comment 51

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 5: IPC for MmsService.retrieve()

Comment 52

[Josh Matthews [:jdm]]

Comment on attachment 731102 [details] [diff] [review]
Part 4: IPC for MmsService.send()

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

Tagging Blake to look at the JSAPI usage, since the rest looks good to me.

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +65,5 @@
> +  , mRead(aData.read())
> +  , mSubject(aData.subject())
> +  , mSmil(aData.smil())
> +{
> +  // Attachments

This comment doesn't add anything.

@@ +242,5 @@
> +MmsMessage::GetMmsMessageData(ContentParent* aParent,
> +                              mobilemessage::MmsMessageData& aData)
> +{
> +  NS_ASSERTION(aParent, "aParent is null");
> +  NS_ASSERTION(aData, "aData is null");

This assertion doesn't make sense.

@@ +254,5 @@
> +  aData.read() = mRead;
> +  aData.subject() = mSubject;
> +  aData.smil() = mSmil;
> +
> +  // Attachement

Doesn't add anything.

@@ +261,5 @@
> +    MmsAttachmentData mma;
> +    const MmsAttachment &element = mAttachments[i];
> +    mma.id().Assign(element.id);
> +    mma.location().Assign(element.location);
> +    mma.contentParent() = aParent->GetOrCreateActorForBlob(element.content);

This can return null if the child process has died. Should this operation fail if that happens?

@@ +265,5 @@
> +    mma.contentParent() = aParent->GetOrCreateActorForBlob(element.content);
> +    aData.attachments().AppendElement(mma);
> +  }
> +
> +  return true;

Just make this method return void instead if it can't fail.

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +179,5 @@
> +
> +  uint32_t len;
> +
> +  // SendMobileMessageRequest.receivers
> +  JSObject &receiversObj = params.receivers.toObject();

This needs to check that receivers is an object first.

@@ +180,5 @@
> +  uint32_t len;
> +
> +  // SendMobileMessageRequest.receivers
> +  JSObject &receiversObj = params.receivers.toObject();
> +  MOZ_ALWAYS_TRUE(JS_GetArrayLength(cx, &receiversObj, &len));

This does us no good in release builds. Please use the
>if (!JS_...) {
>  return false;
>}
pattern that other JSAPI consumers use.

@@ +189,5 @@
> +    JS::Value val;
> +    MOZ_ALWAYS_TRUE(JS_GetElement(cx, &receiversObj, i, &val));
> +
> +    nsDependentJSString str;
> +    MOZ_ALWAYS_TRUE(str.init(cx, val.toString()));

This needs to check that val is a string first.

@@ +215,5 @@
> +
> +    MmsAttachmentData mmsAttachment;
> +    mmsAttachment.id().Assign(attachment.id);
> +    mmsAttachment.location().Assign(attachment.location);
> +    mmsAttachment.contentChild() = cc->GetOrCreateActorForBlob(attachment.content);

This can return null.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +31,5 @@
> +static JSObject*
> +MmsAttachmentDataToJSObject(JSContext* aContext,
> +                            const MmsAttachmentData& aAttachment)
> +{
> +  JSObject* obj = JS_NewObject(aContext, nullptr, nullptr, nullptr);

At minimum, we probably need JSAutoRequest here. We may also need to enter a compartment at some point; make sure Blake takes a look.

@@ +53,5 @@
> +                         nullptr, nullptr, 0)) {
> +    return nullptr;
> +  }
> +
> +  // content.

Nit: Remove the .

@@ +75,5 @@
> +GetParamsFromSendMmsMessageRequest(JSContext* aCx,
> +                                   const SendMmsMessageRequest& aRequest,
> +                                   JS::Value* aParam)
> +{
> +  JSObject* paramsObj = JS_NewObject(aCx, nullptr, nullptr, nullptr);

Probably need JSAutoRequest here as well, same concerns about compartments.

@@ +124,5 @@
> +  JSObject* attachmentArray = JS_NewArrayObject(aCx,
> +                                                aRequest.attachments().Length(),
> +                                                nullptr);
> +  for (uint32_t i = 0; i < aRequest.attachments().Length(); i++) {
> +    JSObject *obj = MmsAttachmentDataToJSObject(aCx,

Nit: * on the left.

@@ +257,5 @@
> +  nsCOMPtr<nsIDOMMozMmsMessage> mmsMsg = do_QueryInterface(aMsg);
> +  if (mmsMsg) {
> +    MmsMessageData data;
> +    ContentParent *parent = static_cast<ContentParent*>(Manager());
> +    static_cast<MmsMessage*>(mmsMsg.get())->GetMmsMessageData(parent, data);

GetMmsMessageData can fail.

@@ +411,5 @@
>  SmsRequestParent::DoRequest(const SendMessageRequest& aRequest)
>  {
> +  switch(aRequest.type()) {
> +  case SendMessageRequest::TSendSmsMessageRequest:
> +    {

Nit: { on the previous line.

@@ +418,3 @@
>  
> +      const SendSmsMessageRequest &data = aRequest.get_SendSmsMessageRequest();
> +      return NS_SUCCEEDED(smsService->Send(data.number(), data.message(), this));

This will kill the child process if Send doesn't succeed. Is this desired?

@@ +420,5 @@
> +      return NS_SUCCEEDED(smsService->Send(data.number(), data.message(), this));
> +    }
> +    break;
> +  case SendMessageRequest::TSendMmsMessageRequest:
> +    {

Ditto.

@@ +431,5 @@
> +              cx,
> +              aRequest.get_SendMmsMessageRequest(),
> +              &params)) {
> +        NS_WARNING("SmsRequestParent: Fail to build MMS params.");
> +        return false;

Same child killing behaviour.

@@ +433,5 @@
> +              &params)) {
> +        NS_WARNING("SmsRequestParent: Fail to build MMS params.");
> +        return false;
> +      }
> +      return NS_SUCCEEDED(mmsService->Send(params, this));

Ditto.

@@ +513,5 @@
> +  nsCOMPtr<nsIDOMMozMmsMessage> mms = do_QueryInterface(aMessage);
> +  if (mms) {
> +    MmsMessage *msg = static_cast<MmsMessage*>(mms.get());
> +    ContentParent *parent = static_cast<ContentParent*>(Manager()->Manager());
> +    MmsMessageData mData;

Just call it data; the m prefix is for members.

@@ +514,5 @@
> +  if (mms) {
> +    MmsMessage *msg = static_cast<MmsMessage*>(mms.get());
> +    ContentParent *parent = static_cast<ContentParent*>(Manager()->Manager());
> +    MmsMessageData mData;
> +    msg->GetMmsMessageData(parent, mData);

GetMmsMessageData can fail.

@@ +545,5 @@
> +  nsCOMPtr<nsIDOMMozMmsMessage> mms = do_QueryInterface(aMessage);
> +  if (mms) {
> +    MmsMessage *msg = static_cast<MmsMessage*>(mms.get());
> +    ContentParent *parent = static_cast<ContentParent*>(Manager()->Manager());
> +    MmsMessageData mData;

s/mData/data/

@@ +546,5 @@
> +  if (mms) {
> +    MmsMessage *msg = static_cast<MmsMessage*>(mms.get());
> +    ContentParent *parent = static_cast<ContentParent*>(Manager()->Manager());
> +    MmsMessageData mData;
> +    msg->GetMmsMessageData(parent, mData);

GetMmsMessageData can fail.

::: dom/mobilemessage/src/ipc/SmsParent.h
@@ +22,4 @@
>  
>  namespace mobilemessage {
>  
> +class MobileMessageCallback;

Nit: This doesn't look necessary.

Comment 53

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

Comment on attachment 731083 [details] [diff] [review]
Part 1: Create SmsIPCService for content process

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

::: dom/mms/interfaces/nsIMmsService.idl
@@ +13,1 @@
>  #define RIL_MMSSERVICE_CONTRACTID "@mozilla.org/mms/rilmmsservice;1"

Please remove RIL_MMSSERVICE_CONTRACTID here.

::: dom/mobilemessage/src/SmsServicesFactory.cpp
@@ +13,3 @@
>  #endif
>  #include "nsServiceManagerUtils.h"
>  

Move RIL_MMSSERVICE_CONTRACTID here.

Comment 54

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

Comment on attachment 731102 [details] [diff] [review]
Part 4: IPC for MmsService.send()

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

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +238,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +MmsMessage::GetMmsMessageData(ContentParent* aParent,

This function is named GetData in SmsMessage, SmsFilter and MobileMessageThread.

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +115,5 @@
>                      nsIMobileMessageCallback* aRequest)
>  {
> +  return SendRequest(SendMessageRequest(SendSmsMessageRequest(nsString(aNumber),
> +                                                              nsString(aMessage))),
> +                     aRequest) ? NS_OK : NS_ERROR_FAILURE;

SendRequest already returns nsresult in bug 838467.

@@ +235,5 @@
> +  if (!GetSendMmsMessageRequestFromParams(aParameters, req)) {
> +    return NS_ERROR_UNEXPECTED;
> +  };
> +  SendRequest(SendMessageRequest(req), aRequest);
> +  return NS_OK;

return SendRequest(...);

Comment 55

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

Comment on attachment 732278 [details] [diff] [review]
Part 5: IPC for MmsService.retrieve()

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

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +241,5 @@
> +NS_IMETHODIMP
> +SmsIPCService::Retrieve(int32_t aId, nsIMobileMessageCallback *aRequest)
> +{
> +  SendRequest(RetrieveMessageRequest(aId), aRequest);
> +  return NS_OK;

return SendRequest(...);

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +451,5 @@
> +{
> +  nsCOMPtr<nsIMmsService> mmsService = do_GetService(RIL_MMSSERVICE_CONTRACTID);
> +  NS_ENSURE_TRUE(mmsService, false);
> +
> +  return NS_SUCCEEDED(mmsService->Retrieve(aRequest.messageId(), this));

This is different from other DoRequest calls. Please have another sync and implement this accordingly.

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 731102 [details] [diff] [review]
Part 4: IPC for MmsService.send()

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

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +233,5 @@
>  {
> +  SendMmsMessageRequest req;
> +  if (!GetSendMmsMessageRequestFromParams(aParameters, req)) {
> +    return NS_ERROR_UNEXPECTED;
> +  };

Nit: nuke the extra semicolon, here.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +103,5 @@
> +  JSObject* receiverArray = JS_NewArrayObject(aCx,
> +                                              aRequest.receivers().Length(),
> +                                              nullptr);
> +  NS_ENSURE_TRUE(receiverArray, false);
> +  for (uint32_t i = 0; i < aRequest.receivers().Length(); i++) {

This should use nsTArrayToJSArray.

Comment 57

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Create SmsIPCService for content process

address comment 53

Comment 58

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: IPC for MmsService.send()

Address comment 52, comment 54 and comment 56

Comment 59

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 5: IPC for MmsService.retrieve()

Address comment 55

Comment 60

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc493a828f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/17549598a514
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0727b1cd52d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bb8c14385c
https://hg.mozilla.org/integration/mozilla-inbound/rev/684ed7a62fb6

Comment 61

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Create SmsIPCService for content process (check-in)

Comment 62

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: mms-* topic -> sms-* topic (check-in)

Comment 63

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: IPDL change (check-in)

Comment 64

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 4: IPC for MmsService.send() (check-in)

Comment 65

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 5: IPC for MmsService.retrieve() (check-in)

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1bc493a828f6
https://hg.mozilla.org/mozilla-central/rev/17549598a514
https://hg.mozilla.org/mozilla-central/rev/f0727b1cd52d
https://hg.mozilla.org/mozilla-central/rev/b6bb8c14385c
https://hg.mozilla.org/mozilla-central/rev/684ed7a62fb6

Comment 67

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

WIP for rebasing on b2g18 https://github.com/kk1fff/mozilla-central/tree/b/847744/b2g18

Comment 68

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

(In reply to Patrick Wang [:kk1fff] from comment #67)
> WIP for rebasing on b2g18
> https://github.com/kk1fff/mozilla-central/tree/b/847744/b2g18

Thank you. Uplift WIP: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/838467/b2g18 , verifying.

Comment 69

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

https://hg.mozilla.org/releases/mozilla-b2g18/rev/90f543e4a4e0
https://hg.mozilla.org/releases/mozilla-b2g18/rev/23fdaa699d6c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/898d1ece6313
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d10ff6cda618
https://hg.mozilla.org/releases/mozilla-b2g18/rev/37f11e04ebf5