B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)

RESOLVED FIXED in Firefox 22

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gene Lian (I already quit Mozilla), Assigned: Gene Lian (I already quit Mozilla))

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla22
ARM
Gonk (Firefox OS)
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [RIL_INTERFACE])

Attachments

(16 attachments, 44 obsolete attachments)

32.76 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Gene Lian (I already quit Mozilla)
: superreview+
Details | Diff | Splinter Review
30.21 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Gene Lian (I already quit Mozilla)
: superreview+
Details | Diff | Splinter Review
71.85 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Details | Diff | Splinter Review
9.66 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Details | Diff | Splinter Review
12.94 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Details | Diff | Splinter Review
41.44 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Details | Diff | Splinter Review
5.38 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Gene Lian (I already quit Mozilla)
: superreview+
Details | Diff | Splinter Review
16.17 KB, patch
Gene Lian (I already quit Mozilla)
: review+
Gene Lian (I already quit Mozilla)
: superreview+
Details | Diff | Splinter Review
33.49 KB, patch
Details | Diff | Splinter Review
30.33 KB, patch
Details | Diff | Splinter Review
71.78 KB, patch
Details | Diff | Splinter Review
9.66 KB, patch
Details | Diff | Splinter Review
12.91 KB, patch
Details | Diff | Splinter Review
41.61 KB, patch
Details | Diff | Splinter Review
5.34 KB, patch
Details | Diff | Splinter Review
16.14 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 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.
(Assignee)

Updated

5 years ago
Summary: B2G MMS: provide nsIDOMMobileMessageManager interface → B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
leo+, this bug is needed to fulfill MMS user stories for v1.1
blocking-b2g: --- → leo+
(Assignee)

Comment 2

5 years ago
Created attachment 718349 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager.sendMMS()
Attachment #718349 - Flags: feedback?(vyang)
(Assignee)

Comment 3

5 years ago
Created attachment 718350 [details] [diff] [review]
Part 2, nsIMmsService.send()
Attachment #718350 - Flags: feedback?(vyang)
(Assignee)

Comment 4

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

5 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.
Whiteboard: [by 3/8]
(Assignee)

Comment 8

5 years ago
Created attachment 720648 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager
Attachment #718349 - Attachment is obsolete: true
Attachment #720648 - Flags: feedback?(vyang)
(Assignee)

Comment 9

5 years ago
Created attachment 720649 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage
Attachment #720649 - Flags: feedback?(vyang)
(Assignee)

Comment 10

5 years ago
Created attachment 720650 [details] [diff] [review]
Part 3, nsIMobileMessageRequest
Attachment #720650 - Flags: feedback?(vyang)
(Assignee)

Comment 11

5 years ago
Created attachment 720652 [details] [diff] [review]
Part 4, nsIMmsService.send()
Attachment #718350 - Attachment is obsolete: true
Attachment #720652 - Flags: feedback?(vyang)
(Assignee)

Comment 12

5 years ago
Created attachment 720654 [details] [diff] [review]
Part 5, nsIDOMMobileMessageManager.send()
Attachment #720654 - Flags: feedback?(vyang)
(Assignee)

Comment 13

5 years ago
Created attachment 720655 [details] [diff] [review]
Part 6, dispatch MMS events
Attachment #720655 - Flags: feedback?(vyang)
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+
Attachment #720655 - Flags: feedback?(vyang) → feedback+
Attachment #720654 - Flags: feedback?(vyang) → feedback+
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 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 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)

Updated

5 years ago
Blocks: 847736
(Assignee)

Updated

5 years ago
Blocks: 847738
(Assignee)

Updated

5 years ago
Blocks: 847744
(Assignee)

Updated

5 years ago
Blocks: 847756
(Assignee)

Updated

5 years ago
Blocks: 847760
(Assignee)

Comment 18

5 years ago
Created attachment 721079 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V2
Attachment #720648 - Attachment is obsolete: true
Attachment #721079 - Flags: review?(vyang)
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

5 years ago
Created attachment 721176 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V2
Attachment #720649 - Attachment is obsolete: true
Attachment #721176 - Flags: feedback?(vyang)
Attachment #721176 - Flags: feedback?(mounir)
(Assignee)

Comment 21

5 years ago
Created attachment 721179 [details] [diff] [review]
Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V2

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

5 years ago
Created attachment 721180 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMmsMessageSent(), V2

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

5 years ago
Attachment #721179 - Attachment description: Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback → Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V2
(Assignee)

Comment 23

5 years ago
Created attachment 721182 [details] [diff] [review]
Part 4, nsIMmsService.send(), V2
Attachment #721182 - Flags: feedback?
(Assignee)

Comment 24

5 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

