744714 - (b2g-ril-stk) B2G RIL: support for SIM card toolkit

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Andreas Gal :gal]

Stk (or its more modern name Cat) is a way for SIM cards to interface with the application processor and have menus and text show up on the screen. Android contains a Java implementation for this, and the standard is pretty well described. libril passes Stk requests through.

http://www.bladox.cz/devel-docs/gen_stk.html

Comment 1

[Andreas Gal :gal]

clee, please add to V2 tracking list (we might have to upgrade to V1)

Comment 2

[Philipp von Weitershausen [:philikon]]

Is this an M3 feature? It seems like we're going to need to add a DOM API for this...

Comment 3

[Andreas Gal :gal]

I think this is Gecko internal, at least for now.

Comment 4

[Chris Lee [:clee]]

Do we want this for M3?  Or for post-B2G/Gaia v1?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Targeting post-M3.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: [WIP] Stk.patch

Current WIP, include parsing BerTLV, ComprehensionTlv, and some Proactive commands, and implement STK_SEND_TERMINAL_RESPONSE

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 618159 [details] [diff] [review]
[WIP] Stk.patch

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

Nice! I took the liberty of doing a drive-by review real quick:

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +46,5 @@
>  
>  var RIL = {};
>  Cu.import("resource://gre/modules/ril_consts.js", RIL);
>  
> +const DEBUG = true; // set to true to see debug messages

...

::: dom/system/gonk/ril_worker.js
@@ +66,5 @@
>  
>  // We leave this as 'undefined' instead of setting it to 'false'. That
>  // way an outer scope can define it to 'true' (e.g. for testing purposes)
>  // without us overriding that here.
> +let DEBUG = true;

...

@@ +1433,5 @@
> +   * Send REQUEST_STK_SEND_TERMINCAL_RESPONSE to ICC.
> +   * See TS 11.14, clause 6.8
> +   * 
> +   * @param options
> +   *        Parameters wil be sent to ICC.

Grammar/typo: s/wil/that will/

However, I think it would be good to document the individual attributes on `options`, too. We've been sloppily using the @param clause for that (cf. some of the other methods on the RIL object.)

@@ +2111,5 @@
> +    if (DEBUG) {
> +      if (delimiter != 0) {
> +        debug("Something's wrong, found string delimiter: " + delimiter);
> +      }
> +    }

At some point we should make a helper for this. We have so many instances of this exact (or similar) code scattered all over ril_worker.js. But feel free to do this in a follow-up.

@@ +4347,5 @@
> +    }
> +
> +    let ctlvs = ComprehensionTlvHelper.decodeChunks(length);
> +    let berTlv = new BerTlv(tag, length,  ctlvs);
> +    return berTlv;

I don't quite see the point for this constructor... What's wrong with a literal?

  return {tag: tag,
          length: length,
          ctlvs: ctlvs};

