Gene Lian (I already quit Mozilla) Assignee
	Description • 12 years ago

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.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 1 • 12 years ago

leo+, this bug is needed to fulfill MMS user stories for v1.1

	Gene Lian (I already quit Mozilla) 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!

	Vicamo Yang [:vicamo][:vyang]
	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.

	Vicamo Yang [:vicamo][:vyang]
	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?

	Gene Lian (I already quit Mozilla) 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.

	Vicamo Yang [:vicamo][: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.

	Vicamo Yang [:vicamo][:vyang]
	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.

	Vicamo Yang [:vicamo][: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.

	Vicamo Yang [:vicamo][: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.

	Vicamo Yang [:vicamo][: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.

	Gene Lian (I already quit Mozilla) 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).

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 24 • 12 years ago

Comment on attachment 721182 [details] [diff] [review]
Part 4, nsIMmsService.send(), V2

Addressing (some) issues at comment #15.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 28 • 12 years ago

Addressing comment #17 except for the read stuff, which should be a bool value for this moment.

	Gene Lian (I already quit Mozilla) 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.

	Vicamo Yang [:vicamo][:vyang]
	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

	Gene Lian (I already quit Mozilla) 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.

	Vicamo Yang [:vicamo][:vyang]
	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?

	Vicamo Yang [:vicamo][:vyang]
	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|

	Vicamo Yang [:vicamo][: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

	Gene Lian (I already quit Mozilla) 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.

	Vicamo Yang [:vicamo][:vyang]
	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"));

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 42 • 12 years ago

Addressing comment #36. Thanks for the review again!

	Vicamo Yang [:vicamo][: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.

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 45 • 12 years ago

Addressing comment #37.

Thanks for the review! Nice suggestions!

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 46 • 12 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 47 • 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
@@ +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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 48 • 12 years ago

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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 49 • 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/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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 50 • 12 years ago

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.

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) 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!

	Vicamo Yang [:vicamo][:vyang]
	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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 54 • 12 years ago

Addressing comment #48.

sr=sicking

	Gene Lian (I already quit Mozilla) Assignee
	Comment 56 • 12 years ago

Addressing comment #40.

	Vicamo Yang [:vicamo][: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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 58 • 12 years ago

Addressing comment #43.

	Mounir Lamouri (:mounir)
	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);

	Gene Lian (I already quit Mozilla) 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!

	Mounir Lamouri (:mounir)
	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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 62 • 12 years ago

Addressing comment #53.

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 65 • 12 years ago

sr=sicking.

Addressing comment #57.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 66 • 12 years ago

sr=sicking.

Addressing comment #57.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 67 • 12 years ago

Addressing comment #59.

r=vicamo,mounir sr=sicking

	Vicamo Yang [:vicamo][:vyang]
	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 :)

	Gene Lian (I already quit Mozilla) 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?

	Vicamo Yang [:vicamo][:vyang]
	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.

	Vicamo Yang [:vicamo][:vyang]
	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(...);|

	Gene Lian (I already quit Mozilla) Assignee
	Comment 72 • 12 years ago

Addressing comment #48 and comment #comment #59.

sr=sicking

	Gene Lian (I already quit Mozilla) Assignee
	Comment 73 • 12 years ago

Addressing comment #53 and comment #59 for:

  s/contentId/id
  s/contentLocation/location

	Vicamo Yang [:vicamo][:vyang]
	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.

	Gene Lian (I already quit Mozilla) 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!

	Gene Lian (I already quit Mozilla) 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!

	Gene Lian (I already quit Mozilla) 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!

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) 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);
  }

	Kai-Chih Hu [:khu]
	Comment 81 • 12 years ago

Now, text and image can be sent on local build.

	Kai-Chih Hu [:khu]
	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.

	Vicamo Yang [:vicamo][:vyang]
	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);

	Vicamo Yang [:vicamo][: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

	Mounir Lamouri (:mounir)
	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.

	Mounir Lamouri (: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.

	Gene Lian (I already quit Mozilla) 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!

	Gene Lian (I already quit Mozilla) 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!

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) 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

	Vicamo Yang [:vicamo][:vyang]
	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.

	Vicamo Yang [:vicamo][:vyang]
	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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 93 • 12 years ago

Addressing comment #92. Thanks for catching this!

	Mounir Lamouri (:mounir)
	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.

	Mounir Lamouri (:mounir)
	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.

	Gene Lian (I already quit Mozilla) 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.

	Gene Lian (I already quit Mozilla) Assignee
	Comment 98 • 12 years ago

Hi Mounir, thanks for your detailed review! Please also see my comment #97 for your questions.

	Gene Lian (I already quit Mozilla) 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;
+      }

	Mounir Lamouri (:mounir)
	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

	Blake Kaplan (:mrbkap) (inactive)
	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;

	Gene Lian (I already quit Mozilla) 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

	Gene Lian (I already quit Mozilla) 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?

	Gene Lian (I already quit Mozilla) Assignee
	Comment 104 • 12 years ago

Fix a nit. Please see the previous comment #103. Thanks!

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 105 • 12 years ago

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

	Gene Lian (I already quit Mozilla) Assignee
	Comment 106 • 12 years ago

Fix another nit. Please see the previous comment #103. Sorry for all the noises.

	Gene Lian (I already quit Mozilla) 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!

	Blake Kaplan (:mrbkap) (inactive)
	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 :-)

	Gene Lian (I already quit Mozilla) Assignee
	Comment 109 • 12 years ago

r=vicamo sr=sicking a=leo+

	Gene Lian (I already quit Mozilla) Assignee
	Comment 110 • 12 years ago

r=vicamo,mounir,mrbkap sr=sicking a=leo+

	Gene Lian (I already quit Mozilla) Assignee
	Comment 111 • 12 years ago

r=vicamo

	Gene Lian (I already quit Mozilla) Assignee
	Comment 112 • 12 years ago

r=vicamo a=leo+

	Gene Lian (I already quit Mozilla) Assignee
	Comment 113 • 12 years ago

r=vicamo a=leo+

	Gene Lian (I already quit Mozilla) Assignee
	Comment 114 • 12 years ago

r=vicamo a=leo+

	Gene Lian (I already quit Mozilla) Assignee
	Comment 115 • 12 years ago

r=vicamo,mounir sr=sicking a=leo+

	Gene Lian (I already quit Mozilla) Assignee
	Comment 116 • 12 years ago

r=vicamo,mounir sr=sicking a=leo+

	Gene Lian (I already quit Mozilla) 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

	Vicamo Yang [:vicamo][:vyang]
	Comment 118 • 12 years ago

\(^O^)/