Bug 744714 (b2g-ril-stk)

B2G RIL: support for SIM card toolkit

RESOLVED FIXED in mozilla18

Status

()

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

People

(Reporter: gal, Assigned: allstars)

Tracking

Trunk
mozilla18
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE: S], URL)

Attachments

(5 attachments, 28 obsolete attachments)

16.91 KB, patch
Details | Diff | Splinter Review
9.11 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.33 KB, patch
Details | Diff | Splinter Review
8.18 KB, patch
smaug
: review+
Details | Diff | Splinter Review
53.06 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Alias: Stk
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Updated

6 years ago
Summary: support for SIM card toolkit (Stk) → support for SIM card toolkit
(Reporter)

Updated

6 years ago
Blocks: 710489
(Reporter)

Comment 1

6 years ago
clee, please add to V2 tracking list (we might have to upgrade to V1)
(Assignee)

Updated

6 years ago
Assignee: nobody → yhuang
Is this an M3 feature? It seems like we're going to need to add a DOM API for this...
Alias: Stk → b2g-stk
Summary: support for SIM card toolkit → B2G RIL: support for SIM card toolkit
(Reporter)

Comment 3

6 years ago
I think this is Gecko internal, at least for now.
OS: All → Gonk
Hardware: All → ARM

Comment 4

6 years ago
Do we want this for M3?  Or for post-B2G/Gaia v1?
Targeting post-M3.
(Assignee)

Comment 6

6 years ago
Created attachment 618159 [details] [diff] [review]
[WIP] Stk.patch

Current WIP, include parsing BerTLV, ComprehensionTlv, and some Proactive commands, and implement STK_SEND_TERMINAL_RESPONSE
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.)
(Reporter)

Comment 8

6 years ago
Nice work. ++yoshi
(Assignee)

Comment 9