(Nit: `length` is obviously not `ctlvs.length`, so maybe we should specify *which* length we're talking about here.)

Comment 8

[Andreas Gal :gal]

Nice work. ++yoshi

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> @@ +2111,5 @@
> > +    if (DEBUG) {
> > +      if (delimiter != 0) {
> > +        debug("Something's wrong, found string delimiter: " + delimiter);
> > +      }
> > +    }
> 
> At some point we should make a helper for this. We have so many instances of
> this exact (or similar) code scattered all over ril_worker.js. But feel free
> to do this in a follow-up.

File a Bug 754777 to address this.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> (Nit: `length` is obviously not `ctlvs.length`, so maybe we should specify
> *which* length we're talking about here.)
Hi,philikon

The data format here is of TLV(Tag-Length-Value)[1] format,
or you can see TS 11.14 Annex D for the data structure.
'length' here means the size of the value field in bytes.

You said "cltvs.length", I presume you mean "The length of the array of comprehension TLVs", right ? 
In that case, Yes there might be a little confusion between berTlv.length and berTlv.ctlvs.length. 

But my idea is to use the original naming convention in [1], also I found Android uses the same naming convention.

I think I should revise it to 
let berTlv = {
  tag: tag,
  length: length,
  value: ctlvs
};

to hide the detail that the value part is actually an array of comprehension tlvs, 
how do you think about this??

I'll address other comments you mentioned as well.

Great thanks to your review and suggestions. 




[1] http://en.wikipedia.org/wiki/Type-length-value

Comment 11

[Dietrich Ayala (:dietrich)]

The use-case for why this blocks shipping is not clear. Marking not-blocking. If there's a use-case that requires us to have STK support in order to ship V1 of the phone, then please re-nominate.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1 : IDL

I wrote up the wiki here: https://wiki.mozilla.org/WebAPI/WebSTK
And now upload patch for review.

thanks

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 637875 [details] [diff] [review]
Part 1 : IDL

As discuss in http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/2c0f7953c4425894/78807e0cc9a11494?lnk=raot#78807e0cc9a11494
, also flag Jonas for super review.

Hi Jonas,
You can see https://wiki.mozilla.org/WebAPI/WebSTK for more detail of STK.

thanks

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 637875 [details] [diff] [review]
Part 1 : IDL

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

::: dom/icc/interfaces/nsIDOMSimToolKit.idl
@@ +314,5 @@
> +  /**
> +   * Used when type in nsIDOMMozStkCommand is
> +   * LAUNCH_BROWSER.
> +   */
> +  readonly attribute nsIDOMMozStkBrowserSetting setting;

should I use 'browserSetting'?

@@ +320,5 @@
> +  /**
> +   * Used when type in nsIDOMMozStkCommand is
> +   * SET_UP_CALL.
> +   */
> +  readonly attribute nsIDOMMozStkSetUpCall call;

should I use 'setUpCall'?

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 637875 [details] [diff] [review]
Part 1 : IDL

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

::: dom/icc/interfaces/nsIDOMSimToolKit.idl
@@ +273,5 @@
> +  const unsigned short CMD_GET_INPUT             = 0x23;
> +  const unsigned short CMD_SELECT_ITEM           = 0x24;
> +  const unsigned short CMD_SET_UP_MENU           = 0x25;
> +  const unsigned short CMD_SET_UP_IDLE_MODE_TEXT = 0x28;
> +

maybe we can add a method here
nsIDOMMozStkResponse obtainResponse();

so we don't have to add another object into navigator.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 637875 [details] [diff] [review]
Part 1 : IDL

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

In this IDL, using 'type' will be ambiguous and too abstract,
especially when it comes to implementation, in RIL message it uses 'type' extensively.
If I am going to change 'type', the other two members needs to be changed as well,
(Command Number, Type of Command, and Command Qualifier are originally extracted from 'Command Details' Comprehension-TLV data object together)

So how do you think if I change the naming to a longer name?

::: dom/icc/interfaces/nsIDOMSimToolKit.idl
@@ +436,5 @@
> +   * number from MozStkCommand.
> +   *
> +   * @see nsIDOMMozStkCommand.number
> +   */
> +  readonly attribute unsigned short number;

from the spec, the original naming is "Command Number",
maybe we should rename this to cmdNumber or commandNumber

@@ +443,5 @@
> +   * One of nsIDOMMozStkCommand.type
> +   *
> +   * @see nsIDOMMozStkCommand.type
> +   */
> +  readonly attribute unsigned short type;

from the spec, the original naming is "Type Of Command",
maybe we should rename this to typeOfCmd or typeOfCommand

@@ +450,5 @@
> +   * Qualifiers specific to the command.
> +   *
> +   * @see nsIDOMMozStkCommand.qualifier
> +   */
> +  readonly attribute unsigned short qualifier;

from the spec, its original term is "Command Qualifier",
maybe we should rename this to cmdQualifier or commandQualifier

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #14)
> ::: dom/icc/interfaces/nsIDOMSimToolKit.idl
> @@ +314,5 @@
> > +  /**
> > +   * Used when type in nsIDOMMozStkCommand is
> > +   * LAUNCH_BROWSER.
> > +   */
> > +  readonly attribute nsIDOMMozStkBrowserSetting setting;
> 
> should I use 'browserSetting'?

How about "browser options"?

> @@ +320,5 @@
> > +  /**
> > +   * Used when type in nsIDOMMozStkCommand is
> > +   * SET_UP_CALL.
> > +   */
> > +  readonly attribute nsIDOMMozStkSetUpCall call;
> 
> should I use 'setUpCall'?

How about "call options"?

Regarding nsIDOMMozStkTextMessage, nsIDOMMozStkMenu, nsIDOMMozStkInput, nsIDOMMozStkBrowserSetting, nsIDOMMozStkSetUpCall: I talked to Jonas and we came to the conclusion that we can simplify this even more. nsIDOMMozStkCommand already exposes the `type` property, so the consumer will know what kind of command to expect. We can then just have a single `options` attribute that's an immutable JSON object (we can use Object.freeze() for that). We can still document the different fields that `options` will have. Instead of doing

  [scriptable, uuid(871817a6-3ecb-4c3c-83b6-9352c91f4c60)]
  interface nsIDOMMozStkTextMessage : nsISupports

just do

  dictionary MozStkTextMessage

And then in `nsIDOMMozStkCommand` we can just have

  readonly attribute jsval options;


We might even take this approach for MozStkResponse/sendStkResponse(). Instead of doing

  var response = new MozStkResponse(command.number, command.type, command.qualifier);
  response.resultCode = MozStkResponse.RESULT_OK;
  response.input = input;
  mozIcc.sendStkResponse(response);

we could just do

  mozIcc.sendStkResponse({
    number: command.number,
    type: command.type,
    qualifier: command.qualifier,
    resultCode: mozIcc.STK_RESPONSE_RESULT_OK,
    input: input,
  });

We could do this by defining `dictionary MozStkResponse` and then in nsIDOMMozIccManager:

  void sendStkResponse(in MozStkResponse options);


The only tricky bit with the dictionaries is where to put the constants. In the example above, I simply stuffed them onto nsIDOMMozIccManager (with the right prefixing, so e.g. STK_RESPONSE_*, STK_MENU_TYPE_*, STK_BROWSER_MODE_*, etc.)

Yoshi, Jonas, what do you guys think?

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 637875 [details] [diff] [review]
Part 1 : IDL

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

Very well documented and clean. I would be fine with committing this as is, but please see comment 17 for some suggested simplifications.

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #17)
> Yoshi, Jonas, what do you guys think?

It's a great suggestion, I'll try to update the IDL by your suggestion.
thanks

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #17)
> 
> We might even take this approach for MozStkResponse/sendStkResponse().
> Instead of doing
> 
>   var response = new MozStkResponse(command.number, command.type,
> command.qualifier);
>   response.resultCode = MozStkResponse.RESULT_OK;
>   response.input = input;
>   mozIcc.sendStkResponse(response);
> 
> we could just do
> 
>   mozIcc.sendStkResponse({
>     number: command.number,
>     type: command.type,
>     qualifier: command.qualifier,
>     resultCode: mozIcc.STK_RESPONSE_RESULT_OK,
>     input: input,
>   });
> 
> We could do this by defining `dictionary MozStkResponse` and then in
> nsIDOMMozIccManager:
> 
>   void sendStkResponse(in MozStkResponse options);
>
                            ^^^^^^^^^^^^^^
Do you mean 'jsval' here?
 
> 
> The only tricky bit with the dictionaries is where to put the constants. In
> the example above, I simply stuffed them onto nsIDOMMozIccManager (with the
> right prefixing, so e.g. STK_RESPONSE_*, STK_MENU_TYPE_*,
> STK_BROWSER_MODE_*, etc.)
> 
> Yoshi, Jonas, what do you guys think?

Also should we make MozStkCommand and MozStkResponse more consistent ?

I mean, we still make MozStkResponse of type nsIDOMMozStkResponse,
instead of changing it  to 'dictionary'. But we move some attributes into 'options', 
as we did in nsIDOMMozStkCommand


[scriptable, uuid(8d89b0f2-8026-4568-b8c9-f52047dd38fb)]
interface nsIDOMMozStkResponse : nsISupports
{
  const unsigned short RESULT_.....;

  readonly attribute unsigned short number;
                                    type;
                                    qualifier; 

  readonly attribute jsval options; 
};

In that case, 

the signature of sendStkResponses remains the same
void sendStkResponse(in nsIDOMMozStkResponse response);

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #20)
> > We could do this by defining `dictionary MozStkResponse` and then in
> > nsIDOMMozIccManager:
> > 
> >   void sendStkResponse(in MozStkResponse options);
> >
>                             ^^^^^^^^^^^^^^
> Do you mean 'jsval' here?

Yeah, you're probably right, XPConnect might not do the conversion automagically, so you want to take jsval (don't forget [implicit_jscontext] so you also get a JSContext* parameter), and then you build the MozStkResponse object yourself in C++. Actually, though, you're probably just going to pass the object on to JavaScript again, so just doing jsval and passing it to RILContentHelper might be enough.

> Also should we make MozStkCommand and MozStkResponse more consistent ?
> 
> I mean, we still make MozStkResponse of type nsIDOMMozStkResponse,
> instead of changing it  to 'dictionary'. But we move some attributes into
> 'options', as we did in nsIDOMMozStkCommand
> 
> 
> [scriptable, uuid(8d89b0f2-8026-4568-b8c9-f52047dd38fb)]
> interface nsIDOMMozStkResponse : nsISupports
> {
>   const unsigned short RESULT_.....;
> 
>   readonly attribute unsigned short number;
>                                     type;
>                                     qualifier; 
> 
>   readonly attribute jsval options; 
> };
> 
> In that case, 
> 
> the signature of sendStkResponses remains the same
> void sendStkResponse(in nsIDOMMozStkResponse response);

Yeah but then you would still have to create a response object from options. Not sure that's a big win. If the goal is symmetry, I'd rather simplify nsIDOMMozStkCommand further :)

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #21)
> If the goal is symmetry, I'd rather simplify
> nsIDOMMozStkCommand further :)
Hi Philikon

So how do you think if we make MozStkCommand/Response both jsval?

dictionary nsIDOMMozStkCommand
{
    unsigned short number;
    unsigned short type;
    unsigned short qualifier;

    jsval options;
};

dictionary nsIDOMMozStkResponse
{
    unsigned short number;
    unsigned short type;
    unsigned short qualifier;

    jsval options;
};

