[meta][B2G] introduce new interfaces as proxy between Telephony modules and SystemMessage service

RESOLVED FIXED in 2.2 S1 (5dec)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: hsinyi, Assigned: bevis)

Tracking

unspecified
2.2 S1 (5dec)
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(feature-b2g:2.2+, tracking-b2g:backlog)

Details

(Whiteboard: [priority1])

Attachments

(5 attachments, 17 obsolete attachments)

17.11 KB, patch
bevis
: review+
Details | Diff | Splinter Review
24.26 KB, patch
bevis
: review+
Details | Diff | Splinter Review
25.17 KB, patch
bevis
: review+
Details | Diff | Splinter Review
30.75 KB, patch
bevis
: review+
Details | Diff | Splinter Review
77.51 KB, patch
bevis
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
We want to greatly reduce the need to change the telephony-related interfaces as we make other changes to gecko. It would be good if we separate the interfaces that we use towards Telephony modules from those that we use in the rest of gecko.

This meta bug is for discussing how we could deal with SystemMessage Services queried in Telephony modules.

I grepped the code, and we now have system messages: telephony-new-call, telephony-call-ended, sms-received, sms-delivery-success, cellbroadcast-received.

Given that the data format of each above message isn't the same and that it's not a good idea to use 'jsval', a proposal in my mind is:

interface nsITelephonyMessenger : nsISupports
{
  void notifyNewCall();
  void notifyCallEnded(in unsigned long serviceId, in AString number,
                       in boolean emergency, in unsigned long duration,
                       in boolean outgoing, in boolean hangUpLocal);
};

interface nsISmsMessenger : nsISupports
{
  const SMS_RECEIVED_NOTIFYCATION = 0;
  const SMS_DELIVERY_SUCCESS_NOTIFICATION = 1;
 
  void notify(in unsigned long type, in AString iccId, in long id,
              in unsigned long long threadId, in AString delivery, in AString deliveryStatus,
              in AString sender, in AString receiver, in AString body,
              in unsigned long timestamp, in unsigned long sentTimestamp,
              in unsigned long deliveryTimestamp, in boolean read);

};

nsICellbroadcastMessenger : nsISupports {
  void notify(in unsigned long serviceId, in AString gsmGeographicalScope,
              in unsigned short messageCode, in unsigned short messageId,
              in AString language, in AString body, in AString messageClass,
              in unsigned long timestamp, in AString etwsWarningType,
              in boolean etwsEmergencyUserAlert, in boolean etwsPopup,
              in long cdmaServiceCategory);

};

Any ideas? Thank you!
(Assignee)

Updated

4 years ago
Assignee: nobody → btseng
(Assignee)

Updated

4 years ago
Depends on: 1072367
(Assignee)

Updated

4 years ago
Blocks: 959978
(Assignee)

Comment 1

4 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #0)
> I grepped the code, and we now have system messages: telephony-new-call,
> telephony-call-ended, sms-received, sms-delivery-success,
> cellbroadcast-received.

Add one more system message from RadioInterface into checking list:
  gSystemMessenger.broadcastMessage("icc-stkcommand",
                                    {iccId: iccId,
                                    command: message});
and we'll have to unpack 'command' property into primitive data as well.
(Assignee)

Comment 2

4 years ago
Summarize the system messages to be handled in this bug:
• "ussd-received" in MobileConnectionService.js
• "telephony-new-call", "telephony-call-ended" in TelephonyService.js
• "cdma-info-rec-received" in RadioInterfaceLayer.js
• "sms-received, "sms-delivery-success", "sms-sent" in RadioInterfaceLayer.js
• "cellbroadcast-received" , CellBroadcastService.js
• "icc-stkcommand" in RadioInterfaceLayer.js
(Reporter)

Updated

4 years ago
Blocks: 1079723
(Assignee)

Comment 3

4 years ago
Update API draft again after further study:

Note:
Most of the system message can be 1-1 mapped to a notifyXXX API, exception 'icc-stkcommand'.
For 'icc-stkcommand', I'd like to treat the structure of a proactive command as a JSON String because 
1. the structure varies among proactive commands.
2. there are too many optional parameters inside a structure.
3. The 3GPP standard keeps adding new optional parameters to the same proactive command.
   We still see the possibility to support more optional parameters of a proactive command in the future.