5 years ago
Attachment #720652 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
Created attachment 721184 [details] [diff] [review]
Part 5, nsIDOMMobileMessageManager.send(), V2
Attachment #720654 - Attachment is obsolete: true
Attachment #721184 - Flags: feedback+
(Assignee)

Comment 26

5 years ago
Created attachment 721185 [details] [diff] [review]
Part 6, dispatch MMS events, V2
Attachment #720655 - Attachment is obsolete: true
Attachment #721185 - Flags: feedback+
(Assignee)

Updated

5 years ago
Attachment #721184 - Flags: feedback?(mounir)
(Assignee)

Updated

5 years ago
Attachment #721185 - Flags: feedback?(mounir)
(Assignee)

Updated

5 years ago
Attachment #721182 - Flags: feedback?(vyang)
Attachment #721182 - Flags: feedback?(mounir)
Duplicate of this bug: 847292
(Assignee)

Comment 28

5 years ago
Created attachment 721609 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V3

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

5 years ago
Created attachment 721612 [details] [diff] [review]
Part 4-1, multiple delivery status, V3
Attachment #721612 - Flags: feedback?(vyang)
(Assignee)

Comment 30

5 years ago
Created attachment 721613 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V3
Attachment #721182 - Attachment is obsolete: true
Attachment #721613 - Flags: feedback?(vyang)
(Assignee)

Updated

5 years ago
Attachment #721612 - Attachment description: Part 4-1, multiple delivery status → Part 4-1, multiple delivery status, V3
(Assignee)

Comment 31

5 years ago
Created attachment 721662 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V4
Attachment #721613 - Attachment is obsolete: true
Attachment #721613 - Flags: feedback?(vyang)
Attachment #721662 - Flags: feedback?(vyang)
(Assignee)

Comment 32

5 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.
Attachment #721179 - Flags: feedback?(vyang) → feedback+
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

5 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 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)
Attachment #721609 - Flags: feedback?(vyang) → feedback+
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 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

5 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

5 years ago
Created attachment 722083 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V3
Attachment #721180 - Attachment is obsolete: true
Attachment #721180 - Flags: feedback?(mounir)
Attachment #722083 - Flags: feedback?(vyang)
Attachment #722083 - Flags: feedback?(mounir)
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

5 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

5 years ago
Created attachment 722121 [details] [diff] [review]
Part 4-1, multiple delivery status, V4

Addressing comment #36. Thanks for the review again!
Attachment #721612 - Attachment is obsolete: true
Attachment #722121 - Flags: feedback?(vyang)
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

5 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

5 years ago
Created attachment 722146 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V5

Addressing comment #37.

Thanks for the review! Nice suggestions!
Attachment #721662 - Attachment is obsolete: true
Attachment #722146 - Flags: feedback?(vyang)
(Assignee)

Updated

5 years ago
Attachment #721609 - Flags: feedback?(mounir) → review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #721179 - Flags: feedback?(mounir) → review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #722083 - Flags: feedback?(mounir) → review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #721184 - Flags: feedback?(mounir) → review?(mounir)
(Assignee)

Updated

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

5 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

5 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

5 years ago
Attachment #721179 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #722083 - Flags: review?(mounir)
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

5 years ago
Created attachment 722680 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V4

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

5 years ago
Attachment #721079 - Flags: review?(mounir) → review?(vyang)
(Assignee)

Updated

5 years ago
Attachment #721079 - Flags: feedback+
(Assignee)

Comment 55

5 years ago
Created attachment 722682 [details] [diff] [review]
Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V3
Attachment #721179 - Attachment is obsolete: true
Attachment #722682 - Flags: review?(vyang)
(Assignee)

Comment 56

5 years ago
Created attachment 722683 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V4

Addressing comment #40.
Attachment #722083 - Attachment is obsolete: true
Attachment #722683 - Flags: review?(vyang)
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

5 years ago
Created attachment 722684 [details] [diff] [review]
Part 4-1, multiple delivery status, V5

Addressing comment #43.
Attachment #722121 - Attachment is obsolete: true
Attachment #722684 - Flags: review?(vyang)
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

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

5 years ago
Created attachment 722708 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V6

Addressing comment #53.
Attachment #722146 - Attachment is obsolete: true
Attachment #722708 - Flags: review?(vyang)
(Assignee)

Comment 63

5 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

5 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

5 years ago
Created attachment 722715 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V3

sr=sicking.

Addressing comment #57.
Attachment #721079 - Attachment is obsolete: true
Attachment #722715 - Flags: superreview+
Attachment #722715 - Flags: review?(vyang)
(Assignee)

Updated

5 years ago
Attachment #722715 - Flags: superreview+
Attachment #722715 - Flags: review?(vyang)
(Assignee)

Comment 66

5 years ago
Created attachment 722724 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V4

sr=sicking.

Addressing comment #57.
Attachment #722715 - Attachment is obsolete: true
Attachment #722724 - Flags: superreview+
Attachment #722724 - Flags: review?(vyang)
(Assignee)

