Closed
Bug 847744
Opened 12 years ago
Closed 12 years ago
B2G MMS: support IPC for MMS DOM API
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: airpingu, Assigned: kk1fff)
References
Details
Attachments
(6 files, 24 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
+++ 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.
Reporter | ||
Updated•12 years ago
|
Comment 1•12 years ago
|
||
this is needed to bring Message app OOP again. Leo+
blocking-b2g: leo? → leo+
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee: josh → pwang
Comment 6•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
I did have an incomplete patch, but I'm ok not being responsible for this work.
Comment 9•12 years ago
|
||
(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•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [target 3/29]
Reporter | ||
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Comment 19•12 years ago
|
||
Comment 20•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Attachment #725354 -
Flags: review?(vyang)
Assignee | ||
Comment 25•12 years ago
|
||
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.
Attachment #725729 -
Flags: feedback?(vyang)
Comment 26•12 years ago
|
||
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.
Attachment #725729 -
Flags: feedback?(vyang) → feedback-
Comment 27•12 years ago
|
||
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
Attachment #725354 -
Flags: review?(vyang)
Comment 28•12 years ago
|
||
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.
Assignee | ||
Comment 29•12 years ago
|
||
(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•12 years ago
|
||
(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 :(
Flags: needinfo?(jonas)
Assignee | ||
Updated•12 years ago
|
Attachment #726499 -
Flags: review?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #726500 -
Flags: feedback?(vyang)
Assignee | ||
Comment 33•12 years ago
|
||
According to comment 30, we need more info to decide what we should do for other API. Implementing send() here.
Attachment #725356 -
Attachment is obsolete: true
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #726500 -
Attachment is obsolete: true
Attachment #726500 -
Flags: feedback?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #726586 -
Flags: review?(vyang)
Attachment #726586 -
Flags: review?(josh)
Assignee | ||
Updated•12 years ago
|
Attachment #726584 -
Flags: review?(vyang)
Attachment #726584 -
Flags: review?(josh)
Comment 37•12 years ago
|
||
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
Attachment #726499 -
Flags: review?(vyang) → review+
Comment 38•12 years ago
|
||
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(...);
Attachment #726583 -
Flags: review?(vyang) → review+
Comment 39•12 years ago
|
||
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.
Attachment #726584 -
Flags: review?(vyang) → review+
Comment 40•12 years ago
|
||
(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
Updated•12 years ago
|
Attachment #726584 -
Flags: review?(josh) → review+
Comment 41•12 years ago
|
||
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.
Attachment #726586 -
Flags: review?(josh) → review-
Assignee | ||
Comment 42•12 years ago
|
||
(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•12 years ago
|
||
Depends on bug 749086 to make life here easier.
Depends on: 749086
Flags: needinfo?(jonas)
Assignee | ||
Comment 44•12 years ago
|
||
WIP for rebasing on bug 749086 - https://github.com/kk1fff/releases-mozilla-central/tree/b/847744
Assignee | ||
Comment 45•12 years ago
|
||
Requesting review again for adding fallback/MmsService.cpp and fallback/MmsService.h.
Attachment #726499 -
Attachment is obsolete: true
Attachment #731083 -
Flags: review?(vyang)
Assignee | ||
Comment 46•12 years ago
|
||
Rebasing without logical change. Keep r+ for this patch.
Attachment #726583 -
Attachment is obsolete: true
Attachment #731085 -
Flags: review+
Assignee | ||
Comment 47•12 years ago
|
||
Rebase without logical change. keep r+.
Attachment #726584 -
Attachment is obsolete: true
Attachment #731088 -
Flags: review+
Assignee | ||
Comment 49•12 years ago
|
||
Attachment #726586 -
Attachment is obsolete: true
Attachment #731089 -
Attachment is obsolete: true
Attachment #726586 -
Flags: review?(vyang)
Attachment #731102 -
Flags: review?(vyang)
Attachment #731102 -
Flags: review?(josh)
Comment 50•12 years ago
|
||
leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed
Whiteboard: [target 3/29] → [target 3/29] NO_UPLIFT
Assignee | ||
Comment 51•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #732278 -
Flags: review?(vyang)
Updated•12 years ago
|
Whiteboard: [target 3/29] NO_UPLIFT → [NO_UPLIFT]
Comment 52•12 years ago
|
||
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(), > + ¶ms)) { > + NS_WARNING("SmsRequestParent: Fail to build MMS params."); > + return false; Same child killing behaviour. @@ +433,5 @@ > + ¶ms)) { > + 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.
Attachment #731102 -
Flags: review?(mrbkap)
Attachment #731102 -
Flags: review?(josh)
Attachment #731102 -
Flags: review+
Comment 53•12 years ago
|
||
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.
Attachment #731083 -
Flags: review?(vyang) → review+
Comment 54•12 years ago
|
||
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(...);
Attachment #731102 -
Flags: review?(vyang) → review+
Comment 55•12 years ago
|
||
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.
Attachment #732278 -
Flags: review?(vyang)
Comment 56•12 years ago
|
||
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.
Attachment #731102 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 57•12 years ago
|
||
address comment 53
Attachment #731083 -
Attachment is obsolete: true
Attachment #735084 -
Flags: review+
Assignee | ||
Comment 58•12 years ago
|
||
Address comment 52, comment 54 and comment 56
Attachment #731102 -
Attachment is obsolete: true
Attachment #735085 -
Flags: review+
Assignee | ||
Comment 59•12 years ago
|
||
Address comment 55
Attachment #732278 -
Attachment is obsolete: true
Attachment #735086 -
Flags: review?(vyang)
Updated•12 years ago
|
Attachment #735086 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 60•12 years ago
|
||
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 66•12 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Assignee | ||
Comment 67•12 years ago
|
||
WIP for rebasing on b2g18 https://github.com/kk1fff/mozilla-central/tree/b/847744/b2g18
Comment 68•12 years ago
|
||
(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•12 years ago
|
||
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
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → wontfix
status-firefox23:
--- → fixed
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
•