4. After checking the structure layout of each proactive command, it's lucky that all of them are able to be interchanged by JSON. See the layout of each structure at the end of this comment for more detail information.
5. JSON is a lightweight data-interchange format and various utilities are available in different programming language. (http://www.json.org/)

--------------------------------------------------------------------------------
interface nsITelephonyMessenger : nsISupports
{
  /* 'telephony-new-call' system message */
  void notifyNewCall();
  /* 'telephony-call-ended' system message */
  void notifyCallEnded(in unsigned long aServiceId,
                       in AString aNumber,
                       in boolean aEmergency,
                       in unsigned long aDuration,
                       in boolean aOutgoing,
                       in boolean aHangUpLocal);
};

interface nsISmsMessenger : nsISupports
{
  /* 'sms-received' system message */
  const unsigned short NOTIFICATION_TYPE_RECEIVED = 0;
  /* 'sms-delivery-success' system message */
  const unsigned short NOTIFICATION_TYPE_DELIVERY_SUCCESS = 1;
  /* 'sms-sent' system message */
  const unsigned short NOTIFICATION_TYPE_SENT = 2;
 
  void notifySms(in unsigned short aNotificationType,
                 in long aId,
                 in unsigned long long aThreadId,
                 in DOMString aIccId,
                 in DOMString aDelivery,
                 in DOMString aDeliveryStatus,
                 in DOMString aSender,
                 in DOMString aReceiver,
                 in DOMString aBody,
                 in DOMString aMessageClass,
                 in DOMTimeStamp aTimestamp,
                 in DOMTimeStamp aSentTimestamp,
                 in DOMTimeStamp aDeliveryTimestamp,
                 in boolean aRead);
};

interface nsICellbroadcastMessenger : nsISupports
{
  void notifyCbsReceived(in unsigned long aServiceId,
                         in unsigned long aGsmGeographicalScope,
                         in unsigned short aMessageCode,
                         in unsigned short aMessageId,
                         in DOMString aLanguage,
                         in DOMString aBody,
                         in unsigned long aMessageClass,
                         in DOMTimeStamp aTimestamp,
                         in unsigned long aCdmaServiceCategory,
                         in boolean aHasEtwsInfo,
                         in unsigned long aEtwsWarningType,
                         in boolean aEtwsEmergencyUserAlert,
                         in boolean aEtwsPopup);
};

interface nsIMobileConnectionMessenger: nsISupports
{
  /* 'ussd-received' system message */
  void notifyUssdReceived(in unsigned long aServiceId,
                          in DOMString aMessage,
                          in boolean aSessionEnded);

  /* 'cdma-info-rec-received' system message with Display Info */
  void notifyCdmaInfoRecDisplay(in unsigned long aServiceId,
                                in DOMString aDisplay);

  /* 'cdma-info-rec-received' system message with Called Party Number Info */
  void notifyCdmaInfoRecCalledPartyNumber(in unsigned long aServiceId,
                                          in unsigned short aType,
                                          in unsigned short aPlan,
                                          in DOMString aNumber,
                                          in unsigned short aPi,
                                          in unsigned short aSi);

  /* 'cdma-info-rec-received' system message with Calling Party Number Info */
  void notifyCdmaInfoRecCallingPartyNumber(in unsigned long aServiceId,
                                           in unsigned short aType,
                                           in unsigned short aPlan,
                                           in DOMString aNumber,
                                           in unsigned short aPi,
                                           in unsigned short aSi);

  /* 'cdma-info-rec-received' system message with Connected Party Number Info */
  void notifyCdmaInfoRecConnectedPartyNumber(in unsigned long aServiceId,
                                             in unsigned short aType,
                                             in unsigned short aPlan,
                                             in DOMString aNumber,
                                             in unsigned short aPi,
                                             in unsigned short aSi);

  /* 'cdma-info-rec-received' system message with Signal Info */
  void notifyCdmaInfoRecSignal(in unsigned long aServiceId,
                               in unsigned short aType,
                               in unsigned short aAlertPitch,
                               in unsigned short aSignal);

  /* 'cdma-info-rec-received' system message with Redirecting Number Info */
  void notifyCdmaInfoRecRedirectingNumber(in unsigned long aServiceId,
                                          in unsigned short aType,
                                          in unsigned short aPlan,
                                          in DOMString aNumber,
                                          in unsigned short aPi,
                                          in unsigned short aSi,
                                          in unsigned short aReason);

  /* 'cdma-info-rec-received' system message with Line Control Info */
  void notifyCdmaInfoRecLineControl(in unsigned long aServiceId,
                                    in unsigned short aPolarityIncluded,
                                    in unsigned short aToggle,
                                    in unsigned short aReverse,
                                    in unsigned short aPowerDenial);

  /* 'cdma-info-rec-received' system message with CLIR Info */
  void notifyCdmaInfoRecClir(in unsigned long aServiceId,
                             in unsigned short aCause);

  /* 'cdma-info-rec-received' system message with Audio Control Info */
  void notifyCdmaInfoRecAudioControl(in unsigned long aServiceId,
                                     in short aUpLink,
                                     in short aDownLink);
};

interface nsIIccMessenger: nsISupports
{
  /* 'icc-stkcommand' system mesage 
  *
  * @param aOptions JSON String with the parameters of the corresponding proactive command.
  */
  void notifyStkProactiveCommand(in DOMString aIccId,
                                 in unsigned short aCommandNumber,
                                 in unsigned short aTypeOfCommand,
                                 in unsigned short aCommandQualifier
                                 in DOMString aOptions);
};

--------------------------------------------------------------------------------
Structure of each Proactive command:
STK_CMD_REFRESH (0x01)
  null
STK_CMD_POLL_INTERVAL (0x03)
  see StkProactiveCmdHelperObject.retrieveDuration()
  { 
    timeUnit  (1 byte),
    timeInterval (1 byte)
  }
STK_CMD_POLL_OFF (0x04)
  null
STK_CMD_PROVIDE_LOCAL_INFO (0x26)
  {
    localInfoType (1 byte)
  }
STK_CMD_SET_UP_EVENT_LIST (0x05)
  {
    eventList: [] (byte array) or null
  }
STK_CMD_SET_UP_MENU (0x25)
STK_CMD_SELECT_ITEM (0x24)
  {
    title?: (String)
    items[]: (array of {
      identifier: (1 byte),
      text: (String),
      iconSelfExplanatory? (boolean)
      icons[]?: array of {
        pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
      }
    } )
    defaultItem?: (1 byte)
    presentationType: (1 byte)
    isHelpAvailable?: (boolean)
    nextActionList[]? : (byte array)
    iconSelfExplanatory?: (boolean)
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_DISPLAY_TEXT (0x21)
  {
    text: (String)
    isHighPriority?: (boolean),
    userClear?: (boolean),
    responseNeeded? : (boolean),
    duration?: {
      timeUnit: (1 byte),
      timeInterval: (1 byte)
    },
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_SET_UP_IDLE_MODE_TEXT (0x28)
  {
    text: (String)
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_GET_INKEY (0x22)
  {
    text: (String),
    minLength: (1 byte),
    maxLength: (1 byte),
    duration?: {
      timeUnit: (1 byte),
      timeInterval: (1 byte)
    },
    isAlphabet?: (boolean),
    isUCS2?: (boolean),
    isYesNoRequested? : (boolean),
    isHelpAvailable? : (boolean),
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_GET_INPUT (0x23)
  {
    text: (String),
    minLength: (1 byte),
    maxLength: (1 byte),
    defaultText: (String),
    isAlphabet?: (boolean),
    isUCS2?: (boolean),
    hideInput?: (boolean),
    isPacked?: (boolean),
    isHelpAvailable?: (boolean),
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_SEND_SS (0x11)
STK_CMD_SEND_USSD (0x12)
STK_CMD_SEND_SMS (0x13)
STK_CMD_SEND_DTMF (0x14)
  {
    text?: (String),
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_SET_UP_CALL (0x10)
  {
    address: (String),
    confirmMessage?: (String),
    callMessage?: (String),
    duration?: {
      timeUnit: (1 byte),
      timeInterval: (1 byte)
    },
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),,
    }
  }
STK_CMD_LAUNCH_BROWSER (0x15)
  {
    url: (String),
    mode: (byte),
    confirmMessage?: (String),
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_PLAY_TONE (0x20)
  {
    isVibrate: (boolean),
    text?: (String),
    tone?: (1 byte),
    duration?: {
      timeUnit: (1 byte),
      timeInterval: (1 byte)
    },
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
STK_CMD_TIMER_MANAGEMENT (0x27)
  {
    timerAction: (1 byte),
    timerId: (byte),
    timerValue?: (unsigned long long),
  }
STK_CMD_OPEN_CHANNEL (0x40)
STK_CMD_CLOSE_CHANNEL(0x41)
STK_CMD_RECEIVE_DATA(0x42)
STK_CMD_SEND_DATA (x43)
  {
    text?: (String),
    iconSelfExplanatory?: (boolean),
    icons[]?: array of {
      pixels: (Array of UInt32),
      codingScheme: (String of "basic", "color", "color-transparency"),
      width: (1 byte),
      height: (1 byte),
    }
  }
(Assignee)

Comment 4

4 years ago
Hi Hsinyi & Edgar,

May I have your feedback of the API draft in comment 3?

Thanks!
Flags: needinfo?(htsai)
Flags: needinfo?(echen)
(Assignee)

Comment 5

4 years ago
Per offline discussion, we still expect to have precise data structures and APIs to understand what and how STK proactive commands are supported.

The plan for for support notifying StkProactiveCommand will be changed from
  void notifyStkProactiveCommand(in DOMString aIccId,
                                 in unsigned short aCommandNumber,
                                 in unsigned short aTypeOfCommand,
                                 in unsigned short aCommandQualifier
                                 in DOMString aOptions);
to 
  void notifyStkProactiveCommand(in DOMString aIccId,
                                 in nsIStkProactiveCommand);
nsIStkProactiveCommand is defined as followed:
interface nsIStkProactiveCommand : nsISupports
{
  readonly attribute unsigned short aCommandNumber;
  readonly attribute unsigned short aTypeOfCommand;
  readonly attribute unsigned short aCommandQualifier;
}

And we will define a series subInterfaces of nsIStkProactiveCommand to support different type of STK proactive commands.
Flags: needinfo?(htsai)
Flags: needinfo?(echen)

Comment 6

4 years ago
Awesome, thank you Bevis.
(Assignee)

Comment 7

4 years ago
Created attachment 8507819 [details] [diff] [review]
WIP: Add TelephonyMessenger as a Wrapper of the TelephonyService-Related System Message.

Hi Hsinyi, Edgar,

This is a WIP patch to have a rough picture of how the wrapper of the telephony related system message to be implemented.
May I have your feedback to see if we are OK to move on based on this design?

I take nsITelephonyMessenger as the example in this patch:
- /dom/system/gonk/RILSystemMessengerHelper.js:
  The implementation of All nsIXxxMessenger.idl
- /dom/system/gonk/RILSystemMessenger.jsm
  The Standalone JSM to create the JS object message with corresponding message type for broadcasting.
  It is defined as a JSM for easier writing of the xpc_shell test cases.
- /dom/system/gonk/tests/test_ril_system_messenger.js
  XPC Shell test cases.
- /dom/telephony/gonk/TelephonyService.js
  Replace gSystemMessenger with gTelephonyMessenger
- /dom/telephony/nsITelephonyMessenger.idl
  The idl definition for nsITelephonyMessenger.

Then, when a new nsIXxxMessenger is introduced, we can have the implementation in RILSystemMessengerHelper and the test cases in test_ril_system_messenger.js respectively.

In addition, an additional JSM is expected to be created for the JS implementation of the idl interfaces of all the STK proactive commands.
Then, we could import it in both RadioInterfaceLayer for the use cases and test_ril_system_messenger.js for the test cases.

Thanks!
Attachment #8507819 - Flags: feedback?(htsai)
Attachment #8507819 - Flags: feedback?(echen)
(Reporter)

Comment 8

4 years ago
Comment on attachment 8507819 [details] [diff] [review]
WIP: Add TelephonyMessenger as a Wrapper of the TelephonyService-Related System Message.

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

Hi Bevis,
The overall structure looks good. This is the way to go.

One minor comment inline, thank you :)

::: dom/system/gonk/RILSystemMessengerHelper.js
@@ +35,5 @@
> + * RILSystemMessengerHelper
> + */
> +function RILSystemMessengerHelper() {
> +  this.messenger = new RSM.RILSystemMessenger();
> +  this.messenger.broadcastMessage = (aType, aMessage, aExtra) => {

Let's define this in RILSystemMessenger.jsm. If xpcshell test needs some fake object, we can still re-define a fake broadcastMessage function in a xpcshell test with a default function.
Attachment #8507819 - Flags: feedback?(htsai) → feedback+

Comment 9

4 years ago
Comment on attachment 8507819 [details] [diff] [review]
WIP: Add TelephonyMessenger as a Wrapper of the TelephonyService-Related System Message.

Looks good, thank you.
Attachment #8507819 - Flags: feedback?(echen) → feedback+
Created attachment 8515886 [details] [diff] [review]
Part 1 v1: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages.

This patch is to 
1. have TelephonyMessenger to take care of the system messages of |telephony-new-call| and |telephony-call-ended|.
2. Add corresponding test cases to ensure the test coverage.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8507819 - Attachment is obsolete: true
Attachment #8515886 - Flags: review?(echen)
Comment on attachment 8515886 [details] [diff] [review]
Part 1 v1: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages.

sorry, wrong reviewer name in the patch.
Attachment #8515886 - Flags: review?(echen)
Created attachment 8515888 [details] [diff] [review]
Part 1 v1: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages.

This patch is to 
1. have TelephonyMessenger to take care of the system messages of |telephony-new-call| and |telephony-call-ended|.
2. Add corresponding test cases to ensure the test coverage.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8515886 - Attachment is obsolete: true
Attachment #8515888 - Flags: review?(echen)
Created attachment 8515889 [details] [diff] [review]
Part 2 v1: Add SmsMessenger as a Wrapper for Sms-Related System Messages.

1. Add SmsMessenger as a Wrapper for Sms-Related System Messages
2. Add corresponding test cases to enhance test coverage.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8515889 - Flags: review?(echen)
Created attachment 8515890 [details] [diff] [review]
Part 3 v1: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.

1. Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.
2. Add corresponding test case to verify this change.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8515890 - Flags: review?(echen)
(Assignee)

Updated

4 years ago
Attachment #8515890 - Attachment description: Part 3: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages. → Part 3 v1: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.
(Assignee)

Updated

4 years ago
Attachment #8515889 - Attachment description: Part 2 v2: Add SmsMessenger as a Wrapper for Sms-Related System Messages. r=echen → Part 2 v1: Add SmsMessenger as a Wrapper for Sms-Related System Messages. r=echen
Created attachment 8515893 [details] [diff] [review]
Part 4 v1: Add MobileConnectionMessenger as a Wrapper for USSD/CdmaInfo System Messages.

1. Add MobileConnectionMessenger as a Wrapper for USSD/CdmaInfo System Messages.
2. Add corresponding test case to verify this change.

Hi Edgar,

May I have your review for this change?

Thanks!
Attachment #8515893 - Flags: review?(echen)
(Assignee)

Updated

4 years ago
Attachment #8515889 - Attachment description: Part 2 v1: Add SmsMessenger as a Wrapper for Sms-Related System Messages. r=echen → Part 2 v1: Add SmsMessenger as a Wrapper for Sms-Related System Messages.
Created attachment 8515901 [details] [diff] [review]
Part 5 v1: Add IccMessenger as a Wrapper for STK-related System Messages.

1. Add IccMessenger as a Wrapper for STK-related System Messages
2. Add corresponding test cases to verify this change.

Hi Hsinyi, Edgar,

Because the change in STK part is huge (about 2.5k lines added),
I'd like to summarize this change here for better review:
1. nsIIccMessenger.idl defines:
   - nsIIccMessenger as the public service to broadcast the 'icc-stkcommand' system message.
   - nsIStkXxxCmd are the corresponding data structures of all STK proactive command we broadcast.
2. StkProactiveCmdFactory.jsm plays the role to:
   - createCommand(): Create nsIStkXxxCmd according to the JS object from RadioInterfaceLayer.
   - createSystemMessage(): Create the system message per nsIStkXxxCmd.

May I have both of your review for this change?

Thanks!
Attachment #8515901 - Flags: review?(htsai)
Attachment #8515901 - Flags: review?(echen)

Comment 17

4 years ago
Comment on attachment 8515888 [details] [diff] [review]
Part 1 v1: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages.

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

Please see my below comments. Thank you.

::: dom/system/gonk/RILSystemMessengerHelper.js
@@ +37,5 @@
> + * RILSystemMessengerHelper
> + */
> +function RILSystemMessengerHelper() {
> +  this.messenger = new RSM.RILSystemMessenger();
> +  this.messenger.broadcastMessage = (aType, aMessage, aExtra) => {

As mentioned in comment #8, could we define this in RILSystemMessenger.jsm?

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +15,5 @@
> + */
> +let RSM;
> +
> +let gReceivedMsgType = null;
> +let gReceivedMessage = null;

nit: space after this line

@@ +42,5 @@
> +  let receivedMessage = gReceivedMessage;
> +  gReceivedMsgType = null;
> +  gReceivedMessage = null;
> +  equal(aType, receivedMsgType);
> +  deepEqual(aMessage, receivedMessage);

nit:

equal(aType, gReceivedMsgType);
deepEqual(aMessage, gReceivedMessage);

gReceivedMsgType = null;
gReceivedMessage = null;

@@ +50,5 @@
> + * Verify that each nsIXxxMessenger could be retrieved.
> + */
> +function run_test() {
> +  // TODO: verify that all nsIXXXMessenger are loaded.
> +  ok(gTelephonyMessenger != null, "Get TelephonyMessenger.");

If |gTelephonyMessenger| is only used here, we don't have to put it in global scrope.

How about 

let telephonyMessenger = Cc["@mozilla.org/ril/system-messenger-helper;1"].getService(Ci.nsITelephonyMessenger)
ok(telephonyMessenger != null, "Get TelephonyMessenger.");
Attachment #8515888 - Flags: review?(echen)

Comment 18

4 years ago
Comment on attachment 8515889 [details] [diff] [review]
Part 2 v1: Add SmsMessenger as a Wrapper for Sms-Related System Messages.

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

::: dom/mobilemessage/interfaces/nsISmsMessenger.idl
@@ +21,5 @@
> +   * Note: Except aNotificationType, all parameters are the attributes of the
> +   * nsIDOMMozSmsMessage generated by nsIMobileMessageService.createSmsMessage().
> +   *
> +   * @param aNotificationType
> +   *        Predefined constant of nsISmsMessenger.NOTIFICATION_TYPE

nit: s/NOTIFICATION_TYPE/NOTIFICATION_TYPE_*/

@@ +29,5 @@
> +   *        The unique identity of the thread this message belongs to.
> +   * @param aIccId
> +   *        Integrated Circuit Card Identifier. null if ICC is not available.
> +   * @param aDelivery
> +   *        Should be "received", "sending", "sent" or "error".

Per offline discussion, let consts it in nsISmsService.idl.

@@ +31,5 @@
> +   *        Integrated Circuit Card Identifier. null if ICC is not available.
> +   * @param aDelivery
> +   *        Should be "received", "sending", "sent" or "error".
> +   * @param aDeliveryStatus
> +   *        Should be "success", "pending", "not-applicable" or "error".

Ditto.

@@ +39,5 @@
> +   *        Receiver address. null if not available.
> +   * @param aBody
> +   *        Text message body. null if not available.
> +   * @param aMessageClass
> +   *        Should be "normal", "class-0", "class-1", "class-2" or "class-3".

And this
Attachment #8515889 - Flags: review?(echen)

Comment 19

4 years ago
Comment on attachment 8515890 [details] [diff] [review]
Part 3 v1: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.

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

::: dom/cellbroadcast/interfaces/nsICellbroadcastMessenger.idl
@@ +13,5 @@
> +   *
> +   * @param aServiceId
> +   *        The ID of Service where this info is notified from.
> +   * @param aGsmGeographicalScope
> +   *        @See nsICellBroadcastService.GSM_GEOGRAPHICAL_SCOPE

nit: s/GSM_GEOGRAPHICAL_SCOPE/GSM_GEOGRAPHICAL_SCOPE_*/

@@ +24,5 @@
> +   *        ISO-639-1 language code for this message. Null if unspecified.
> +   * @param aBody
> +   *        Text message carried by the message.
> +   * @param aMessageClass
> +   *        @See nsICellBroadcastService.GSM_MESSAGE_CLASS

Ditto, _*

@@ +32,5 @@
> +   *        CDMA Service Category.
> +   * @param aHasEtwsInfo
> +   *        True if ETWS Info is included in this message.
> +   * @param aEtwsWarningType
> +   *        @See nsICellBroadcastService.GSM_ETWS_WARNING

And here.

::: dom/system/gonk/RILSystemMessengerHelper.js
@@ +88,5 @@
> +   */
> +  notifyCbMessageReceived: function(aServiceId, aGsmGeographicalScope, aMessageCode,
> +                                    aMessageId, aLanguage, aBody, aMessageClass,
> +                                    aTimestamp, aCdmaServiceCategory, aHasEtwsInfo,
> +                                    aEtwsWarningType, aEtwsEmergencyUserAlert,aEtwsPopup) {

nit: space after ,

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +60,5 @@
>  function run_test() {
>    // TODO: verify that all nsIXXXMessenger are loaded.
>    ok(gTelephonyMessenger != null, "Get TelephonyMessenger.");
>    ok(gSmsMessenger != null, "Get SmsMessenger.");
> +  ok(gCellbroadcastMessenger != null, "Get CellbroadcastMessenger.");

Same as comment #17, don't have to put gCellbroadcastMessenger in global scrope.
Attachment #8515890 - Flags: review?(echen)
Comment on attachment 8515901 [details] [diff] [review]
Part 5 v1: Add IccMessenger as a Wrapper for STK-related System Messages.

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

Sorry, I found I made a mistake to wrap the system message of 'icc-stkcommand' in a wrong way.
The correct only shall be the one in-lined. :(
I'll correct this in next update.

::: dom/icc/gonk/StkProactiveCmdFactory.jsm
@@ +974,5 @@
> +  /**
> +   * @param  nsIStkProactiveCmd instance.
> +   * @return a Javascript object with the same structure to MozStkCommandEvent.
> +   */
> +  createSystemMessage: function(aIccId, aStkProactiveCmd) {

Shall be change to 
createSystemMessage: function(aStkProactiveCmd) {

::: dom/system/gonk/RILSystemMessenger.jsm
@@ +302,5 @@
> +   * Wrapper to send 'icc-stkcommand' system message with Audio Control Info.
> +   */
> +  notifyStkProactiveCommand: function(aIccId, aCommand) {
> +    this.broadcastMessage("icc-stkcommand",
> +                          gStkCmdFactory.createSystemMessage(aIccId, aCommand));

Shall be changed to 
    this.broadcastMessage("icc-stkcommand",
                          { 
                            iccId: aIccId,
                            command: gStkCmdFactory.createSystemMessage(aIccId, aCommand)
                          });

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +1143,5 @@
> +    messenger.notifyStkProactiveCommand(iccId,
> +                                        gStkCmdFactory.createCommand(aMessage));
> +
> +    aMessage.iccId = iccId; // append IccId for comparison.
> +    equal_received_system_message("icc-stkcommand", aMessage);

equal_received_system_message("icc-stkcommand", 
                                  { iccId: iccId,
                                    command: aMessage});

Comment 21

4 years ago
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #20)
> Comment on attachment 8515901 [details] [diff] [review]
> Part 5 v1: Add IccMessenger as a Wrapper for STK-related System Messages.
> 
> Review of attachment 8515901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I found I made a mistake to wrap the system message of
> 'icc-stkcommand' in a wrong way.
> The correct only shall be the one in-lined. :(
> I'll correct this in next update.
> 

This reminds me bug 945576. :p
Let's see if the problem I met (bug 945576 comment #1) is still there.

Comment 22

4 years ago
Comment on attachment 8515901 [details] [diff] [review]
Part 5 v1: Add IccMessenger as a Wrapper for STK-related System Messages.

Cancel r? per comment #20.
Attachment #8515901 - Flags: review?(htsai)
Attachment #8515901 - Flags: review?(echen)

Comment 23

4 years ago
Comment on attachment 8515893 [details] [diff] [review]
Part 4 v1: Add MobileConnectionMessenger as a Wrapper for USSD/CdmaInfo System Messages.

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

Looks good, but I would like to review it again after the review comments of other parts are addressed. Thank you.

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +66,5 @@
>    // TODO: verify that all nsIXXXMessenger are loaded.
>    ok(gTelephonyMessenger != null, "Get TelephonyMessenger.");
>    ok(gSmsMessenger != null, "Get SmsMessenger.");
>    ok(gCellbroadcastMessenger != null, "Get CellbroadcastMessenger.");
> +  ok(gMobileConnectionMessenger != null, "Get MobileConnectionMessenger.");

Same as comment #17, don't have to put gMobileConnectionMessenger in global scrope.
Attachment #8515893 - Flags: review?(echen)
(Assignee)

Updated

4 years ago
Depends on: 1091454

Comment 24

4 years ago
Hsin-Yi, wondering instead of creating yet another set of interfaces, can we not expose these messages through the existing interfaces such as nsITelephonyProvider, nsICellbroadcastService and so on?
Flags: needinfo?(htsai)
(Reporter)

Comment 25

4 years ago
(In reply to Anshul from comment #24)
> Hsin-Yi, wondering instead of creating yet another set of interfaces, can we
> not expose these messages through the existing interfaces such as
> nsITelephonyProvider, nsICellbroadcastService and so on?

Hello Anshul,

I discussed with teams, but we are not sure about the benefit from putting Messenger interfaces & implementation into existing Gonk Services like TelephonyService, CellBroadcastService, etc? Could you explain and describe difficulty to you due to the current design if any? 

Also, the new Messenger is implemented by Mozilla and QC only has to use it instead of re-implement it. Thus, it does sound reasonable for me to separate Messengers interfaces from nsITelephonyService interfaces, which you have your own implementation.
Flags: needinfo?(htsai)

Comment 26

4 years ago
Hsin-Yi, no difficulty in implementing at all; was just trying to reduce the number of interfaces. How about having one common interface for all such messages instead of separate one? Again, its really your call, my idea was just to keep the number of interfaces we have to not grow very big.
(Reporter)

Comment 27

4 years ago
(In reply to Anshul from comment #26)
> Hsin-Yi, no difficulty in implementing at all; was just trying to reduce the
> number of interfaces. How about having one common interface for all such
> messages instead of separate one? Again, its really your call, my idea was
> just to keep the number of interfaces we have to not grow very big.

Hi Anshul,
If there's no difficulty in your implementation, let's keep separate interfaces as now to have great modularity, though I have to confess your concern (long list) sounds valid. Thank you.
Created attachment 8521177 [details] [diff] [review]
Part 1 v2: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages.

1. precisely define the stub of |broadcastMessage| as its prototype.
2. address the nits in comment 17.
Attachment #8515888 - Attachment is obsolete: true
Attachment #8521177 - Flags: review?(echen)
Created attachment 8521181 [details] [diff] [review]
Part 2 v2: Add SmsMessenger as a Wrapper for Sms-Related System Messages.

Define |delivery|, |deliveryStatus|, and |messageClass| as predefined constants.
Attachment #8515889 - Attachment is obsolete: true
Attachment #8521181 - Flags: review?(echen)
Created attachment 8521182 [details] [diff] [review]
Part 3 v2: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.

address the suggestion in comment 19 to test CellbroadcastMessenger in local scrope.
Attachment #8515890 - Attachment is obsolete: true
Attachment #8521182 - Flags: review?(echen)
Created attachment 8521184 [details] [diff] [review]
Part 4 v2: Add MobileConnectionMessenger as a Wrapper for USSD/CdmaInfo System Messages.

Validate MobileConnectionMessenger in local scrope.
Attachment #8515893 - Attachment is obsolete: true
Attachment #8521184 - Flags: review?(echen)
Created attachment 8521185 [details] [diff] [review]
Part 5 v2: Add IccMessenger as a Wrapper for STK-related System Messages.

1. Fix the problem mentioned in comment 20.
2. Create icons for SET_UP_CALL/LAUNCH_BROWSER according to the change in bug 1091454.
Attachment #8515901 - Attachment is obsolete: true
Attachment #8521185 - Flags: review?(htsai)
Attachment #8521185 - Flags: review?(echen)

Comment 33

4 years ago
Comment on attachment 8521177 [details] [diff] [review]
Part 1 v2: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages.

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

::: dom/system/gonk/RILSystemMessengerHelper.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

nit: don't need this.

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +37,5 @@
> +function equal_received_system_message(aType, aMessage) {
> +  let receivedMsgType = gReceivedMsgType;
> +  let receivedMessage = gReceivedMessage;
> +  equal(aType, receivedMsgType);
> +  deepEqual(aMessage, receivedMessage);

nit: It seems we could use |gReceived*| directly here, then we don't need |receivedMsg| and |receivedMessage|.
Attachment #8521177 - Flags: review?(echen) → review+

Comment 34

4 years ago
Comment on attachment 8521181 [details] [diff] [review]
Part 2 v2: Add SmsMessenger as a Wrapper for Sms-Related System Messages.

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

The patch itself looks good, but per offline discussion, defining a consts for `INVALID` input value (e.g. DELIVERY_TYPE_INVALID) seems not necessary. Please help to remove them.
Thank you.

::: dom/mobilemessage/interfaces/nsISmsMessenger.idl
@@ +14,5 @@
> +  const unsigned short NOTIFICATION_TYPE_SENT = 1;
> +  /* 'sms-delivery-success' system message */
> +  const unsigned short NOTIFICATION_TYPE_DELIVERY_SUCCESS = 2;
> +
> +  const unsigned short NOTIFICATION_TYPE_INVALID = 3;

Remove this and all other `*_INVALID`

::: dom/system/gonk/RILSystemMessenger.jsm
@@ +59,5 @@
>  
>      this.broadcastMessage("telephony-call-ended", data);
> +  },
> +
> +  _SMS_NOTIFICATION_TYPES: [

_SMS_NOTIFICATION_TYPES is only being used in notifySms(), could we move it there?
Or you put it here for some purpose?

@@ +65,5 @@
> +    "sms-sent",
> +    "sms-delivery-success"
> +  ],
> +
> +  _SMS_DELIVERY: [

Ditto, move it to _convertSmsDelivery().

@@ +72,5 @@
> +    "sent",
> +    "error"
> +  ],
> +
> +  _SMS_DELIVERY_STATUS: [

Ditto, move it to _convertSmsDeliveryStatus().

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +244,5 @@
> +    run_next_test();
> +    return;
> +  }
> +
> +  ok(false, "Failed to verify the protection of invalid nsISmsMessenger.NOTIFICATION_TYPE!");

nit: I prefer below code sequence,

try {
  messenger.notifySms(...);
  ok(false, "...");
} catch (e) {}

run_next_test();
Attachment #8521181 - Flags: review?(echen)

Comment 35

4 years ago
Comment on attachment 8521182 [details] [diff] [review]
Part 3 v2: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.

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

Patch looks good, but per discussion, I would like to see a new version removing the `*_INVALID` consts defined in nsICellbroadcastService.idl.
Thank you very much.
Attachment #8521182 - Flags: review?(echen)

Comment 36

4 years ago
Comment on attachment 8521184 [details] [diff] [review]
Part 4 v2: Add MobileConnectionMessenger as a Wrapper for USSD/CdmaInfo System Messages.

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

Looks good to me, thank you.

::: dom/telephony/gonk/TelephonyService.js
@@ +1323,3 @@
>      }
>  
>      let info = {

remove this as well.
Attachment #8521184 - Flags: review?(echen) → review+
Created attachment 8522717 [details] [diff] [review]
Part 1 v3: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages. r=echen

address nits in comment 33.
Attachment #8521177 - Attachment is obsolete: true
Attachment #8522717 - Flags: review+
Created attachment 8522719 [details] [diff] [review]
Part 2 v3: Add SmsMessenger as a Wrapper for Sms-Related System Messages.

remove unnecessary *_INVALID constants.
Attachment #8521181 - Attachment is obsolete: true
Attachment #8522719 - Flags: review?(echen)
Created attachment 8522720 [details] [diff] [review]
Part 3 v3: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.

remove *_INVALID constants if possible.
Attachment #8521182 - Attachment is obsolete: true
Attachment #8522720 - Flags: review?(echen)
Created attachment 8522721 [details] [diff] [review]
Part 4 v3: Add MobileConnectionMessenger as a Wrapper for USSD/CdmaInfo System Messages. r=echen

address the problem in comment 36.
Attachment #8521184 - Attachment is obsolete: true
Attachment #8522721 - Flags: review+

Comment 41

4 years ago
Comment on attachment 8522719 [details] [diff] [review]
Part 2 v3: Add SmsMessenger as a Wrapper for Sms-Related System Messages.

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

Thank you.
Attachment #8522719 - Flags: review?(echen) → review+
(Reporter)

Comment 42

4 years ago
Comment on attachment 8521185 [details] [diff] [review]
Part 5 v2: Add IccMessenger as a Wrapper for STK-related System Messages.

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

One major comment is about |.bind(this)| usage. "=>" is a very compact style but I am concerned we seem introduce too many unnecessary |.bind(this)| which is relatively heavy. Could we revise them a bit? (Sorry that I didn't highlight this in previous reviews.)

Overall this is awesome! You rock, Bevis :)

::: dom/icc/gonk/StkProactiveCmdFactory.jsm
@@ +29,5 @@
> +function mapIconInfoToStkIconInfo(aIconInfo) {
> +  let mapIconToStkIcon = (aIcon) => {
> +    return new StkIcon(aIcon.width, aIcon.height,
> +                       aIcon.codingScheme, aIcon.pixels);
> +  };

Though this is okay, |.bind(this)| isn't necessary.

@@ +50,5 @@
> +}
> +
> +function appendIconInfo(aTarget, aStkIconInfo) {
> +  aTarget.iconSelfExplanatory = aStkIconInfo.iconSelfExplanatory;
> +  aTarget.icons = aStkIconInfo.getIcons().map((aStkIcon) => {

ditto

@@ +298,5 @@
> +  };
> +
> +  let eventList = aStkSetupEventListCmd.getEventList();
> +
> + // Note: we got empty array [] instead of null even null is return from getEventList.

The code looks good, but I don't quite understand the comment. When did we get an empty array @@?

@@ +315,5 @@
> +  if (options.title) {
> +    this.title = options.title;
> +  }
> +
> +  this.items = options.items.map((aItem) => {

ditto: though it's okay, don't think it's necessary to have |.bind(this)|

@@ +385,5 @@
> +  // Call |StkCommandMessage| constructor.
> +  StkCommandMessage.call(this, aStkMenuCmd);
> +
> +  this.options = {
> +    items: aStkMenuCmd.getItems().map((aStkItem) => {

ditto

::: dom/icc/interfaces/nsIIccMessenger.idl
@@ +123,5 @@
> +
> +  const unsigned long TIMER_VALUE_INVALID = 0xFFFFFFFF;
> +
> +  /**
> +   * (Conditional)

Not really get what the keyword "conditional" represents :P Mind clarifying?

@@ +151,5 @@
> +
> +/**
> + * The base class of all STK Proactive Commands.
> + *
> + * This interface is to be applied by the commands without options:

I understand what "options" is in this sentence but it's not clear to who don't read the code as there's no "options" defined in the .idl. Please revise the comment a bit!

@@ +214,5 @@
> +[scriptable, uuid(5f796dec-5e6a-11e4-aaf3-bb675cb36df5)]
> +interface nsIStkSetupEventListCmd : nsIStkProactiveCmd
> +{
> +  /**
> +   * Get the list of events. (Optional)

nit: put "(Optional)" above this as others do.

@@ +396,5 @@
> +  readonly attribute boolean responseNeeded;
> +};
> +
> +/**
> + * This based interface for nsIStkInputKeyCmd, nsIStkInputTextCmd.

nit: The base interface of ... ...

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +828,5 @@
> +      commandQualifier: 0x81, // High Priority, User Clear
> +      options: {
> +        text: "Display Text 1",
> +        isHighPriority: true,
> +        userClear: false,

should be 'true'?

I realized the value doesn't actually affect the test result as here it just verifies the encoding and decoding functions in the proxy. But to reduce potential confusion, let's still get the values right :)

@@ +829,5 @@
> +      options: {
> +        text: "Display Text 1",
> +        isHighPriority: true,
> +        userClear: false,
> +        responseNeeded: true

nit: I guess you wanted this value to be "false" as you said this part is for 'mandatory properties.'

@@ +839,5 @@
> +      typeOfCommand: RIL.STK_CMD_DISPLAY_TEXT,
> +      commandQualifier: 0x81, // High Priority, User Clear
> +      options: {
> +        text: "Display Text 2",
> +        isHighPriority: false,

this should be 'true'

@@ +841,5 @@
> +      options: {
> +        text: "Display Text 2",
> +        isHighPriority: false,
> +        userClear: true,
> +        responseNeeded: false,

nit: I guess you wanted this value to be "true" as you said this part is for 'optional properties.'

@@ +915,5 @@
> +          timeUnit: RIL.STK_TIME_UNIT_SECOND,
> +          timeInterval: 0x0A
> +        },
> +        isAlphabet: true,
> +        isUCS2: false,

should be 'true'

@@ +932,5 @@
> +        text: "Get Input Text",
> +        minLength: 1,
> +        maxLength: 255,
> +        defaultText: "Default Input Text",
> +        isAlphabet: false,

should be true

@@ +934,5 @@
> +        maxLength: 255,
> +        defaultText: "Default Input Text",
> +        isAlphabet: false,
> +        isUCS2: true,
> +        isHelpAvailable: false,

true here

@@ +936,5 @@
> +        isAlphabet: false,
> +        isUCS2: true,
> +        isHelpAvailable: false,
> +        hideInput: true,
> +        isPacked: false,

true here

@@ +1097,5 @@
> +    },
> +    null // Termination condition to run_next_test()
> +  ];
> +
> +  messages.forEach((aMessage) => {

ditto
Attachment #8521185 - Flags: review?(htsai)
Comment on attachment 8521185 [details] [diff] [review]
Part 5 v2: Add IccMessenger as a Wrapper for STK-related System Messages.

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

::: dom/icc/gonk/StkProactiveCmdFactory.jsm
@@ +29,5 @@
> +function mapIconInfoToStkIconInfo(aIconInfo) {
> +  let mapIconToStkIcon = (aIcon) => {
> +    return new StkIcon(aIcon.width, aIcon.height,
> +                       aIcon.codingScheme, aIcon.pixels);
> +  };

Will do.

@@ +50,5 @@
> +}
> +
> +function appendIconInfo(aTarget, aStkIconInfo) {
> +  aTarget.iconSelfExplanatory = aStkIconInfo.iconSelfExplanatory;
> +  aTarget.icons = aStkIconInfo.getIcons().map((aStkIcon) => {

Will do.

@@ +298,5 @@
> +  };
> +
> +  let eventList = aStkSetupEventListCmd.getEventList();
> +
> + // Note: we got empty array [] instead of null even null is return from getEventList.

I think I should remove this comment to prevent confusing. :p

My original implementation is 
if (eventList) {
  this.options.eventList = eventList;
}

However, when testing, even the implementation in StkSetupEventListCmd.getEventList() return null,
I always got [] instead of null.

Same problem was found in StkIconInfo.getIcons(), StkMenuCmd.getItems() and StkMenuCmd.getNextActionList().

@@ +315,5 @@
> +  if (options.title) {
> +    this.title = options.title;
> +  }
> +
> +  this.items = options.items.map((aItem) => {

Will do.

@@ +385,5 @@
> +  // Call |StkCommandMessage| constructor.
> +  StkCommandMessage.call(this, aStkMenuCmd);
> +
> +  this.options = {
> +    items: aStkMenuCmd.getItems().map((aStkItem) => {

will do.

::: dom/icc/interfaces/nsIIccMessenger.idl
@@ +123,5 @@
> +
> +  const unsigned long TIMER_VALUE_INVALID = 0xFFFFFFFF;
> +
> +  /**
> +   * (Conditional)

M... 
According to 6.6.21 TIMER MANAGEMENT in TS 11.14,
"
The SIM shall supply this data object only when a timer has to be started.
"
this is mandatory only if timerAction == TIMER_ACTION_START.
I'll update the comment to explain more about this. :p

@@ +151,5 @@
> +
> +/**
> + * The base class of all STK Proactive Commands.
> + *
> + * This interface is to be applied by the commands without options:

Opps! 
You are right. It's too implementation-specific.
I'll see how to explain it in a better way.

@@ +214,5 @@
> +[scriptable, uuid(5f796dec-5e6a-11e4-aaf3-bb675cb36df5)]
> +interface nsIStkSetupEventListCmd : nsIStkProactiveCmd
> +{
> +  /**
> +   * Get the list of events. (Optional)

Will do.

@@ +396,5 @@
> +  readonly attribute boolean responseNeeded;
> +};
> +
> +/**
> + * This based interface for nsIStkInputKeyCmd, nsIStkInputTextCmd.

Will do.

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +829,5 @@
> +      options: {
> +        text: "Display Text 1",
> +        isHighPriority: true,
> +        userClear: false,
> +        responseNeeded: true

I think, I didn't set commandQualifier according to the attributes in these options.
I'll update the commandQualifier with correct value specified.

@@ +841,5 @@
> +      options: {
> +        text: "Display Text 2",
> +        isHighPriority: false,
> +        userClear: true,
> +        responseNeeded: false,

ditto.

@@ +917,5 @@
> +        },
> +        isAlphabet: true,
> +        isUCS2: false,
> +        isHelpAvailable: true,
> +        isYesNoRequested: true,

ditto.

@@ +932,5 @@
> +        text: "Get Input Text",
> +        minLength: 1,
> +        maxLength: 255,
> +        defaultText: "Default Input Text",
> +        isAlphabet: false,

ditto.

@@ +934,5 @@
> +        maxLength: 255,
> +        defaultText: "Default Input Text",
> +        isAlphabet: false,
> +        isUCS2: true,
> +        isHelpAvailable: false,

ditto.

@@ +936,5 @@
> +        isAlphabet: false,
> +        isUCS2: true,
> +        isHelpAvailable: false,
> +        hideInput: true,
> +        isPacked: false,

ditto.

@@ +1097,5 @@
> +    },
> +    null // Termination condition to run_next_test()
> +  ];
> +
> +  messages.forEach((aMessage) => {

Will do.

Comment 44

4 years ago
Comment on attachment 8522720 [details] [diff] [review]
Part 3 v3: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages.

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

Looks good, thank you.
Attachment #8522720 - Flags: review?(echen) → review+

Comment 45

4 years ago
Comment on attachment 8521185 [details] [diff] [review]
Part 5 v2: Add IccMessenger as a Wrapper for STK-related System Messages.

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

::: dom/icc/gonk/StkProactiveCmdFactory.jsm
@@ +265,5 @@
> +    this.eventList = eventList.slice();
> +  }
> +}
> +StkSetupEventListCmd.prototype = {
> +  __proto__: StkProactiveCommand.prototype,

Since __proto__ is deprecated [1], let's do inheritance in following way,

StkSetupEventListCmd.prototype = Object.create(StkProactiveCommand.prototype, {
  ....
});

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto

@@ +585,5 @@
> +  minLength: 1,
> +  maxLength: 1,
> +  defaultText: null,
> +  isAlphabet: false,
> +  isUCS2:false,

nit: space after :

@@ +665,5 @@
> +                                         Ci.nsIStkInputCmd,
> +                                         Ci.nsIStkInputTextCmd]),
> +  // nsIStkInputTextCmd
> +  hideInput: false,
> +  isPacked:false

nit: space after :

::: dom/icc/interfaces/nsIIccMessenger.idl
@@ +6,5 @@
> +
> +[scriptable, uuid(1510cf0c-5db6-11e4-9869-6bf419e26642)]
> +interface nsIStkDuration : nsISupports
> +{
> +  /*

nit: /**

@@ +22,5 @@
> +
> +[scriptable, uuid(c7b6e57a-696d-11e4-bcaa-bfe8386e75a9)]
> +interface nsIStkIcon : nsISupports
> +{
> +  /*

Ditto

@@ +29,5 @@
> +  const unsigned short CODING_SCHEME_BASIC              = 0x11;
> +  const unsigned short CODING_SCHEME_COLOR              = 0x21;
> +  const unsigned short CODING_SCHEME_COLOR_TRANSPARENCY = 0x22;
> +
> +  /*

Ditto

@@ +34,5 @@
> +   * Width of the icon.
> +   */
> +  readonly attribute unsigned long width;
> +
> +  /*

Ditto

@@ +86,5 @@
> +   *
> +   * @returns a copy of the list of icons.
> +   */
> +  void getIcons([optional] out unsigned long aCount,
> +                 [array, size_is(aCount), retval] out nsIStkIcon aIcons);

nit: indention
Attachment #8521185 - Flags: review?(echen)
Created attachment 8524467 [details] [diff] [review]
Part 5 v3: Add IccMessenger as a Wrapper for STK-related System Messages.

Update this patch to:
1. Replace unnecessary "arrow function" with the traditional function declaration.
2. Use XXX.prototype = Object.create() for inheritance instead of the assignment to __prototype__.
3. Refine the API documentation of nsIIccMessenger.idl.
4. Specify valid commandQualifier values in the test cases.
5. refine the try/catch flow in test case.
Attachment #8521185 - Attachment is obsolete: true
Attachment #8524467 - Flags: review?(htsai)
Attachment #8524467 - Flags: review?(echen)
(Reporter)

Comment 47

4 years ago
Comment on attachment 8524467 [details] [diff] [review]
Part 5 v3: Add IccMessenger as a Wrapper for STK-related System Messages.

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

Thank you, Bevis :)

::: dom/icc/interfaces/nsIIccMessenger.idl
@@ +156,5 @@
> +
> +/**
> + * The base class of all STK Proactive Commands.
> + *
> + * This interface is to be applied by the commands that provide info no more then:

nit: s/then/than/
Attachment #8524467 - Flags: review?(htsai) → review+
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2+
Whiteboard: [priority1]

Comment 48

4 years ago
Comment on attachment 8524467 [details] [diff] [review]
Part 5 v3: Add IccMessenger as a Wrapper for STK-related System Messages.

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

Thank you.

::: dom/icc/gonk/StkProactiveCmdFactory.jsm
@@ +21,5 @@
> +function mapDurationToStkDuration(aDuration) {
> +  return (aDuration)
> +    ? new StkDuration(aDuration.timeUnit, aDuration.timeInterval)
> +    : null;
> +}

nit: Add one blank line here.
Attachment #8524467 - Flags: review?(echen) → review+
Comment on attachment 8524467 [details] [diff] [review]
Part 5 v3: Add IccMessenger as a Wrapper for STK-related System Messages.

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

::: dom/icc/gonk/StkProactiveCmdFactory.jsm
@@ +21,5 @@
> +function mapDurationToStkDuration(aDuration) {
> +  return (aDuration)
> +    ? new StkDuration(aDuration.timeUnit, aDuration.timeInterval)
> +    : null;
> +}

will do.

::: dom/icc/interfaces/nsIIccMessenger.idl
@@ +156,5 @@
> +
> +/**
> + * The base class of all STK Proactive Commands.
> + *
> + * This interface is to be applied by the commands that provide info no more then:

will do.
Created attachment 8527463 [details] [diff] [review]
Part 1: Add TelephonyMessenger as a Wrapper for TelephonyService-Related System Messages. r=echen

update final patch.
Attachment #8522717 - Attachment is obsolete: true
Attachment #8527463 - Flags: review+
Created attachment 8527465 [details] [diff] [review]
Part 2: Add SmsMessenger as a Wrapper for Sms-Related System Messages. r=echen

update final patch.
Attachment #8527465 - Flags: review+
Created attachment 8527466 [details] [diff] [review]
Part 3: Add CellBroadcastMessenger as a Wrapper for CellBroadcast System Messages. r=echen

update final patch.
Attachment #8522719 - Attachment is obsolete: true
Attachment #8522720 - Attachment is obsolete: true
Attachment #8527466 - Flags: review+
Created attachment 8527467 [details] [diff] [review]
Part 4: Add MobileConnectionMessenger as a Wrapper for USSD/CdmaInfo System Messages. r=echen

update final patch.
Attachment #8522721 - Attachment is obsolete: true
Attachment #8527467 - Flags: review+
Created attachment 8527468 [details] [diff] [review]
Part 5: Add IccMessenger as a Wrapper for STK-related System Messages. r=htsai,echen

update final patch and address the nits in comment 47, comment 48.
Attachment #8524467 - Attachment is obsolete: true
Attachment #8527468 - Flags: review+
try server result is green:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b7aada9a596

Set |checkin-needed|:
Please ensure that the dependency: bug 1091454 is landed before landing these patches.
Keywords: checkin-needed
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #55)
> try server result is green:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b7aada9a596
> 
> Set |checkin-needed|:
> Please ensure that the dependency: bug 1091454 is landed before landing
> these patches.

sorry missed this and repushed in https://treeherder.mozilla.org/ui/#/jobs?repo=b2g-inbound&revision=a61cacb954c5

Updated

4 years ago
Blocks: 791161

Updated

4 years ago
Blocks: 1123624
(Assignee)

Updated

3 years ago
Blocks: 1126198
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.