6 years ago
(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.
(Assignee)

Comment 10

6 years ago
(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
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.
No longer blocks: 710489
blocking-basecamp: --- → -
(Assignee)

Comment 12

6 years ago
Created attachment 637875 [details] [diff] [review]
Part 1 : IDL

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

thanks
(Assignee)

Updated

6 years ago
Attachment #637875 - Flags: review?(philipp)
(Assignee)

Comment 13

5 years ago
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
Attachment #637875 - Flags: superreview?(jonas)
(Assignee)

Comment 14

5 years ago
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'?
(Assignee)

Comment 15

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

Comment 16

5 years ago
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
(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 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.
Attachment #637875 - Flags: review?(philipp) → review+
(Assignee)

Comment 19

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

Comment 20

5 years ago
(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);
(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 :)
(Assignee)

Comment 22

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

Comment 23

5 years ago
Created attachment 641320 [details] [diff] [review]
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
Attachment #618159 - Attachment is obsolete: true
Attachment #637875 - Attachment is obsolete: true
Attachment #637875 - Flags: superreview?(jonas)
Attachment #641320 - Flags: review?(philipp)
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`?
Attachment #641320 - Flags: superreview?(jonas)
Attachment #641320 - Flags: review?(philipp)
Attachment #641320 - Flags: review+
(Assignee)

Comment 25

5 years ago
(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
Ah I see. After reading comment 16 this makes sense.
(Assignee)

Comment 27

5 years ago
Created attachment 642377 [details] [diff] [review]
Part 2: IccManager
Attachment #642377 - Flags: review?(philipp)
(Assignee)

Comment 28

5 years ago
Created attachment 642378 [details] [diff] [review]
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~~
Attachment #642378 - Flags: feedback?(philipp)
(Assignee)

Comment 29

5 years ago
Created attachment 642379 [details] [diff] [review]
Part 4: DOMEvent
Attachment #642379 - Flags: review?(philipp)
(Assignee)

Comment 30

5 years ago
Created attachment 642380 [details] [diff] [review]
Part 5: Hook up to Navigator
Attachment #642380 - Flags: review?(philipp)
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.
Attachment #642377 - Flags: review?(philipp) → review?(bent.mozilla)
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!
Attachment #642380 - Flags: review?(philipp) → review?(bent.mozilla)
Comment on attachment 642379 [details] [diff] [review]
Part 4: DOMEvent

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

One more... sorry, bent :)
Attachment #642379 - Flags: review?(philipp) → review?(bent.mozilla)
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.
(Assignee)

Comment 35

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

Comment 36

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

Comment 37

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

Comment 39

5 years ago
Created attachment 643268 [details] [diff] [review]
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
Attachment #642378 - Attachment is obsolete: true
Attachment #642378 - Flags: feedback?(philipp)
(Assignee)

Comment 40

5 years ago
Created attachment 643270 [details] [diff] [review]
Part 3: RIL Implementations

sorry, forgetting qref
Attachment #643268 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
(Assignee)

Comment 41

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

Comment 42

5 years ago
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~
Attachment #642377 - Flags: review?(bent.mozilla) → review?(mounir)
(Assignee)

Comment 43

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

Hi Mounir
2nd Patch,
This patch is about DOMEvent

Thanks
Attachment #642379 - Flags: review?(bent.mozilla) → review?(mounir)
(Assignee)

Comment 44

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

Hi, Mounir
Last one, 
hook up to navigator.

Thanks
Attachment #642380 - Flags: review?(bent.mozilla) → review?(mounir)
(Assignee)

Comment 45

5 years ago
Created attachment 643716 [details] [diff] [review]
Part 3: RIL implementations
Attachment #643270 - Attachment is obsolete: true
(Assignee)

Comment 46

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

Comment 47

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

Another patch for review, for DOMEvent
Attachment #642379 - Flags: review?(mounir) → review?(bugs)
(Assignee)

Comment 48

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

Last one, hook up to Navigator.
Attachment #642380 - Flags: review?(mounir) → review?(bugs)
(Assignee)

Comment 49

5 years ago
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
Attachment #643716 - Flags: review?(philipp)
(Assignee)

Comment 50

5 years ago
(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.

Updated

5 years ago
Blocks: 715784
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.)
Attachment #642379 - Flags: review?(bugs) → review+
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
Attachment #643716 - Flags: review?(philipp) → review+

Updated

5 years ago
Attachment #642380 - Flags: review?(bugs) → review+
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
Attachment #642377 - Flags: review?(bugs) → review+
(Assignee)

Comment 54

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

Comment 56

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

Comment 58

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

Comment 61

5 years ago
(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
(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.
BTW, what are the use cases of STK? It's quite not clear.
(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.)
(Assignee)

Comment 65

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

Comment 66

5 years ago
Created attachment 645175 [details] [diff] [review]
Part 2: IccManager v2
Attachment #645175 - Flags: review?(bugs)
(Assignee)

Comment 67

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

sorry 
accident hit the keyboard
Attachment #645175 - Attachment is obsolete: true
Attachment #645175 - Flags: review?(bugs)
(Assignee)

Comment 68

5 years ago
Created attachment 645177 [details] [diff] [review]
Part 2: IccManager v2

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

Thanks
Attachment #642377 - Attachment is obsolete: true
Attachment #645177 - Flags: review?(bugs)
(Assignee)

Comment 69

5 years ago
Created attachment 645178 [details] [diff] [review]
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.
Attachment #643716 - Attachment is obsolete: true
(Assignee)

Comment 70

5 years ago
Created attachment 645179 [details] [diff] [review]
Part 4: DOMEvent v2
Attachment #645179 - Flags: review?
(Assignee)

Comment 71

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

sorry hit the keyboard again..
Attachment #645179 - Attachment is obsolete: true
Attachment #645179 - Flags: review?
(Assignee)

Comment 72

5 years ago
Created attachment 645181 [details] [diff] [review]
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
Attachment #642379 - Attachment is obsolete: true
Attachment #645181 - Flags: review?(bugs)
(Assignee)

Comment 73

5 years ago
Created attachment 645182 [details] [diff] [review]
Part 5: Hook up to navigator. v2

Hi, smaug

Update:
* rebase
* add #ifdef MOZ_B2G_RIL

Thanks
Attachment #642380 - Attachment is obsolete: true
Attachment #645182 - Flags: review?(bugs)
(Assignee)

Comment 74

5 years ago
Created attachment 645214 [details] [diff] [review]
Part 5: Hook up to navigator. v2

forgot adding r=smaug
Attachment #645182 - Attachment is obsolete: true
Attachment #645182 - Flags: review?(bugs)
Attachment #645214 - Flags: review?(bugs)

Updated

5 years ago
Attachment #645181 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #645177 - Flags: review?(bugs) → review+
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.
Attachment #645214 - Flags: review?(bugs) → review-
(Assignee)

Comment 76

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

Updated

5 years ago
Blocks: 778679
(Assignee)

Updated

5 years ago
Blocks: 779036
(Assignee)

Comment 77

5 years ago
Hi
jonas, sr ping?
smaug, can you take a look in comment 76 ?
icc sounds still way too abbreviated in navigator context. But I'm ok with it if jonas is.
(Assignee)

Comment 79

5 years ago
(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 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.
Attachment #641320 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 81

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

Comment 82

5 years ago
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
Attachment #645214 - Flags: feedback?(jonas)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 83

5 years ago
hi jonas, 
feeback ping in comment 82
(Assignee)

Comment 84

5 years ago
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.
Attachment #645214 - Flags: feedback?(jonas)
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.
(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.
Oh, I forgot about that one. I agree, that sounds even better.
(Assignee)

Comment 88

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

Comment 89

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

Comment 91

5 years ago
(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.
I would prefer that to stay out of dom/network/. AFAIUI, STK isn't directly related to networking.
(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.
are we spending too much time on this bug? Please not that it is *not* a blocker.
(Assignee)

Comment 95

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

Comment 96

5 years ago
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
Attachment #645214 - Flags: feedback?(bugs)
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

5 years ago
This one should have blocking-basecamp+. 
Sorry for any confusion.
blocking-basecamp: - → +
(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.

Updated

5 years ago
Attachment #645214 - Flags: feedback?(bugs)
(Assignee)

Comment 100

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

Updated

5 years ago
Whiteboard: [LOE: S]
(Assignee)

Comment 101

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

Comment 105

5 years ago
(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
Just put it the code to dom/icc (even if some other place has the code for the getter)

Comment 107

5 years ago
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/yhuang@mozilla.com-8cf776b19b3e

Comment 108

5 years ago
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/yhuang@mozilla.com-fa8009404120

Comment 109

5 years ago
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/yhuang@mozilla.com-e0e964a83a95

Comment 110

5 years ago
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/yhuang@mozilla.com-7b9cd0dce9ae
(Assignee)

Comment 111

5 years ago
my latest try result
https://tbpl.mozilla.org/?tree=Try&rev=95ecf40d47f7
(Assignee)

Comment 112

5 years ago
Created attachment 653312 [details] [diff] [review]
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
Attachment #641320 - Attachment is obsolete: true
(Assignee)

Comment 113

5 years ago
Created attachment 653315 [details] [diff] [review]
Part 2: IccManager v3

Update:
Rebase.

This patch has been r+ by smaug
Attachment #645177 - Attachment is obsolete: true
(Assignee)

Comment 114

5 years ago
Created attachment 653318 [details] [diff] [review]
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.
Attachment #645178 - Attachment is obsolete: true
(Assignee)

Comment 115

5 years ago
Created attachment 653319 [details] [diff] [review]
Part 4: DOMEvent v3

Update:
rebase

This patch has been r+ by smaug in comment 51.
Attachment #645181 - Attachment is obsolete: true
(Assignee)

Comment 116

5 years ago
Created attachment 653321 [details] [diff] [review]
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
Attachment #645214 - Attachment is obsolete: true
Attachment #653321 - Flags: superreview?(jonas)
Attachment #653321 - Flags: review?(bugs)
(Assignee)

Comment 117

5 years ago
Created attachment 653324 [details] [diff] [review]
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.
Attachment #653324 - Flags: review?(bugs)

Comment 118

5 years ago
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/yhuang@mozilla.com-b14b53a503a9

Comment 119

5 years ago
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/yhuang@mozilla.com-95ecf40d47f7
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
Attachment #653321 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #653324 - Flags: review?(bugs) → review+
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

5 years ago
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/yhuang@mozilla.com-ccad26e4a8c8
(Assignee)

Comment 123

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

Comment 124

5 years ago
(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
Attachment #653321 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 125

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

Thanks
(Assignee)

Comment 126

5 years ago
Created attachment 654083 [details] [diff] [review]
Part 2: IccManager v4

- Rebase
- Add ifdef MOZ_B2G_RIL
Attachment #653315 - Attachment is obsolete: true
(Assignee)

Comment 127

5 years ago
Created attachment 654085 [details] [diff] [review]
Part 3: RIL implementations v3

Rebase
Attachment #653318 - Attachment is obsolete: true
(Assignee)

Comment 128

5 years ago
Created attachment 654086 [details] [diff] [review]
Part 4: DOMEvent v4

Rebase 
Add ifdef MOZ_B2G_RIL
Attachment #653319 - Attachment is obsolete: true
(Assignee)

Comment 129

5 years ago
Created attachment 654088 [details] [diff] [review]
Part 5: Hook up to Mobileconnection.  V2

Rebase
Address to smaug's review comments
Add ifdef MOZ_B2G_RIL
Attachment #653321 - Attachment is obsolete: true
Attachment #653324 - Attachment is obsolete: true
(Assignee)

Comment 130

5 years ago
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
Attachment #654083 - Flags: review?(bugs)
(Assignee)

Comment 131

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

Also this, StkCommandEvent
Attachment #654086 - Flags: review?(bugs)
(Assignee)

Comment 132

5 years ago
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
Attachment #654088 - Flags: review?(bugs)
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?
or, hmm, why do we even enter to that Makefile.in if MOZ_B2G_RIL isn't defined.
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.
Attachment #654083 - Flags: review?(bugs) → review-
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
Attachment #654086 - Flags: review?(bugs) → review+
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.
Attachment #654088 - Flags: review?(bugs) → review-
(Assignee)

Updated

5 years ago
Depends on: 784709
(Assignee)

Updated

5 years ago
No longer depends on: 784709
(Assignee)

Updated

5 years ago
Depends on: 785942
(Assignee)

Comment 138

5 years ago
Created attachment 656142 [details] [diff] [review]
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.
Attachment #653312 - Attachment is obsolete: true
(Assignee)

Comment 139

5 years ago
Created attachment 656143 [details] [diff] [review]
Part 2: IccManager v5

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

Thanks
Attachment #654083 - Attachment is obsolete: true
Attachment #656143 - Flags: review?(bugs)
(Assignee)

Comment 140

5 years ago
Created attachment 656145 [details] [diff] [review]
Part 3: RIL implementations. v3

rebase.
Attachment #654085 - Attachment is obsolete: true
(Assignee)

Comment 141

5 years ago
Created attachment 656146 [details] [diff] [review]
Part 4: DOMEvent v5

Address to smaug's comments.
Attachment #654086 - Attachment is obsolete: true
(Assignee)

Comment 142

5 years ago
Created attachment 656148 [details] [diff] [review]
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
Attachment #654088 - Attachment is obsolete: true
Attachment #656148 - Flags: review?(bugs)

Updated

5 years ago
Attachment #656143 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #656148 - Flags: review?(bugs) → review+
(Assignee)

Comment 143

5 years ago
Created attachment 656205 [details] [diff] [review]
Part 3: RIL implementations. v4

update to use broadcaseAsyncMessage
Attachment #656145 - Attachment is obsolete: true
(Assignee)

Comment 144

5 years ago
try server result:
https://tbpl.mozilla.org/?tree=Try&rev=86cfa8979254
(Assignee)

Comment 145

5 years ago
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
Target Milestone: --- → mozilla18
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Alias: b2g-stk → b2g-ril-stk
Blocks: 792751
(Assignee)

Updated

5 years ago
Blocks: 791161
Blocks: 800268
You need to log in before you can comment on or make changes to this bug.