Closed
Bug 844431
Opened 12 years ago
Closed 12 years ago
B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: airpingu, Assigned: airpingu)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [RIL_INTERFACE])
Attachments
(16 files, 44 obsolete files)
After bug 844429 is done, then we can start to design a new |nsIDOMMobileMessageManager| interface, which is going to be a direct copy from |nsIDOMSmsManager|. We're hoping to reserve the existing |nsIDOMSmsManager| to avoid breaking the compatibilities with content. In the future, we'll deprecate the |nsIDOMSmsManager| gradually and then move it into |nsIDOMMobileMessageManager|.
All the implementations of |nsIDOMMobileMessageManager| will be hooked up to the current SMS implementations of |nsIDOMSmsManager|. Due to the MMS timeline, we don't attempt to s/SMS/MobileMessage for now, which is a super big work and we'll revisit this in the future.
Assignee | ||
Updated•12 years ago
|
Summary: B2G MMS: provide nsIDOMMobileMessageManager interface → B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Comment 1•12 years ago
|
||
leo+, this bug is needed to fulfill MMS user stories for v1.1
blocking-b2g: --- → leo+
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #718349 -
Flags: feedback?(vyang)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #718350 -
Flags: feedback?(vyang)
Assignee | ||
Comment 4•12 years ago
|
||
Hi Vicamo,
This is a very quick draft. Could you please take a look to let me know if I'm on the right way? Thanks!
Comment 5•12 years ago
|
||
Comment on attachment 718349 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager.sendMMS()
Review of attachment 718349 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +88,5 @@
> +
> + // Check the Mms Service:
> + nsCOMPtr<nsIMmsService> mmsService = do_GetService(RIL_MMSSERVICE_CONTRACTID);
> + NS_ENSURE_TRUE(mmsService, nullptr);
> + mmsService->HasSupport(&result);
Having two checks here is really strange for me. Please remove both HasSupport() checks here. MOZ_WEBSMS_BACKEND above should be sufficient.
@@ +236,5 @@
> + nsCOMPtr<nsIDOMDOMRequest> request;
> + nsresult rv = rs->CreateRequest(GetOwner(), getter_AddRefs(request));
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + // TODO: use |mmsService| to send the MMS message.
You can simply return NS_ERROR_NOT_IMPLEMENTED at the very beginning of this function and move all the implmentation code to your next patch.
@@ +299,5 @@
> +MobileMessageManager::GetMessages(nsIDOMMozSmsFilter* aFilter, bool aReverse,
> + nsIDOMMozSmsRequest** aRequest)
> +{
> + nsCOMPtr<nsIDOMMozSmsFilter> filter = aFilter;
> +
trailing ws.
::: dom/mobilemessage/src/SmsRequest.cpp
@@ +13,5 @@
> #include "nsPIDOMWindow.h"
> #include "SmsCursor.h"
> #include "SmsMessage.h"
> #include "SmsManager.h"
> +#include "MobileMessageManager.h"
Please move to the next patch.
::: dom/mobilemessage/src/SmsRequest.h
@@ +47,5 @@
> nsCOMPtr<nsISmsRequest> mRealRequest;
> };
>
> class SmsManager;
> +class MobileMessageManager;
ditto.
Attachment #718349 -
Flags: feedback?(vyang) → feedback+
Comment 6•12 years ago
|
||
Comment on attachment 718350 [details] [diff] [review]
Part 2, nsIMmsService.send()
Review of attachment 718350 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mms/src/ril/MmsService.js
@@ +871,5 @@
> },
>
> + send: function send(aParams, aRequest) {
> + // TODO: to prepare the message object.
> + let message = {};
You will implement it, right?
Attachment #718350 -
Flags: feedback?(vyang) → feedback+
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #5)
Thank you for the feedback!
> ::: dom/mobilemessage/src/SmsRequest.cpp
> @@ +13,5 @@
> > #include "nsPIDOMWindow.h"
> > #include "SmsCursor.h"
> > #include "SmsMessage.h"
> > #include "SmsManager.h"
> > +#include "MobileMessageManager.h"
>
> Please move to the next patch.
I don't think we need to move this to the next patch because this is needed for
SmsRequest(MobileMessageManager* aManager);
which has nothing to do with the new |nsIDOMDOMRequest| stuff. Right?
>
> ::: dom/mobilemessage/src/SmsRequest.h
> @@ +47,5 @@
> > nsCOMPtr<nsISmsRequest> mRealRequest;
> > };
> >
> > class SmsManager;
> > +class MobileMessageManager;
>
> ditto.
The same. I think we have to keep it in this patch.
Updated•12 years ago
|
Whiteboard: [by 3/8]
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #718349 -
Attachment is obsolete: true
Attachment #720648 -
Flags: feedback?(vyang)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #720649 -
Flags: feedback?(vyang)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #720650 -
Flags: feedback?(vyang)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #718350 -
Attachment is obsolete: true
Attachment #720652 -
Flags: feedback?(vyang)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #720654 -
Flags: feedback?(vyang)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #720655 -
Flags: feedback?(vyang)
Comment 14•12 years ago
|
||
Comment on attachment 720648 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager
Review of attachment 720648 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mms/interfaces/nsIMmsService.idl
@@ +4,5 @@
>
> #include "nsISupports.idl"
>
> +%{C++
> +#define RIL_MMSSERVICE_CONTRACTID "@mozilla.org/mms/rilmmsservice;1"
Not needed in this patch. Besides, that's a backend specific contract id. Do we really need it to be in a generic service interface idl?
::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +64,5 @@
> + bool enabled = false;
> + Preferences::GetBool("dom.sms.enabled", &enabled);
> + NS_ENSURE_TRUE(enabled, nullptr);
> +
> + nsCOMPtr<nsIPermissionManager> permMgr =
I know that's just copied from SmsManager, but newly added DOM components use Navigator::CheckPermission() instead. Please refer to Voicemail, CellBroadcast, Telephony, etc.
Attachment #720648 -
Flags: feedback?(vyang) → feedback+
Updated•12 years ago
|
Attachment #720655 -
Flags: feedback?(vyang) → feedback+
Updated•12 years ago
|
Attachment #720654 -
Flags: feedback?(vyang) → feedback+
Comment 15•12 years ago
|
||
Comment on attachment 720652 [details] [diff] [review]
Part 4, nsIMmsService.send()
Review of attachment 720652 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mms/interfaces/nsIMmsService.idl
@@ +10,5 @@
> %{C++
> #define RIL_MMSSERVICE_CONTRACTID "@mozilla.org/mms/rilmmsservice;1"
> %}
>
> +dictionary MMSAttachment
Anyway to share these dictionary types with MobileMessageManager.idl?
@@ +29,4 @@
> [scriptable, uuid(217ddd76-75db-4210-955d-8806cd8d87f9)]
> interface nsIMmsService : nsISupports
> {
> + boolean hasSupport();
We don't need hasSupport() anymore. Please remove it.
::: dom/mms/src/ril/MmsService.js
@@ +912,5 @@
> + // - |message.deliveryStatus|
> + // - |message.timestamp|
> + message["type"] = "mms";
> + message["receiver"] = aParams.receivers[0];
> + message["deliveryStatus"] = "pending";
There are actually multiple delivery statuses in MMS. One for each receiver. And this field should better be initialized in saveSendingMessage() instead.
@@ +917,5 @@
> + message["timestamp"] = Date.now();
> +
> + message["receivers"] = receivers;
> + message["smil"] = aParams.smil;
> + message["attachments"] = aParams.attachments;
There are no "smil" & "attachments" fields in a MMS database record. Assign to message.parts instead.
::: dom/mobilemessage/interfaces/nsIMobileMessageService.idl
@@ +13,5 @@
> +
> +[scriptable, builtinclass, uuid(4cbc9594-84c3-11e2-a274-ebada93fa6cd)]
> +interface nsIMobileMessageService : nsISupports
> +{
> + boolean hasSupport();
We don't really need it, right?
::: dom/mobilemessage/src/MobileMessageServicesFactory.h
@@ +13,5 @@
> +namespace mozilla {
> +namespace dom {
> +namespace mobilemessage {
> +
> +class MobileMessageServicesFactory
I would rather rename SmsServicesFactory to MobileMessageServicesFactory and add one creation function instead.
Attachment #720652 -
Flags: feedback?(vyang)
Comment 16•12 years ago
|
||
Comment on attachment 720650 [details] [diff] [review]
Part 3, nsIMobileMessageRequest
Review of attachment 720650 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/MobileMessageRequest.cpp
@@ +32,5 @@
> +MobileMessageRequest::Create(MobileMessageManager* aManager,
> + nsIDOMDOMRequest* aDOMRequest)
> +{
> + nsCOMPtr<nsIMobileMessageRequest> request =
> + new MobileMessageRequest(aManager, aDOMRequest);
This Create() does nothing but |new mobileMessageRequest()|. So I don't think we need it.
@@ +55,5 @@
> + nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(mManager->GetOwner());
> + NS_ENSURE_TRUE(sgo, NS_ERROR_FAILURE);
> +
> + nsIScriptContext *scriptContext = sgo->GetScriptContext();
> + NS_ENSURE_TRUE(scriptContext, NS_ERROR_FAILURE);
Can we call |mDOMRequest->GetContextForEventHandlers(&rv)| instead and remove mManager from MobileMessageManager?
::: dom/mobilemessage/src/MobileMessageRequest.h
@@ +19,5 @@
> +
> +namespace mobilemessage {
> +
> +// We need this forwarder to avoid a QI to nsIClassInfo.
> +// See: https://bugzilla.mozilla.org/show_bug.cgi?id=775997#c51
We don't need a forwarder for MobileMessageRequest because we don't return it to content anymore. That's different from SmsRequest.
@@ +51,5 @@
> + static already_AddRefed<nsIMobileMessageRequest>
> + Create(MobileMessageManager* aManager, nsIDOMDOMRequest* aDOMRequest);
> +
> +private:
> + MobileMessageRequest() MOZ_DELETE;
Don't need it.
Attachment #720650 -
Flags: feedback?(vyang)
Comment 17•12 years ago
|
||
Comment on attachment 720649 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage
Review of attachment 720649 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl
@@ +12,5 @@
> + /**
> + * Should be "not-downloaded", "received", "sending", "sent" or "error".
> + */
> + readonly attribute DOMString deliveryState;
> + readonly attribute DOMString deliveryStatus;
deliveryStatus in MMS is an array.
@@ +22,5 @@
> +
> + [implicit_jscontext]
> + readonly attribute jsval timestamp; // Date
> +
> + readonly attribute boolean read;
read in MMS is an array.
::: dom/mobilemessage/src/Constants.h
@@ +21,5 @@
> +#define DELIVERY_RECEIVED NS_LITERAL_STRING("received")
> +#define DELIVERY_SENDING NS_LITERAL_STRING("sending")
> +#define DELIVERY_SENT NS_LITERAL_STRING("sent")
> +#define DELIVERY_ERROR NS_LITERAL_STRING("error")
> +#define DELIVERY_NOT_DOWNLOADED NS_LITERAL_STRING("not-downloaded")
trailing ws
::: dom/mobilemessage/src/MmsMessage.cpp
@@ +308,5 @@
> +
> + JSObject* attachments = JS_NewArrayObject(aCx, length, nullptr);
> + if (!attachments) {
> + return NS_ERROR_OUT_OF_MEMORY;
> + }
ditto
::: dom/mobilemessage/src/MmsMessage.h
@@ +45,5 @@
> + nsIDOMMozMmsMessage** aMessage);
> +
> +private:
> + // Don't try to use the default constructor.
> + MmsMessage();
Don't need it because we have already a non-trivial ctor.
Attachment #720649 -
Flags: feedback?(vyang)
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #720648 -
Attachment is obsolete: true
Attachment #721079 -
Flags: review?(vyang)
Comment 19•12 years ago
|
||
Comment on attachment 721079 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V2
Review of attachment 721079 [details] [diff] [review]:
-----------------------------------------------------------------
Also ask review from DOM peers.
Attachment #721079 -
Flags: review?(vyang)
Attachment #721079 -
Flags: review?(mounir)
Attachment #721079 -
Flags: feedback+
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #720649 -
Attachment is obsolete: true
Attachment #721176 -
Flags: feedback?(vyang)
Attachment #721176 -
Flags: feedback?(mounir)
Assignee | ||
Comment 21•12 years ago
|
||
We hope to do s/nsISmsRequest/nsIMobileMessageCallback, which sounds more generic to be inherited by the original |SmsRequest| and a new |MobileMessageRequest| (please see the part 3-2 patch).
Attachment #721179 -
Flags: feedback?(vyang)
Attachment #721179 -
Flags: feedback?(mounir)
Assignee | ||
Comment 22•12 years ago
|
||
To provide the following 2 callbacks used for sending MMS:
nsIMobileMessageCallback.notifyMmsMessageSent(in nsIDOMMozMmsMessage message);
nsIMobileMessageCallback.notifySendMmsMessageFailed(in long error);
which are inherited and implemented by the new |MobileMessageRequest| class.
Attachment #720650 -
Attachment is obsolete: true
Attachment #721180 -
Flags: feedback?(vyang)
Attachment #721180 -
Flags: feedback?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #721179 -
Attachment description: Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback → Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V2
Assignee | ||
Comment 23•12 years ago
|
||
Attachment #721182 -
Flags: feedback?
Assignee | ||
Comment 24•12 years ago
|
||
Comment on attachment 721182 [details] [diff] [review]
Part 4, nsIMmsService.send(), V2
Addressing (some) issues at comment #15.
Attachment #721182 -
Flags: feedback?(vyang)
Attachment #721182 -
Flags: feedback?(mounir)
Attachment #721182 -
Flags: feedback?
Assignee | ||
Updated•12 years ago
|
Attachment #720652 -
Attachment is obsolete: true
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #720654 -
Attachment is obsolete: true
Attachment #721184 -
Flags: feedback+
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #720655 -
Attachment is obsolete: true
Attachment #721185 -
Flags: feedback+
Assignee | ||
Updated•12 years ago
|
Attachment #721184 -
Flags: feedback?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #721185 -
Flags: feedback?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #721182 -
Flags: feedback?(vyang)
Attachment #721182 -
Flags: feedback?(mounir)
Assignee | ||
Comment 28•12 years ago
|
||
Addressing comment #17 except for the read stuff, which should be a bool value for this moment.
Attachment #721176 -
Attachment is obsolete: true
Attachment #721176 -
Flags: feedback?(vyang)
Attachment #721176 -
Flags: feedback?(mounir)
Attachment #721609 -
Flags: feedback?(vyang)
Attachment #721609 -
Flags: feedback?(mounir)
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #721612 -
Flags: feedback?(vyang)
Assignee | ||
Comment 30•12 years ago
|
||
Attachment #721182 -
Attachment is obsolete: true
Attachment #721613 -
Flags: feedback?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #721612 -
Attachment description: Part 4-1, multiple delivery status → Part 4-1, multiple delivery status, V3
Assignee | ||
Comment 31•12 years ago
|
||
Attachment #721613 -
Attachment is obsolete: true
Attachment #721613 -
Flags: feedback?(vyang)
Attachment #721662 -
Flags: feedback?(vyang)
Assignee | ||
Comment 32•12 years ago
|
||
Hi Mounir,
Do you have time to go through the DOM codes changes in these days? We were asked to finish the Gecko part by this weekend so that the Gaia guys can start on the UI design and get everything done before the shipping date on 3/15 (a crazy deadline I know...).
Note that these patches simply provide non-IPC functions due to the time line. We hope to let Gaia folks be able to have some initiative APIs to try in the first step and we can work on the IPC part to support multi-process at the same time. We're addressing the IPC issue at bug 847744.
Updated•12 years ago
|
Attachment #721179 -
Flags: feedback?(vyang) → feedback+
Comment 33•12 years ago
|
||
Comment on attachment 721179 [details] [diff] [review]
Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V2
Review of attachment 721179 [details] [diff] [review]:
-----------------------------------------------------------------
Some interface UUIDs remain unchanged. Althought they might be changed in latter patches, but just want to ensure we won't miss any one.
::: dom/mobilemessage/interfaces/nsIMobileMessageDatabaseService.idl
@@ +16,3 @@
>
> [scriptable, uuid(ce9232ca-6a08-11e2-b971-c795004622e7)]
> interface nsIMobileMessageDatabaseService : nsISupports
uuid
::: dom/mobilemessage/interfaces/nsISmsService.idl
@@ +13,5 @@
> #define SMS_SERVICE_CONTRACTID "@mozilla.org/sms/smsservice;1"
> %}
>
> [scriptable, builtinclass, uuid(4310bdb5-eefa-4f70-965a-74041228ab26)]
> interface nsISmsService : nsISupports
uuid
::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +316,5 @@
> nsIDOMMozSmsSegmentInfo getSegmentInfoForText(in DOMString text);
>
> void sendSMS(in DOMString number,
> in DOMString message,
> + in nsIMobileMessageCallback request);
uuid
Assignee | ||
Comment 34•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #33)
> Comment on attachment 721179 [details] [diff] [review]
> Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V2
>
> Review of attachment 721179 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Some interface UUIDs remain unchanged. Althought they might be changed in
> latter patches, but just want to ensure we won't miss any one.
Thanks for catching this. Fixed in my local change.
Comment 35•12 years ago
|
||
Comment on attachment 721180 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMmsMessageSent(), V2
Review of attachment 721180 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl
@@ +32,5 @@
> void notifyMessageSent(in nsIDOMMozSmsMessage message);
> void notifySendMessageFailed(in long error);
>
> + void notifyMmsMessageSent(in nsIDOMMozMmsMessage message);
> + void notifySendMmsMessageFailed(in long error);
We can share |notifySendMessageFailed| here. And please also rename |notifyMessageSent| to |notifySmsMessageSent|.
::: dom/mobilemessage/src/MobileMessageRequest.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_mobilemessage_MobileMessageRequest_h
> +#define mozilla_dom_mobilemessage_MobileMessageRequest_h
Since the interface is named nsIMobileMessageCallback, shouldn't this be named MobileMessageCallback instead?
Attachment #721180 -
Flags: feedback?(vyang)
Updated•12 years ago
|
Attachment #721609 -
Flags: feedback?(vyang) → feedback+
Comment 36•12 years ago
|
||
Comment on attachment 721612 [details] [diff] [review]
Part 4-1, multiple delivery status, V3
Review of attachment 721612 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +780,5 @@
> return;
> }
>
> + // MMS-specific checks.
> + if (aMessage.type === "mms") {
No, please just initialize |aMessage.deliveryStatus| in MmsService. Or better, rename |aMessage.deliveryStatus| to |aMessage.deliveryStatusRequested| and let's initialize |aMessage.deliveryStatus| accordingly.
@@ +873,5 @@
> + debug("This is not a valid MMS record. Fail to set delivery status.");
> + }
> + return;
> + }
> + let index = record.receivers.indexOf(receiver);
Previous declaration of |index| is in line 868.
@@ +892,5 @@
> + }
> +
> + if (Array.isArray(record.deliveryStatus)) {
> + let statusArray = record.deliveryStatus;
> + if (receiver) {
setMessageDelivery() changes delivery state and status for one and only one receiver.
@@ +898,5 @@
> + statusArray[index] = deliveryStatus;
> + isRecordChanged = true;
> + }
> + } else {
> + for (int i = 0; i < statusArray.length; i++) {
ditto.
@@ +906,5 @@
> + }
> + }
> + }
> + } else {
> + if (record.deliveryStatus != deliveryStatus) {
|else if|
Attachment #721612 -
Flags: feedback?(vyang)
Comment 37•12 years ago
|
||
Comment on attachment 721662 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V4
Review of attachment 721662 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mms/src/ril/MmsService.js
@@ +911,5 @@
> + let message = {};
> +
> + // The following 4 attributes are needed for DB.
> + // - |message.type|
> + // - |message.receiver|
Please move this code segment to a new function named, for example, createMmsRecordFromMessage() and return a newly created record or null upon errors.
@@ +971,5 @@
> + // Succeed to send.
> + gMobileMessageDatabaseService.setMessageDelivery(sendingRecord.id,
> + null,
> + "sent",
> + "success",
Wow, that's a problem. :(
@@ +979,5 @@
> + Services.obs.notifyObservers(sentMmsMessage, kMmsSentObserverTopic, null);
> + }.bind(this));
> + } else {
> + // Fail to send.
> + aRequest.notifySendMmsMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR);
bail out early:
if (aMmsStatus == MMS.MMS_PDU_ERROR_OK) {
aRequest.notifySendMmsMessageFailed(...);
return;
}
::: dom/mobilemessage/src/SmsServicesFactory.cpp
@@ +33,5 @@
> return smsService.forget();
> }
>
> +/* static */ already_AddRefed<nsIMobileMessageService>
> +MobileMessageServicesFactory::CreateMobileMessageService()
Sorry, I should have known. MobileMessageService will be the same class in both content and chrome process. We don't really need a factory for it. That means you don't have to rename SmsServicesFactory here but:
1) create nsIMobileMessageService by MobileMessageService::getInstance()
2) move nsISmsService::createSmsMessage to nsIMobileMessageService::createSmsMessage.
::: dom/mobilemessage/src/ril/MobileMessageService.h
@@ +16,5 @@
> +{
> +public:
> + NS_DECL_ISUPPORTS
> + NS_DECL_NSIMOBILEMESSAGESERVICE
> + MobileMessageService();
New line
Attachment #721662 -
Flags: feedback?(vyang)
Assignee | ||
Comment 38•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #35)
> Comment on attachment 721180 [details] [diff] [review]
> Part 3-2, nsIMobileMessageCallback.notifyMmsMessageSent(), V2
>
> Review of attachment 721180 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl
> @@ +32,5 @@
> > void notifyMessageSent(in nsIDOMMozSmsMessage message);
> > void notifySendMessageFailed(in long error);
> >
> > + void notifyMmsMessageSent(in nsIDOMMozMmsMessage message);
> > + void notifySendMmsMessageFailed(in long error);
>
> We can share |notifySendMessageFailed| here. And please also rename
> |notifyMessageSent| to |notifySmsMessageSent|.
Per off-line discussion with Vicamo, we think we can even share |notifyMessageSent(...)| for both |nsIDOMMozSmsMessage| and |nsIDOMMozMmsMessage|. That is,
void notifyMessageSent(in nsISupports message);
where the |message| can be either |nsIDOMMozSmsMessage| or |nsIDOMMozMmsMessage|. In this way, we don't need to create SMS/MMS specific functions.
Assignee | ||
Comment 39•12 years ago
|
||
Attachment #721180 -
Attachment is obsolete: true
Attachment #721180 -
Flags: feedback?(mounir)
Attachment #722083 -
Flags: feedback?(vyang)
Attachment #722083 -
Flags: feedback?(mounir)
Comment 40•12 years ago
|
||
Comment on attachment 722083 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V3
Review of attachment 722083 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/MobileMessageCallback.cpp
@@ +82,5 @@
> + MOZ_ASSERT(false, "Unknown error value.");
> + }
> +
> + nsCOMPtr<nsIDOMRequestService> rs = do_GetService(DOMREQUEST_SERVICE_CONTRACTID);
> + NS_ENSURE_TRUE(rs, NS_ERROR_FAILURE);
Move the two lines to the beginning of this function, so we can bail out early. Then just have:
case nsIMobileMessageCallback::NO_SIGNAL_ERROR:
return rs->FireError(mDOMRequest, NS_LITERAL_STRING("NoSignalError"));
Attachment #722083 -
Flags: feedback?(vyang) → feedback+
Assignee | ||
Comment 41•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #40)
> Comment on attachment 722083 [details] [diff] [review]
> Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V3
>
> Review of attachment 722083 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/mobilemessage/src/MobileMessageCallback.cpp
> @@ +82,5 @@
> > + MOZ_ASSERT(false, "Unknown error value.");
> > + }
> > +
> > + nsCOMPtr<nsIDOMRequestService> rs = do_GetService(DOMREQUEST_SERVICE_CONTRACTID);
> > + NS_ENSURE_TRUE(rs, NS_ERROR_FAILURE);
>
> Move the two lines to the beginning of this function
Thanks for the review. Fixed in my local change.
Assignee | ||
Comment 42•12 years ago
|
||
Addressing comment #36. Thanks for the review again!
Attachment #721612 -
Attachment is obsolete: true
Attachment #722121 -
Flags: feedback?(vyang)
Comment 43•12 years ago
|
||
Comment on attachment 722121 [details] [diff] [review]
Part 4-1, multiple delivery status, V4
Review of attachment 722121 [details] [diff] [review]:
-----------------------------------------------------------------
f+ with the two redundent blocks removed.
::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +900,5 @@
> + return;
> + }
> + let receivers = record.receivers;
> + let deliveryStatusArray = record.deliveryStatus;
> + if (!Array.isArray(receivers) || !Array.isArray(deliveryStatusArray)) {
You don't have to validate data fetched from db.
@@ +925,5 @@
> + isRecordUpdated = true;
> + }
> + } else {
> + if (DEBUG) {
> + debug("Invalid record type. Fail to set delivery status.");
Will never reach here.
Attachment #722121 -
Flags: feedback?(vyang) → feedback+
Assignee | ||
Comment 44•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #43)
>
> f+ with the two redundent blocks removed.
Thanks for the review! Will be fixed in my local codes.
Assignee | ||
Comment 45•12 years ago
|
||
Addressing comment #37.
Thanks for the review! Nice suggestions!
Attachment #721662 -
Attachment is obsolete: true
Attachment #722146 -
Flags: feedback?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #721609 -
Flags: feedback?(mounir) → review?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #721179 -
Flags: feedback?(mounir) → review?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #722083 -
Flags: feedback?(mounir) → review?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #721184 -
Flags: feedback?(mounir) → review?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #721185 -
Flags: feedback?(mounir) → review?(mounir)
Comment on attachment 721079 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V2
Review of attachment 721079 [details] [diff] [review]:
-----------------------------------------------------------------
The interfaces here look fine. I didn't look at the implementation so great if vicamo or someone can do that.
Attachment #721079 -
Flags: superreview+
Comment on attachment 721184 [details] [diff] [review]
Part 5, nsIDOMMobileMessageManager.send(), V2
Review of attachment 721184 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
@@ +21,5 @@
> +dictionary MMSParameters
> +{
> + jsval receivers; // DOMString[]
> + DOMString? subject;
> + DOMString? smil;
I might have called this "body" rather than "smil", but totally not important at all. If it's annoying to change then don't bother.
Attachment #721184 -
Flags: superreview+
Attachment #721185 -
Flags: superreview+
Comment on attachment 721609 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V3
Review of attachment 721609 [details] [diff] [review]:
-----------------------------------------------------------------
I only looked at the interfaces here, so vicamo or someone should review the implementation.
::: dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl
@@ +25,5 @@
> + [implicit_jscontext]
> + readonly attribute jsval timestamp; // Date
> +
> + readonly attribute boolean read;
> + readonly attribute DOMString smil;
Actually, maybe sticking with "smil" is better since it makes it clear that it's the full smil document as opposed to for example a parsed set of slides.
@@ +28,5 @@
> + readonly attribute boolean read;
> + readonly attribute DOMString smil;
> +
> + [implicit_jscontext]
> + readonly attribute jsval attachments; // nsIDOMBlob[]
The comment here seems wrong. Isn't this an array of
{ contentId: "...", contentLocation: "...", content: obj }
objects?
Attachment #721609 -
Flags: superreview+
Comment on attachment 722083 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V3
Review of attachment 722083 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl
@@ +13,5 @@
> DOMString body;
> unsigned long long unreadCount;
> };
>
> +[scriptable, builtinclass, uuid(18d7c4da-848f-11e2-a3a5-f749c4ba31b5)]
Since this does change the binary API, it's good that you're changing the uuid here.
I looked at all the interface changes here for the public APIs that we expose to apps and they look great.
Still needs review of the actual code though of course.
Assignee | ||
Comment 51•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #50)
> I looked at all the interface changes here for the public APIs that we
> expose to apps and they look great.
>
> Still needs review of the actual code though of course.
All right. Thanks Jonas! So my understanding is it means we've got a DOM peer review here. Right? Vicamo should be able to take the review for implementation details (actually, we've already gone through most of them). Hope we can get this landed ASAP after everything is confirmed.
Assignee | ||
Comment 52•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #48)
> > + [implicit_jscontext]
> > + readonly attribute jsval attachments; // nsIDOMBlob[]
>
> The comment here seems wrong. Isn't this an array of
>
> { contentId: "...", contentLocation: "...", content: obj }
>
> objects?
Thanks Jonas for catching this! My bad! Will fix up all the implementations for it!
Assignee | ||
Updated•12 years ago
|
Attachment #721179 -
Flags: review?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #722083 -
Flags: review?(mounir)
Comment 53•12 years ago
|
||
Comment on attachment 722146 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V5
Review of attachment 722146 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mms/src/ril/MmsService.js
@@ +891,5 @@
> + let receivers = aParams.receivers;
> + for (let i = 0; i < receivers.length; i++) {
> + headersTo.push({"address": receivers[i], "type": "PLMN"});
> + }
> + headers["subject"] = aParams.subject;
Might have no subject at all. All values must be valid before passing to MmsPduHelper.compose().
@@ +899,5 @@
> + for (let i = 0; i < aParams.attachments.length; i++) {
> + let attachment = aParams.attachments[i];
> + let content = attachment.content;
> + let part = {
> + "index": i,
We don't need an |index| here.
@@ +919,5 @@
> + parts.push(part);
> + }
> +
> + // TODO: where to save SMIL?
> + message["smil"] = aParams.smil;
Insert SMIL as the first attachment. And don't forget SMIL could also be null in messages of simple Text content classes and messages with only PIM objects. See MMS-CONF section 7.1.8.
@@ +954,5 @@
> + let message = this.createSendableMmsMessageFromParams(aParams);
> +
> + // The following attributes are needed for saving message into DB.
> + message["type"] = "mms";
> + message["receiver"] = receivers[0];
We don't need this.
@@ +955,5 @@
> +
> + // The following attributes are needed for saving message into DB.
> + message["type"] = "mms";
> + message["receiver"] = receivers[0];
> + message["deliveryStatusRequested"] = true;
This should depend on another preference "requestDeliveryReport". I've create bug 849112 for this. Please just have a "confRequestDeliveryReport" in MmsService and set it to true.
@@ +968,5 @@
> + // Start to send.
> + let transaction = new SendTransaction(message);
> + transaction.run(function callback(aMmsStatus, aMsg) {
> + if (aMmsStatus != MMS.MMS_PDU_ERROR_OK) {
> + aRequest.notifySendMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR);
Also have to set delivery status to sent or error.
::: dom/mobilemessage/interfaces/nsISmsService.idl
@@ -24,5 @@
> in DOMString message,
> in nsIMobileMessageCallback request);
>
> - [implicit_jscontext]
> - nsIDOMMozSmsMessage createSmsMessage(in long id,
Yes! The last Prime is DEAD! Now I can move SMS code out of RadioInterfaceLayer.js :D
@@ -38,1 @@
> nsIDOMMozSmsSegmentInfo createSmsSegmentInfo(in long segments,
Well, I didn't notice there is still a |createSmsSegmentInfo()|. Could you also move it into nsIMobileMessageService? Please~
::: dom/mobilemessage/src/ril/SmsService.cpp
@@ -48,5 @@
> return NS_OK;
> }
>
> NS_IMETHODIMP
> -SmsService::CreateSmsMessage(int32_t aId,
I expect removals in fallback/ and android/ as well.
Attachment #722146 -
Flags: feedback?(vyang)
Assignee | ||
Comment 54•12 years ago
|
||
Addressing comment #48.
sr=sicking
Attachment #721609 -
Attachment is obsolete: true
Attachment #721609 -
Flags: review?(mounir)
Attachment #722680 -
Flags: superreview+
Attachment #722680 -
Flags: review?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #721079 -
Flags: review?(mounir) → review?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #721079 -
Flags: feedback+
Assignee | ||
Comment 55•12 years ago
|
||
Attachment #721179 -
Attachment is obsolete: true
Attachment #722682 -
Flags: review?(vyang)
Assignee | ||
Comment 56•12 years ago
|
||
Addressing comment #40.
Attachment #722083 -
Attachment is obsolete: true
Attachment #722683 -
Flags: review?(vyang)
Comment 57•12 years ago
|
||
Comment on attachment 721079 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V2
Review of attachment 721079 [details] [diff] [review]:
-----------------------------------------------------------------
Also ask review from DOM peers.
::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
Please use `hg cp` to minimize the differences.
::: dom/mobilemessage/interfaces/nsIDOMNavigatorMobileMessage.idl
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
Ditto.
::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
Ditto.
Attachment #721079 -
Flags: review?(vyang)
Assignee | ||
Comment 58•12 years ago
|
||
Addressing comment #43.
Attachment #722121 -
Attachment is obsolete: true
Attachment #722684 -
Flags: review?(vyang)
Comment 59•12 years ago
|
||
Comment on attachment 721184 [details] [diff] [review]
Part 5, nsIDOMMobileMessageManager.send(), V2
Review of attachment 721184 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
@@ +13,5 @@
>
> +dictionary MMSAttachment
> +{
> + DOMString? contentId;
> + DOMString? contentLocation;
I would have called those id and location. If you could change this, that would be great. Otherwise, this will happen later anyway.
::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +180,5 @@
> NS_IMETHODIMP
> +MobileMessageManager::SendMMS(const JS::Value& aParams, nsIDOMDOMRequest** aRequest)
> +{
> + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner());
> + NS_ADDREF(*aRequest = request);
I think it would be more appropriate to call this:
request.forget(aRequest);
at the end of the method (before the |return|).
@@ +183,5 @@
> + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner());
> + NS_ADDREF(*aRequest = request);
> +
> + nsCOMPtr<nsIMobileMessageCallback> msgRequest = new MobileMessageRequest(request);
> + NS_ENSURE_TRUE(msgRequest, NS_ERROR_FAILURE);
No need for that NS_ENSURE_TRUE. |new| is guaranteed to succeed.
@@ +190,5 @@
> + NS_ENSURE_TRUE(mmsService, NS_ERROR_FAILURE);
> +
> + mmsService->Send(aParams, msgRequest);
> +
> + return NS_OK;
return mmsService->Send(aParams, msgRequest);
Attachment #721184 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 60•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #53)
> Comment on attachment 722146 [details] [diff] [review]
> Part 4-2, nsIMmsService.send(), V5
>
> Review of attachment 722146 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/mms/src/ril/MmsService.js
> @@ +968,5 @@
> > + // Start to send.
> > + let transaction = new SendTransaction(message);
> > + transaction.run(function callback(aMmsStatus, aMsg) {
> > + if (aMmsStatus != MMS.MMS_PDU_ERROR_OK) {
> > + aRequest.notifySendMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR);
>
> Also have to set delivery status to sent or error.
Prefer filing another bug to do this later since it needs more changes to set multiple delivery states/status at one shot for .setMessageDelivery().
Thanks for the review!
Comment 61•12 years ago
|
||
Comment on attachment 721185 [details] [diff] [review]
Part 6, dispatch MMS events, V2
Review of attachment 721185 [details] [diff] [review]:
-----------------------------------------------------------------
The code looks good but there is no send failure handling nor delivery (success or failure). The send() feature should include those. Let me know if there are bugs for that. At a quick glance, it's not obvious that another patch is this bug takes care of it.
::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +325,5 @@
> + NS_NewDOMMozMmsEvent(getter_AddRefs(event), nullptr, nullptr);
> + NS_ASSERTION(event, "This should never fail!");
> +
> + nsCOMPtr<nsIDOMMozMmsEvent> se = do_QueryInterface(event);
> + MOZ_ASSERT(se);
nit: no need for the MOZ_ASSERT. We would crash the next line anyway...
@@ +422,5 @@
> + }
> +
> + DispatchTrustedMmsEventToSelf(SENT_EVENT_NAME, message);
> + return NS_OK;
> + }
I think it would be good to refactor this method to step duplicating all this code.
You could for example verify and set the |message| at the beginning of the method.
Attachment #721185 -
Flags: review?(mounir) → review-
Assignee | ||
Comment 62•12 years ago
|
||
Addressing comment #53.
Attachment #722146 -
Attachment is obsolete: true
Attachment #722708 -
Flags: review?(vyang)
Assignee | ||
Comment 63•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #61)
> Comment on attachment 721185 [details] [diff] [review]
> Part 6, dispatch MMS events, V2
>
> Review of attachment 721185 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> The code looks good but there is no send failure handling nor delivery
> (success or failure). The send() feature should include those. Let me know
> if there are bugs for that. At a quick glance, it's not obvious that another
> patch is this bug takes care of it.
Hi Mounir! Yes, you're right! The following patch already include that:
Part 4-2, nsIMmsService.send()
Sorry for the confusing.
Assignee | ||
Comment 64•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #59)
> Comment on attachment 721184 [details] [diff] [review]
> Part 5, nsIDOMMobileMessageManager.send(), V2
Thanks for all the reviews. I'll include them in my local change.
Assignee | ||
Comment 65•12 years ago
|
||
sr=sicking.
Addressing comment #57.
Attachment #721079 -
Attachment is obsolete: true
Attachment #722715 -
Flags: superreview+
Attachment #722715 -
Flags: review?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #722715 -
Flags: superreview+
Attachment #722715 -
Flags: review?(vyang)
Assignee | ||
Comment 66•12 years ago
|
||
sr=sicking.
Addressing comment #57.
Attachment #722715 -
Attachment is obsolete: true
Attachment #722724 -
Flags: superreview+
Attachment #722724 -
Flags: review?(vyang)
Assignee | ||
Comment 67•12 years ago
|
||
Addressing comment #59.
r=vicamo,mounir sr=sicking
Attachment #721184 -
Attachment is obsolete: true
Attachment #722728 -
Flags: superreview+
Attachment #722728 -
Flags: review+
Comment 68•12 years ago
|
||
Comment on attachment 722724 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V4
Review of attachment 722724 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you :)
Attachment #722724 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 69•12 years ago
|
||
Addressing comment #61.
Hi Mounir! Yes, you're right! The Part 4-2 patch already included:
Services.obs.notifyObservers(mmsMessage, kMmsSendingObserverTopic, null);
Services.obs.notifyObservers(mmsMessage, kMmsSentObserverTopic, null);
This patch only handles the events dispatching to the content from manager.
Btw, I don't think we could have big improvement for refactoring the codes becasue we use different types to catch do_QueryInterface() for SMS and MMS like:
nsCOMPtr<nsIDOMMozSmsMessage> message = do_QueryInterface(aSubject);
nsCOMPtr<nsIDOMMozMmsMessage> message = do_QueryInterface(aSubject);
Also, the following functions are also different for its implementations:
DispatchTrustedSmsEventToSelf(...)
DispatchTrustedMmsEventToSelf(...)
Do you have better ideas for improving them? Or just let it go?
Attachment #721185 -
Attachment is obsolete: true
Attachment #722746 -
Flags: superreview+
Attachment #722746 -
Flags: review?(mounir)
Updated•12 years ago
|
Attachment #722682 -
Flags: review?(vyang) → review+
Comment 70•12 years ago
|
||
Comment on attachment 722682 [details] [diff] [review]
Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V3
Review of attachment 722682 [details] [diff] [review]:
-----------------------------------------------------------------
nits: please wrap long lines.
Comment 71•12 years ago
|
||
Comment on attachment 722683 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V4
Review of attachment 722683 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/MobileMessageCallback.cpp
@@ +69,5 @@
> +
> + switch (aError) {
> + case nsIMobileMessageCallback::NO_SIGNAL_ERROR:
> + rs->FireError(mDOMRequest, NS_LITERAL_STRING("NoSignalError"));
> + break;
Please see my comment #40. |return rs->FireError(...);|
Attachment #722683 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 72•12 years ago
|
||
Addressing comment #48 and comment #comment #59.
sr=sicking
Attachment #722680 -
Attachment is obsolete: true
Attachment #722680 -
Flags: review?(vyang)
Attachment #722751 -
Flags: superreview+
Attachment #722751 -
Flags: review?(vyang)
Attachment #722751 -
Flags: review?(mounir)
Assignee | ||
Comment 73•12 years ago
|
||
Addressing comment #53 and comment #59 for:
s/contentId/id
s/contentLocation/location
Attachment #722708 -
Attachment is obsolete: true
Attachment #722708 -
Flags: review?(vyang)
Attachment #722752 -
Flags: review?(vyang)
Updated•12 years ago
|
Attachment #722684 -
Flags: review?(vyang) → review+
Comment 74•12 years ago
|
||
Comment on attachment 722752 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V7
Review of attachment 722752 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mms/src/ril/MmsService.js
@@ +926,5 @@
> + "content-type": {
> + "media": content.type,
> + "params": {
> + "charset": {
> + "charset": "utf-8"
We don't need charset for every attachment. Please remove it.
@@ +991,5 @@
> + // Start to send.
> + let transaction = new SendTransaction(message);
> + transaction.run(function callback(aMmsStatus, aMsg) {
> + if (aMmsStatus != MMS.MMS_PDU_ERROR_OK) {
> + // TODO: we need to use .setMessageDelivery() to set the
Whenever we're placing a TODO in comments, it must be followed by a bug id.
Attachment #722752 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 75•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #70)
> Comment on attachment 722682 [details] [diff] [review]
> Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V3
>
> Review of attachment 722682 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> nits: please wrap long lines.
Done in my local codes. Thanks!
Assignee | ||
Comment 76•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #71)
> Comment on attachment 722683 [details] [diff] [review]
> Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V4
>
> Review of attachment 722683 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/mobilemessage/src/MobileMessageCallback.cpp
> @@ +69,5 @@
> > +
> > + switch (aError) {
> > + case nsIMobileMessageCallback::NO_SIGNAL_ERROR:
> > + rs->FireError(mDOMRequest, NS_LITERAL_STRING("NoSignalError"));
> > + break;
>
> Please see my comment #40. |return rs->FireError(...);|
Thanks! Done in my local change!
Assignee | ||
Comment 77•12 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #74)
> Comment on attachment 722752 [details] [diff] [review]
> Part 4-2, nsIMmsService.send(), V7
>
> Review of attachment 722752 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/mms/src/ril/MmsService.js
>
> We don't need charset for every attachment. Please remove it.
>
> Whenever we're placing a TODO in comments, it must be followed by a bug id.
The abobe two comments are done in my local codes. Thanks for the review!
Assignee | ||
Comment 78•12 years ago
|
||
Comment on attachment 722746 [details] [diff] [review]
Part 6, dispatch MMS events, V3
Hi Mounir and Vicamo,
Sorry for bothering but we're hurry to get this done. The part-2 and part-6 patches still need (either of) your final review. I can work during the weekend. Hope I can get your feedback if possible.
Attachment #722746 -
Flags: review?(vyang)
Assignee | ||
Comment 80•12 years ago
|
||
Hi Vicamo,
One quick question when merging your latest .saveSendingMessage(). So we need to do something like below for SMS/MMS specific branches. Right?
saveSendingMessage: function saveSendingMessage(aMessage, aCallback) {
if (aMessage.type === undefined ||
(aMessage.type == "sms" && aMessage.receiver === undefined) ||
(aMessage.type == "mms" && aMessage.receiver*s* === undefined) ||
aMessage.deliveryStatus === undefined ||
aMessage.timestamp === undefined) {
if (aCallback) {
aCallback.notify(Cr.NS_ERROR_FAILURE, null);
}
return;
}
(skip...)
let addresses;
if (aMessage.type == "sms") {
addresses = [aMessage.receiver];
} else if (aMessage.type == "mms") {
addresses = aMessage.receiver*s*;
}
return this.saveRecord(aMessage, addresses, aCallback);
}
Flags: needinfo?(vyang)
Comment 81•12 years ago
|
||
Now, text and image can be sent on local build.
Comment 82•12 years ago
|
||
It's under review right now.
Hi, Mounir, and Vicamo, could you help to review and tell us how long you need to review on this bug? Gene can also provide a local build for testing if it's required. Thanks.
Flags: needinfo?(mounir)
Updated•12 years ago
|
Attachment #722746 -
Flags: review?(vyang) → review+
Comment 83•12 years ago
|
||
Comment on attachment 722752 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V7
Review of attachment 722752 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mms/src/ril/MmsService.js
@@ +881,5 @@
> let messageId = msg.headers["message-id"];
> debug("handleDeliveryIndication: got delivery report for " + messageId);
> },
>
> + createSendableMmsMessageFromParams: function createSendableMmsMessageFromParams(aParams) {
Have a |createSavableFromParams()| in symmertic to bug 845643.
@@ +980,5 @@
> + // The following attributes are needed for saving message into DB.
> + message["type"] = "mms";
> + message["deliveryStatusRequested"] = true;
> + message["timestamp"] = Date.now();
> + message["receivers"] = receivers;
Move these lines into |createSavableFromParams()| and always use 'receiver' (no 's').
@@ +988,5 @@
> + let mmsMessage = this.createMmsMessageFromRecord(record);
> + Services.obs.notifyObservers(mmsMessage, kMmsSendingObserverTopic, null);
> +
> + // Start to send.
> + let transaction = new SendTransaction(message);
new SendTransaction(record);
Flags: needinfo?(vyang)
Comment 84•12 years ago
|
||
Comment on attachment 722751 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V5
Review of attachment 722751 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/MmsMessage.h
@@ +53,5 @@
> + JSContext* aCx,
> + nsIDOMMozMmsMessage** aMessage);
> +
> +private:
> +
trailing ws
Attachment #722751 -
Flags: review?(vyang) → review+
Comment 85•12 years ago
|
||
Comment on attachment 722746 [details] [diff] [review]
Part 6, dispatch MMS events, V3
Review of attachment 722746 [details] [diff] [review]:
-----------------------------------------------------------------
My main concern is that there is no delivery nor error events handled by that patch. I am not sure if this is handled by another patch or another bug and I do not think I saw anything explaining that.
Please, re-ask for review if I missed something.
Attachment #722746 -
Flags: review?(mounir) → review-
Flags: needinfo?(mounir)
Comment 86•12 years ago
|
||
(In reply to khu from comment #82)
> It's under review right now.
> Hi, Mounir, and Vicamo, could you help to review and tell us how long you
> need to review on this bug? Gene can also provide a local build for testing
> if it's required. Thanks.
I was travelling today. I will do the other review after some sleep.
Assignee | ||
Comment 87•12 years ago
|
||
Hi Vicamo, I need your second review for this because I slightly changed the function of .setMessageDelivery(). What I did is if the |aReceiver| is null, we'll update each element in the |messageRecord.deliveryStatus| array for MMS. Thanks!
Attachment #722684 -
Attachment is obsolete: true
Attachment #723853 -
Flags: review?(vyang)
Assignee | ||
Comment 88•12 years ago
|
||
Hi Vicamo, I need your second review for this because I slightly changed the .send() to use .setMessageDelivery() to reset the delivery state to "sent" or "error" after sending an MMS message. Thanks!
Attachment #722752 -
Attachment is obsolete: true
Attachment #723854 -
Flags: review?(vyang)
Assignee | ||
Comment 89•12 years ago
|
||
Hi Mounir,
Not I understand what you're concerning. Thanks for catching this. :) Indeed, we missed the "mms-failed" event. However, for "mms-delivery-success" or "mms-delivery-error", we decide to fire another bug 850140 and leave a TODO comment for it because it doesn't block any MMS user stories so far.
Attachment #722746 -
Attachment is obsolete: true
Attachment #723856 -
Flags: superreview+
Attachment #723856 -
Flags: review?(mounir)
Assignee | ||
Comment 90•12 years ago
|
||
(In reply to Gene Lian [:gene] from comment #89)
> Hi Mounir,
>
> Not I understand what you're concerning. Thanks for catching this. :)
^^^Now
Comment 91•12 years ago
|
||
Comment on attachment 723853 [details] [diff] [review]
Part 4-1, multiple delivery status, V6
Review of attachment 723853 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1099,5 @@
> saveSendingMessage: function saveSendingMessage(aMessage, aCallback) {
> + if ((aMessage.type != "sms" && aMessage.type != "mms") ||
> + (aMessage.type == "sms" && aMessage.receiver === undefined) ||
> + (aMessage.type == "mms" && aMessage.receivers === undefined) ||
> + aMessage.deliveryStatusRequested === undefined ||
Use '==' instead.
@@ +1233,5 @@
> + messageRecord.deliveryStatus[index] = deliveryStatus;
> + isRecordUpdated = true;
> + }
> + }
> + }
trailing ws.
Attachment #723853 -
Flags: review?(vyang) → review+
Comment 92•12 years ago
|
||
Comment on attachment 723854 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V8
Review of attachment 723854 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/mobilemessage/src/android/MobileMessageService.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
Since these three variants, fallback/android/ril, shares exactly the same MobileMessageService implementation, let's move it upward to dom/mobilemessage/src.
::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +22,5 @@
>
> +NS_IMPL_ISUPPORTS3(SmsIPCService,
> + nsISmsService,
> + nsIMobileMessageService,
> + nsIMobileMessageDatabaseService)
Sorry, I should have found this more earlier. SmsIPCService should not inherit nsIMobileMessageService. MobileMessageService is now globally available, both content and chrome, and has nothing to do with IPC.
@@ +102,5 @@
> aCx, aMessage);
> }
>
> NS_IMETHODIMP
> +SmsIPCService::CreateMmsMessage(int32_t aId,
So we should not add CreateMmsMessage() but remove CreateSmsMessage() and CreateSmsSegmentInfo() instead.
::: dom/mobilemessage/src/ipc/SmsIPCService.h
@@ +6,5 @@
> #ifndef mozilla_dom_mobilemessage_SmsIPCService_h
> #define mozilla_dom_mobilemessage_SmsIPCService_h
>
> #include "nsISmsService.h"
> +#include "nsIMobileMessageService.h"
Ditto.
Attachment #723854 -
Flags: review?(vyang)
Assignee | ||
Comment 93•12 years ago
|
||
Addressing comment #92. Thanks for catching this!
Attachment #723854 -
Attachment is obsolete: true
Attachment #723874 -
Flags: review?(vyang)
Updated•12 years ago
|
Attachment #723874 -
Flags: review?(vyang) → review+
Comment 95•12 years ago
|
||
Comment on attachment 723856 [details] [diff] [review]
Part 6, dispatch MMS events, V4
Review of attachment 723856 [details] [diff] [review]:
-----------------------------------------------------------------
Please try to land delivery events before shipping that.
Attachment #723856 -
Flags: review?(mounir) → review+
Comment 96•12 years ago
|
||
Comment on attachment 722751 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V5
Review of attachment 722751 [details] [diff] [review]:
-----------------------------------------------------------------
Could you ask for another review after the comments being answered/applied?
::: dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl
@@ +13,5 @@
> + nsIDOMBlob content;
> +};
> +
> +[scriptable, builtinclass, uuid(df002ad2-71d7-11e2-9f1c-af6fa139069f)]
> +interface nsIDOMMozMmsMessage : nsISupports
Why not trying to inherit from nsIDOMMozSmsMessage? That would have been handy.
@@ +23,5 @@
> + */
> + readonly attribute DOMString deliveryState;
> +
> + [implicit_jscontext]
> + readonly attribute jsval deliveryStatus; // DOMString[]
I do not understand why there is "deliveryStatus" and "deliveryState"... weird...
::: dom/mobilemessage/src/MmsMessage.cpp
@@ +53,5 @@
> + const nsAString& aDeliveryState,
> + const jsval& aDeliveryStatus,
> + const nsAString& aSender,
> + const jsval& aReceivers,
> + const jsval& aTimestamp,
please, s/jsval/JS::Value/
@@ +81,5 @@
> +
> + // Set |deliveryStatus|.
> + if (aDeliveryStatus == JSVAL_NULL) {
> + return NS_ERROR_INVALID_ARG;
> + }
No need for that check, .isObject() will return false in that case and you test that just after.
@@ +87,5 @@
> + if (!aDeliveryStatus.isObject()) {
> + return NS_ERROR_INVALID_ARG;
> + }
> +
> + JSObject& deliveryStatusObj = aDeliveryStatus.toObject();
Actually, you could do:
JSObject* deliveryStatusObj = aDeliveryStatus.toObjectOrNull();
if (!deliveryStatusObj || !JS_IsArrayObject(aCx, deliveryStatusObj)) {
return NS_ERROR_INVALID_ARG;
}
That way, you have a pointer and all the rest of the code seems to be expecting a pointer. You could remove the .isObject() check above too.
@@ +97,5 @@
> + JS_ALWAYS_TRUE(JS_GetArrayLength(aCx, &deliveryStatusObj, &length));
> +
> + nsTArray<DeliveryStatus> deliveryStatus;
> + for (uint32_t i = 0; i < length; ++i) {
> + jsval statusJsVal;
JS::Value
@@ +103,5 @@
> + return NS_ERROR_INVALID_ARG;
> + }
> +
> + if (!statusJsVal.isString()) {
> + return NS_ERROR_INVALID_ARG;
Merge that with the line above:
if (!JS_GetElement(aCx, &deliveryStatusObj, i, &statusJsVal) ||
!statusJsVal.isString()) {
return NS_ERROR_INVALID_ARG;
}
@@ +135,5 @@
> + return NS_ERROR_INVALID_ARG;
> + }
> +
> + JSObject& receiversObj = aReceivers.toObject();
> + if (!JS_IsArrayObject(aCx, &receiversObj)) {
Same comments as above.
@@ +143,5 @@
> + JS_ALWAYS_TRUE(JS_GetArrayLength(aCx, &receiversObj, &length));
> +
> + nsTArray<nsString> receivers;
> + for (uint32_t i = 0; i < length; ++i) {
> + jsval receiverJsVal;
JS::Value
@@ +148,5 @@
> + if (!JS_GetElement(aCx, &receiversObj, i, &receiverJsVal)) {
> + return NS_ERROR_INVALID_ARG;
> + }
> +
> + if (!receiverJsVal.isString()) {
Same comment as above.
@@ +186,5 @@
> + return NS_ERROR_INVALID_ARG;
> + }
> +
> + JSObject& attachmentsObj = aAttachments.toObject();
> + if (!JS_IsArrayObject(aCx, &attachmentsObj)) {
Same comments as above regarding manipulating arrays.
@@ +194,5 @@
> + nsTArray<MmsAttachment> attachments;
> + JS_ALWAYS_TRUE(JS_GetArrayLength(aCx, &attachmentsObj, &length));
> +
> + for (uint32_t i = 0; i < length; ++i) {
> + jsval attachmentJsVal;
JS::Value
@@ +198,5 @@
> + jsval attachmentJsVal;
> + if (!JS_GetElement(aCx, &attachmentsObj, i, &attachmentJsVal)) {
> + return NS_ERROR_INVALID_ARG;
> + }
> + if (!attachmentJsVal.isObject()) {
if (!JS_GetElement(aCx, &attachmentsObj, i, &attachmentJVal) ||
!attachmentJsVal.isObject()) {
return NS_ERROR_INVALID_ARG;
}
@@ +212,5 @@
> + MmsAttachment attachment;
> +
> + // Set |attachment.mId|.
> + if (!JS_GetProperty(aCx, &attachmentObj, "id", &tmpJsVal) ||
> + !(tmpJsStr = JS_ValueToString(aCx, tmpJsVal)) ||
Don't put assignments in a if.
@@ +220,5 @@
> + attachment.mId = nsString(tmpDepJsStr);
> +
> + // Set |attachment.mLocation|.
> + if (!JS_GetProperty(aCx, &attachmentObj, "location", &tmpJsVal) ||
> + !(tmpJsStr = JS_ValueToString(aCx, tmpJsVal)) ||
ditto
@@ +228,5 @@
> + attachment.mLocation = nsString(tmpDepJsStr);
> +
> + // Set |attachment.mContent|.
> + if (!JS_GetProperty(aCx, &attachmentObj, "content", &tmpJsVal) ||
> + !JS_ValueToObject(aCx, tmpJsVal, &tmpJsObj)) {
ditto
@@ +250,5 @@
> + timestamp,
> + aRead,
> + nsString(aSmil),
> + attachments);
> + message.swap(*aMessage);
.forget() would be better.
@@ +295,5 @@
> +MmsMessage::GetDeliveryStatus(JSContext* aCx, jsval* aDeliveryStatus)
> +{
> + uint32_t length = mDeliveryStatus.Length();
> + if (length == 0) {
> + *aDeliveryStatus = JSVAL_NULL;
So, I'm not really convinced this is the best behaviour but I am not sure when mDeliveryStatus.Length() is equal to zero. I would agree that 'null' is a good value to return in some cases but I would better see that depending on the state of the MmsMessage (like 'receiving' should make .deliveryStatus = null). Could you fix that in a follow-up?
@@ +328,5 @@
> + statusStr.Length()));
> + }
> +
> + aDeliveryStatus->setObjectOrNull(JS_NewArrayObject(aCx, length, deliveryStatus));
> + NS_ENSURE_TRUE(aDeliveryStatus->isObject(), NS_ERROR_FAILURE);
return aDeliveryStatus->isObject() ? NS_OK : NS_ERROR_FAILURE;
@@ +345,5 @@
> +MmsMessage::GetReceivers(JSContext* aCx, jsval* aReceivers)
> +{
> + uint32_t length = mReceivers.Length();
> + if (length == 0) {
> + *aReceivers = JSVAL_NULL;
How could that happen?
@@ +358,5 @@
> + mReceivers[i].Length()));
> + }
> +
> + aReceivers->setObjectOrNull(JS_NewArrayObject(aCx, length, receivers));
> + NS_ENSURE_TRUE(aReceivers->isObject(), NS_ERROR_FAILURE);
return aReceivers->isObject() ? NS_OK : NS_ERROR_FAILURE;
@@ +389,5 @@
> +MmsMessage::GetAttachments(JSContext* aCx, jsval* aAttachments)
> +{
> + uint32_t length = mAttachments.Length();
> + if (length == 0) {
> + *aAttachments = JSVAL_NULL;
I think it could be better to return an empty array instead but I am not sure... Could you file a follow-up and CC jonas and I.
@@ +443,5 @@
> + }
> +
> + aAttachments->setObjectOrNull(attachments);
> + NS_ENSURE_TRUE(aAttachments->isObject(), NS_ERROR_FAILURE);
> + return NS_OK;
return aAttachments->isObject() ? NS_OK : NS_ERROR_FAILURE;
::: dom/mobilemessage/src/MmsMessage.h
@@ +10,5 @@
> +#include "nsString.h"
> +#include "jspubtd.h"
> +#include "mozilla/dom/mobilemessage/Types.h"
> +#include "mozilla/Attributes.h"
> +#include "nsIDOMFile.h"
I doubt you need to include nsIDOMFile.h
@@ +16,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +
> +struct MmsAttachment
Couldn't that live inside the class?
@@ +22,5 @@
> + nsString mId;
> + nsString mLocation;
> + nsCOMPtr<nsIDOMBlob> mContent;
> +};
> +typedef struct MmsAttachment MmsAttachment;
You don't need "typedef struct ..." in C++.
@@ +32,5 @@
> + NS_DECL_NSIDOMMOZMMSMESSAGE
> +
> + MmsMessage(int32_t aId,
> + mobilemessage::DeliveryState aDeliveryState,
> + nsTArray<mobilemessage::DeliveryStatus>& aDeliveryStatus,
const nsTArray<mobilemessage::DeliveryStatus>&
@@ +34,5 @@
> + MmsMessage(int32_t aId,
> + mobilemessage::DeliveryState aDeliveryState,
> + nsTArray<mobilemessage::DeliveryStatus>& aDeliveryStatus,
> + const nsString& aSender,
> + nsTArray<nsString>& aReceivers,
const nsTArray<nsString>&
@@ +38,5 @@
> + nsTArray<nsString>& aReceivers,
> + uint64_t aTimestamp,
> + bool aRead,
> + const nsString& aSmil,
> + nsTArray<MmsAttachment>& aAttachments);
const nsTArray<MmsAttachment>&
@@ +63,5 @@
> + nsTArray<nsString> mReceivers;
> + uint64_t mTimestamp;
> + bool mRead;
> + nsString mSmil;
> + nsTArray<MmsAttachment> mAttachments;
SmsMessage uses an IPDL structure to store the data so the {de,}serialization comes for free when using IPC. Why not doing the same here? How do you handle IPC?
::: dom/mobilemessage/src/Types.h
@@ -11,5 @@
> namespace mozilla {
> namespace dom {
> namespace mobilemessage {
>
> -// For SmsMessageData.delivery.
nit: do not remove the comment, just update it:
// For {Mms,Sms}MessageData.delivery.
@@ -24,5 @@
> // This state should stay at the end.
> eDeliveryState_EndGuard
> };
>
> -// For SmsMessageData.deliveryStatus.
ditto
@@ -34,5 @@
> // This state should stay at the end.
> eDeliveryStatus_EndGuard
> };
>
> -// For SmsFilterData.read
ditto
@@ -43,5 @@
> // This state should stay at the end.
> eReadState_EndGuard
> };
>
> -// For SmsFilterData.messageClass
ditto
::: mobile/android/base/GeckoSmsManager.java
@@ +318,5 @@
> + private final static int kDeliveryStateSending = 2;
> + private final static int kDeliveryStateError = 3;
> + private final static int kDeliveryStateUnknown = 4;
> + private final static int kDeliveryStateNotDownloaded = 5;
> + private final static int kDeliveryStateEndGuard = 6;
oh...
@@ +328,5 @@
> private final static int kDeliveryStatusNotApplicable = 0;
> private final static int kDeliveryStatusSuccess = 1;
> private final static int kDeliveryStatusPending = 2;
> private final static int kDeliveryStatusError = 3;
> + private final static int kDeliveryStatusEndGuard = 4;
No need to add EndGuard here. Leave it as is.
@@ +348,5 @@
> + private final static int kMessageClassClass0 = 1;
> + private final static int kMessageClassClass1 = 2;
> + private final static int kMessageClassClass2 = 3;
> + private final static int kMessageClassClass3 = 4;
> + private final static int kMessageClassEndGuard = 5;
No need to add EndGuard here. Leave it as is.
Attachment #722751 -
Flags: review?(mounir) → review-
Assignee | ||
Comment 97•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #96)
> Comment on attachment 722751 [details] [diff] [review]
> Part 2, nsIDOMMozMmsMessage, V5
>
> Review of attachment 722751 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Could you ask for another review after the comments being answered/applied?
Sure! Before that, I can answer some questions not related JS API as below:
>
> ::: dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl
> @@ +13,5 @@
> > + nsIDOMBlob content;
> > +};
> > +
> > +[scriptable, builtinclass, uuid(df002ad2-71d7-11e2-9f1c-af6fa139069f)]
> > +interface nsIDOMMozMmsMessage : nsISupports
>
> Why not trying to inherit from nsIDOMMozSmsMessage? That would have been
> handy.
Please see the IDL proposal at bug 760065. We planned to treat SMS and MMS as separate IDL structure. We believe it makes sense because most of attributes are totally different.
>
> @@ +23,5 @@
> > + */
> > + readonly attribute DOMString deliveryState;
> > +
> > + [implicit_jscontext]
> > + readonly attribute jsval deliveryStatus; // DOMString[]
>
> I do not understand why there is "deliveryStatus" and "deliveryState"...
> weird...
All right. Let's s/deliveryState/state, which is also the original design at bug 760065, to avoid the ambiguity with |deliveryStatus|.
> @@ +63,5 @@
> > + nsTArray<nsString> mReceivers;
> > + uint64_t mTimestamp;
> > + bool mRead;
> > + nsString mSmil;
> > + nsTArray<MmsAttachment> mAttachments;
>
> SmsMessage uses an IPDL structure to store the data so the
> {de,}serialization comes for free when using IPC. Why not doing the same
> here? How do you handle IPC?
Yes, we know that. We're addressing the IPC-related issues at Bug 847744. Due to the MMS timeline, we're hoping to provide a non-ipc version to let Gaia folks have something to test in the first place, so that the Gecko and Gaia can work on them at the same time. We'll apply an IPC structure like MmsMessageData to encapsulate all the values.
Assignee | ||
Comment 98•12 years ago
|
||
Hi Mounir, thanks for your detailed review! Please also see my comment #97 for your questions.
Attachment #722751 -
Attachment is obsolete: true
Attachment #724266 -
Flags: superreview+
Attachment #724266 -
Flags: review?(mounir)
Assignee | ||
Comment 99•12 years ago
|
||
Hi Vicamo,
Just fixed a potential bug when dealing with SMIL like below:
+ // Don't need to consider the SMIL part if it's present.
+ if (headers["content-type"]["media"] == "application/smil") {
+ smil = part.content;
+ continue;
+ }
Attachment #723874 -
Attachment is obsolete: true
Attachment #724378 -
Flags: review+
Attachment #724378 -
Flags: feedback?(vyang)
Comment 100•12 years ago
|
||
Comment on attachment 724266 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V6
Review of attachment 724266 [details] [diff] [review]:
-----------------------------------------------------------------
r=me but I would like Blake to have a look at the JS API usage. He told me we could do that quickly.
::: dom/mobilemessage/src/MmsMessage.cpp
@@ +118,5 @@
> +
> + // Set |receivers|.
> + if (aReceivers == JSVAL_NULL) {
> + return NS_ERROR_INVALID_ARG;
> + }
Remove that check. The next check should implicitly check that.
@@ +161,5 @@
> + }
> +
> + // Set |attachments|.
> + if (aAttachments == JSVAL_NULL) {
> + return NS_ERROR_INVALID_ARG;
Remove that check,
@@ +165,5 @@
> + return NS_ERROR_INVALID_ARG;
> + }
> +
> + if (!aAttachments.isObject()) {
> + return NS_ERROR_INVALID_ARG;
and that check,
@@ +169,5 @@
> + return NS_ERROR_INVALID_ARG;
> + }
> +
> + JSObject* attachmentsObj = aAttachments.toObjectOrNull();
> + if (!JS_IsArrayObject(aCx, attachmentsObj)) {
and do:
if (!attachmentsObj || !JS_IsArrayObject(aCx, attachmentsObj) {
::: dom/mobilemessage/src/Types.h
@@ +11,5 @@
> namespace mozilla {
> namespace dom {
> namespace mobilemessage {
>
> +// For {Mms,Sms}MessageData.state.
nit:
// For MmsMessageData.state and SmsMessageData.deliveryState
Attachment #724266 -
Flags: review?(mounir) → review+
Updated•12 years ago
|
Attachment #724266 -
Flags: review?(mrbkap)
Comment 101•12 years ago
|
||
Comment on attachment 724266 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V6
Review of attachment 724266 [details] [diff] [review]:
-----------------------------------------------------------------
I'd like to see another version of this with my comments addressed.
::: dom/mobilemessage/src/MmsMessage.cpp
@@ +81,5 @@
> + }
> +
> + // Set |deliveryStatus|.
> + JSObject* deliveryStatusObj = aDeliveryStatus.toObjectOrNull();
> + if (!deliveryStatusObj || !JS_IsArrayObject(aCx, deliveryStatusObj)) {
Unfortunately, this isn't quite right. You need to make sure that aDeliveryStatus is an object. I'd write this:
if (!aDeliveryStatus.isObject()) {
return NS_ERROR_INVALID_ARG;
}
JSObject* deliveryStatusObj = &aDeliveryStatus.toObject();
This comment also goes for the other instances of this pattern, further down.
@@ +200,5 @@
> + tmpJsStr = JS_ValueToString(aCx, tmpJsVal);
> + if (!tmpJsStr || !tmpDepJsStr.init(aCx, tmpJsStr)) {
> + return NS_ERROR_INVALID_ARG;
> + }
> + attachment.mId = nsString(tmpDepJsStr);
Why do you have the explicit constructor here? Can you not simply do attachment.mId.Assign(tmpDepJsStr)?
@@ +234,5 @@
> + nsString(aSender),
> + receivers,
> + timestamp,
> + aRead,
> + nsString(aSmil),
Why the two explicit nsString constructors here?
@@ +288,5 @@
> + *aDeliveryStatus = JSVAL_NULL;
> + return NS_OK;
> + }
> +
> + JS::Value* deliveryStatus = new JS::Value[length];
Unfortunately, this doesn't get the rooting correct, which could lead to crashes during GC. You should follow the same general pattern as <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/public/nsTArrayHelpers.h#58> since for now, the GC knows how to trace an array that has a reference on the stack but has no way of tracing a heap array of values.
@@ +291,5 @@
> +
> + JS::Value* deliveryStatus = new JS::Value[length];
> +
> + for (uint32_t i = 0; i < length; ++i) {
> + nsString statusStr = EmptyString();
No need to explicitly initialize the string (the default constructor creates an empty string).
@@ +311,5 @@
> + default:
> + MOZ_NOT_REACHED("We shouldn't get any other delivery status!");
> + return NS_ERROR_UNEXPECTED;
> + }
> + deliveryStatus[i].setString(JS_NewUCStringCopyN(aCx,
JS_NewUCStringCopyN can fail. You need to check if it returns null and make sure you throw an exception if it does. There are a few other instances of this below that also need to be fixed.
@@ +316,5 @@
> + statusStr.get(),
> + statusStr.Length()));
> + }
> +
> + aDeliveryStatus->setObjectOrNull(JS_NewArrayObject(aCx, length, deliveryStatus));
Once you've fixed up the rooting here, you should also freeze your array (as the code I pointed out above does) to make sure that users of this code don't try to change the array of receivers in order to change the internal state.
@@ +335,5 @@
> + if (length == 0) {
> + return NS_ERROR_UNEXPECTED;
> + }
> +
> + JS::Value* receivers = new JS::Value[length];
This function can simply use nsTArrayToJSArray(aCx, mReceivers, aReceivers).
@@ +428,5 @@
> + }
> + }
> +
> + aAttachments->setObjectOrNull(attachments);
> + return aAttachments->isObject() ? NS_OK : NS_ERROR_FAILURE;
attachments can't be null here, so you can use ...->setObject(*attachments); return NS_OK;
Attachment #724266 -
Flags: review?(mrbkap) → review-
Assignee | ||
Comment 102•12 years ago
|
||
Hi Jonas and Mounir,
Need your final confirmation regarding the IDL namings. I saw our final design has some slight inconsistencies with the W3C Message API proposal:
1. Use |navigator.mozMobileMessage| or |navigator.messaging| (or |navigator.mozMessaging| if it's moz-specific for now) [1]?
2. In the MmsAttachment, Mounir used to suggest using id/location, but the W3C document is using contentId/contentLocation [2]. Which one is preferred or it doesn't matter for this moment? Note that "content-id" and "content-location" are special terms in the lower-level MMS spec.
3. The subject/smil/attachments are wrapped into another MmsContent [3]. Do we need to follow that or it doesn't matter for this moment?
4. In the nsIDOMMozMmsMessage structure, we should use |recipients| or |receivers| or it doesn't matter?
[1] http://sysapps.github.com/sysapps/proposals/Messaging/Messaging.html#navigator-interface
[2] http://sysapps.github.com/sysapps/proposals/Messaging/Messaging.html#mmsattachment-dictionary
[3] http://sysapps.github.com/sysapps/proposals/Messaging/Messaging.html#mmscontent-dictionary
[4] http://sysapps.github.com/sysapps/proposals/Messaging/Messaging.html#mmsmessage-interface
Flags: needinfo?(mounir)
Flags: needinfo?(jonas)
Updated•12 years ago
|
Attachment #724378 -
Flags: feedback?(vyang) → feedback+
Assignee | ||
Comment 103•12 years ago
|
||
Addressing comment #100 and comment #101. Thanks for the reviews!
Hi Blake,
In MmsMessage::GetDeliveryStatus(...), I directly declare a local nsTArray<nsString> and use the utility function nsTArrayToJSArray(...) to convert it, so that we don't need to do the extra rooting and freezing stuff. Does that sound reasonable to you?
Attachment #724266 -
Attachment is obsolete: true
Attachment #724807 -
Flags: superreview+
Attachment #724807 -
Flags: review?(mrbkap)
Assignee | ||
Comment 104•12 years ago
|
||
Fix a nit. Please see the previous comment #103. Thanks!
Attachment #724807 -
Attachment is obsolete: true
Attachment #724807 -
Flags: review?(mrbkap)
Attachment #724811 -
Flags: review?(mrbkap)
(In reply to Gene Lian [:gene] from comment #102)
> Hi Jonas and Mounir,
>
> Need your final confirmation regarding the IDL namings. I saw our final
> design has some slight inconsistencies with the W3C Message API proposal:
>
> 1. Use |navigator.mozMobileMessage| or |navigator.messaging| (or
> |navigator.mozMessaging| if it's moz-specific for now) [1]?
I don't have a strong preference.
I would just stick with using whatever name we currently are using as to not have more of the gaia code than needed. But I'm totally fine with renaming the property to .mozMobileMessage or .mozMesaging too if you prefer.
Don't remove the 'moz' prefix though. The API is going to be in flux for a while longer. So we shouldn't remove the prefix until we're more confident that it is stable.
> 2. In the MmsAttachment, Mounir used to suggest using id/location, but the
> W3C document is using contentId/contentLocation [2]. Which one is preferred
> or it doesn't matter for this moment? Note that "content-id" and
> "content-location" are special terms in the lower-level MMS spec.
I would prefer 'id' and 'location'. Or even 'id' and 'name'. This is something we should fix in the spec too. Using short names are generally preferred by JS authors.
> 3. The subject/smil/attachments are wrapped into another MmsContent [3]. Do
> we need to follow that or it doesn't matter for this moment?
I don't think this matters for the moment. Like I said, I suspect that this API will change many times between now and when it's "done", so I wouldn't worry too much about not following the spec exactly at this early stage.
> 4. In the nsIDOMMozMmsMessage structure, we should use |recipients| or
> |receivers| or it doesn't matter?
I think using 'receivers' for now to be consistent with the SMS API is best.
In general though, I don't think we should hold the patch for naming issues. We can always fix those later. So if names are the only thing remaining then I'd say go ahead and land. Details like this is easy to fix during standardization.
Flags: needinfo?(jonas)
Assignee | ||
Comment 106•12 years ago
|
||
Fix another nit. Please see the previous comment #103. Sorry for all the noises.
Attachment #724811 -
Attachment is obsolete: true
Attachment #724811 -
Flags: review?(mrbkap)
Attachment #724836 -
Flags: review?(mrbkap)
Assignee | ||
Comment 107•12 years ago
|
||
Hi Vicamo,
In the new patch, I added the following 3 things:
1. Try/catch |new SendTransaction()|.
2. Expose |subject|.
3. More debug messages.
Please return me some feedback for the update. Thanks!
Attachment #724378 -
Attachment is obsolete: true
Attachment #724873 -
Flags: review+
Attachment #724873 -
Flags: feedback?(vyang)
Comment 108•12 years ago
|
||
Comment on attachment 724836 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V9
Review of attachment 724836 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with a few last things fixed.
::: dom/mobilemessage/src/MmsMessage.cpp
@@ +88,5 @@
> + if (!aDeliveryStatus.isObject()) {
> + return NS_ERROR_INVALID_ARG;
> + }
> + JSObject* deliveryStatusObj = &aDeliveryStatus.toObject();
> + if (!deliveryStatusObj || !JS_IsArrayObject(aCx, deliveryStatusObj)) {
Here and in a few cases below, JS::Value::isObject returning true implies that the value is a non-null object, so no need to null-check.
@@ +191,5 @@
> + JSString* tmpJsStr;
> + JSObject* tmpJsObj;
> + nsDependentJSString tmpDepJsStr;
> +
> + MmsAttachment attachment;
Sorry that I missed this on the first review, but because this is a webidl dictionary, you can replace the code to read out the values here with a call to attachment.init(aCx, attachmentJsVal); That has the advantage of following the spec to the letter and reducing the amount of code you have to write here.
@@ +329,5 @@
> + JSObject* deliveryStatusObj = nullptr;
> + nsresult rv = nsTArrayToJSArray(aCx, tempStrArray, &deliveryStatusObj);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + aDeliveryStatus->setObjectOrNull(deliveryStatusObj);
If nsTArraytoJSArray returns NS_OK, you can assume that the returned object is non-null.
This solution is clearly superior to what I was suggesting :-)
Attachment #724836 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #724873 -
Flags: feedback?(vyang) → feedback+
Assignee | ||
Comment 109•12 years ago
|
||
r=vicamo sr=sicking a=leo+
Attachment #722724 -
Attachment is obsolete: true
Attachment #725284 -
Flags: superreview+
Attachment #725284 -
Flags: review+
Assignee | ||
Comment 110•12 years ago
|
||
r=vicamo,mounir,mrbkap sr=sicking a=leo+
Attachment #724836 -
Attachment is obsolete: true
Attachment #725285 -
Flags: superreview+
Attachment #725285 -
Flags: review+
Assignee | ||
Comment 111•12 years ago
|
||
r=vicamo
Attachment #722682 -
Attachment is obsolete: true
Attachment #725286 -
Flags: review+
Assignee | ||
Comment 112•12 years ago
|
||
r=vicamo a=leo+
Attachment #722683 -
Attachment is obsolete: true
Attachment #725288 -
Flags: review+
Assignee | ||
Comment 113•12 years ago
|
||
r=vicamo a=leo+
Attachment #723853 -
Attachment is obsolete: true
Attachment #725289 -
Flags: review+
Assignee | ||
Comment 114•12 years ago
|
||
r=vicamo a=leo+
Attachment #724873 -
Attachment is obsolete: true
Attachment #725291 -
Flags: review+
Assignee | ||
Comment 115•12 years ago
|
||
r=vicamo,mounir sr=sicking a=leo+
Attachment #722728 -
Attachment is obsolete: true
Attachment #725293 -
Flags: superreview+
Attachment #725293 -
Flags: review+
Assignee | ||
Comment 116•12 years ago
|
||
r=vicamo,mounir sr=sicking a=leo+
Attachment #723856 -
Attachment is obsolete: true
Attachment #725295 -
Flags: superreview+
Attachment #725295 -
Flags: review+
Assignee | ||
Comment 117•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a999914becb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/71828abe98b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b3e06cacd1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/eae203b80dd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3b82e8f068
https://hg.mozilla.org/integration/mozilla-inbound/rev/45ec3f65d863
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc24619bdc8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/41aed8cfdfa9
Comment 118•12 years ago
|
||
\(^O^)/
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(mounir)
Assignee | ||
Updated•12 years ago
|
Attachment #725284 -
Attachment description: Part 1, nsIDOMMobileMessageManager, V-checked-in → Part 1, nsIDOMMobileMessageManager, V5 (checked-in)
Assignee | ||
Updated•12 years ago
|
Attachment #725285 -
Attachment description: Part 2, nsIDOMMozMmsMessage, V-checked-in → Part 2, nsIDOMMozMmsMessage, V10 (checked-in)
Assignee | ||
Updated•12 years ago
|
Attachment #725286 -
Attachment description: Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V-checked-in → Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V4 (checked-in)
Assignee | ||
Updated•12 years ago
|
Attachment #725288 -
Attachment description: Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V-checked-in → Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V5 (checked-in)
Assignee | ||
Updated•12 years ago
|
Attachment #725289 -
Attachment description: Part 4-1, multiple delivery status, V-checked-in → Part 4-1, multiple delivery status, V7 (checked-in)
Assignee | ||
Updated•12 years ago
|
Attachment #725291 -
Attachment description: Part 4-2, nsIMmsService.send(), V-checked-in → Part 4-2, nsIMmsService.send(), V12 (checked-in)
Assignee | ||
Updated•12 years ago
|
Attachment #725293 -
Attachment description: Part 5, nsIDOMMobileMessageManager.send(), V-checked-in → Part 5, nsIDOMMobileMessageManager.send(), V4 (checked-in)
Assignee | ||
Updated•12 years ago
|
Attachment #725295 -
Attachment description: Part 6, dispatch MMS events, V-checked-in → Part 6, dispatch MMS events, V5 (checked-in)
Comment 119•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a999914becb7
https://hg.mozilla.org/mozilla-central/rev/71828abe98b5
https://hg.mozilla.org/mozilla-central/rev/8b3e06cacd1d
https://hg.mozilla.org/mozilla-central/rev/eae203b80dd0
https://hg.mozilla.org/mozilla-central/rev/0a3b82e8f068
https://hg.mozilla.org/mozilla-central/rev/45ec3f65d863
https://hg.mozilla.org/mozilla-central/rev/dc24619bdc8b
https://hg.mozilla.org/mozilla-central/rev/41aed8cfdfa9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
Comment 120•12 years ago
|
||
Goes without saying that this is going to need a b2g18-specific set of patches.
Assignee | ||
Comment 121•12 years ago
|
||
Thanks for the reminder. I'll prepare them later.
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 122•12 years ago
|
||
Assignee | ||
Comment 123•12 years ago
|
||
Assignee | ||
Comment 124•12 years ago
|
||
Hi Blake and Smaug,
I encountered some difficulties when attempting to generate b2g18-specific for the part-2 patch, which cannot pass the compilation due to:
/home/gene/mozilla-b2g18/objdir-gonk-unagi/js/xpconnect/src/DictionaryHelpers.cpp:51:24: error: nsIDOMBlob.h: No such file or directory
because the dictionary I used in the part-2 patch include nsIDOMBlob stuff:
dictionary MmsAttachment
{
DOMString? id;
DOMString? location;
nsIDOMBlob content;
};
It seems the dictionary enhancement for blob change wasn't uplifted to b2g18 apparently. It's not very clear to me what the history exactly is. Could you please point out which patches/bugs else should be uplifted?
If the dependency is too complicated to uplift everything then I'm afraid I need to strip out the dictionary things for my b2g18-specific patch (that is, creating my own structure to manage that). I'm glad to do this, though.
Flags: needinfo?(mrbkap)
Flags: needinfo?(bugs)
Comment 125•12 years ago
|
||
nsIDOMBlob is in rather unusual place
http://mxr.mozilla.org/mozilla-b2g18/source/content/base/public/nsIDOMFile.idl#32
So I guess you may need to add nsIDOMFile.h to
http://mxr.mozilla.org/mozilla-b2g18/source/js/xpconnect/src/event_impl_gen.conf.in#38
and nsIDOMBlob to http://mxr.mozilla.org/mozilla-b2g18/source/js/xpconnect/src/event_impl_gen.conf.in#45
Flags: needinfo?(bugs)
Comment 126•12 years ago
|
||
Er, wrong links.
Comment 127•12 years ago
|
||
Replace event_impl_gen.conf.in with dictionary_helper_gen.conf
Assignee | ||
Comment 128•12 years ago
|
||
Note to myself: we need to include partial changes at Bug 834165 and Bug 839528.
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 129•12 years ago
|
||
Attachment #728135 -
Attachment is obsolete: true
Assignee | ||
Comment 130•12 years ago
|
||
Assignee | ||
Comment 131•12 years ago
|
||
Assignee | ||
Comment 132•12 years ago
|
||
Assignee | ||
Comment 133•12 years ago
|
||
Assignee | ||
Comment 134•12 years ago
|
||
Assignee | ||
Comment 135•12 years ago
|
||
Assignee | ||
Comment 136•12 years ago
|
||
The b2g18-specific patches are generated and ready to land but hoping to have more tests on next Monday. Lots of conflicts between m-c and b2g18 are encountered. Needs to be very careful to ensure everything is OK to land.
Assignee | ||
Comment 137•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7c396708825f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/73504eed1e86
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e70a9bd44f85
https://hg.mozilla.org/releases/mozilla-b2g18/rev/98be4b810ed9
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d1ee33d0f75c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b9a4802153f1
https://hg.mozilla.org/releases/mozilla-b2g18/rev/daad2bb44d97
https://hg.mozilla.org/releases/mozilla-b2g18/rev/dec694d585e1
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Assignee | ||
Updated•12 years ago
|
Whiteboard: [NO_UPLIFT] → [NO_UPLIFT][RIL_INTERFACE]
Assignee | ||
Comment 139•12 years ago
|
||
Added [NO_UPLIFT][RIL_INTERFACE] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out.
------------------------------
If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible:
Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643)
Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages
Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS
Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments.
Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741).
Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS
Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix)
Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix)
Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead().
Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete().
Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage().
Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Bug 845643 - B2G MMS: Save retrieved MMS into database.
Assignee | ||
Comment 140•12 years ago
|
||
Following the previous comment, some more needs to back out:
Bug 792321 - Check max values of MMS parameters in sendRequest.
Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS
Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS
Bug 839436 - B2G MMS: make DB be able to save MMS messages.
Assignee | ||
Comment 141•12 years ago
|
||
Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.
Comment 143•12 years ago
|
||
seems like this wasn't backed out from b2g18 after all. Should we remove NO_UPLIFT then ?
Assignee | ||
Comment 144•12 years ago
|
||
Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.
Whiteboard: [NO_UPLIFT][RIL_INTERFACE] → [RIL_INTERFACE]
Updated•12 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•