Comment 67

5 years ago
Created attachment 722728 [details] [diff] [review]
Part 5, nsIDOMMobileMessageManager.send(), V3

Addressing comment #59.

r=vicamo,mounir sr=sicking
Attachment #721184 - Attachment is obsolete: true
Attachment #722728 - Flags: superreview+
Attachment #722728 - Flags: review+
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

5 years ago
Created attachment 722746 [details] [diff] [review]
Part 6, dispatch MMS events, V3

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)
Attachment #722682 - Flags: review?(vyang) → review+
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 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

5 years ago
Created attachment 722751 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V5

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

5 years ago
Created attachment 722752 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V7

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)
Attachment #722684 - Flags: review?(vyang) → review+
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

5 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

5 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)

Updated

5 years ago
Blocks: 849197
(Assignee)

Comment 77

5 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

5 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)

Updated

5 years ago
Duplicate of this bug: 842451
(Assignee)

Comment 80

5 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

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

Comment 82

5 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)
(Assignee)

Updated

5 years ago
Blocks: 843445
(Assignee)

Updated

5 years ago
Blocks: 849739
Attachment #722746 - Flags: review?(vyang) → review+
(Assignee)

Updated

5 years ago
Blocks: 849741
(Assignee)

Updated

5 years ago
Blocks: 849742
(Assignee)

Updated

5 years ago
No longer blocks: 849742
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 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 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)
(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)

Updated

5 years ago
Blocks: 850140
(Assignee)

Comment 87

5 years ago
Created attachment 723853 [details] [diff] [review]
Part 4-1, multiple delivery status, V6

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

5 years ago
Created attachment 723854 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V8

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

5 years ago
Created attachment 723856 [details] [diff] [review]
Part 6, dispatch MMS events, V4

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

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

5 years ago
Created attachment 723874 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V9

Addressing comment #92. Thanks for catching this!
Attachment #723854 - Attachment is obsolete: true
Attachment #723874 - Flags: review?(vyang)
Attachment #723874 - Flags: review?(vyang) → review+
(Assignee)

Updated

5 years ago
Duplicate of this bug: 849197
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 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

5 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)

Updated

5 years ago
Blocks: 850525
(Assignee)

Updated

5 years ago
Blocks: 850529
(Assignee)

Updated

5 years ago
Blocks: 850530
(Assignee)

Comment 98

5 years ago
Created attachment 724266 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V6

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

5 years ago
Created attachment 724378 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V10

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 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+
Attachment #724266 - Flags: review?(mrbkap)
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

5 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)
Attachment #724378 - Flags: feedback?(vyang) → feedback+
(Assignee)

Comment 103

5 years ago
Created attachment 724807 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V7

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

5 years ago
Created attachment 724811 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V8

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

5 years ago
Created attachment 724836 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V9

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

5 years ago
Created attachment 724873 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V11

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 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+
Attachment #724873 - Flags: feedback?(vyang) → feedback+
(Assignee)

Comment 109

5 years ago
Created attachment 725284 [details] [diff] [review]
Part 1, nsIDOMMobileMessageManager, V5 (checked-in)

r=vicamo sr=sicking a=leo+
Attachment #722724 - Attachment is obsolete: true
Attachment #725284 - Flags: superreview+
Attachment #725284 - Flags: review+
(Assignee)

Comment 110

5 years ago
Created attachment 725285 [details] [diff] [review]
Part 2, nsIDOMMozMmsMessage, V10 (checked-in)

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

5 years ago
Created attachment 725286 [details] [diff] [review]
Part 3-1, s/nsISmsRequest/nsIMobileMessageCallback, V4 (checked-in)

r=vicamo
Attachment #722682 - Attachment is obsolete: true
Attachment #725286 - Flags: review+
(Assignee)

Comment 112

5 years ago
Created attachment 725288 [details] [diff] [review]
Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V5 (checked-in)

r=vicamo a=leo+
Attachment #722683 - Attachment is obsolete: true
Attachment #725288 - Flags: review+
(Assignee)

Comment 113

5 years ago
Created attachment 725289 [details] [diff] [review]
Part 4-1, multiple delivery status, V7 (checked-in)

r=vicamo a=leo+
Attachment #723853 - Attachment is obsolete: true
Attachment #725289 - Flags: review+
(Assignee)

Comment 114

5 years ago
Created attachment 725291 [details] [diff] [review]
Part 4-2, nsIMmsService.send(), V12 (checked-in)

r=vicamo a=leo+
Attachment #724873 - Attachment is obsolete: true
Attachment #725291 - Flags: review+
(Assignee)

Comment 115

5 years ago
Created attachment 725293 [details] [diff] [review]
Part 5, nsIDOMMobileMessageManager.send(), V4 (checked-in)

