854326 - B2G Multi-SIM: support multiple SIM cards for SMS/MMS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Gene Lian (I already quit Mozilla)]

As title.

Comment 2

[Gene Lian (I already quit Mozilla)]

Attachment: Proposal

Comment 3

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL (WIP)

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, IPDL for IPC (WIP)

Comment 5

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, add ICC ID (WIP)

Comment 6

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID (WIP)

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, add ICC ID (WIP)

Comment 8

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID (WIP)

Comment 9

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID (WIP)

Comment 10

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5, MMS APN settings (WIP)

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 811070 [details] [diff] [review]
Part 1, DOM API IDL (WIP)

Review of attachment 811070 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl
@@ +31,5 @@
> +   * Integrated Circuit Card Identifier.
> +   *
> +   * Will be an empty string if ICC is not available.
> +   */
> +  readonly attribute DOMString iccId;

I don't think we have to be so specific by saying this string *IS* ICC identifier.  This should be also named 'serviceId' in ideal, but that introduces naming conflict with the newly added parameter in |send()| and |sendMMS()|.  Guess I have to let go here and modify it again once any progress made in W3C.

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 811070 [details] [diff] [review]
Part 1, DOM API IDL (WIP)

Review of attachment 811070 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +344,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +MmsMessage::GetIccId(nsAString& aIccId)

I'm kind of lost.  Maybe move these empty imp. into part 2?

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 811073 [details] [diff] [review]
Part 2, IPDL for IPC (WIP)

Review of attachment 811073 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/interfaces/nsIMmsService.idl
@@ +16,1 @@
>  interface nsIMmsService : nsISupports

Please move interface changes to part 1.