Then the sendStkResponse will become

void sendStkResponse(in jsval response);

and event is 

[scriptable, builtinclass, uuid(06bbc6fa-9b59-4db6-b66b-3b26f9c379df)]
interface nsIDOMMozStkCommandEvent : nsIDOMEvent
{
  readonly attribute jsval command;
};


Or if symmetry is not our biggest concern here, 
do you think it's better we still keep MozStkCommand in C++,
whereas MozStkResponse is in JS, said 'dictionary' ?

Thanks

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: IDL v2

Hi, Philikon
I revise the IDL by your suggestions, also I update the nsIDOMMozStkCommand to dictionary as well.
What do you think about this?

Thanks

Comment 24

[Philipp von Weitershausen [:philikon]]

Comment on attachment 641320 [details] [diff] [review]
Part 1: IDL v2

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

Thanks, Yoshi! This looks good to me apart from a few minor points below. Also, I'm not sure about the naming convention for dictionaries. I don't think we usually prefix them with `nsIDOM`, but I'll let Jonas answer that conclusively in his superreview.

::: dom/icc/interfaces/nsIDOMSimToolKit.idl
@@ +250,5 @@
> +   * by ICC may take any hexadecimal value betweean '01' and 'FE'.
> +   *
> +   * @see TS 11.14, clause 6.5.1
> +   */
> +  unsigned short commandNumber;

Why not just `number`? We're inside a command object. Namespaces FTW!

@@ +255,5 @@
> +
> +  /**
> +   * One of STK_CMD_*
> +   */
> +  unsigned short typeOfCommand;

Why not just `type`?

@@ +260,5 @@
> +
> +  /**
> +   * Qualifiers specific to the command.
> +   */
> +  unsigned short commandQualifier;

Why not just `qualifier`?

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #24)
> Comment on attachment 641320 [details] [diff] [review]
> Part 1: IDL v2
> 
> Review of attachment 641320 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, Yoshi! This looks good to me apart from a few minor points below.
> Also, I'm not sure about the naming convention for dictionaries. I don't
> think we usually prefix them with `nsIDOM`, but I'll let Jonas answer that
> conclusively in his superreview.
> 
> ::: dom/icc/interfaces/nsIDOMSimToolKit.idl
> @@ +250,5 @@
> > +   * by ICC may take any hexadecimal value betweean '01' and 'FE'.
> > +   *
> > +   * @see TS 11.14, clause 6.5.1
> > +   */
> > +  unsigned short commandNumber;
> 
> Why not just `number`? We're inside a command object. Namespaces FTW!
> 
> @@ +255,5 @@
> > +
> > +  /**
> > +   * One of STK_CMD_*
> > +   */
> > +  unsigned short typeOfCommand;
> 
> Why not just `type`?
> 
> @@ +260,5 @@
> > +
> > +  /**
> > +   * Qualifiers specific to the command.
> > +   */
> > +  unsigned short commandQualifier;
> 
> Why not just `qualifier`?

Hi, philikon

I have changed the naming according to comment 16,
Or if you think we could use the 'type' property here and shorter name is better,
I'll change it back to original one.

Thanks

Comment 26

[Philipp von Weitershausen [:philikon]]

Ah I see. After reading comment 16 this makes sense.

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: IccManager

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP - Part 3: RIL implementations.

Hi, philikon:
I am still working on this patch, so there are some dummy debug message I haven't removed yet, and the debug is still on, but I'd like to ask if you can kindly give me some feedback in this patch. Thanks~~

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: DOMEvent

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: Hook up to Navigator

Comment 31

[Philipp von Weitershausen [:philikon]]

Comment on attachment 642377 [details] [diff] [review]
Part 2: IccManager

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

I'm not a DOM peer, bent is a better reviewer for this.

Comment 32

[Philipp von Weitershausen [:philikon]]

Comment on attachment 642380 [details] [diff] [review]
Part 5: Hook up to Navigator

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

Same as with Part 2... over the wall to bent!

Comment 33

[Philipp von Weitershausen [:philikon]]

Comment on attachment 642379 [details] [diff] [review]
Part 4: DOMEvent

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

One more... sorry, bent :)

Comment 34

[Philipp von Weitershausen [:philikon]]

Comment on attachment 642378 [details] [diff] [review]
WIP - Part 3: RIL implementations.

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

I'm going to have to read this carefully. For now I've taken a quick look and found a few questions.

(Off-topic: ril_worker.js is getting large... might be time to split it into several files at some point.)

::: dom/system/gonk/RILContentHelper.js
@@ +552,5 @@
> +      //TODO:
> +      case "RIL:StkCommand":
> +        debug("XXX RIL:StkCommand cmd="+JSON.stringify(msg.json));
> +        let jsonString = JSON.stringify(msg.json);
> +        Services.obs.notifyObservers(null, kStkCommandTopic, jsonString);

Can you explain what you're trying to do here?

