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

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

assign to Gene for now

Reassigning to Josh that will help there.

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. :)

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.

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

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

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

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

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

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

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?

Sure.

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

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

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

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

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

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

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

Depends on bug 749086 to make life here easier.

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

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

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

Rebase without logical change. keep r+.

Rebase and address comment 41.

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

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

address comment 53

Address comment 52, comment 54 and comment 56

Address comment 55

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

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

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

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

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