::: dom/mobilemessage/src/ipc/PSms.ipdl
@@ +15,5 @@
>  namespace mobilemessage {
>  
>  struct SendMmsMessageRequest
>  {
> +  uint32_t serviceId;

Move to part 1, please.

::: dom/mobilemessage/src/ipc/SmsTypes.ipdlh
@@ +27,5 @@
>  struct SmsMessageData
>  {
>    int32_t        id;
>    uint64_t       threadId;
> +  nsString       iccId;

ditto.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 811070 [details] [diff] [review]
Part 1, DOM API IDL (WIP)

Review of attachment 811070 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
@@ +20,5 @@
>    // of DOMStrings.
>    // The method returns a DOMRequest object if one number has been passed.
>    // An array of DOMRequest objects otherwise.
> +  jsval send(in jsval number, in DOMString message,
> +             [optional] in unsigned long serviceId);

[implicit_jscontext, optional_argc]

@@ +25,3 @@
>  
> +  nsIDOMDOMRequest sendMMS(in jsval parameters /* MmsParameters */,
> +                           [optional] in unsigned long serviceId);

ditto.

Comment 15

[Gene Lian (I already quit Mozilla)]

Hi Vicamo, I was hoping to make each patch independently compilable itself so I added dummy implementation in some of them. This is beneficial from the point of view of compilation. Also, as far as I know, we don't need to ask dom peer reviews for the internal IDLs since there are too many implementation details for dom peers to follow up. Anyway, if you mind, I can ask dom peers' reviews for part-2 patch as well when it comes to internal IDL changes. :)

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 811073 [details] [diff] [review]
Part 2, IPDL for IPC (WIP)

Review of attachment 811073 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +172,5 @@
>    JSAutoCompartment ac(cx, global);
>  
>    if (aNumber.isString()) {
>      JS::Rooted<JSString*> str(cx, aNumber.toString());
> +    return Send(cx, global, aServiceId, str, aMessage, aReturn);

We need bug 926302 to determine default service id.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 812601 [details] [diff] [review]
Part 3, add ICC ID (WIP)

Review of attachment 812601 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/interfaces/nsIWapPushApplication.idl
@@ +24,5 @@
>     * @param options
>     *        An object containing various attributes from lower transport layer.
>     */
> +  void receiveWapPush(in unsigned long                   aServiceId,
> +                      [array, size_is(aLength)] in octet aData,

Please just have aServiceId be an additional attribute of aOptions.  The comment has it for 'various attributes from lower transport layer'.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1448,5 @@
>          (aMessage.type == "sms" && aMessage.receiver == undefined) ||
>          (aMessage.type == "mms" && !Array.isArray(aMessage.receivers)) ||
>          aMessage.deliveryStatusRequested == undefined ||
> +        aMessage.timestamp == undefined ||
> +        aMessage.iccId == undefined) {

One might try to send a SMS out when he's not registered to the network.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1651,5 @@
>        destinationAddress: this.rilContext.iccInfo.msisdn,
>        destinationPort: message.header.destinationPort,
>      };
> +    WAP.WapPushManager.receiveWdpPDU(this.clientId, message.fullData,
> +                                     message.fullData.length, 0, options);

ditto: just made it an attribute of |options|.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 812951 [details] [diff] [review]
Part 4, handle service ID (WIP)

Review of attachment 812951 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +135,5 @@
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "gMmsConnections", function () {
> +
> +  function MmsConnection(aServiceId) {

Please just move MmsConnection definition out of defineLazyGetter call.

@@ +362,5 @@
> +
> +          // The network state change doesn't belong to this service.
> +          if (network.serviceId != this.serviceId) {
> +            return;
> +          }

Please file another issue for this.  We don't have |network.serviceId| now.

@@ +426,5 @@
> +              .getService(Ci["nsIRadioInterfaceLayer"]);
> +  for (let i = 0; i < ril.numRadioInterfaces; i++) {
> +    let connection = new MmsConnection(i);
> +    connection.init();
> +    connections.push(connection)

Always lazy create MmsConnection here.  Maybe we can have:

  XPCOMUtils.defineLazyGetter(this, "gMmsConnections", function () {
    return {
      _connections: null,
      getConnByServiceId: function (id) {
        if (!this._connections) {
          this._connections = [];
        }

        let conn = this._connections[id];
        if (conn) {
          return conn;
        }

        conn = this._connections[id] = new MmsConnection(id);
        conn.init();
        return conn;
      },
    };
  }

@@ +437,2 @@
>    this.uri = Services.io.newURI(url, null, null);
> +  this.serviceId = serviceId;

MmsProxyFilter(mmsConnection, url)

@@ +457,5 @@
> +            JSON.stringify({ uri: JSON.stringify(this.uri),
> +                             mmsProxyInfo: mmsProxyInfo }));
> +    }
> +
> +    return mmsProxyInfo ? mmsProxyInfo : proxyInfo;

So we can have:

  return this.mmsConnection.proxyInfo ? this.mmsConnection.proxyInfo : proxyInfo;

@@ +478,5 @@
>       * @param callback
>       *        A callback function that takes two arguments: one for http
>       *        status, the other for wrapped PDU data for further parsing.
>       */
> +    sendRequest: function sendRequest(serviceId, method, url, istream, callback) {

sendRequest(mmsConnection, method, url, istream, callback)

@@ +482,5 @@
> +    sendRequest: function sendRequest(serviceId, method, url, istream, callback) {
> +      let mmsConnection = gMmsConnections[serviceId];
> +      if (method == "POST" && url == null) {
> +        url = mmsConnection.mmsc
> +      }

You don't need these lines.

@@ +539,5 @@
>            return;
>          }
>  
>          if (DEBUG) debug("sendRequest: register proxy filter to " + url);
> +        let proxyFilter = new MmsProxyFilter(serviceId, url);

new MmsProxyFilter(mmsConnection, url);

@@ +733,5 @@
>   *        X-Mms-Report-Allowed of the response.
>   *
>   * @see OMA-TS-MMS_ENC-V1_3-20110913-A section 6.2
>   */
> +function NotifyResponseTransaction(serviceId, transactionId, status, reportAllowed) {

pass mmsConnection instead.

@@ +868,5 @@
>   *
>   * @param contentLocation
>   *        X-Mms-Content-Location of the message.
>   */
> +function RetrieveTransaction(serviceId, cancellableId, contentLocation) {

ditto

@@ +968,5 @@
>   * SendTransaction.
>   *   Class for sending M-Send.req to MMSC, which inherits CancellableTransaction.
>   *   @throws Error("Check max values parameters fail.")
>   */
> +function SendTransaction(serviceId, cancellableId, msg, requestDeliveryReport) {

ditto

@@ +1198,5 @@
>   *        X-Mms-Report-Allowed of the response.
>   *
>   * @see OMA-TS-MMS_ENC-V1_3-20110913-A section 6.4
>   */
> +function AcknowledgeTransaction(serviceId, transactionId, reportAllowed) {

ditto

@@ +1344,5 @@
>        case RETRIEVAL_MODE_AUTOMATIC:
>          intermediate.deliveryStatus = [DELIVERY_STATUS_PENDING];
>          break;
>        case RETRIEVAL_MODE_AUTOMATIC_HOME:
> +        if (gMmsConnections[serviceId].isVoiceRoaming()) {

|serviceId| is not defined here, and please use |mmsConnection| instead.

@@ +1423,5 @@
>     *        the other parsed MMS message.
>     * @param aDomMessage
>     *        The nsIDOMMozMmsMessage object.
>     */
> +  retrieveMessage: function retrieveMessage(aServiceId, aContentLocation,

pass mmsConnection instead.

@@ +1499,5 @@
>  
>    /**
>     * Callback for retrieveMessage.
>     */
> +  retrieveMessageCallback: function retrieveMessageCallback(serviceId,

ditto

@@ +1579,5 @@
>  
>    /**
>     * Callback for saveReceivedMessage.
>     */
> +  saveReceivedMessageCallback: function saveReceivedMessageCallback(serviceId,

ditto

@@ +1994,5 @@
>        // This is the entry point starting to send MMS.
>        let sendTransaction;
>        try {
>          sendTransaction =
> +          new SendTransaction(aServiceId, aDomMessage.id, savableMessage,

Get a MmsConnection instance by |aServiceId| first.

@@ +2065,5 @@
> +        aRequest.notifyGetMessageFailed(Ci.nsIMobileMessageCallback.NO_SIM_CARD_ERROR);
> +        return;
> +      }
> +      // TODO Convert ICC ID to service ID.
> +      let serviceId = 0;

Need a bug number for every TODO label.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2110,5 @@
>          break;
>        case kNetworkInterfaceStateChangedTopic:
>          let network = subject.QueryInterface(Ci.nsINetworkInterface);
> +        if (network.state != Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED ||
> +            network.serviceId != this.clientId) {

Please leave |network.serviceId| thing to another bug.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 812996 [details] [diff] [review]
Part 5, MMS APN settings (WIP)

Review of attachment 812996 [details] [diff] [review]:
-----------------------------------------------------------------

Please just leave MMS APN settings, networkInterface.serviceId stuff to another bug

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +140,5 @@
>      this.serviceId = aServiceId;
>      let ril = Cc["@mozilla.org/ril;1"]
>                  .getService(Ci["nsIRadioInterfaceLayer"]);
>      this.radioInterface = ril.getRadioInterface(aServiceId);
> +    this.mmsApnSettings = this.radioInterface.apnSettings.byType.mms;

It's not an interface attribute, so you can't access it outside RadioInterfaceLayer.js.

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

According to bug 918558, 
"As a user, if I receive a subscription#2 MMS message, while SIM/data is active on subscription#1, I should have a way to switch to subscription#2 to retrieve the MMS. (manual)",

if MMS is coming from the SIM (not the default one for data connection), then we wouldn't auto retrieve that MMS.

Gene, we talked about this yesterday. I just leave a note here as a kind reminder. Thank you. :)

Comment 21

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL, V2

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, IPDL for IPC, V2

Comment 23

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, add ICC ID, V2

Comment 24

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, add ICC ID, V2.1

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 821431 [details] [diff] [review]
Part 1, DOM API IDL, V2

Review of attachment 821431 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed. Thanks!

::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
@@ +18,5 @@
>  
>    // The first parameter can be either a DOMString (only one number) or an array
>    // of DOMStrings.
>    // The method returns a DOMRequest object if one number has been passed.
>    // An array of DOMRequest objects otherwise.

Please comment on 'serviceId' such as how/where could we get it and it's valid value. Thank you.

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +162,5 @@
>      return NS_ERROR_INVALID_ARG;
>    }
>  
> +  nsresult rv;
> +  nsIScriptContext* sc = GetContextForEventHandlers(&rv);

NS_ENSURE_STATE(sc);

@@ +201,5 @@
>  
>  NS_IMETHODIMP
> +MobileMessageManager::SendMMS(const JS::Value& aParams,
> +                              uint32_t aServiceId,
> +                              JSContext* aCx, uint8_t aArgc,

Please make the coding style consistent, either wrap at 80th character or line parameters up.

Comment 26

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, add ICC ID, V2.2

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 821431 [details] [diff] [review]
Part 1, DOM API IDL, V2

Review of attachment 821431 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
@@ +26,4 @@
>  
> +  [implicit_jscontext, optional_argc]
> +  nsIDOMDOMRequest sendMMS(in jsval parameters /* MmsParameters */,
> +                           [optional] in unsigned long serviceId);

We've already foreseen the requirement for additional read report requisition parameter in bug 927718, as well as delivery report in bug 927716.  So let's have a dictionary here (and |send| for SMS) to prevent another ABI change.  I think we need only a new WebIDL-based dictionary type in |dom/webidl/MobileMessageManager.webidl|.

Comment 28

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID, V2

Comment 29

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #27)
> Comment on attachment 821431 [details] [diff] [review]
> Part 1, DOM API IDL, V2
> 
> Review of attachment 821431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl
> @@ +26,4 @@
> >  
> > +  [implicit_jscontext, optional_argc]
> > +  nsIDOMDOMRequest sendMMS(in jsval parameters /* MmsParameters */,
> > +                           [optional] in unsigned long serviceId);
> 
> We've already foreseen the requirement for additional read report
> requisition parameter in bug 927718, as well as delivery report in bug
> 927716.  So let's have a dictionary here (and |send| for SMS) to prevent
> another ABI change.  I think we need only a new WebIDL-based dictionary type
> in |dom/webidl/MobileMessageManager.webidl|.

Sounds good to me. Other patches still stand. Please go ahead to review. Thanks!

Comment 30

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5, MMS APN settings, V2

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 821438 [details] [diff] [review]
Part 2, IPDL for IPC, V2

Review of attachment 821438 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/interfaces/nsIMmsService.idl
@@ +16,4 @@
>  interface nsIMmsService : nsISupports
>  {
> +  void send(in unsigned long serviceId,
> +            in jsval parameters /* MmsParameters */,

Please move |serviceId| after |parameters|.

::: dom/mobilemessage/interfaces/nsISmsService.idl
@@ +21,5 @@
>    void getSegmentInfoForText(in DOMString text,
>                               in nsIMobileMessageCallback request);
>  
> +  void send(in unsigned long serviceId,
> +            in DOMString number,

ditto.

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 821518 [details] [diff] [review]
Part 4, handle service ID, V2

Review of attachment 821518 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +447,2 @@
>    this.uri = Services.io.newURI(url, null, null);
> +  this.mmsConnection = mmsConnection;

nit: move this before |this.uri = ...|

@@ +883,5 @@
>    // Call |CancellableTransaction| constructor.
>    CancellableTransaction.call(this, cancellableId);
>  
>    this.contentLocation = contentLocation;
> +  this.mmsConnection = mmsConnection;

nit: order these assignments by the order of the arguments.

@@ +1036,5 @@
>  
>    if (DEBUG) debug("msg: " + JSON.stringify(msg));
>  
>    this.msg = msg;
> +  this.mmsConnection = mmsConnection;

ditto

@@ +1395,5 @@
>        intermediate.sender = "anonymous";
>      }
>      intermediate.receivers = [];
> +    intermediate.phoneNumber = this.getPhoneNumber(mmsConnection.serviceId);
> +    intermediate.iccId = this.getIccId(mmsConnection.serviceId);

Now we know we should move the two utility functions into MmsConnection as well.

@@ +1670,2 @@
>                           this.retrieveMessageCallback.bind(this,
> +                                                           mmsConnection,

I'll read this patch again.  See if we can find a way to make passing |mmsConnection| along the call path, not argument binding.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +564,5 @@
>    getRadioInterface: function getRadioInterface(clientId) {
>      return this.radioInterfaces[clientId];
> +  },
> +
> +  getClientIdByIccId: function getClientIdByIccId(iccId) {

This should really go to MobileConnection, not RadioInterfaceLayer.  Possible?

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 821505 [details] [diff] [review]
Part 3, add ICC ID, V2.2

Review of attachment 821505 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1243,5 @@
>     *
>     * Otherwise, the phone number is in mdn.
>     * @see nsIDOMMozCdmaIccInfo
>     */
> +  getPhoneNumber: function getPhoneNumber(aServiceId) {

Let's move it into MmsConnection.

@@ +1247,5 @@
> +  getPhoneNumber: function getPhoneNumber(aServiceId) {
> +    let ril = Cc["@mozilla.org/ril;1"]
> +                .getService(Ci["nsIRadioInterfaceLayer"]);
> +    let radioInterface = ril.getRadioInterface(aServiceId);
> +

nit: redundant line

@@ +1272,5 @@
> +    *
> +    * @param aServiceId
> +    *        The ID of the RIL service.
> +    */
> +  getIccId: function getIccId(aServiceId) {

ditto.

@@ +1276,5 @@
> +  getIccId: function getIccId(aServiceId) {
> +    let ril = Cc["@mozilla.org/ril;1"]
> +                .getService(Ci["nsIRadioInterfaceLayer"]);
> +    let radioInterface = ril.getRadioInterface(aServiceId);
> +

ditto

@@ +1304,5 @@
>     *        Retrieval mode for MMS receiving setting.
>     * @param intermediate
>     *        Intermediate MMS message parsed from PDU.
>     */
> +  convertIntermediateToSavable: function convertIntermediateToSavable(serviceId,

So this function don't need |serviceId| for now.

@@ +1339,5 @@
>        intermediate.sender = "anonymous";
>      }
>      intermediate.receivers = [];
> +    intermediate.phoneNumber = this.getPhoneNumber(serviceId);
> +    intermediate.iccId = this.getIccId(serviceId);

And here it will be |intermediate.iccId = gMmsConnection.getIccId();|

@@ +1614,5 @@
>     * @param notification
>     *        The parsed MMS message object.
>     */
> +  handleNotificationIndication: function handleNotificationIndication(serviceId,
> +                                                                      notification) {

not needed.

@@ +1632,5 @@
>        } catch (e) {}
>  
> +      let savableMessage = this.convertIntermediateToSavable(serviceId,
> +                                                             notification,
> +                                                             retrievalMode);

ditto

@@ +1739,5 @@
>     * name-parameter of Content-Type header nor filename parameter of Content-Disposition
>     * header is available, Content-Location header SHALL be used if available.
>     */
> +  createSavableFromParams: function createSavableFromParams(aServiceId,
> +                                                            aParams, aMessage) {

ditto

@@ +1835,5 @@
>      aMessage["type"] = "mms";
>      aMessage["timestamp"] = Date.now();
>      aMessage["receivers"] = receivers;
> +    aMessage["sender"] = this.getPhoneNumber(aServiceId);
> +    aMessage["iccId"] = this.getIccId(aServiceId);

ditto

@@ +1940,5 @@
>      };
>  
>      let savableMessage = {};
> +    let errorCode = this.createSavableFromParams(aServiceId, aParams,
> +                                                 savableMessage);

ditto

Comment 34

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-1, clean up the APN setting logic, V1

Comment 35

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL, V3

Comment 36

[Ken Chang[:kenkai]]

Comment on attachment 822094 [details] [diff] [review]
Part 5-1, clean up the APN setting logic, V1

Review of attachment 822094 [details] [diff] [review]:
-----------------------------------------------------------------

Hsinyi, can you please help to review?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1597,4 @@
>                     (inputApnSetting.password || '');
> +
> +      if (!this.apnSettings.byApn[apnKey]) {
> +        this.apnSettings.byApn[apnKey] = inputApnSetting;

You cannot directly assign an object to a undefined element.

@@ +1608,2 @@
>        for each (let type in inputApnSetting.types) {
> +        this.apnSettings.byType[type] = this.apnSettings.byApn[apnKey];

ditto.
And a trailing whitespace in comments.

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 822094 [details] [diff] [review]
Part 5-1, clean up the APN setting logic, V1

Review of attachment 822094 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 38

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Ken Chang from comment #36)
> Part 5-1, clean up the APN setting logic, V1
> -----------------------------------------------------------------
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1597,4 @@
> >                     (inputApnSetting.password || '');
> > +
> > +      if (!this.apnSettings.byApn[apnKey]) {
> > +        this.apnSettings.byApn[apnKey] = inputApnSetting;
> 
> You cannot directly assign an object to a undefined element.

That's fine.

Comment 39

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, IPDL for IPC, V3

Rebase and use GetXXXDefaultServiceId().

Comment 40

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #31)
> Please move |serviceId| after |parameters|.

We used to have a meeting discussing this. We should put all the |serviceId| as the first parameter for internal functions, which makes much sense when it comes to hierarchy.

Comment 41

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #32)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +564,5 @@
> >    getRadioInterface: function getRadioInterface(clientId) {
> >      return this.radioInterfaces[clientId];
> > +  },
> > +
> > +  getClientIdByIccId: function getClientIdByIccId(iccId) {
> 
> This should really go to MobileConnection, not RadioInterfaceLayer. 
> Possible?

I'd prefer keeping it in the RIL since it's more like a utility function.

Comment 42

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, add ICC ID, V3

Comment 43

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID, V3

Comment 44

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID, V3.1

Comment 45

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-1, clean up the APN setting logic, V1.1

Carry on r=vicamo.

Comment 46

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-2, MMS APN settings, V3

Comment 47

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-2, MMS APN settings, V3.1

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 823277 [details] [diff] [review]
Part 4, handle service ID, V3.1

Review of attachment 823277 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with preference observer setup restored.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +144,5 @@
> +  this.radioInterface = ril.getRadioInterface(aServiceId);
> +};
> +
> +MmsConnection.prototype = {
> +    // To Vicamo: for your convenience of reviewing, I will align the following codes before landing.

Thank you :)  Usually I'll have a separate patch that moves the code to its destination first.  But this works, too.

@@ +485,5 @@
> +      let conn = this._connections[id];
> +      if (conn) {
> +        return conn;
> +      }
> + 

nit: trailing ws

::: dom/mobilemessage/src/gonk/SmsService.cpp
@@ -51,5 @@
> -  NS_WARN_IF_FALSE(mRadioInterface, "This shouldn't fail!");
> -
> -  // Initialize observer.
> -  Preferences::AddStrongObservers(this, kObservedPrefs);
> -  mDefaultServiceId = getDefaultServiceId();

Why do you remove observer and mDefaultServiceId setup?

Comment 49

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 823299 [details] [diff] [review]
Part 5-2, MMS APN settings, V3.1

Review of attachment 823299 [details] [diff] [review]:
-----------------------------------------------------------------

I had this in comment 19 already.  I __really__ don't want to expose ApnSettings structure in RilNetworkInterface.  The things you need are the three attributes -- mmsc, mmsport, mmsproxy.

Comment 50

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #48)
> ::: dom/mobilemessage/src/gonk/SmsService.cpp
> @@ -51,5 @@
> > -  NS_WARN_IF_FALSE(mRadioInterface, "This shouldn't fail!");
> > -
> > -  // Initialize observer.
> > -  Preferences::AddStrongObservers(this, kObservedPrefs);
> > -  mDefaultServiceId = getDefaultServiceId();
> 
> Why do you remove observer and mDefaultServiceId setup?

Sorry for the mistake and thanks for catching this. I removed this by accident when rebasing. :(

Comment 51

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID, V4

Solve comment #48 and make getClientIdByIccId() throw exceptions.

Comment 52

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-2, MMS APN settings, V4

Solve comment #49.

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 823840 [details] [diff] [review]
Part 4, handle service ID, V4

Review of attachment 823840 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +2128,5 @@
> +      } catch (e) {}
> +      if (serviceId == undefined) {
> +        if (DEBUG) debug("RIL service is not available for ICC ID.");
> +        aRequest.notifyGetMessageFailed(Ci.nsIMobileMessageCallback.NO_SIM_CARD_ERROR);
> +        return;

nit: move into |catch (e) { ... }|.

Comment 54

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID, V4.1

Fix comment #53 and carry on r=vicamo.

Comment 55

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 823843 [details] [diff] [review]
Part 5-2, MMS APN settings, V4

Review of attachment 823843 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +162,5 @@
> +    get mmsc() {
> +      let mmsc = this._mmsc;
> +      if (!mmsc) {
> +        try {
> +          mmsc = Services.prefs.getCharPref("ril.mms.mmsc");

From now on, access only |network.mmsFoo| in components other than RadioInterfaceLayer.js.  Let backward-compatibility code live only in RIL.

::: dom/system/gonk/NetworkManager.js
@@ +283,4 @@
>              // Update data connection when Wifi connected/disconnected
>              if (network.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {
> +              for (let i = 0; i < this.mRil.numRadioInterfaces; i++) {
> +                this.mRil.getRadioInterface(i).updateRILNetworkInterface();

I think you have to rebase to include bug 929864, which is currently in b2g-inbound.

@@ +511,5 @@
> +        mmsHosts = this.resolveHostname([network.mmsProxy, network.mmsc]);
> +      } else {
> +        mmsHosts = this.resolveHostname(
> +                     [Services.prefs.getCharPref("ril.mms.mmsproxy"),
> +                      Services.prefs.getCharPref("ril.mms.mmsc")]);

Move these lines for mms pref retrieving into RIL.

@@ +536,5 @@
> +        mmsHosts = this.resolveHostname([network.mmsProxy, network.mmsc]);
> +      } else {
> +        mmsHosts = this.resolveHostname(
> +                     [Services.prefs.getCharPref("ril.mms.mmsproxy"),
> +                      Services.prefs.getCharPref("ril.mms.mmsc")]);

ditto

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3378,5 @@
> +  get mmsc() {
> +    if (this.inConnectedTypes("mms")) {
> +      return this.apnSetting.mmsc;
> +    }
> +    return null;

if (!this.inConnectedTypes(mms)) {
  throw Cr.NS_ERROR_UNEXPECTED;
}

return this.apnSetting.mmsc;

@@ +3385,5 @@
> +  get mmsProxy() {
> +    if (this.inConnectedTypes("mms")) {
> +      return this.apnSetting.mmsproxy;
> +    }
> +    return null;

ditto

@@ +3392,5 @@
> +  get mmsPort() {
> +    if (this.inConnectedTypes("mms")) {
> +      return this.apnSetting.mmsport;
> +    }
> +    return null;

ditto

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +17,5 @@
> +
> +  /* The following attributes are for MMS proxy settings. */
> +  readonly attribute jsval mmsc;
> +  readonly attribute jsval mmsProxy;
> +  readonly attribute jsval mmsPort;

Use primary types and throw an exception when accessed on network interfaces that do not have "mms" in connected types.

Comment 56

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-2, MMS APN settings, V5

Comment 57

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 824525 [details] [diff] [review]
Part 5-2, MMS APN settings, V5

Review of attachment 824525 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you :)

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +495,5 @@
>       *        The MMS connection.
>       * @param method
>       *        "GET" or "POST".
>       * @param url
> +     *        Target url string (only needed for "GET").

"Target url string or null to be replaced by mmsc url."

@@ +560,5 @@
>          }
>  
> +        // MMSC is available after an MMS connection is successfully acquired.
> +        if (method == "POST") {
> +          url = mmsConnection.mmsc;

if (!url) {
  url = mmsConnection.mmsc;
}

Comment 58

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-2, MMS APN settings, V6

Fix the last comment and ready to land. r=vicamo

Comment 59

[Gene Lian (I already quit Mozilla)]

Attachment: Patch 6, test cases, V1

Comment 60

[Gene Lian (I already quit Mozilla)]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=01507d469e70

Comment 61

[Gene Lian (I already quit Mozilla)]

Comment on attachment 825244 [details] [diff] [review]
Patch 6, test cases, V1

I just fixed the existing tests. Not sure how much degree we need to or can do. Please let me know what else you think we'd better to add. Thanks!

Comment 62

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #61)
> I just fixed the existing tests. Not sure how much degree we need to or can
> do. Please let me know what else you think we'd better to add. Thanks!

Nothing special here.  Thank you :)

Comment 63

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, DOM API IDL, V3.1

Rebased.

Comment 64

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, IPDL for IPC, V3.1

Rebased.

Comment 65

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, add ICC ID, V3.1

Rebased.

Comment 66

[Gene Lian (I already quit Mozilla)]

Attachment: Part 4, handle service ID, V4.2

Rebased.

Comment 67

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-1, clean up the APN setting logic, V1.2

Comment 68

[Gene Lian (I already quit Mozilla)]

Attachment: Part 5-2, MMS APN settings, V6.1

Comment 69

[Gene Lian (I already quit Mozilla)]

Attachment: Patch 6, test cases, V1.1

Comment 70

[Gene Lian (I already quit Mozilla)]

Attachment: Patch 7, clean up for alignment, V1

Please see comment #48. Carry on r=vicamo.

Comment 71

[Gene Lian (I already quit Mozilla)]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=359a69d234b1

Comment 72

[Gene Lian (I already quit Mozilla)]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/dc235101657e
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/8a89fbff71e6
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/04d2ad9dda01
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/811fb99ab9c0
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/781a9cd66ba5
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/8aff9442f3e1
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/7e1e62c5b0ff
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/c9d921400e85

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc235101657e
https://hg.mozilla.org/mozilla-central/rev/8a89fbff71e6
https://hg.mozilla.org/mozilla-central/rev/04d2ad9dda01
https://hg.mozilla.org/mozilla-central/rev/811fb99ab9c0
https://hg.mozilla.org/mozilla-central/rev/781a9cd66ba5
https://hg.mozilla.org/mozilla-central/rev/8aff9442f3e1
https://hg.mozilla.org/mozilla-central/rev/7e1e62c5b0ff
https://hg.mozilla.org/mozilla-central/rev/c9d921400e85

Comment 74

[Jason Smith [:jsmith]]

This completely broke MMS. This needs to be backed out.

Comment 75

[Rick Waldron [:rwaldron]]

I'm not sure this is directly related, but in the same time frame that this was introduced, my T-Mobile SIM card is no longer recognized by my Inari (ZTE Open) device. The SIM works fine in the unagi device.

Comment 76

[Anshul]

Hello Gene, I have following questions about this change.
 
- Why is IccId exposed in the SMS Message and not the client Id instead? 
- Why is IccId exposed only for GSM and not for CDMA?

Also could you please point us to the DSDS UX for SMS.

Comment 77

[Gene Lian (I already quit Mozilla)]

(In reply to Jason Smith [:jsmith] from comment #74)
> This completely broke MMS. This needs to be backed out.

Please don't do that. I think it's too early to say so. I did carefully test all the functions before landing. Needless to say we have other patches landed after bug 854326 [1], which are all supposed to be tested before landing as well. It's almost impossible landing so many patches and no one is aware of the failure of sending.

In this week, we did encounter a Gaia regression that would make MMS fail to send. That is, the SMIL library cannot be recognized and loaded. Steve knows what's happening. I'm not sure if he's already opened/solved that bug or not.

[1] http://hg.mozilla.org/mozilla-central/filelog/a07aebef20e7/dom/mobilemessage/src/gonk/MmsService.js

Comment 78

[Gene Lian (I already quit Mozilla)]

(In reply to Anshul from comment #76)
> Hello Gene, I have following questions about this change.
>  
> - Why is IccId exposed in the SMS Message and not the client Id instead? 

We need a way to specify which SIM the message came from or send out. This is especially needed for manually downloading MMS, which could have a time delay after switching the SIM card. Because users would have chances to change SIM card or put the original SIM card from one slot to another, saving client Id (i.e. an Id somehow specifies the slot) for each message is not reliable.

> - Why is IccId exposed only for GSM and not for CDMA?

CDMA doesn't needs a SIM card to be installed so we cannot save/expose any ICC Id for that.

Comment 79

[Anshul]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #78)
> (In reply to Anshul from comment #76)
> > Hello Gene, I have following questions about this change.
> >  
> > - Why is IccId exposed in the SMS Message and not the client Id instead? 
> 
> We need a way to specify which SIM the message came from or send out. This
> is especially needed for manually downloading MMS, which could have a time
> delay after switching the SIM card. Because users would have chances to
> change SIM card or put the original SIM card from one slot to another,
> saving client Id (i.e. an Id somehow specifies the slot) for each message is
> not reliable.
Thanks for your reply Gene. Couldn't we have used the receiver's phone number which would be unique for each SIM.

> 
> > - Why is IccId exposed only for GSM and not for CDMA?
> 
> CDMA doesn't needs a SIM card to be installed so we cannot save/expose any
> ICC Id for that.
Sure but what about the case where you have a CDMA SIM?

Comment 80

[Gene Lian (I already quit Mozilla)]

Sorry for the delayed response. Anshul!

(In reply to Anshul from comment #79)
> Thanks for your reply Gene. Couldn't we have used the receiver's phone
> number which would be unique for each SIM.

Not all the SIM expose MSISDN (phone number). In this case, we'd get trouble to index which SIM we used to receive the MMS notification, thus failing to know which SIM is supposed to be used later to download MMS. That's why we choose ICC ID, which should be available for all the GSM SIM.

> Sure but what about the case where you have a CDMA SIM?

Are you talking about a CDMA phone that also supports GSM SIM? Yes, that's a potential defect. When user choose the GSM mode, we can assign a valid ICC ID for each incoming/outgoing message. However, for the CDMA mode, we cannot. We somehow need to find a way to index the CDMA service. Using service ID to index CDMA service might make sense because the service ID should be always fixed to specify a CDMA service for a given phone. However, as mentioned above it won't work for GSM SIM since SIM card can be reinstalled in a different slot.

Somehow, we need to find a better way to make it compatible with all the CDMA/GSM cases. Maybe, change |.iccId| to just |.Id| and prefix that with the type of service. For example,

  - for CDMA: |.id| = "CDMA:0"
  - for GSM:  |.id| = "GSM:898601YYMHAAAXXXXXXP"

Comment 81

[Anshul]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #80)
> Sorry for the delayed response. Anshul!
> 
> (In reply to Anshul from comment #79)
> > Thanks for your reply Gene. Couldn't we have used the receiver's phone
> > number which would be unique for each SIM.
> 
> Not all the SIM expose MSISDN (phone number). In this case, we'd get trouble
> to index which SIM we used to receive the MMS notification, thus failing to
> know which SIM is supposed to be used later to download MMS. That's why we
> choose ICC ID, which should be available for all the GSM SIM.
> 
> > Sure but what about the case where you have a CDMA SIM?
> 
> Are you talking about a CDMA phone that also supports GSM SIM? Yes, that's a
Sorry I meat CDMA RUIM card which is what is used most widely in North America for CDMA subscriptions. Those cards also have IccId that could be used.

> potential defect. When user choose the GSM mode, we can assign a valid ICC
> ID for each incoming/outgoing message. However, for the CDMA mode, we
> cannot. We somehow need to find a way to index the CDMA service. Using
> service ID to index CDMA service might make sense because the service ID
> should be always fixed to specify a CDMA service for a given phone. However,
> as mentioned above it won't work for GSM SIM since SIM card can be
> reinstalled in a different slot.
> 
> Somehow, we need to find a better way to make it compatible with all the
> CDMA/GSM cases. Maybe, change |.iccId| to just |.Id| and prefix that with
> the type of service. For example,
> 
>   - for CDMA: |.id| = "CDMA:0"
>   - for GSM:  |.id| = "GSM:898601YYMHAAAXXXXXXP"