::: dom/system/gonk/ril_worker.js
@@ +1854,5 @@
> +    // Command Details
> +    let cmdDetails = {
> +      commandNumber: response.commandNumber,
> +      typeOfCommand: response.typeOfCommand,
> +      commandQualifier: response.commandQualifier

Is there a good reason to create this partial copy of `response`? Could you not just use `response` below?

Same question applies to several other occurrences below.

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642378 [details] [diff] [review]
WIP - Part 3: RIL implementations.

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

::: dom/system/gonk/RILContentHelper.js
@@ +552,5 @@
> +      //TODO:
> +      case "RIL:StkCommand":
> +        debug("XXX RIL:StkCommand cmd="+JSON.stringify(msg.json));
> +        let jsonString = JSON.stringify(msg.json);
> +        Services.obs.notifyObservers(null, kStkCommandTopic, jsonString);

Sure,
in Part 4, DOMEvent, IccManager will get this event,
and IccManager will create a StkCommandEvent by this json string.

::: dom/system/gonk/ril_worker.js
@@ +1854,5 @@
> +    // Command Details
> +    let cmdDetails = {
> +      commandNumber: response.commandNumber,
> +      typeOfCommand: response.typeOfCommand,
> +      commandQualifier: response.commandQualifier

I create two functions for sending Terminal Response:
1. sendStkTerminalResponse
   It's a more high level API, and its parameters are those defined in IDL.(Part 1). In this function, it will convert those arguments into Comprehension-TLVs, which is needed below.

2. sendICCTerminalResponse
   It's a lower level function, and its parameters are exactly the same with specification (TS 11.14, clause 6.8), and the parameters are of Comprehension-TLV format. In this function, it will convert those TLVs into Hex-format String and use the REQUEST_STK_SEND_TERMINAL_RESPONSE.

So back to your question, I create those objects just to have a better ADT, otherwise the lower function will get overwhelmed by the code that conversion of Comprehension-TLV.

Does that make sense to you? Or should I merge the two functions together ?

Thanks for your comments~~

Comment 36

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642378 [details] [diff] [review]
WIP - Part 3: RIL implementations.

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

::: dom/system/gonk/ril_worker.js
@@ +1845,5 @@
> +   * @param typeOfCommand
> +   * @param commandQualifier
> +   * @param resultCode
> +   * @param [optional] itemIdentifier
> +   * @param [optional] input

will add another parameter 'hasConfirmed' in my next patch

@@ +1906,5 @@
> +        value = {
> +          codingScheme: (response.isUCS2 ? STK_TEXT_CODING_UCS2 :
> +                           (response.isPacked ? STK_TEXT_CODING_GSM_7BIT_PACKED : 
> +                              STK_TEXT_CODING_GSM_8BIT)),
> +          text: text

should convert text to raw bytes here.

@@ +1964,5 @@
> +      GsmPDUHelper.writeHexOctet(textString.tag);
> +      GsmPDUHelper.writeHexOctet(textString.length);
> +      GsmPDUHelper.writeHexOctet(textString.value.codingScheme);
> +      for (let i = 0; i < textString.value.text.length; i++) {
> +        GsmPDUHelper.writeHexOctet(textString.value.text[i].charCodeAt());

ditto, will remove charCodeAt() here.

@@ +3673,5 @@
> +  debug("XXX handler for REQUEST_STK_SEND_TERMINAL_RESPONSE length = "+length+ "error="+options.rilRequestError);
> +};
> +RIL[REQUEST_STK_HANDLE_CALL_SETUP_REQUESTED_FROM_SIM] = function REQUEST_STK_HANDLE_CALL_SETUP_REQUESTED_FROM_SIM(length, options) {
> +  debug("XXX handle for REQUEST_STK_HANDLE_CALL_SETUP_REQUESTED_FROM_SIM ");
> +};

debug message above and below are for my own debug purpose, will remove them in my final patch.

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642378 [details] [diff] [review]
WIP - Part 3: RIL implementations.

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

::: dom/system/gonk/ril_worker.js
@@ +1854,5 @@
> +    // Command Details
> +    let cmdDetails = {
> +      commandNumber: response.commandNumber,
> +      typeOfCommand: response.typeOfCommand,
> +      commandQualifier: response.commandQualifier

Never mind now,
I'll try to merge these two functions and also try not to create extra objects.
I'll update this in my next patch.

Thanks

Comment 38

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Philipp von Weitershausen [:philikon] from comment #33)
> One more... sorry, bent :)

I'm off until next week, might want to find someone else if this is time-sensitive.

Comment 39

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL Implementations

I've finished the RIL implementation part,
but since I am still waiting for Jonas's superreview,
so r? will be sent after sr+

thanks

Comment 40

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL Implementations

sorry, forgetting qref

Comment 41

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, Jonas:
I've updated the wiki for WebSTK,
but since WebSTK is P1 for now,
Could you kindly help to super-review the IDL and give me some feedback?

thanks

Comment 42

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642377 [details] [diff] [review]
Part 2: IccManager

Hi, Mounir
Since bent is off this week,
can you kindly help to review my patch??

thanks~

Comment 43

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642379 [details] [diff] [review]
Part 4: DOMEvent

Hi Mounir
2nd Patch,
This patch is about DOMEvent

Thanks

Comment 44

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642380 [details] [diff] [review]
Part 5: Hook up to Navigator

Hi, Mounir
Last one, 
hook up to navigator.

Thanks

Comment 45

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL implementations

Comment 46

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642377 [details] [diff] [review]
Part 2: IccManager

Hi, smaug
Bent is off this week, can you kindly help to review this patch?

Thanks

Comment 47

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642379 [details] [diff] [review]
Part 4: DOMEvent

Another patch for review, for DOMEvent

Comment 48

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 642380 [details] [diff] [review]
Part 5: Hook up to Navigator

Last one, hook up to Navigator.

Comment 49

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 643716 [details] [diff] [review]
Part 3: RIL implementations

Hi, philikon
   Can you help to review my patch? 
   Not sure about the status of Jonas, so I'd like to start to request for review now.

Thanks

Comment 50

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to ben turner [:bent] from comment #38)
> (In reply to Philipp von Weitershausen [:philikon] from comment #33)
> > One more... sorry, bent :)
> 
> I'm off until next week, might want to find someone else if this is
> time-sensitive.
Thanks, bent.

Now I sent requests to smuag to review these DOM implementations, 
originally I sent request to mounir, but found he's busy on reviewing Web activity now, so I forward those reviews to smaug.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642379 [details] [diff] [review]
Part 4: DOMEvent


>+  DOM_CLASSINFO_MAP_BEGIN(MozStkCommandEvent, nsIDOMMozStkCommandEvent)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozStkCommandEvent)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMEvent)
>+  DOM_CLASSINFO_MAP_END

s/DOM_CLASSINFO_MAP_ENTRY(nsIDOMEvent)/DOM_CLASSINFO_EVENT_MAP_ENTRIES/

(Looks like some other event implementations have the same problem. I'll file a bug to fix those.)

Comment 52

[Philipp von Weitershausen [:philikon]]

Comment on attachment 643716 [details] [diff] [review]
Part 3: RIL implementations

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

Nice!

::: dom/system/gonk/ril_worker.js
@@ +4983,5 @@
>      Buf.writeUint16(0);
> +  },
> +};
> +
> +let CommandParamsFactory = {

Nit: StkCommandParamsFactory

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642377 [details] [diff] [review]
Part 2: IccManager

>   NS_DEFINE_CLASSINFO_DATA(MozMobileConnection, nsDOMGenericSH,
>                            DOM_DEFAULT_SCRIPTABLE_FLAGS)
> 
>   NS_DEFINE_CLASSINFO_DATA(USSDReceivedEvent, nsDOMGenericSH,
>                            DOM_DEFAULT_SCRIPTABLE_FLAGS)
> 
>+  NS_DEFINE_CLASSINFO_DATA(MozIccManager, nsDOMGenericSH,
>+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
>+
Shouldn't this and rest of the code in the patches be 
inside some ifdef MOZ_B2G
>+IccManager::IccManager()
>+{
>+
>+}
Extra newline after {


>+
>+void
>+IccManager::Init(nsPIDOMWindow* aWindow)
>+{
>+  BindToOwner(aWindow);
>+}
>+
>+void
>+IccManager::Shutdown()
>+{
>+
>+}
Extra newline after {



With the #ifdef problem fixed, r=me

Comment 54

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #53)
> Comment on attachment 642377 [details] [diff] [review]
> Part 2: IccManager
> 
> >   NS_DEFINE_CLASSINFO_DATA(MozMobileConnection, nsDOMGenericSH,
> >                            DOM_DEFAULT_SCRIPTABLE_FLAGS)
> > 
> >   NS_DEFINE_CLASSINFO_DATA(USSDReceivedEvent, nsDOMGenericSH,
> >                            DOM_DEFAULT_SCRIPTABLE_FLAGS)
> > 
> >+  NS_DEFINE_CLASSINFO_DATA(MozIccManager, nsDOMGenericSH,
> >+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)
> >+
> Shouldn't this and rest of the code in the patches be 
> inside some ifdef MOZ_B2G
...
> With the #ifdef problem fixed, r=me

Hi smauh:
Thanks for your review, but you mean MOZ_B2G_RIL right?

Hi philikon:
Do I also need to add MOZ_B2G_RIL for WebMobileConnection as well?
Since in WebMobileConnection it also deals with ICC stuff.
(Also with others USSDEvent and VoicemailEvent)

Thanks

Comment 55

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #54)
> Do I also need to add MOZ_B2G_RIL for WebMobileConnection as well?
> Since in WebMobileConnection it also deals with ICC stuff.
> (Also with others USSDEvent and VoicemailEvent)

No. WebMobileConnection already handles the absence of B2G/RIL code gracefully. No #ifdef needed.

Comment 56

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #55)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #54)
> > Do I also need to add MOZ_B2G_RIL for WebMobileConnection as well?
> > Since in WebMobileConnection it also deals with ICC stuff.
> > (Also with others USSDEvent and VoicemailEvent)
> 
> No. WebMobileConnection already handles the absence of B2G/RIL code
> gracefully. No #ifdef needed.

Hi philikon,
So I don't have to add #ifdef for IccManager as smaug asked in comment 53, right?

When B2G/RIL is not enabled, the call to IccManager will simply return failure as in my patch Part 3: RIL implementations.


Thanks

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

I don't want to expose this stuff on desktop Firefox. So some #ifdef 
(I don't know which one, just some MOZ_B2G_foobar ).

Comment 58

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #57)
> I don't want to expose this stuff on desktop Firefox. So some #ifdef 
> (I don't know which one, just some MOZ_B2G_foobar ).

Okay, I'll add MOZ_B2G_RIL in my patches (Part 2, Part 4 and Part 5)

Comment 59

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #56)
> So I don't have to add #ifdef for IccManager as smaug asked in comment 53,
> right?

Smaug and I were talking about different things. Smaug would like you to #ifdef IccManager. This makes sense. You don't need any #ifdefs in WebMobileConnection, so please don't add any there.

Comment 60

[Mounir Lamouri (:mounir)]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #58)
> (In reply to Olli Pettay [:smaug] from comment #57)
> > I don't want to expose this stuff on desktop Firefox. So some #ifdef 
> > (I don't know which one, just some MOZ_B2G_foobar ).
> 
> Okay, I'll add MOZ_B2G_RIL in my patches (Part 2, Part 4 and Part 5)

Don't use MOZ_B2G_RIL to hide the interfaces. Just a new #define like MOZ_B2G_STK.

Comment 61

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Mounir Lamouri (:mounir) from comment #60)
> Don't use MOZ_B2G_RIL to hide the interfaces. Just a new #define like
> MOZ_B2G_STK.
Hi mounir, 
Thanks for your suggestion,
but since SMS/MMS, telephony and MobileConnection they all use MOZ_B2G_RIL, 
maybe I should do the same ?

Thanks

Comment 62

[Mounir Lamouri (:mounir)]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #61)
> (In reply to Mounir Lamouri (:mounir) from comment #60)
> > Don't use MOZ_B2G_RIL to hide the interfaces. Just a new #define like
> > MOZ_B2G_STK.
> Hi mounir, 
> Thanks for your suggestion,
> but since SMS/MMS, telephony and MobileConnection they all use MOZ_B2G_RIL, 
> maybe I should do the same ?

Does this code depends on RIL? If yes, indeed. Otherwise, no.

Comment 63

[Mounir Lamouri (:mounir)]

BTW, what are the use cases of STK? It's quite not clear.

Comment 64

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:mounir) from comment #62)
> Does this code depends on RIL? If yes, indeed. Otherwise, no.

Yes.

(In reply to Mounir Lamouri (:mounir) from comment #63)
> BTW, what are the use cases of STK? It's quite not clear.

The SIM STK allows carriers to ship small "applications" (prompts, menus, etc.) on the SIM card. By having a Web API for it we can write the UI support for these in Gaia. If you have more questions, let's take it to the webapi list (where we've been discussing this feature and the API for a while.)

Comment 65

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #64)
> (In reply to Mounir Lamouri (:mounir) from comment #62)
> (In reply to Mounir Lamouri (:mounir) from comment #63)
> > BTW, what are the use cases of STK? It's quite not clear.
> 
> The SIM STK allows carriers to ship small "applications" (prompts, menus,
> etc.) on the SIM card. By having a Web API for it we can write the UI
> support for these in Gaia. If you have more questions, let's take it to the
> webapi list (where we've been discussing this feature and the API for a
> while.)

philikon, thanks for answering this question for me.

Hi mounir, I could take my SIM as an example, in my SIM card, the carrier has put a small application in it, it has several services I can access, for example, dial voice mail, dial to the taxi operator to get a taxi, dial to stock market to get the latest stock information(These all use STK_CMD_SET_UP_CALL), and a dictionary service(STK_CMD_GET_INPUT, I key-in a word, then the operator will send the meaning of the word to me by SMS), etc.

Comment 66

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: IccManager v2

Comment 67

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 645175 [details] [diff] [review]
Part 2: IccManager v2

sorry 
accident hit the keyboard

Comment 68

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: IccManager v2

Hi, smaug
Update:
* rebase 
* add #ifdef MOZ_B2G_RIL
* remove empty lines in IccManager.cpp
* add r=smaug

Thanks

Comment 69

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL implementations v2

Address to philikon's comments
* rebase
* rename CommandParamsFactory to StkCommandParamsFactory

Also I update the comments in ril_worker.js stkHandleCallSetup

This patch is r+ by philikon in comment 52

Thanks for the quick review, philikon.

Comment 70

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: DOMEvent v2

Comment 71

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 645179 [details] [diff] [review]
Part 4: DOMEvent v2

sorry hit the keyboard again..

Comment 72

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: DOMEvent v2

Hi, smaug
Update:
* rebase
* add #ifdef MOZ_B2G_RIL
* s/DOM_CLASSINFO_MAP_ENTRY(nsIDOMEvent)/DOM_CLASSINFO_EVENT_MAP_ENTRIES/
* add r=smaug

Thanks

Comment 73

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: Hook up to navigator. v2

Hi, smaug

Update:
* rebase
* add #ifdef MOZ_B2G_RIL

Thanks

Comment 74

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: Hook up to navigator. v2

forgot adding r=smaug

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 645214 [details] [diff] [review]
Part 5: Hook up to navigator. v2


>+interface nsIDOMMozIccManager;
>+
>+[scriptable, uuid(f888b10b-3aa1-4cc1-8a3c-03a5b43ee31e)]
>+interface nsIDOMMozNavigatorIcc : nsISupports
>+{
>+  readonly attribute nsIDOMMozIccManager mozIcc;
>+};
>+


Icc in .navigator context doesn't say anything to me. We need a better name for the property.

Comment 76

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] (limited connectivity July 26-29) from comment #75)
> Comment on attachment 645214 [details] [diff] [review]
> Part 5: Hook up to navigator. v2
> Icc in .navigator context doesn't say anything to me. We need a better name
> for the property.
Hi, smaug
Thanks for your quick review,
but the (U)ICC term is a more general term for SIM in 2G and USIM in 3G, see [1]

Also in Android framework, many functions/classes related to SIM already renamed to ICC[2], that's why this term 'ICC' is used. 

How do you think?

[1]: http://en.wikipedia.org/wiki/UICC
[2]: http://androidxref.com/4.1.1/xref/frameworks/base/telephony/java/com/android/internal/telephony/

Comment 77

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi
jonas, sr ping?
smaug, can you take a look in comment 76 ?

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

icc sounds still way too abbreviated in navigator context. But I'm ok with it if jonas is.

Comment 79

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #78)
> icc sounds still way too abbreviated in navigator context. But I'm ok with
> it if jonas is.

Thanks for your comment. :)

Comment 80

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 641320 [details] [diff] [review]
Part 1: IDL v2

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

I don't fully understand what's going on here, but it looks mostly fine to me. Generally I think it's nicer to use string constants than numeric constants + LONG_CONSTANT_NAMES, but it's not a big deal.

Why do we need to expose things like encoding to the caller? Can't we detect if incoming data has UCS2 characters or not internally?

sr=me assuming there's a good reason for that.

Comment 81

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) from comment #80)
Hi, jonas 
Thanks for your super-review.
I know you're busy reviewing lotsssssssssss patches,
and hope this won't take too much time for you. :)

> Generally I think it's nicer to use string constants than numeric
> constants + LONG_CONSTANT_NAMES, but it's not a big deal.
> 
okay, I'll check this and do it as a follow-up if necessary.

> Why do we need to expose things like encoding to the caller? Can't we detect
> if incoming data has UCS2 characters or not internally?
> 
> sr=me assuming there's a good reason for that.
The encoding 'UCS2' is requested from ICC(SIM) (see 'isUCS2' in nsIDOMMozStkInput),  not from the user.
In nsIDOMMozStkInput, when 'isUCS2' is true, it means ICC requests the input key-in by user is encoded in UCS2. So even user just enters normal ASCII alphabets, it should still be encoded into UCS2.

But if we didn't expose the field 'isUCS2' in nsIDOMMozStkResponse, then implementation needs a way to remember the encoding scheme in the corresponding stk command. This complicates the design and implementation because then STK needs to implement a queue or cache to remember those stk commands sent to ME and match the correct stk command when it receives response from user.

So there are some fields in nsIDOMMozStkResponse are added due to this reason, like commandNumber, commandQualifier, isUCS2, isYesNo and isPacked.

How do you think?

Also philikon has raised his concern about nsIDOM prefix for those dictionaries in comment 24, can you also comment on this ?

Appreciate for your comment and thank you very much.

Comment 82

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 645214 [details] [diff] [review]
Part 5: Hook up to navigator. v2

Hi, jonas.

smaug has some concerns about the naming 'mozIcc' in navigator in comment 75, and I replied why the term is used in comment 76. And he would also like to hear your opinion about this in comment 78.

In Gecko, functions related to SIM are already renamed to ICC, and Android also does the same thing.
And I have checked the spec, for example TS 101.220, TS 102.221, TS 31.101.
They all use UICC, and U means Universal. 

Can you give me some feedback about this, or can you suggest the better naming for me?

Thank you

Comment 83

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

hi jonas, 
feeback ping in comment 82

Comment 84

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 645214 [details] [diff] [review]
Part 5: Hook up to navigator. v2

Just discussed this with sicking.
He suggested we can use navigator.mozTelephony.icc.

Thanks, sicking.

Comment 85

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sorry about the slow reply here :(

> > Generally I think it's nicer to use string constants than numeric
> > constants + LONG_CONSTANT_NAMES, but it's not a big deal.
> > 
> okay, I'll check this and do it as a follow-up if necessary.

Sounds good. I definitely don't think we should worry about it for v1.

> > Why do we need to expose things like encoding to the caller? Can't we detect
> > if incoming data has UCS2 characters or not internally?
> > 
> > sr=me assuming there's a good reason for that.
> The encoding 'UCS2' is requested from ICC(SIM) (see 'isUCS2' in
> nsIDOMMozStkInput),  not from the user.
> In nsIDOMMozStkInput, when 'isUCS2' is true, it means ICC requests the input
> key-in by user is encoded in UCS2. So even user just enters normal ASCII
> alphabets, it should still be encoded into UCS2.
> 
> But if we didn't expose the field 'isUCS2' in nsIDOMMozStkResponse, then
> implementation needs a way to remember the encoding scheme in the
> corresponding stk command. This complicates the design and implementation
> because then STK needs to implement a queue or cache to remember those stk
> commands sent to ME and match the correct stk command when it receives
> response from user.

I see. I suspect this is the better design once we decide to push for standardization of this API. But seems ok to leave as-is for now.

> Also philikon has raised his concern about nsIDOM prefix for those
> dictionaries in comment 24, can you also comment on this ?

Good catch! Yeah, you can remove the nsIDOM prefix from these dictionaries.

Comment 86

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #84)
> Just discussed this with sicking.
> He suggested we can use navigator.mozTelephony.icc.

I'd rather use navigator.mozMobileConnection.icc. mozTelephony and mozSms have a set of clearly defined functionality. mozMobileConnection is a good bag to throw anything else that doesn't fit into.

Comment 87

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, I forgot about that one. I agree, that sounds even better.

Comment 88

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) from comment #87)
> Oh, I forgot about that one. I agree, that sounds even better.

Thanks, then I'll move it into mobileconnection.icc.

Comment 89

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #86)
> I'd rather use navigator.mozMobileConnection.icc.
Hi, philikon

If icc should be reside in mozMobileconnection, so in my patches Part1 ~ Part4, should I also have to move those files from dom/icc to dom/network? Or move them to dom/network/icc ?

What do you think?

Thanks

Comment 90

[Philipp von Weitershausen [:philikon]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #89)
> If icc should be reside in mozMobileconnection, so in my patches Part1 ~
> Part4, should I also have to move those files from dom/icc to dom/network?
> Or move them to dom/network/icc ?

I'm not a module owner or peer for the DOM, but adding it to the dom/network directory structure sounds good to me.

Comment 91

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #90)
> I'm not a module owner or peer for the DOM, but adding it to the dom/network
> directory structure sounds good to me.

Thanks, I'll proceed to move them into dom/network.
My first idea was to put icc under navigator, so I put them in dom/icc. 
Now since we want to move it into mobileconnection, I think it's better to move it there, too.

Thank you for your comments.

Comment 92

[Mounir Lamouri (:mounir)]

I would prefer that to stay out of dom/network/. AFAIUI, STK isn't directly related to networking.

Comment 93

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:mounir) from comment #92)
> I would prefer that to stay out of dom/network/. AFAIUI, STK isn't directly
> related to networking.

Can you make a constructive suggestion? We can leave it in dom/icc, but IIRC smaug had some objections about "icc" not being a clear name. Or maybe that was just for navigator.mozIcc.

Comment 94

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

are we spending too much time on this bug? Please not that it is *not* a blocker.

Comment 95

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) from comment #94)
> are we spending too much time on this bug? Please not that it is *not* a
> blocker.
Hi Jonas,
I was told STK is v1 in Brazil.
Most patches are already r+, so the final patch is where the icc object should be.

And since you and philikon agreed to move icc into mobileconnection in comment 86 and comment 87, so I am working on this right now.

I am not sure about how this decision is made(by you or need to be agreed by another DOM peer). But I think now I also need to send a feedback flag to smaug since he's the one who reviewed that patch originally, and also mounir has raised some questions about this.

Thanks.

Comment 96

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 645214 [details] [diff] [review]
Part 5: Hook up to navigator. v2

Hi, smaug

You gave a 'r-' in this patch for the name 'icc' under navigator originally.
I have also ask sicking about this,

But now I'd like to request for your feedback about this.

In comment 86 and comment 87, philikon and sicking suggest I can move it into mobileconnection,(but still use the name 'icc')

And in comment 92 mounir has different opinion (although he didn't give a suggestion then)

How do you think about this? 
In your original review, do you think I should use another name, but still put it under navigator? If so, can you provide a suggestion?

Or do you think I should move it to mobileconnection or telephony... etc, and in that case, should I still use the name 'icc'?

Appreciate for your comments. :)
Thanks

Comment 97

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Only things marked as blocking-basecamp+ are v1 requirements. It could very well be that this was discussed as a requirement earlier, but we're having to cut down to the bare essentials for scheduling reasons.

If a bug isn't marked as blocking-basecamp+ or blocking-basecamp- then definitely nominate the bug if you aren't sure if it's a blocker.

And of course if you think that something that is marked as blocking-basecamp- should be a blocker, also nominate the bug again. We do make mistakes :)

Comment 98

[Kai-Chih Hu [:khu]]

This one should have blocking-basecamp+. 
Sorry for any confusion.

Comment 99

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #96)

> How do you think about this? 
> In your original review, do you think I should use another name, but still
> put it under navigator? If so, can you provide a suggestion?
> 
> Or do you think I should move it to mobileconnection or telephony... etc,
> and in that case, should I still use the name 'icc'?
> 

icc in some other object, for example mobilconnection would be better than having it in navigator.

Comment 100

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #99)
> icc in some other object, for example mobilconnection would be better than
> having it in navigator.

Hi, smaug.
Thanks for your opinion. I really appreciate it. :)

Hi, mounir, philikon and sicking
Thanks for your comments,

I just discuss with smaug, 
I'll still keep those files in dom/icc, and add mozIcc under navigator.mozMobileConnection.

How do you think about this?

Comment 101

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi, mounir
Can you kindly help to provide some suggestions here?
icc currently will be under mozMobileConnection, 
do you still have concerns about putting those files in dom/network?

Thanks

Comment 102

[Mounir Lamouri (:mounir)]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #101)
> Hi, mounir
> Can you kindly help to provide some suggestions here?
> icc currently will be under mozMobileConnection, 
> do you still have concerns about putting those files in dom/network?

I have concerns about putting anything in dom/network/ that isn't directly related to networking, yes.

Comment 103

[Dietrich Ayala (:dietrich)]

Yoshi has asked politely *twice* over *two* days, and has gotten no actionable response from any developer here.

This lack of helpful guidance *makes us slow*.

Mounir, please explain what your recommendation for Yoshi is, so that we can move forward. If you're not the right person to make that call, please suggest the right decision maker that can help here.

Comment 104

[Mounir Lamouri (:mounir)]

dom/icc, dom/stk or dom/telephony would be fine. However, I'm probably not the best to take the decision.

My only concern was to put non-network related code inside dom/network/ just because some code in dom/network/ was using it. With that kind of policy, we wouldn't have a lot of directories in m-c.
As long as the code is put in a directory based on what it is doing instead of where it is used, I'm fine with anything.

Comment 105

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Mounir Lamouri (:mounir) from comment #104)
> My only concern was to put non-network related code inside dom/network/ just
> because some code in dom/network/ was using it. With that kind of policy, we
> wouldn't have a lot of directories in m-c.
> As long as the code is put in a directory based on what it is doing instead
> of where it is used, I'm fine with anything.

Hi, Mounir
Thanks for your suggestions. :)

Hi, smaug and sicking
Do you have other opinion?

Thanks

Comment 106

[Olli Pettay [:smaug][[e-mail]]]

Just put it the code to dom/icc (even if some other place has the code for the getter)

Comment 107

[Mozilla RelEng Bot]

Try run for 8cf776b19b3e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8cf776b19b3e
Results (out of 18 total builds):
    exception: 13
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8cf776b19b3e

Comment 108

[Mozilla RelEng Bot]

Try run for fa8009404120 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fa8009404120
Results (out of 18 total builds):
    exception: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fa8009404120

Comment 109

[Mozilla RelEng Bot]

Try run for e0e964a83a95 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e0e964a83a95
Results (out of 244 total builds):
    exception: 34
    success: 173
    warnings: 26
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e0e964a83a95

Comment 110

[Mozilla RelEng Bot]

Try run for 7b9cd0dce9ae is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7b9cd0dce9ae
Results (out of 264 total builds):
    exception: 1
    success: 220
    warnings: 32
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7b9cd0dce9ae

Comment 111

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

my latest try result
https://tbpl.mozilla.org/?tree=Try&rev=95ecf40d47f7

Comment 112

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: IDL v3

Update:
- Remove nsIDOM prefix for dictionary objects as philikon suggested in comment 24 and sicking agreed in comment 85
- Rename the filename nsIDOMSimToolKit.idl to SimToolKit.idl


This patch has been r+ by philikon in comment 18 and sr+ by sicking in comment 80

Comment 113

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: IccManager v3

Update:
Rebase.

This patch has been r+ by smaug

Comment 114

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL implementations v3

Update:
- Rebase,
- Update to use rilMessageType for message-passing
- Update to use read8BitUnpackedToString in retrieveTextString and length check in StkProactiveCmdHelper.retrieveTextString

This patch has been r+ by philikon in comment 52.

Comment 115

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: DOMEvent v3

Update:
rebase

This patch has been r+ by smaug in comment 51.

Comment 116

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: Hook up to Mobileconnection. V1

Hi, Jonas and smaug,
Per comment 86 comment 87 and comment 99, I add 'icc' into MobileConnection, also still keep icc files in dom/icc,
Would you help to review this patch for me ?

Thanks

Comment 117

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 6: Add MozIccManager and MozStkCommandEvent into global interface list

Hi smaug,
This patch is added in order to pass mochitest, as I add MozIccManager in Part 2 and MozStkCommandEvent in Part 4.

Can you also review this patch? 

Thank you.

Comment 118

[Mozilla RelEng Bot]

Try run for b14b53a503a9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b14b53a503a9
Results (out of 264 total builds):
    success: 231
    warnings: 21
    failure: 12
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b14b53a503a9

Comment 119

[Mozilla RelEng Bot]

Try run for 95ecf40d47f7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=95ecf40d47f7
Results (out of 264 total builds):
    success: 232
    warnings: 21
    failure: 11
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-95ecf40d47f7

Comment 120

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 653321 [details] [diff] [review]
Part 5: Hook up to Mobileconnection.  V1

>@@ -40,16 +41,17 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MobileConnection,
>                                                 nsDOMEventTargetHelper)
>   NS_CYCLE_COLLECTION_UNLINK_EVENT_HANDLER(cardstatechange)
>   NS_CYCLE_COLLECTION_UNLINK_EVENT_HANDLER(voicechange)
>   NS_CYCLE_COLLECTION_UNLINK_EVENT_HANDLER(datachange)
>   NS_CYCLE_COLLECTION_UNLINK_EVENT_HANDLER(ussdreceived)
>   tmp->mProvider = nullptr;
>+  tmp->mIccManager = nullptr;
> NS_IMPL_CYCLE_COLLECTION_UNLINK_END
So we don't need to traverse mIccManager, because it is manually released also in
Shutdown which is called when Navigator object is disconnected from the window, right?



> NS_IMETHODIMP
>+MobileConnection::GetIcc(nsIDOMMozIccManager** icc)
>+{
>+  NS_ADDREF(*icc = mIccManager);
>+  return NS_OK;
>+}
NS_IF_ADDREF since after Shutdown or Unlink has been called, NS_ADDREF would crash.
Also, the parameter name should be aIcc

Comment 121

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 653324 [details] [diff] [review]
Part 6: Add MozIccManager and MozStkCommandEvent into global interface list

But actually why? We don't run these tests on b2g, right?
I hope the idl is #ifdef'ed in Makefile.in

Comment 122

[Mozilla RelEng Bot]

Try run for ccad26e4a8c8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ccad26e4a8c8
Results (out of 18 total builds):
    success: 4
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ccad26e4a8c8

Comment 123

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #120);
> >+  tmp->mIccManager = nullptr;
> > NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> So we don't need to traverse mIccManager, because it is manually released
> also in
> Shutdown which is called when Navigator object is disconnected from the
> window, right?
Yes

> 
> 
> > NS_IMETHODIMP
> >+MobileConnection::GetIcc(nsIDOMMozIccManager** icc)
> >+{
> >+  NS_ADDREF(*icc = mIccManager);
> >+  return NS_OK;
> >+}
> NS_IF_ADDREF since after Shutdown or Unlink has been called, NS_ADDREF would
> crash.
> Also, the parameter name should be aIcc
Okay, will address this in next patch.

Thank you.

Comment 124

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #121)
> Comment on attachment 653324 [details] [diff] [review]
> Part 6: Add MozIccManager and MozStkCommandEvent into global interface list
> 
> But actually why? We don't run these tests on b2g, right?
> I hope the idl is #ifdef'ed in Makefile.in

Yes, we don't run these tests on b2g, but now try server has run some tests on desktop platform and it fails. So I add this 'Part 6 Patch' to pass those desktop tests.
But I think in the future, B2G may still need to run that 'global namespace test', and I still have to add these classes anyway.

Also in these new patches(v3), since icc is moved into mobileconnection, so I didn't use ifdef for icc-related classes.

Do you think I need to add ifdef for those ICC files? In that case then we don't need the 'Part 6 patch' in order to pass those desktop tests, and mozMobileConnection.icc is only available on b2g. 

How do you think? 

Thanks

Comment 125

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

just discuss with smaug,
I'll add ifdef MOZ_B2G_RIL for icc, so desktop build won't get icc under mobileconnection.

Thanks

Comment 126

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: IccManager v4

- Rebase
- Add ifdef MOZ_B2G_RIL

Comment 127

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL implementations v3

Rebase

Comment 128

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: DOMEvent v4

Rebase 
Add ifdef MOZ_B2G_RIL

Comment 129

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: Hook up to Mobileconnection. V2

Rebase
Address to smaug's review comments
Add ifdef MOZ_B2G_RIL

Comment 130

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 654083 [details] [diff] [review]
Part 2: IccManager v4

Hi smaug
I move icc-related stuff into MOZ_B2G_RIL
Can you review this for me ?

Thanks

Comment 131

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 654086 [details] [diff] [review]
Part 4: DOMEvent v4

Also this, StkCommandEvent

Comment 132

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 654088 [details] [diff] [review]
Part 5: Hook up to Mobileconnection.  V2

Last one, icc in mobileconnection
Also I remove the Part 6 patch, since now desktop build won't see IccManager and StkCommandEvent


Thanks

Comment 133

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 654083 [details] [diff] [review]
Part 2: IccManager v4


> include $(DEPTH)/config/autoconf.mk
> 
>-PARALLEL_DIRS = interfaces
>+PARALLEL_DIRS = interfaces 
>+ifdef MOZ_B2G_RIL
>+PARALLEL_DIRS += src
>+endif

Why interfaces isn't inside ifdef?

Comment 134

[Olli Pettay [:smaug][[e-mail]]]

or, hmm, why do we even enter to that Makefile.in if MOZ_B2G_RIL isn't defined.

Comment 135

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 654083 [details] [diff] [review]
Part 2: IccManager v4

As far as I see this would lead 
"MozIccManager" in window to be true.

Comment 136

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 654086 [details] [diff] [review]
Part 4: DOMEvent v4


>+NS_IMETHODIMP
>+StkCommandEvent::GetCommand(JSContext* aCx, jsval* aCommand)
>+
>+{
>+  nsCOMPtr<nsIJSON> json(new nsJSON());
>+  jsval cmd;
>+
>+  if (!mCommand.IsEmpty()) {
>+    nsresult rv = json->DecodeToJSVal(mCommand, aCx, &cmd);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    cmd = JSVAL_VOID;
>+  }
>+
>+  *aCommand = cmd;
>+  return NS_OK;

Why you need jsval cmd?
Couldn't you just pass aCommand to DecodeToJSVal
or set *aComment to JSVAL_VOID

Comment 137

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 654088 [details] [diff] [review]
Part 5: Hook up to Mobileconnection.  V2


> NS_IMETHODIMP
>+MobileConnection::GetIcc(nsIDOMMozIccManager** aIcc)
>+{
>+#ifdef MOZ_B2G_RIL
>+  NS_IF_ADDREF(*aIcc = mIccManager);
>+#else 
>+  *aIcc = nullptr;
>+#endif
>+  return NS_OK;
>+}
Ahaa, this is why you need the interfaces.
But we really don't want to have the interfaces in global scope.
So someone needs to ifdef the whole MobileConnection thing. Sorry.

Comment 138

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: IDL v4

This IDL has been sr+ by sicking and r+ by philikon.
This time I add ifdef MOZ_B2G_RIL for icc.

Comment 139

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: IccManager v5

Hi, smaug 
Would you help to review this patch for me?

Thanks

Comment 140

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL implementations. v3

rebase.

Comment 141

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: DOMEvent v5

Address to smaug's comments.

Comment 142

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: Hook up to MobileConnection. v3

Hi, smaug,
Since MobileConnection is already ifdef MOZ_B2G_RIL,
so in this version I remove those ifdef originally.

Thanks

Comment 143

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: RIL implementations. v4

update to use broadcaseAsyncMessage

Comment 144

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

try server result:
https://tbpl.mozilla.org/?tree=Try&rev=86cfa8979254

Comment 145

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9e30093151
https://hg.mozilla.org/integration/mozilla-inbound/rev/86069e9122f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5611e4202ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf18897cdf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/430b14724dca

Comment 146

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e9e30093151
https://hg.mozilla.org/mozilla-central/rev/86069e9122f9
https://hg.mozilla.org/mozilla-central/rev/a5611e4202ef
https://hg.mozilla.org/mozilla-central/rev/1cf18897cdf4
https://hg.mozilla.org/mozilla-central/rev/430b14724dca