r=vicamo,mounir sr=sicking a=leo+
Attachment #722728 - Attachment is obsolete: true
Attachment #725293 - Flags: superreview+
Attachment #725293 - Flags: review+
(Assignee)

Comment 116

5 years ago
Created attachment 725295 [details] [diff] [review]
Part 6, dispatch MMS events, V5 (checked-in)

r=vicamo,mounir sr=sicking a=leo+
Attachment #723856 - Attachment is obsolete: true
Attachment #725295 - Flags: superreview+
Attachment #725295 - Flags: review+
(Assignee)

Updated

5 years ago
Flags: needinfo?(mounir)
(Assignee)

Updated

5 years ago
Attachment #725284 - Attachment description: Part 1, nsIDOMMobileMessageManager, V-checked-in → Part 1, nsIDOMMobileMessageManager, V5 (checked-in)
(Assignee)

Updated

5 years ago
Attachment #725285 - Attachment description: Part 2, nsIDOMMozMmsMessage, V-checked-in → Part 2, nsIDOMMozMmsMessage, V10 (checked-in)
(Assignee)

Updated

5 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

5 years ago
Attachment #725288 - Attachment description: Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V-checked-in → Part 3-2, nsIMobileMessageCallback.notifyMessageSent(), V5 (checked-in)
(Assignee)

Updated

5 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

5 years ago
Attachment #725291 - Attachment description: Part 4-2, nsIMmsService.send(), V-checked-in → Part 4-2, nsIMmsService.send(), V12 (checked-in)
(Assignee)

Updated

5 years ago
Attachment #725293 - Attachment description: Part 5, nsIDOMMobileMessageManager.send(), V-checked-in → Part 5, nsIDOMMobileMessageManager.send(), V4 (checked-in)
(Assignee)

Updated

5 years ago
Attachment #725295 - Attachment description: Part 6, dispatch MMS events, V-checked-in → Part 6, dispatch MMS events, V5 (checked-in)
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
No longer blocks: 850530
Depends on: 850530
(Assignee)

Updated

5 years ago
No longer blocks: 850529
Depends on: 850529
(Assignee)

Updated

5 years ago
No longer blocks: 850525
Depends on: 850525
(Assignee)

Updated

5 years ago
Depends on: 852471
Goes without saying that this is going to need a b2g18-specific set of patches.
(Assignee)

Comment 121

5 years ago
Thanks for the reminder. I'll prepare them later.
(Assignee)

Updated

5 years ago
No longer blocks: 849112
Depends on: 849112
(Assignee)

Updated

5 years ago
Depends on: 853329
(Assignee)

Comment 122

5 years ago
Created attachment 728127 [details] [diff] [review]
[b2g18] Patch Part 1
(Assignee)

Comment 123

5 years ago
Created attachment 728135 [details] [diff] [review]
[b2g18] Patch Part 2
(Assignee)

Comment 124

5 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)
Er, wrong links.
Replace event_impl_gen.conf.in with dictionary_helper_gen.conf
(Assignee)

Comment 128

5 years ago
Note to myself: we need to include partial changes at Bug 834165 and Bug 839528.
Flags: needinfo?(mrbkap)
(Assignee)

Comment 129

5 years ago
Created attachment 728163 [details] [diff] [review]
[b2g18] Patch Part 2
Attachment #728135 - Attachment is obsolete: true
(Assignee)

Comment 130

5 years ago
Created attachment 728265 [details] [diff] [review]
[b2g18] Patch Part 3-1
(Assignee)

Comment 131

5 years ago
Created attachment 728267 [details] [diff] [review]
[b2g18] Patch Part 3-2
(Assignee)

Comment 132

5 years ago
Created attachment 728268 [details] [diff] [review]
[b2g18] Patch Part 4-1
(Assignee)

Comment 133

5 years ago
Created attachment 728270 [details] [diff] [review]
[b2g18] Patch Part 4-2
(Assignee)

Comment 134

5 years ago
Created attachment 728272 [details] [diff] [review]
[b2g18] Patch Part 5
(Assignee)

Comment 135

5 years ago
Created attachment 728274 [details] [diff] [review]
[b2g18] Patch Part 6
(Assignee)

Comment 136

5 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.
this should not be uplifted yet
Whiteboard: [by 3/8] → [NO_UPLIFT]
(Assignee)

Updated

5 years ago
Whiteboard: [NO_UPLIFT] → [NO_UPLIFT][RIL_INTERFACE]
(Assignee)

Comment 139

5 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

5 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

5 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.
Duplicate of this bug: 713471
seems like this wasn't backed out from b2g18 after all. Should we remove NO_UPLIFT then ?
(Assignee)

Comment 144

5 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

5 years ago
Blocks: 868190

Updated

5 years ago
Blocks: 868225

Updated

5 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.