1081789 - [Ril] Redefine nsIIccProvider.idl interface for support SE/SIM access

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Vincent Chang[:vchang][changyihsin]]

Security element APIs and access control enforcer(ACE) for NFC need to use icc communication channel APIs defined in nsIIccProvider.idl [1] to exchange APDUs.
The client using these APIs could be in chrome process or content process. However, current
APIs definition doesn't seem to meet this requirement when using the APIs in chrome process. Because these APIs use nsIDOMRequest as return value and window object in its second argument. We would like to redefine the interface to fulfill our requirement for NFC development. 

[1] http://dxr.mozilla.org/mozilla-central/source/dom/icc/interfaces/nsIIccProvider.idl?from=nsIIccProvider.idl&case=true#94

Comment 1

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

I believe this issue could be fixed in Bug 864489, which is huge work. Considering the schedule, I realize that we need to have a *transient* solution for SE and that would be refining only "iccOpenChannel", "iccExchangeAPDU" and "iccCloseChannel" first IMO. 

We'd like to avoid jsval as much as possible; however, as this is a transient solution, I'd also like to keep this bug as simple as possible, i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will be discarded eventually in bug 864489.

A possible solution would look like:

/* New interface */
interface nsIIccCallback : nsISupports
{
  void notifySuccess();
  void notifyError(in AString error);
  void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString response);
};

partial interface nsIIccProvider
{
  void iccOpenChannel(in unsigned long clientId,
                      in AString aid,
                      in nsIIccCallback callback);
  void iccExchangeAPDU(in unsigned long clientId,
                       in long channel,
                       in jsval apdu,
                       in nsIIccCallback callback);
  void iccCloseChannel(in unsigned long clientId,
                       in long channel,
                       in nsIIccCallback callback);
};

How does that sound?

Comment 2

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> I believe this issue could be fixed in Bug 864489, which is huge work.
> Considering the schedule, I realize that we need to have a *transient*
> solution for SE and that would be refining only "iccOpenChannel",
> "iccExchangeAPDU" and "iccCloseChannel" first IMO. 
> 
> We'd like to avoid jsval as much as possible; however, as this is a
> transient solution, I'd also like to keep this bug as simple as possible,
> i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will
> be discarded eventually in bug 864489.
> 
> A possible solution would look like:
> 
> /* New interface */
> interface nsIIccCallback : nsISupports
> {
>   void notifySuccess();
>   void notifyError(in AString error);
>   void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString
> response);
> };
> 
> partial interface nsIIccProvider
> {
>   void iccOpenChannel(in unsigned long clientId,
>                       in AString aid,
>                       in nsIIccCallback callback);
>   void iccExchangeAPDU(in unsigned long clientId,
>                        in long channel,
>                        in jsval apdu,
>                        in nsIIccCallback callback);
>   void iccCloseChannel(in unsigned long clientId,
>                        in long channel,
>                        in nsIIccCallback callback);
> };
> 
> How does that sound?

Looks good to me, but we need one more success callback which carries `channelId` for `iccOpenChannel`.
Thank you.

Comment 3

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

(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > I believe this issue could be fixed in Bug 864489, which is huge work.
> > Considering the schedule, I realize that we need to have a *transient*
> > solution for SE and that would be refining only "iccOpenChannel",
> > "iccExchangeAPDU" and "iccCloseChannel" first IMO. 
> > 
> > We'd like to avoid jsval as much as possible; however, as this is a
> > transient solution, I'd also like to keep this bug as simple as possible,
> > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will
> > be discarded eventually in bug 864489.
> > 
> > A possible solution would look like:
> > 
> > /* New interface */
> > interface nsIIccCallback : nsISupports
> > {
> >   void notifySuccess();
> >   void notifyError(in AString error);
> >   void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString
> > response);
> > };
> > 
> > partial interface nsIIccProvider
> > {
> >   void iccOpenChannel(in unsigned long clientId,
> >                       in AString aid,
> >                       in nsIIccCallback callback);
> >   void iccExchangeAPDU(in unsigned long clientId,
> >                        in long channel,
> >                        in jsval apdu,
> >                        in nsIIccCallback callback);
> >   void iccCloseChannel(in unsigned long clientId,
> >                        in long channel,
> >                        in nsIIccCallback callback);
> > };
> > 
> > How does that sound?
> 
> Looks good to me, but we need one more success callback which carries
> `channelId` for `iccOpenChannel`.
> Thank you.

Ah ha, you are right :)

Comment 4

[Vincent Chang[:vchang][changyihsin]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> I believe this issue could be fixed in Bug 864489, which is huge work.
> Considering the schedule, I realize that we need to have a *transient*
> solution for SE and that would be refining only "iccOpenChannel",
> "iccExchangeAPDU" and "iccCloseChannel" first IMO. 
> 
> We'd like to avoid jsval as much as possible; however, as this is a
> transient solution, I'd also like to keep this bug as simple as possible,
> i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will
> be discarded eventually in bug 864489.
> 
Thanks for Hsin-Yi and Edgar's proposal. It looks good to me. To meet our milestone for NFC SE development, I think it makes sense to minimize the effort for interface change in this bug. It requires QC to release a transient version of QC RIL based on the interface discussed here. I think Hsin-Yi's team is working hardly to make freezing RIL related interfaces happened. After they finish that, we could have QC to release formal version of QC RIL. Does it make sense to you guys?

Comment 5

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> I believe this issue could be fixed in Bug 864489, which is huge work.
> Considering the schedule, I realize that we need to have a *transient*
> solution for SE and that would be refining only "iccOpenChannel",
> "iccExchangeAPDU" and "iccCloseChannel" first IMO. 
> 
> We'd like to avoid jsval as much as possible; however, as this is a
> transient solution, I'd also like to keep this bug as simple as possible,
> i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will
> be discarded eventually in bug 864489.
> 
> A possible solution would look like:
> 
> /* New interface */
> interface nsIIccCallback : nsISupports
> {
>   void notifySuccess();
>   void notifyError(in AString error);
>   void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString
> response);
> };
> 
> partial interface nsIIccProvider
> {
>   void iccOpenChannel(in unsigned long clientId,
>                       in AString aid,
>                       in nsIIccCallback callback);
>   void iccExchangeAPDU(in unsigned long clientId,
>                        in long channel,
>                        in jsval apdu,
>                        in nsIIccCallback callback);
>   void iccCloseChannel(in unsigned long clientId,
>                        in long channel,
>                        in nsIIccCallback callback);
> };
> 
> How does that sound?
Hsin-Yi, even though this is a transient solution the use of JSVAL just adds unnecessary complexity in our implementation. Since apdu is sort of a structure of fixed data types, can we please either define a new strongly type interface that we pass as apdu or expand apdu inline in the exchange APDU?

Freezing of RIL Interface is still far from completion so we shouldn't really wait for that before making this change. Ideally we make the change as soon as Moz has resources to do so and QC will adapt to it ASAP.

Comment 6

[Kamil Leszczuk [:kamituel]]

Having icc* methods to accept a callback instead of returning a DOMRequest works for me. Thanks!

Comment 7

[Kamil Leszczuk [:kamituel]]

Oh, I was to fast in my previous comment. I think we need to have a success callback which will pass a channel ID, right? It could be something like:

> interface nsIIccCallback : nsISupports
> {
>   void notifySuccess(in long channelId);
>   void notifyError(in AString error);
>   void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString
> response);
> };

For |iccOpenChannel| and |iccCloseChannel| we would put the channel ID there, and for |iccExchangeAPDU| we would use the |notifyExchangeAPDUSuccess| anyway.

What do you think?

Comment 8

[psiddh]

- Can we rename 'nsIIccCallback' interface to 'nsIIccChannelCallback' or some such (because 'Icc' is much broader in that sense) ?
- Can we rename 'notifyExchangeAPDUSuccess' to 'notifyExchangeAPDUResponse' as sw1 and sw2 can contain error codes as well?

My two cents on the following:
Can we not simply have one callback (overload it with args ?)
 interface nsIIccChannelCallback : nsISupports
 {
   // 'details.channelId ' : Present for all three operations
   // 'details.sw1 , details.sw2 etc' present only for iccExchangeApdu()
   onResult(in long status, in jsval details); 

                    (or, simply)

   handleMessage (in jsval details);
 };

Comment 9

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

(In reply to Anshul from comment #5)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > I believe this issue could be fixed in Bug 864489, which is huge work.
> > Considering the schedule, I realize that we need to have a *transient*
> > solution for SE and that would be refining only "iccOpenChannel",
> > "iccExchangeAPDU" and "iccCloseChannel" first IMO. 
> > 
> > We'd like to avoid jsval as much as possible; however, as this is a
> > transient solution, I'd also like to keep this bug as simple as possible,
> > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will
> > be discarded eventually in bug 864489.
> > 
> > A possible solution would look like:
> > 
> > /* New interface */
> > interface nsIIccCallback : nsISupports
> > {
> >   void notifySuccess();
> >   void notifyError(in AString error);
> >   void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString
> > response);
> > };
> > 
> > partial interface nsIIccProvider
> > {
> >   void iccOpenChannel(in unsigned long clientId,
> >                       in AString aid,
> >                       in nsIIccCallback callback);
> >   void iccExchangeAPDU(in unsigned long clientId,
> >                        in long channel,
> >                        in jsval apdu,
> >                        in nsIIccCallback callback);
> >   void iccCloseChannel(in unsigned long clientId,
> >                        in long channel,
> >                        in nsIIccCallback callback);
> > };
> > 
> > How does that sound?
> Hsin-Yi, even though this is a transient solution the use of JSVAL just adds
> unnecessary complexity in our implementation. 

I had this proposal because I tried to look for good balance b/w the tight schedule for SE and technical solution. I thought we have been used jsval for exchangeAPDU for a long time, and you already had that implementation. Since we didn't change the basic behaviour of exchangeAPDU() I reason it could create barely overhead for you, and this could be acceptable for all of us, no? Please let us know if I didn't get your situation clear.

> Since apdu is sort of a
> structure of fixed data types, can we please either define a new strongly
> type interface that we pass as apdu or expand apdu inline in the exchange
> APDU?

Could you provide a complete data structure here?

> 
> Freezing of RIL Interface is still far from completion so we shouldn't
> really wait for that before making this change. Ideally we make the change
> as soon as Moz has resources to do so and QC will adapt to it ASAP.

Comment 10

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

(In reply to Siddartha P from comment #8)
> - Can we rename 'nsIIccCallback' interface to 'nsIIccChannelCallback' or
> some such (because 'Icc' is much broader in that sense) ?

Naming nsIIccChannelCallback sounds good to me. However, I'd like to see if Edgar has concerns, especially considering our further plan bug 864489.

> - Can we rename 'notifyExchangeAPDUSuccess' to 'notifyExchangeAPDUResponse'
> as sw1 and sw2 can contain error codes as well?
> 

Good idea. No objection :)

> My two cents on the following:
> Can we not simply have one callback (overload it with args ?)
>  interface nsIIccChannelCallback : nsISupports
>  {
>    // 'details.channelId ' : Present for all three operations
>    // 'details.sw1 , details.sw2 etc' present only for iccExchangeApdu()
>    onResult(in long status, in jsval details); 
> 
>                     (or, simply)
> 
>    handleMessage (in jsval details);
>  };

Sadly, no. For a new interface, we don't want to use jsval that causes trouble to other partners. We should have a well-defined arguments or interfaces. And actually we are already working on getting rid of jsval usage for ril internal interfaces.

Comment 11

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> (In reply to Siddartha P from comment #8)
> > - Can we rename 'nsIIccCallback' interface to 'nsIIccChannelCallback' or
> > some such (because 'Icc' is much broader in that sense) ?
> 
> Naming nsIIccChannelCallback sounds good to me. However, I'd like to see if
> Edgar has concerns, especially considering our further plan bug 864489.

I am ok with `nsIIccChannelCallback` if it makes more sense. ((For bug 864489, we can have another callback for the other icc requests.))
Thank you.

Comment 12

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> (In reply to Anshul from comment #5)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > > I believe this issue could be fixed in Bug 864489, which is huge work.
> > > Considering the schedule, I realize that we need to have a *transient*
> > > solution for SE and that would be refining only "iccOpenChannel",
> > > "iccExchangeAPDU" and "iccCloseChannel" first IMO. 
> > > 
> > > We'd like to avoid jsval as much as possible; however, as this is a
> > > transient solution, I'd also like to keep this bug as simple as possible,
> > > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will
> > > be discarded eventually in bug 864489.
> > > 
> > > A possible solution would look like:
> > > 
> > > /* New interface */
> > > interface nsIIccCallback : nsISupports
> > > {
> > >   void notifySuccess();
> > >   void notifyError(in AString error);
> > >   void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString
> > > response);
> > > };
> > > 
> > > partial interface nsIIccProvider
> > > {
> > >   void iccOpenChannel(in unsigned long clientId,
> > >                       in AString aid,
> > >                       in nsIIccCallback callback);
> > >   void iccExchangeAPDU(in unsigned long clientId,
> > >                        in long channel,
> > >                        in jsval apdu,
> > >                        in nsIIccCallback callback);
> > >   void iccCloseChannel(in unsigned long clientId,
> > >                        in long channel,
> > >                        in nsIIccCallback callback);
> > > };
> > > 
> > > How does that sound?
> > Hsin-Yi, even though this is a transient solution the use of JSVAL just adds
> > unnecessary complexity in our implementation. 
> 
> I had this proposal because I tried to look for good balance b/w the tight
> schedule for SE and technical solution. I thought we have been used jsval
> for exchangeAPDU for a long time, and you already had that implementation.
> Since we didn't change the basic behaviour of exchangeAPDU() I reason it
> could create barely overhead for you, and this could be acceptable for all
> of us, no? Please let us know if I didn't get your situation clear.
Hsin-Yi, I understand your concern. My point simply is that you are getting away with JSVAL in bug 864489. 
Once this bug lands, we are expecting no more JSVAls in the ICC interface including SEEK APIs. So seems like we would have to modify the NFC APIs again once bug 864489 lands. This reworking on the same APIs again and again is frankly very wasteful for both you and us. Therefore I was suggesting to come up with a final API design as part of this bug as the cost of doing a simple solution now and a redesign later is far more than coming up with one final design right now.
 
> 
> > Since apdu is sort of a
> > structure of fixed data types, can we please either define a new strongly
> > type interface that we pass as apdu or expand apdu inline in the exchange
> > APDU?
> 
> Could you provide a complete data structure here?
> 
> > 
> > Freezing of RIL Interface is still far from completion so we shouldn't
> > really wait for that before making this change. Ideally we make the change
> > as soon as Moz has resources to do so and QC will adapt to it ASAP.

Comment 13

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

(In reply to Anshul from comment #12)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> > (In reply to Anshul from comment #5)
> > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > > > I believe this issue could be fixed in Bug 864489, which is huge work.
> > > > Considering the schedule, I realize that we need to have a *transient*
> > > > solution for SE and that would be refining only "iccOpenChannel",
> > > > "iccExchangeAPDU" and "iccCloseChannel" first IMO. 
> > > > 
> > > > We'd like to avoid jsval as much as possible; however, as this is a
> > > > transient solution, I'd also like to keep this bug as simple as possible,
> > > > i.e. I suggest we keep use jsval in "iccExchangeAPDU" here. jsval usage will
> > > > be discarded eventually in bug 864489.
> > > > 
> > > > A possible solution would look like:
> > > > 
> > > > /* New interface */
> > > > interface nsIIccCallback : nsISupports
> > > > {
> > > >   void notifySuccess();
> > > >   void notifyError(in AString error);
> > > >   void notifyExchangeAPDUSuccess(in long sw1, in long sw2, in AString
> > > > response);
> > > > };
> > > > 
> > > > partial interface nsIIccProvider
> > > > {
> > > >   void iccOpenChannel(in unsigned long clientId,
> > > >                       in AString aid,
> > > >                       in nsIIccCallback callback);
> > > >   void iccExchangeAPDU(in unsigned long clientId,
> > > >                        in long channel,
> > > >                        in jsval apdu,
> > > >                        in nsIIccCallback callback);
> > > >   void iccCloseChannel(in unsigned long clientId,
> > > >                        in long channel,
> > > >                        in nsIIccCallback callback);
> > > > };
> > > > 
> > > > How does that sound?
> > > Hsin-Yi, even though this is a transient solution the use of JSVAL just adds
> > > unnecessary complexity in our implementation. 
> > 
> > I had this proposal because I tried to look for good balance b/w the tight
> > schedule for SE and technical solution. I thought we have been used jsval
> > for exchangeAPDU for a long time, and you already had that implementation.
> > Since we didn't change the basic behaviour of exchangeAPDU() I reason it
> > could create barely overhead for you, and this could be acceptable for all
> > of us, no? Please let us know if I didn't get your situation clear.
> Hsin-Yi, I understand your concern. My point simply is that you are getting
> away with JSVAL in bug 864489. 
> Once this bug lands, we are expecting no more JSVAls in the ICC interface
> including SEEK APIs. So seems like we would have to modify the NFC APIs
> again once bug 864489 lands. This reworking on the same APIs again and again
> is frankly very wasteful for both you and us. Therefore I was suggesting to
> come up with a final API design as part of this bug as the cost of doing a
> simple solution now and a redesign later is far more than coming up with one
> final design right now.
>  

That's indeed a valid concern. I am fine with this suggestion. But note that it might take more time to finish this bug.

According to [1], the adpu structure contains "cla (int32), command (int32), channel (int32), path (string), data (string), data2 (string), p1 (int32), p2 (int32), p3 (int32)." Are these the apdu interface needs? Anything missing, Vincent/Kamil/Siddartha ? Thank you!

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#1480

Comment 14

[Vincent Chang[:vchang][changyihsin]]

Security element command and response interface are defined in Bug 879861 [1]. But I think we could define more generic interface for APDU because it may be used in cases other than security element as well. 

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=879861&attachment=8491514

interface SECommand {
  attribute octet cla; // 1 Byte : Class Byte
  attribute octet ins; // 1 Byte : Instruction Byte
  attribute octet p1; // 1 Byte : First Octet of Parameters Byte
  attribute octet p2; // 1 Byte : Second Octet of Parameters Byte
  [Cached, Pure] attribute sequence<octet> data; // Sequence of octets
  attribute short le; // The length of the expected
  // response data or -1 if none is expected
}

interface SEResponse {
  // Response received on this 'channel' object.
  readonly attribute SEChannel channel;
  // First octet of response's status word
  readonly attribute octet sw1;
  // Second octet of response's status word
  readonly attribute octet sw2;
  // The response's data field bytes
  [Cached, Pure] readonly attribute sequence<octet> data;
};

Comment 15

[Krzysztof Mioduszewski[:tauzen]]

To be compliant with Open Mobile API we also need to able to retrieve the response to SELECT applet command which is currently done automatically when the channel is opened. I think that the success callback for opening channel should also carry the response to applet selection.

Comment 16

[psiddh]

Maybe we don't have to change the interfaces described in 'nsIIccProvider' interface and still support 'openResponse'.
For SecureElement parent process, it will be translated into two calls. (Since anyways the baseband  creates the two APDU commands 1) Implicit selection , MANAGE CHANNEL [OPEN] , 2) Explicit Selection, SELECT [with AID] for iccOpenLogicalChannel() command)
 
1) iccOpenChannel() : returns the channel id , say XX
2) iccExchangeAPDU() : { cla: xx, ins: 0xC0, 0x00 , 0x00, 0x00 }  // 0xCO is the INS for 'GET RESPONSE command APDU'. The result of this operation 'GET RESPONSE response APDU' ('SEResponse') then used to update 'openResponse' attribute in SEChannel interface.

Comment 17

[Vincent Chang[:vchang][changyihsin]]

Hi sid, do you think you can take the bug?

Comment 18

[psiddh]

Ok Vincent. Sure!

Comment 19

[Vincent Chang[:vchang][changyihsin]]

Thanks for your great help.

Comment 20

[psiddh]

Attachment: 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch

Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider this a 'needinfo' on all). I have basically collated all the comments from the bug and uploaded this patch. (However please note that I have now renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) , etc)

- Please note that TMobile Poland (Wallet developers using SE API) are in need of 'selectResponse' that comes from opening a logical channel successfully.
This patch considers this request.

@Anshul, Is it possible for FxOS QCOM RIL to implement the callback 'onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse)' ?
It implies that QCOM RIL should perform a two step process in order to retrieve before calling this callback.
After getting the 'channel number / handle (say 'XX')' , QCOM RIL (instead of immediately notifying upper layers of the 'channel number / handle' - This is what happens currently) should also send "XX C0 00 00 00" APDU again to the baseband  to retrieve 'select response' and then notify upper layers (Secure Element parent process) using the callback 'onOpenChannelDone( xx, selectResponse-apdu)'.

Comment 21

[psiddh]

Also forgot to add this comment. I am not sure if we can use keywords such as 'partial interface' 'sequence<octet>' etc in an idl (*.idl). These should be ok in a webidl though. Again please correct me / provide feedback if that's not the case .
This patch modifies earlier 3 definitions in nsIIccProvider.idl inline.

Comment 22

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

(In reply to Siddartha P from comment #20)
> Created attachment 8515406 [details] [diff] [review]
> 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider
> this a 'needinfo' on all). I have basically collated all the comments from
> the bug and uploaded this patch. (However please note that I have now
> renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) ,
> etc)
> 
> - Please note that TMobile Poland (Wallet developers using SE API) are in
> need of 'selectResponse' that comes from opening a logical channel
> successfully.
> This patch considers this request.
> 

Hi Siddartha,

Thanks for the patch! I am really new to SE so please forgive my trivial questions :)

If I understood the SE WebAPI correctly, there will be another asyn operation to retrieve APDUResponse after openChannel. Then why cannot we also have the same design in this internal interface? Why do we need to have "selectResponse" along with iccOpenChannel callback, especially in AOSP [2] I don't see it's possible to retrieve the response in one-step request?

Another question is, does it always apply that "selectResponse" necessarily come with iccOpenChannel, or is it simply specific to TMobile Poland?

Thank you :)

[1] SE API current draft https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=879861&attachment=8491514
[2] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h

> @Anshul, Is it possible for FxOS QCOM RIL to implement the callback
> 'onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse)' ?
> It implies that QCOM RIL should perform a two step process in order to
> retrieve before calling this callback.
> After getting the 'channel number / handle (say 'XX')' , QCOM RIL (instead
> of immediately notifying upper layers of the 'channel number / handle' -
> This is what happens currently) should also send "XX C0 00 00 00" APDU again
> to the baseband  to retrieve 'select response' and then notify upper layers
> (Secure Element parent process) using the callback 'onOpenChannelDone( xx,
> selectResponse-apdu)'.

Comment 23

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

Comment on attachment 8515406 [details] [diff] [review]
0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch

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

Comments in-line in addition to my question comment 22, thank you very much :)

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +57,5 @@
> +   *        The Channel Number/Handle that is successfully opened.
> +   * @param selectResponse
> +   *        'SelectResponse' (GET RESPONSE) result on openening a channel.
> +   */
> +  void onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse);

I'd prefer this follows RIL interface convention. Let's name it "notifyOpenChannelSuccess".

@@ +63,5 @@
> +  /**
> +   * Callback function to notify on successfully closing the logical channel.
> +   *
> +   */
> +  void onCloseChannelDone();

ditto: s/onCloseChannelDone/notifyCloseChannelSuccess/

@@ +72,5 @@
> +   * @param response
> +   *        APDU response containing status bytes and the optional data.
> +   *
> +   */
> +  void onExchangeAPDUResponse(in nsIAPDUResponse response);

ditto: s/onExchangeAPDUResponse/notifyExchangeAPDUResponseSuccess/

@@ +75,5 @@
> +   */
> +  void onExchangeAPDUResponse(in nsIAPDUResponse response);
> +
> +  /**
> +   * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel)

I don't get it ... Why is onError not applicable to iccExchangeAPDU? Then, which callback should be called when iccExchangeAPDU failed?

@@ +78,5 @@
> +  /**
> +   * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel)
> +   *
> +   */
> +  void onError(in AString error);

ditto: s/onError/notifyError/

Comment 24

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #23)
> Comment on attachment 8515406 [details] [diff] [review]
> 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8515406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Comments in-line in addition to my question comment 22, thank you very much
> :)
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +57,5 @@
> > +   *        The Channel Number/Handle that is successfully opened.
> > +   * @param selectResponse
> > +   *        'SelectResponse' (GET RESPONSE) result on openening a channel.
> > +   */
> > +  void onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse);
> 
> I'd prefer this follows RIL interface convention. Let's name it
> "notifyOpenChannelSuccess".
> 
> @@ +63,5 @@
> > +  /**
> > +   * Callback function to notify on successfully closing the logical channel.
> > +   *
> > +   */
> > +  void onCloseChannelDone();
> 
> ditto: s/onCloseChannelDone/notifyCloseChannelSuccess/
> 
> @@ +72,5 @@
> > +   * @param response
> > +   *        APDU response containing status bytes and the optional data.
> > +   *
> > +   */
> > +  void onExchangeAPDUResponse(in nsIAPDUResponse response);
> 
> ditto: s/onExchangeAPDUResponse/notifyExchangeAPDUResponseSuccess/

Sorry. I recalled comment 10. This callback could be called in error cases as well.
so, my suggestion is s/onExchangeAPDUResponse/notifyExchangeAPDUResponse/ :)

> 
> @@ +75,5 @@
> > +   */
> > +  void onExchangeAPDUResponse(in nsIAPDUResponse response);
> > +
> > +  /**
> > +   * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel)
> 
> I don't get it ... Why is onError not applicable to iccExchangeAPDU? Then,
> which callback should be called when iccExchangeAPDU failed?
> 

To be clear, my question is what if modem simply informs the exchange fails through a REQUEST error instead of sw1/sw2?
Are you going to call |onExchangeAPDUResponse| with some manual specified sw1/sw2?

> @@ +78,5 @@
> > +  /**
> > +   * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel)
> > +   *
> > +   */
> > +  void onError(in AString error);
> 
> ditto: s/onError/notifyError/

Comment 25

[Vincent Chang[:vchang][changyihsin]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #22)
> (In reply to Siddartha P from comment #20)
> > Created attachment 8515406 [details] [diff] [review]
> > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> > 
> > Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider
> > this a 'needinfo' on all). I have basically collated all the comments from
> > the bug and uploaded this patch. (However please note that I have now
> > renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) ,
> > etc)
> > 
> > - Please note that TMobile Poland (Wallet developers using SE API) are in
> > need of 'selectResponse' that comes from opening a logical channel
> > successfully.
> > This patch considers this request.
> > 
> 
> Hi Siddartha,
> 
> Thanks for the patch! I am really new to SE so please forgive my trivial
> questions :)
> 
> If I understood the SE WebAPI correctly, there will be another asyn
> operation to retrieve APDUResponse after openChannel. Then why cannot we
> also have the same design in this internal interface? Why do we need to have
> "selectResponse" along with iccOpenChannel callback, especially in AOSP [2]
> I don't see it's possible to retrieve the response in one-step request?
> 
> Another question is, does it always apply that "selectResponse" necessarily
> come with iccOpenChannel, or is it simply specific to TMobile Poland?
> 
Looks like comment 15 has answer the question. So the question would be should we comply with open mobile API?

Comment 26

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8515406 [details] [diff] [review]
0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch

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

Looks good for me. Thank you.

Comment 27

[psiddh]

(In reply to Vincent Chang[:vchang] from comment #25)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #22)
> > (In reply to Siddartha P from comment #20)
> > > Created attachment 8515406 [details] [diff] [review]
> > > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> > > 
> > > Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider
> > > this a 'needinfo' on all). I have basically collated all the comments from
> > > the bug and uploaded this patch. (However please note that I have now
> > > renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) ,
> > > etc)
> > > 
> > > - Please note that TMobile Poland (Wallet developers using SE API) are in
> > > need of 'selectResponse' that comes from opening a logical channel
> > > successfully.
> > > This patch considers this request.
> > > 
> > 
> > Hi Siddartha,
> > 
> > Thanks for the patch! I am really new to SE so please forgive my trivial
> > questions :)
> > 
> > If I understood the SE WebAPI correctly, there will be another asyn
> > operation to retrieve APDUResponse after openChannel. Then why cannot we
> > also have the same design in this internal interface? Why do we need to have
> > "selectResponse" along with iccOpenChannel callback, especially in AOSP [2]
> > I don't see it's possible to retrieve the response in one-step request?
> > 
> > Another question is, does it always apply that "selectResponse" necessarily
> > come with iccOpenChannel, or is it simply specific to TMobile Poland?
> > 
> Looks like comment 15 has answer the question. So the question would be
> should we comply with open mobile API?

As the earlier comments says, the Global platform specification states that every time openLogicalChannel AT command is issued to the baseband, it results in two commands mentioned below:
1) MANAGE (OPEN) CHANNEL : This command's response fetches the 'channel number'
2) GET RESPONSE : This command shall fetch the openChannel's response.

Earlier API exposed ( notifySuccess(long channelId) ), only retrieves the channel number. Hence the change request to the API. Please note that 'select response (response to command 2)' is as per the specification and this is not a custom implementation.

@Vincent, As you said, open mobile API / seek-for-android and W3C system group's SE API proposal expose a high level API to retrieve 'select response'. Our proposal seems to be inline with it. Moreover TMobile Poland (First consumers of SE API) have a definitive use-case to handle 'select response' at Gaia level.

Comment 28

[psiddh]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #23)
> > Comment on attachment 8515406 [details] [diff] [review]
> > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> > 
> > Review of attachment 8515406 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Comments in-line in addition to my question comment 22, thank you very much
> > :)
> > 
> > ::: dom/icc/interfaces/nsIIccProvider.idl
> > @@ +57,5 @@
> > > +   *        The Channel Number/Handle that is successfully opened.
> > > +   * @param selectResponse
> > > +   *        'SelectResponse' (GET RESPONSE) result on openening a channel.
> > > +   */
> > > +  void onOpenChannelDone(in long channel, in nsIAPDUResponse selectResponse);
> > 
> > I'd prefer this follows RIL interface convention. Let's name it
> > "notifyOpenChannelSuccess".
Ok.
> > 
> > @@ +63,5 @@
> > > +  /**
> > > +   * Callback function to notify on successfully closing the logical channel.
> > > +   *
> > > +   */
> > > +  void onCloseChannelDone();
> > 
> > ditto: s/onCloseChannelDone/notifyCloseChannelSuccess/
Ok
> > 
> > @@ +72,5 @@
> > > +   * @param response
> > > +   *        APDU response containing status bytes and the optional data.
> > > +   *
> > > +   */
> > > +  void onExchangeAPDUResponse(in nsIAPDUResponse response);
> > 
> > ditto: s/onExchangeAPDUResponse/notifyExchangeAPDUResponseSuccess/
Ok
> 
> Sorry. I recalled comment 10. This callback could be called in error cases
> as well.
> so, my suggestion is s/onExchangeAPDUResponse/notifyExchangeAPDUResponse/ :)
> 
> > 
> > @@ +75,5 @@
> > > +   */
> > > +  void onExchangeAPDUResponse(in nsIAPDUResponse response);
> > > +
> > > +  /**
> > > +   * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel)
> > 
> > I don't get it ... Why is onError not applicable to iccExchangeAPDU? Then,
> > which callback should be called when iccExchangeAPDU failed?
> > 
> 
> To be clear, my question is what if modem simply informs the exchange fails
> through a REQUEST error instead of sw1/sw2?
> Are you going to call |onExchangeAPDUResponse| with some manual specified
> sw1/sw2?
> 
This callback 'onExchangeAPDUResponse' shall be called for both success & failure scenarios for the given IccChannel command iccExchangeAPDU(..). Following are some of the examples of sw1 , sw2 scenarios

SW1     SW2           DESCP
90	00	Command successfully executed (OK).
92	10      Insufficient memory. No more storage available.
97	89      Service not available
99	00      PIN try left
6A	82      File not found
68	81      Logical channel not supported
.
.
.
.
As you may noted, the above list is exhaustive and combination of sw1 & sw2 values determine if it is  a success or failure for a given APDU command. SE stack relays this information (sw1 & sw2) all the way to the Gaia Apps.

> > @@ +78,5 @@
> > > +  /**
> > > +   * Callback function to notify error (Applicable only to iccOpenChannel & iccCloseChannel)
> > > +   *
> > > +   */
> > > +  void onError(in AString error);
> > 
> > ditto: s/onError/notifyError/
Ok

Comment 29

[psiddh]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #22)
> (In reply to Siddartha P from comment #20)
> > Created attachment 8515406 [details] [diff] [review]
> > 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> > 
> > Moving this bug forward. Add a 'needInfo' on a few. (Infact please consider
> > this a 'needinfo' on all). I have basically collated all the comments from
> > the bug and uploaded this patch. (However please note that I have now
> > renamed callbacks to 'onIccOpenChannelDone' , etc.. from notifySuccess(..) ,
> > etc)
> > 
> > - Please note that TMobile Poland (Wallet developers using SE API) are in
> > need of 'selectResponse' that comes from opening a logical channel
> > successfully.
> > This patch considers this request.
> > 
> 
> Hi Siddartha,
> 
> Thanks for the patch! I am really new to SE so please forgive my trivial
> questions :)
> 
No, Thank You for your time and feedback.

> If I understood the SE WebAPI correctly, there will be another asyn
> operation to retrieve APDUResponse after openChannel. Then why cannot we
> also have the same design in this internal interface? Why do we need to have
> "selectResponse" along with iccOpenChannel callback, especially in AOSP [2]
> I don't see it's possible to retrieve the response in one-step request?
> 
Yes you are right. This is not a one-step request!
Consider 'GET RESPONSE' (Select response a.k.a openResponse) is simply another APDU command that could be sent by the SE stack using iccExchangeAPDU(..) . Its response could be simply interpreted as 'selectResponse'.
But since nsIIccProvider.idl is an internal contract , (and one level above ril contract - ril.h), it'd be nicer (from the users of iccChannel APIs point of view) if the contract supports both (channel id & selectResponse) in single callback. Hope I have addressed your concern.

> Another question is, does it always apply that "selectResponse" necessarily
> come with iccOpenChannel, or is it simply specific to TMobile Poland?
> 
This is in line with the specifications and not specific to TMobile Poland!
> 
> [1] SE API current draft
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=879861&attachment=8491514
> [2]
> https://github.com/android/platform_hardware_ril/blob/master/include/
> telephony/ril.h
>

Comment 30

[psiddh]

Attachment: (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch

Uploading the idl's second version!
Difference between this version and earlier is renaming of callback functions from
'onIccxxxx' to 'notifyIccxxx'.
Thanks Anshul and Vincent for your quick feedback. I assume that this change should be ok to you guys.

Comment 31

[psiddh]

Hi Vincent , HsinYi and All,

Once idl is locked down, I assume that we would also have to make changes to dom/icc , RILContentHelper & RadioInterfaceLayer on Mozilla RIL side of things in order for these components to comply with the latest idl changes.

Comment 32

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

Comment on attachment 8516917 [details] [diff] [review]
(v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch

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

hi Sid,

Thanks for the explanation that sounds reasonable to me. :)

Just some minor comments for convention.

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +19,5 @@
>  
> +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)]
> +interface nsIAPDUCommand : nsISupports
> +{
> +  // 1 Byte : Class Byte

Let's follow the convention we used for RIL. Comment applies here and below.

e.g. 
/**
  * 1 Byte: Instrunction Byte
  */

@@ +20,5 @@
> +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)]
> +interface nsIAPDUCommand : nsISupports
> +{
> +  // 1 Byte : Class Byte
> +  readonly attribute octet cla;

Please add an extra empty line. Comment applies here and below.

Comment 33

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

(In reply to Siddartha P from comment #31)
> Hi Vincent , HsinYi and All,
> 
> Once idl is locked down, I assume that we would also have to make changes to
> dom/icc , RILContentHelper & RadioInterfaceLayer on Mozilla RIL side of
> things in order for these components to comply with the latest idl changes.

Hi Sid,

As we chat on IRC, you need to have new files for IccChannelCallback and APDUCommand. Changes to RILContentHelper and RadioInterfaceLayer are required. In addition, changes to ril_worker.js are needed for 1) correct data format in [1], and 2) to support 'selectResponse' coming along with iccOpenChannel 

Let me know if it isn't clear enough. :)

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#1481

Comment 34

[Vincent Chang[:vchang][changyihsin]]

Sure, you are going to continue that patch, right? To speed up the SE development, I think QC can start to develop new QC Ril once interface is locked down and get r+.

Comment 35

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

Comment on attachment 8516917 [details] [diff] [review]
(v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch

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

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +32,5 @@
> +  readonly attribute octet p3;
> +  // bytes of data
> +  readonly attribute AString data;
> +  // The length of the expected response data or -1 if none is expected
> +  readonly attribute short le;

Hi Sid, 
I have a question about "le". I don't see this attribute is defined in AOSP ril.h [1]. Can you explain how it is going to be used? I am wondering if this is needed in this interface. Thank you!

[1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L400

Comment 36

[Ming Yin]

http://en.wikipedia.org/wiki/Smart_card_application_protocol_data_unit
According to ISO/IEC 7816-4, there are 4 cases of APDU commands.

Command APDU:

Field name 	Length (bytes) 	Description
-----------------------------------------------------------------------------------------------------
CLA 	         1 	        Instruction class - indicates the type of command
INS 	         1      	Instruction code - indicates the specific command, e.g. "write data"
P1-P2 	         2 	        Instruction parameters for the command, e.g. offset into file at which to write the data
Lc 	         0, 1 or 3 	Encodes the number (Nc) of bytes of command data to follow
Data 	         N              N bytes of data
Le 	         0,1,2 or 3 	Encodes the maximum number (Ne) of response bytes expected

Case 1 - Command:  CLA|INS|P1|P2
(Case 1 is a simple command without command data and don't expect response data.)

Case 2 - Command:  CLA|INS|P1|P2|Le
(In Case 2, the command has no command data, but expected response data.)

Case 3 - Command:  CLA|INS|P1|P2|Lc|Data
(Case 3 describes a command with command data, the expected no response data)

Case 4 - Command:  CLA|INS|P1|P2|Lc|Data|Le
(A Case 4 command has both command and response data)

All these 4 cases could be required by NFC payments.

Comment 37

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

Comment on attachment 8516917 [details] [diff] [review]
(v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch

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

I think Hsinyi's question is if the instruction byte is SELECT_FILE or SELECT, which lc and le both could have values.
How does Android RIL address this since it only has 1 byte (p3) to do this.

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +28,5 @@
> +  readonly attribute octet p1;
> +  // 1 Byte : Second Octet of Parameters Byte
> +  readonly attribute octet p2;
> +  // 1 Byte : Third Octet of Parameters Byte
> +  readonly attribute octet p3;

Should be lc

@@ +30,5 @@
> +  readonly attribute octet p2;
> +  // 1 Byte : Third Octet of Parameters Byte
> +  readonly attribute octet p3;
> +  // bytes of data
> +  readonly attribute AString data;

Should be some byte array. 
Each AString stores 2 bytes.

Comment 38

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

Thanks Ming for the information.

If my understanding is correct, I wonder if it sounds more reasonable to replace "octet p3" with "short le" which indicates the length of the command data? Having "p3" here along with "le" really confuses. 

And do we really need to use "-1" to indicate the case that no response data is expected? Can't we just use 0 for that?

Comment 39

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

(In reply to Yoshi Huang[:allstars.chh] from comment #37)
> Comment on attachment 8516917 [details] [diff] [review]
> (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8516917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think Hsinyi's question is if the instruction byte is SELECT_FILE or
> SELECT, which lc and le both could have values.
> How does Android RIL address this since it only has 1 byte (p3) to do this.
> 

Thank you, Yoshi! This is what I tried to ask!

Comment 40

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #38)
> Thanks Ming for the information.
> 
> If my understanding is correct, I wonder if it sounds more reasonable to
> replace "octet p3" with "short le"
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ typo, should be |replace "octet p3" with short *lc*|


> which indicates the length of the command
> data? Having "p3" here along with "le" really confuses. 
> 
> And do we really need to use "-1" to indicate the case that no response data
> is expected? Can't we just use 0 for that?

Comment 41

[psiddh]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Comment on attachment 8516917 [details] [diff] [review]
> (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8516917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +32,5 @@
> > +  readonly attribute octet p3;
> > +  // bytes of data
> > +  readonly attribute AString data;
> > +  // The length of the expected response data or -1 if none is expected
> > +  readonly attribute short le;
> 
> Hi Sid, 
> I have a question about "le". I don't see this attribute is defined in AOSP
> ril.h [1]. Can you explain how it is going to be used? I am wondering if
> this is needed in this interface. Thank you!
> 
> [1]
> https://github.com/android/platform_hardware_ril/blob/master/include/
> telephony/ril.h#L400

Hi HsinYi,

Here are my thoughts on this one.
APDU command is header + body . At the lowest level when reader sends APDU commands to the smart card, here is how it will be interpreted
CLA, INS (command) , p1, p2 : header  [Note this is as per T1


Thanks Sid(In reply to Hsin-Yi Tsai [:hsinyi] from comment #32)
> Comment on attachment 8516917 [details] [diff] [review]
> (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8516917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hi Sid,
> 
> Thanks for the explanation that sounds reasonable to me. :)
> 
> Just some minor comments for convention.
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +19,5 @@
> >  
> > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)]
> > +interface nsIAPDUCommand : nsISupports
> > +{
> > +  // 1 Byte : Class Byte
> 
> Let's follow the convention we used for RIL. Comment applies here and below.
> 
> e.g. 
> /**
>   * 1 Byte: Instrunction Byte
>   */
> 
> @@ +20,5 @@
> > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)]
> > +interface nsIAPDUCommand : nsISupports
> > +{
> > +  // 1 Byte : Class Byte
> > +  readonly attribute octet cla;
> 
> Please add an extra empty line. Comment applies here and below.

Btw , I noted these comments

Comment 42

[psiddh]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Comment on attachment 8516917 [details] [diff] [review]
> (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8516917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +32,5 @@
> > +  readonly attribute octet p3;
> > +  // bytes of data
> > +  readonly attribute AString data;
> > +  // The length of the expected response data or -1 if none is expected
> > +  readonly attribute short le;
> 
> Hi Sid, 
> I have a question about "le". I don't see this attribute is defined in AOSP
> ril.h [1]. Can you explain how it is going to be used? I am wondering if
> this is needed in this interface. Thank you!
> 
> [1]
> https://github.com/android/platform_hardware_ril/blob/master/include/
> telephony/ril.h#L400

Hi HsinYi,

Here are my thoughts on this one.
APDU command is header + body . At the lowest level when reader sends APDU commands to the smart card, here is how it will be interpreted
CLA, INS (command) , p1, p2 : header  [Note this is as per T1


Thanks Sid(In reply to Hsin-Yi Tsai [:hsinyi] from comment #32)
> Comment on attachment 8516917 [details] [diff] [review]
> (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8516917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hi Sid,
> 
> Thanks for the explanation that sounds reasonable to me. :)
> 
> Just some minor comments for convention.
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +19,5 @@
> >  
> > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)]
> > +interface nsIAPDUCommand : nsISupports
> > +{
> > +  // 1 Byte : Class Byte
> 
> Let's follow the convention we used for RIL. Comment applies here and below.
> 
> e.g. 
> /**
>   * 1 Byte: Instrunction Byte
>   */
> 
> @@ +20,5 @@
> > +[scriptable, uuid(7a93578d-b389-4bec-9ea3-5ee4cf048710)]
> > +interface nsIAPDUCommand : nsISupports
> > +{
> > +  // 1 Byte : Class Byte
> > +  readonly attribute octet cla;
> 
> Please add an extra empty line. Comment applies here and below.

Btw , I noted these comments

Comment 43

[psiddh]

(In reply to Vincent Chang[:vchang] from comment #34)
> Sure, you are going to continue that patch, right? To speed up the SE
> development, I think QC can start to develop new QC Ril once interface is
> locked down and get r+.

Sure Vincent , I have noted your comment as well. I am going to post my update very soon on this. I would need RIL experts opinion followed by Qualcomm's. Thanks

Comment 44

[psiddh]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Comment on attachment 8516917 [details] [diff] [review]
> (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8516917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +32,5 @@
> > +  readonly attribute octet p3;
> > +  // bytes of data
> > +  readonly attribute AString data;
> > +  // The length of the expected response data or -1 if none is expected
> > +  readonly attribute short le;
> 
> Hi Sid, 
> I have a question about "le". I don't see this attribute is defined in AOSP
> ril.h [1]. Can you explain how it is going to be used? I am wondering if
> this is needed in this interface. Thank you!
> 
> [1]
> https://github.com/android/platform_hardware_ril/blob/master/include/
> telephony/ril.h#L400

Hi HsinYi, I have started replying to your questions in my earlier comments. But I submitted the reply too soon. So correcting my response here for your comment.
------------------------------------------------------------------

I must confess that the usage of 'Le' is not super clear / obvious.


According to ISO 7816 standards, there are different transmission protocols such T=0, T=1 etc etc.
According to T=1 protocol, here is how APDU command is interpreted at IC card level (smart card)

HEADER : CLA, INS , P1, P2 (Note that there is no P3 here). P3 can be also reffered to as 'Lc' for historical reasons, I guess.
BODY   : P3, data & Le

FYI, according to T=0 protocol, HEADER comprised of 'P3' as well (Header in T= 0, CLA, INS , P1, P2, P3)
But from T=1, the first byte of body is 'P3'. This way it was upward compatible within different 


Last byte of the body is 'Le'. (in our case, last byte of of 'data' field in nsIAPDUCommand)
i;e; if 'n' is the length of data, data[n] will contain Le.

To the smart card, apdu will be dispatched as bytes[] in the end. At the smart card side of things, it does the following marshalling
Read 'header' : CLA + INS + P1 + P2
Now for the body :  Body Length (len) = 1 (Lc) + N (num of bytes of 'data') + 1 (Le);
Read the first byte which is length of data.
body[0..'len -1'] is what will be sent as 'data' in Android RIL as well as in FxOS RIL. To make it explicit from vendor RIL , the presence of extra byte (Le), hence the added in nsIApduCommand interface)


Following are valid APDU commands

Case 1 - 
Only Header : CLA, INS, P1, P2

Case 2 -
Header & Body (w/o Le)
CLA, INS, P1, P2  ||  Lc , data
Lc is encoded on the fifth byte of the T=0 T-APDU

Case 3 -
Header & Only Le in body
CLA, INS, P1, P2 || Le

In this case as you may have noted,  Le is encoded on the fifth byte of T-APDU,

Case 4 -  
Header , Data with P3 (Lc) & Le
CLA, INS, P1, P2 || Lc, Data, Le

So in my opinion 'Le' is part of 'data' and is appended.
Adding the explicit Le value, say Le=n bytes  to the APDU perhaps needed, if the user of trasmit API expect any output / response bytes.

Disclaimer: I am no expert in this area, but from whatever I have read thus far, I am sharing some of my thoughts :)

Comment 45

[psiddh]

(In reply to Yoshi Huang[:allstars.chh] from comment #37)
> Comment on attachment 8516917 [details] [diff] [review]
> (v 1.2) 0001-Bug-1081789-Ril-Redefine-IccOpenChannel-IccCloseChan.patch
> 
> Review of attachment 8516917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think Hsinyi's question is if the instruction byte is SELECT_FILE or
> SELECT, which lc and le both could have values.
> How does Android RIL address this since it only has 1 byte (p3) to do this.
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +28,5 @@
> > +  readonly attribute octet p1;
> > +  // 1 Byte : Second Octet of Parameters Byte
> > +  readonly attribute octet p2;
> > +  // 1 Byte : Third Octet of Parameters Byte
> > +  readonly attribute octet p3;
> 
> Should be lc
Ok. I think either 'Lc' or 'P3' have the same significance given the smart card spec history.
That said, I will rename it to 'Lc' in the next iteration!
> 
> @@ +30,5 @@
> > +  readonly attribute octet p2;
> > +  // 1 Byte : Third Octet of Parameters Byte
> > +  readonly attribute octet p3;
> > +  // bytes of data
> > +  readonly attribute AString data;
> 
> Should be some byte array. 
> Each AString stores 2 bytes.
Oh! Yes we would need a byte array. I will fix this as well in the next iteration!

Comment 46

[psiddh]

All,

Quick update from my side. I have been in the process of making few changes in Moz-RIL side of things and I think, I have managed to successfully communicate with already released version of Qualcomm build. 
So my pitch at this point (Proposal 2) that we may not need another Qualcomm release as it may be a time consuming process (because of release procedure between Qualcomm <-> T2M <-> DT ).

So what are my changes ?

1. First apply these changes https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 that are already landed in master, I suppose. These are gecko/dom/icc related changes which is not needed as we are exposing any 'channel apis' to content through webIDL

2. Modified RILContentHelper.js for everyone's feedback. Please note that this is not a final code to be submitted and only demonstration of how RILContentHelper is to be modified to make things work.
<< Refer to patch - proposal1_RILContentHellper.patch >>

3. Modified nsIIccProvider.idl to redefine the 3 APIs (Remove the 'window' argument so that it is accessible to other modules namely secure element parent process)
<< Refer to patch - proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch >>

With the above changes (Lets call them Proposal 2) SE stack (parent process) is able to access all the three APIs and communicate with SIM and get the responses!

Benefits of Proposal 2
- No extra build release needed from T2M as this can be time consuming process (in the order of 3 - 4 weeks at-least after we land RIL changes). This will push the initial SE stack landing around mid-dec further.

- Minimal set of changes and everything work as-is

- Since we are not modifying anything other RILContentHelper, we do not need another release from Qualcomm.

Disadvantages of Proposal 2
- The APIs in nsIIccProvider.idl still use 'jsval' as against proper XPCOM objs such as 'nsIAPDUCommand' & 'nsIAPDUResponse' as identified. (In my opinion, this should be ok for now. Requesting Anshul to chime in and voice his opinion)

- Select Response will be a two step process. (Again in my opinion, this is not a deal breaker as we have already validated the stack and noted that Qualcomm baseband already supports GET RESPONSE. This will just be another extra call in SE parent process)

Future work of Proposal 2
- We can migrate / port these APIs along with other RILContentHelper functions from 'jsval' to proper XPCOM objs. If needed, DT can still own this task for three identified channel interfaces in future as well.


Proposal 1 (Original one with proper XPCOM objs + new Release from Qualcomm through T2M) can delay the deliverables overall.

Doing a needinfo on few, but please treat this as needInfo on all. Adding Edgar to the discussion as well as I was in really helpful discussions with him on IRC over the last week.

Comment 47

[psiddh]

Attachment: proposal1_RILContentHellper.patch

Comment 48

[psiddh]

Attachment: proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch

Comment 49

[psiddh]

For the completion sake, I was able to access the APIs and receive the callbacks from SecureElement Parent Process using the following code snippet. Sharing it for everyone's feedback

  _openChannel: function(data, callback) {
    let clientId = 1;
    let status = FAILURE;
    // TBD: Validate the AID 'data.aid' with ACE before opening a channel!
    iccProvider.iccOpenChannel(clientId, data.aid , {
      notifyOpenChannelSuccess: function(channel) {
        let token = null;
	if (((this.cardState === CARDSTATE_READY)) && (data.type === 'uicc')) {
          let token = UUIDGenerator.generateUUID().toString();
	  status = gSEMessageManager._addChannelToSession(channel, token, data);
	}
	if (callback) {
	  callback({ status: status,
                     token : token });
	}
      },

      notifyCloseChannelSuccess: function() {},

      notifyExchangeAPDUResponse: function(response) {},

      notifyError: function(error) {
        callback({ status: status, error: error });
      }
    });
  },

Comment 50

[psiddh]

It looks like I have mixed up names of the patches.
I meant "proposal2_RILContentHellper.patch" instead of 'proposal1_RILContentHellper.patch' and
"proposal2_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch" instead of "proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch"

Comment 51

[psiddh]

It looks like I have mixed up names of the patches.
I meant "proposal2_RILContentHellper.patch" instead of 'proposal1_RILContentHellper.patch' and
"proposal2_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch" instead of "proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch"

Comment 52

[Vincent Chang[:vchang][changyihsin]]

Hi Sid, thanks for your hard work. 
One question about nsIIccProvider.idl change, my understanding is that QCM ril implements nsIIccProvider.idl interface, I am wondering how can nsIIccProvider.idl APIs interface change works without modification of QCM ril?

Comment 53

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

Hello Sid,

Thanks for the proposal. I am willing to support a way to help avoid any extra comril release. As a result, "Select Response will be a two step process" is acceptable to me. 

However, as we talked on irc, let's try our best to get rid of 'jsval.' We also don't really need the interfaces of nsIAPDUCommand and nsIAPDUResponse, while with these two new interfaces, we just create tedious steps of un-wrapping a object to retrieve some parameters and wrapping those values. Instead, we could simply spread those attributes. For example, 

nsIIccChannelCallback.notifyExchangeAPDUResponse(sw1, sw2, data);

Comment 54

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

Comment on attachment 8520384 [details] [diff] [review]
proposal1_0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel-.patch

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

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +40,5 @@
> +   * @param response
> +   *        APDU response containing status bytes and the optional data.
> +   *
> +   */
> +  void notifyExchangeAPDUResponse(in jsval response);

let's just spread arguments up:

notifyExchangeAPDUResponse(in octet sw1, in octet sw2, in unsigned long length, [array, size_is(length)] in octet data);

@@ +128,5 @@
> +                      in DOMString aid,
> +                      in nsIIccChannelCallback callback);
> +  void iccExchangeAPDU(in unsigned long clientId,
> +                       in long channel,
> +                       in jsval apdu,

ditto: spread "apdu"

this argument list is a bit longer but I am fine with it, especially considering repeated wrap-unwrap steps.

Comment 55

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

Comment on attachment 8520383 [details] [diff] [review]
proposal1_RILContentHellper.patch

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

The overall direction looks right :)

::: dom/system/gonk/RILContentHelper.js
@@ +1060,4 @@
>      });
>    },
>  
> +  iccOpenChannel: function(clientId, aid, callbackInterface) {

s/callbackInterface/callback

@@ +1074,3 @@
>    },
>  
> +  iccExchangeAPDU: function(clientId, channel, apdu, callbackInterface) {

ditto.

@@ +1089,3 @@
>    },
>  
> +  iccCloseChannel: function(clientId, channel, callbackInterface) {

ditto.

@@ +1517,4 @@
>      }
>    },
>  
> +  addIccChannelCallback: function(requestId, channelCb) {

Please rename it _addIccChannelCallback. The _ prefix means it's a private or internal member.

@@ +1526,5 @@
> +
> +    if (DEBUG) debug("Unable to add channelCbInterface for requestId : " + requestId);
> +  },
> +
> +  getIccChannelCallback: function(requestId) {

ditto.

@@ +1932,5 @@
> +  handleIccOpenChannel: function(message) {
> +    let requestId = message.requestId;
> +    let channel   = message.channel;
> +
> +    let callbackInterface = this.getIccChannelCallback(requestId);

We could just call it "callback."

@@ +1941,5 @@
> +  },
> +
> +  handleIccCloseChannel: function(message) {
> +    let requestId = message.requestId;
> +    let callbackInterface = this.getIccChannelCallback(requestId);

ditto.

@@ +1954,5 @@
> +    let requestId = message.requestId;
> +    let callbackInterface = this.getIccChannelCallback(requestId);
> +
> +    if (callbackInterface) {
> +      let result = [message.sw1, message.sw2, message.simResponse, message.errorMsg];

Wondering if message.errorMsg is actually needed. Per my understanding of comment 28, combination of sw1 and sw2 is able to provide error messages.

Comment 56

[Anshul]

Sid and Hsin-Yi, I am fine with the Sid's proposal #2 for now. Perhaps the cleanup of JSVALs can be done as part of moving the ICC APIs to IPDL in bug 864489.

Comment 57

[psiddh]

Attachment: (v1.1) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

Hi HsinYi,

I think I have addressed all your comments in this patch for your review.

I have one question though. For this patch to compile and function as expected,
https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 , we would need this change as well. (This is already landed on the main line :Bug#1030602). Since for SE stack development, we are based off Qualcomm's release on v2.1 branch.
So two things should happen
1) On REL 2.1 branch, Bug#1030602 change should be landed along with the current changes / patch of redefining the interfaces.
2) The current patch should land on the mainline.

Any suggestions?

Comment 58

[psiddh]

(In reply to Anshul from comment #56)
> Sid and Hsin-Yi, I am fine with the Sid's proposal #2 for now. Perhaps the
> cleanup of JSVALs can be done as part of moving the ICC APIs to IPDL in bug
> 864489.

Thanks Anshul

Comment 59

[psiddh]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #55)
> Comment on attachment 8520383 [details] [diff] [review]
> proposal1_RILContentHellper.patch
> 
> Review of attachment 8520383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The overall direction looks right :)
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1060,4 @@
> >      });
> >    },
> >  
> > +  iccOpenChannel: function(clientId, aid, callbackInterface) {
> 
> s/callbackInterface/callback
> 
> @@ +1074,3 @@
> >    },
> >  
> > +  iccExchangeAPDU: function(clientId, channel, apdu, callbackInterface) {
> 
> ditto.
> 
> @@ +1089,3 @@
> >    },
> >  
> > +  iccCloseChannel: function(clientId, channel, callbackInterface) {
> 
> ditto.
> 
> @@ +1517,4 @@
> >      }
> >    },
> >  
> > +  addIccChannelCallback: function(requestId, channelCb) {
> 
> Please rename it _addIccChannelCallback. The _ prefix means it's a private
> or internal member.
> 
> @@ +1526,5 @@
> > +
> > +    if (DEBUG) debug("Unable to add channelCbInterface for requestId : " + requestId);
> > +  },
> > +
> > +  getIccChannelCallback: function(requestId) {
> 
> ditto.
> 
> @@ +1932,5 @@
> > +  handleIccOpenChannel: function(message) {
> > +    let requestId = message.requestId;
> > +    let channel   = message.channel;
> > +
> > +    let callbackInterface = this.getIccChannelCallback(requestId);
> 
> We could just call it "callback."
> 
> @@ +1941,5 @@
> > +  },
> > +
> > +  handleIccCloseChannel: function(message) {
> > +    let requestId = message.requestId;
> > +    let callbackInterface = this.getIccChannelCallback(requestId);
> 
> ditto.
> 
> @@ +1954,5 @@
> > +    let requestId = message.requestId;
> > +    let callbackInterface = this.getIccChannelCallback(requestId);
> > +
> > +    if (callbackInterface) {
> > +      let result = [message.sw1, message.sw2, message.simResponse, message.errorMsg];
> 
> Wondering if message.errorMsg is actually needed. Per my understanding of
> comment 28, combination of sw1 and sw2 is able to provide error messages.

It looks like even in failed transmit APDU scenarios, (say sw1 & sw2 is 6x 00 and not 9x 00 - which is a success ), RILContentHelper receives these values with 'errorMsg being null'.
So I am yet to see a transmit apdu usecase with an errorMsg. But that said, since this value is being sent by lower layers, I made use of it just as how it was (just in case).

Comment 60

[Krzysztof Mioduszewski[:tauzen]]

Hi Sid, since we don't have Le in iccEchangeAPDU, I don't see any way to send GET RESPONSE [1] or READ BINARY [2] commands with number of response bytes specified. I still can get the whole response if I just set Lc to 0. Am I missing something here? I think that requesting specific number of bytes in the response should be also supported.

[1] http://www.cardwerk.com/smartcards/smartcard_standard_ISO7816-4_7_transmission_interindustry_commands.aspx#chap7_1
[2] http://www.cardwerk.com/smartcards/smartcard_standard_ISO7816-4_6_basic_interindustry_commands.aspx#chap6_1

Comment 61

[psiddh]

Hi Krzysztof,

Here are my thoughts!

As you may have noted, CLA+INS+P1+P2 make up the header.
For the body part, Its generally Lc , Lc Bytes and / of Le

The meaning of P3 could be either 'Lc/Le' depending on which case we are dealing with coupled with Transmission protocol (T=0 or T=1).

> I still can get the whole response if I just set Lc to 0
I think, firstly we should fix my patch, and rename Lc to simply 'P3' to avoid all confusion.
This use-case of yours is interpreted as :
 Case 3 - Header & Only Le in body
 CLA, INS, P1, P2 || Le

Since you have set Lc to zero, it is the last byte of body. So at the lowest level when terminal (baseband) is interacting with UICC, it will be interpreted as 'P3 = Le'. As I said, I will simply rename the iccExchaneAPDU arg from 'Lc' to 'P3' to avoid confusion and be in-consistent with the spec. High Level SE APIs can have both Lc and Le though.

Now coming back to other case where, you want to specify both Lc and Le (Case 4, as stated above).
a) Application Layer (C-APDU)  : CLA INS P1 P2 Lc [Lc bytes of DATA] Le
  - The C-APDU is mapped onto the C-TPDU by cutting off the last byte (Le) of the body.
b) Transport layer (C-TPDU)  : CLA INS P1 P2 P3=Lc [Lc bytes of DATA]
c) Terminal will receive SW1 and SW2
d) Upon receipt if values are (9x 00) , then
   Terminal issues [Get Response, P3 = Le ] to the UICC again
e) This response, R_APDU : resp bytes, SW1, SW2 will be delivered to SE parent process eventually

What this means , in short is, if you want to specify 'Le' bytes explicitly, make sure it is the last byte of body (data).

Comment 62

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

changing feature flag to 2.2? since we are still clarifying how we can verify/test this patch for 2.2.

Comment 63

[Krzysztof Mioduszewski[:tauzen]]

(In reply to Siddartha P from comment #61)
> I think, firstly we should fix my patch, and rename Lc to simply 'P3' to
> avoid all confusion.
I agree to to that. We also need to modify data argument attributes in iccExchangeAPDU. Right now we have                        [array, size_is(lc)], this does not allow me to set data to null and Lc/P3 to the expected response length and I'm getting the following error:
>JavaScript Array does not have as many elements as indicated by size argument arg 7 [nsIIccProvider.iccExchangeAPDU
So data should be nullable and independent of Lc/P3 value. At least in the idl, we could do APDU validation later on in the code. 

If I set p3 to expected response length and data to null directly in RILContentHelper the response is ok.

Comment 64

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

Comment on attachment 8523376 [details] [diff] [review]
(v1.1) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

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

Hi Sid,
.idl part looks really good though comment 61 is a valid concern and a revision is required.

I believe the approach for either m-c or 2.1 branch is almost the same, except some rebasing effort. I am more than happy to see this landing on m-c as long as we test the patch as we usually do. Thank you :)

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +141,5 @@
> +                       in octet cla,
> +                       in octet ins,
> +                       in octet p1,
> +                       in octet p2,
> +                       in unsigned long lc,

Agree with comment 61. Let's rename it "p3" to avoid confusion.

::: dom/system/gonk/RILContentHelper.js
@@ +1074,3 @@
>    },
>  
> +  iccExchangeAPDU: function(clientId, channel, cla, ins, p1, p2, lc, data, callback) {

s/lc/p3

@@ +1082,5 @@
> +      cla: cla,
> +      command: ins,
> +      p1: p1,
> +      p2: p2,
> +      p3: lc,

s/lc/p3/

@@ +1536,5 @@
> +
> +    if (DEBUG) debug("Unable to add channelCbInterface for requestId : " + requestId);
> +  },
> +
> +  _getIccChannelCallback: function(requestId) {

We should also delete the callback when the request is done.

@@ +1685,5 @@
> +
> +    return bytes;
> +  },
> +
> +  _byte2hexString: function(array) {

s/_byte2hexString/_byteToHexString/

@@ +1690,5 @@
> +    let hexString = "";
> +    let hex;
> +
> +    for (let i = 0; i < array.length; i++) {
> +      hex = array[i].toString(16).toUpperCase();

Declare a variable when it's really needed, so let's declare "hex" here.

@@ +1970,5 @@
> +    let channel   = message.channel;
> +
> +    let callback = this._getIccChannelCallback(requestId);
> +    if (callback) {
> +      let notify = (!message.errorMsg) ? callback.notifyOpenChannelSuccess(channel) :

nit: parenthesis around message.errorMsg not necessary

@@ +1980,5 @@
> +    let requestId = message.requestId;
> +    let callback = this._getIccChannelCallback(requestId);
> +
> +    if (callback) {
> +      let notify = (!message.errorMsg) ? callback.notifyCloseChannelSuccess() :

ditto

@@ +1989,3 @@
>    handleIccExchangeAPDU: function(message) {
> +    let requestId = message.requestId;
> +    let callback = this._getIccChannelCallback(requestId);

Bail out earlier, please do:

if (!callback) {
  return;
}

@@ +1989,5 @@
>    handleIccExchangeAPDU: function(message) {
> +    let requestId = message.requestId;
> +    let callback = this._getIccChannelCallback(requestId);
> +
> +    let responseBytes = (message.simResponse !== null && message.simResponse.length > 0) ?

wrap at 80th character

@@ +1991,5 @@
> +    let callback = this._getIccChannelCallback(requestId);
> +
> +    let responseBytes = (message.simResponse !== null && message.simResponse.length > 0) ?
> +                          this._hexStringToBytes(message.simResponse) : null;
> +    let responseLength = (responseBytes !== null ) ? responseBytes.length : 0;

It looks we check the very similar thing repeatedly.
How about this:

let responseBytes = null;
let responseLength = 0;
if (message.simResponse) { // 
  responseBytes = this._hexStringToBytes(message.simResponse);
  responseLength = responseBytes.length;
}

@@ +1992,5 @@
> +
> +    let responseBytes = (message.simResponse !== null && message.simResponse.length > 0) ?
> +                          this._hexStringToBytes(message.simResponse) : null;
> +    let responseLength = (responseBytes !== null ) ? responseBytes.length : 0;
> +    if (callback) {

As we bail out earlier, we don't need this if-sentence.

Comment 65

[psiddh]

Attachment: (v1.2) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

Hi Hsinyi,
Thanks for your comments. I have addressed all of them in v1.2 patch.

However I also made another important change to the idl (specifically to one of iccExchangeAPDU arguments, changed the argument to 'in DOMString data,' instead of '[array, size_is(lc)] in octet data'. ) 

"[array, size_is(lc)] in octet data" : This may have multiple issues.
Since we are locking the size of 'data' to 'lc' (now 'P3' in the current patch), we may not be able to support usecases like
--> lc is set, data (lc bytes) is present and le is also present
Lets says lc = 0x04, 'data' will contain 4 bytes. There is no way to send 'le' with the old proposed API. 
--> Now another usecase, consider only Le is present (say 0x02). Effectively 'p3' is 'Le' and data will be not set or say 'null'. Again we may have issues
I think there are few such cases that old proposed API may not be extensible.

With the current one, I just changed it to 'string' and therefore not worry about any specific length. Anyways Qualcomm RIL expects , a string only. So there is no need to convert in RILConetenHelper using byte2HexString() helpers.

-------------------------------------------------------------------------
There is also another way of supporting all the 'le' and 'lc' usecases if we change the iccExchangeAPDU() args in the following way
...
...
in unsigned long lc,
[array, size_is(lc)] in octet data,
in unsigned short le,
callback);

This ensures that we send all the required info to RILContentHelper and the onus is on RILContentHelper to compute 'P3' roughly something in this order:

+  iccExchangeAPDU: function(clientId, channel, cla, ins, p1, p2, lc, data, le, callback) {
+    let requestId = UUIDGenerator.generateUUID().toString();
+    this._addIccChannelCallback(requestId, callback);
+    let apdu = "";
+    let p3 = 0;
+
+    // First check if lc is set and data is present
+    if (lc > 0 && !!data) {
+      p3 = lc;
+      apdu = this._byte2hexString(data);
+    }
+
+    // check if le is set
+    if (le !== -1) {
+      // If 'p3' already set, then append 'le' to 'apdu'
+      if (p3 > 0) {
+        apdu += this._byte2hexString(le);
+      } else {  // p3 is 'le' in case 'lc' is not set
+        p3 = le;
+        apdu = null;
+      }
+    }
+
+    if (DEBUG) debug('P3 : ' + p3 + ', apdu: ' + apdu);
+    let apdu = {
+      cla: cla,
+      command: ins,
+      p1: p1,
+      p2: p2,
+      p3: p3,
+      data: apdu
+    };
+
+    //Potentially you need serialization here and can't pass the jsval through
+    cpmm.sendAsyncMessage("RIL:IccExchangeAPDU", {
+      clientId: clientId,
+      data: {
+        requestId: requestId,
+        channel: channel,
+        apdu: apdu
+      }
+    });
+  }, 

I simply chose 'DOMString' over option 2. Please let me know your preference. Anyways I have uploaded for your review.

Thanks

Comment 66

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

Comment on attachment 8528762 [details] [diff] [review]
(v1.2) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

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

Hello Sid,
Your new change to iccExchangeAPDU looks good to me. Here is a further idea based on your new proposal - Can we just replace all current 'octet' type with 'long' as I've seen that's the type ril implementation uses, details inline, thank you!

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +45,5 @@
> +   *        Length of response's data field
> +   * @param data
> +   *        Response's data field bytes
> +   */
> +  void notifyExchangeAPDUResponse(in octet sw1,

Sorry for the second thought: Per ril.h, the type of sw1 is Int. How about we use 'long'?

@@ +46,5 @@
> +   * @param data
> +   *        Response's data field bytes
> +   */
> +  void notifyExchangeAPDUResponse(in octet sw1,
> +                                  in octet sw2,

ditto.

@@ +48,5 @@
> +   */
> +  void notifyExchangeAPDUResponse(in octet sw1,
> +                                  in octet sw2,
> +                                  in unsigned long length,
> +                                  [array, size_is(length)] in octet data);

Wondering can't we have the type DOMString instead of octect for 'data' in order to make the interface more consistent.

@@ +138,4 @@
>  
> +  void iccExchangeAPDU(in unsigned long clientId,
> +                       in long channel,
> +                       in octet cla,

ditto.

@@ +138,5 @@
>  
> +  void iccExchangeAPDU(in unsigned long clientId,
> +                       in long channel,
> +                       in octet cla,
> +                       in octet ins,

ditto.

@@ +139,5 @@
> +  void iccExchangeAPDU(in unsigned long clientId,
> +                       in long channel,
> +                       in octet cla,
> +                       in octet ins,
> +                       in octet p1,

ditto.

@@ +140,5 @@
> +                       in long channel,
> +                       in octet cla,
> +                       in octet ins,
> +                       in octet p1,
> +                       in octet p2,

ditto.

@@ +142,5 @@
> +                       in octet ins,
> +                       in octet p1,
> +                       in octet p2,
> +                       in unsigned long p3,
> +                       in DOMString data,

I am fine with DOMString, thank you.

::: dom/system/gonk/RILContentHelper.js
@@ +1957,5 @@
> +    let channel   = message.channel;
> +
> +    let callback = this._getIccChannelCallback(requestId);
> +    if (callback) {
> +      delete this._iccChannelCallback[requestId];

Can we move the delete operation into _getIccChannelCallback()? Or have you foreseen possible cases that we'd like to get callback but not to delete it?

Comment 67

[psiddh]

Attachment: (v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

Comment 68

[psiddh]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #66)
> Comment on attachment 8528762 [details] [diff] [review]
> (v1.2) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces
> in nsIIccProvider.idl
> 
> Review of attachment 8528762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hello Sid,
> Your new change to iccExchangeAPDU looks good to me. Here is a further idea
> based on your new proposal - Can we just replace all current 'octet' type
> with 'long' as I've seen that's the type ril implementation uses, details
> inline, thank you!
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +45,5 @@
> > +   *        Length of response's data field
> > +   * @param data
> > +   *        Response's data field bytes
> > +   */
> > +  void notifyExchangeAPDUResponse(in octet sw1,
> 
> Sorry for the second thought: Per ril.h, the type of sw1 is Int. How about
> we use 'long'?
> 
Though I don't have an answer why ril.h  have an 'ints' while say a char would have sufficed.
Please refer to this spec: http://www.3gpp.org/ftp/tsg_t/TSG_T/TSGT_05/docs/PDFs/TP-99185.pdf, 
section 10.2 Command APDU Structure. All the specs say that 'cla' , 'ins' , 'p1' , 'p2' are 1 byte sized.
In any case, this idl interfaces are one level above ril exchange, maybe it is still ok to leave them as 'octet' including status bytes sw1 & sw2. Just my opinion. What do you think ?

> @@ +46,5 @@
> > +   * @param data
> > +   *        Response's data field bytes
> > +   */
> > +  void notifyExchangeAPDUResponse(in octet sw1,
> > +                                  in octet sw2,
> 
> ditto.
> 
> @@ +48,5 @@
> > +   */
> > +  void notifyExchangeAPDUResponse(in octet sw1,
> > +                                  in octet sw2,
> > +                                  in unsigned long length,
> > +                                  [array, size_is(length)] in octet data);
> 
> Wondering can't we have the type DOMString instead of octect for 'data' in
> order to make the interface more consistent.
> 
Agreed! My thoughts too.

> @@ +138,4 @@
> >  
> > +  void iccExchangeAPDU(in unsigned long clientId,
> > +                       in long channel,
> > +                       in octet cla,
> 
> ditto.
> 
> @@ +138,5 @@
> >  
> > +  void iccExchangeAPDU(in unsigned long clientId,
> > +                       in long channel,
> > +                       in octet cla,
> > +                       in octet ins,
> 
> ditto.
> 
> @@ +139,5 @@
> > +  void iccExchangeAPDU(in unsigned long clientId,
> > +                       in long channel,
> > +                       in octet cla,
> > +                       in octet ins,
> > +                       in octet p1,
> 
> ditto.
> 
> @@ +140,5 @@
> > +                       in long channel,
> > +                       in octet cla,
> > +                       in octet ins,
> > +                       in octet p1,
> > +                       in octet p2,
> 
> ditto.
> 
> @@ +142,5 @@
> > +                       in octet ins,
> > +                       in octet p1,
> > +                       in octet p2,
> > +                       in unsigned long p3,
> > +                       in DOMString data,
> 
> I am fine with DOMString, thank you.
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1957,5 @@
> > +    let channel   = message.channel;
> > +
> > +    let callback = this._getIccChannelCallback(requestId);
> > +    if (callback) {
> > +      delete this._iccChannelCallback[requestId];
> 
> Can we move the delete operation into _getIccChannelCallback()? Or have you
> foreseen possible cases that we'd like to get callback but not to delete it?

Actually no such use-cases I have in mind. I have made the change suggested by you in the latest patch

Comment 69

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8532993 [details] [diff] [review]
(v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

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

Hi Edgar, can you help to review the patches at the moment?

Comment 70

[Edgar Chen [:edgar]]

Comment on attachment 8532993 [details] [diff] [review]
(v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

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

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +44,5 @@
> +   * @param data
> +   *        Response's data
> +   */
> +  void notifyExchangeAPDUResponse(in octet sw1,
> +                                  in octet sw2,

Since specs say sw1 and sw2 are both 1 bytes size, I am ok with octet.

@@ +137,5 @@
> +                       in long channel,
> +                       in octet cla,
> +                       in octet ins,
> +                       in octet p1,
> +                       in octet p2,

ditto, I am fine with octet.

@@ +138,5 @@
> +                       in octet cla,
> +                       in octet ins,
> +                       in octet p1,
> +                       in octet p2,
> +                       in unsigned long p3,

I am wondering how does idl handle the case that APDU contains only header (CLA, INS, P1, P2).
From the ril.h [1], it seems it could be achieved in ril level by assigning p3 to a negative value.
Wondering should we use `short` for p3 [2] in idl, instead of `unsigned long`.
        
[1] https://github.com/android/platform_hardware_ril/blob/master/include/telephony/ril.h#L410
[2] From the spec, http://www.3gpp.org/ftp/tsg_t/TSG_T/TSGT_05/docs/PDFs/TP-99185.pdf, table 10.1: Contents of Command, the lc and le are both 1 bytes if present, it seems short is enough.

Comment 71

[Edgar Chen [:edgar]]

Comment on attachment 8532993 [details] [diff] [review]
(v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

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

Hi Sid,

The overall looks good, but there are still few questions need to be clarified. Please see comment #70.
Thanks for your work.

::: dom/system/gonk/RILContentHelper.js
@@ +1079,2 @@
>  
> +    if (!data)

if (!data) {
 ...
}

even if only one line in `if` block.

Comment 72

[psiddh]

Attachment: (v1.4) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

Comment 73

[psiddh]

(In reply to Edgar Chen [:edgar][:echen] from comment #70)
> Comment on attachment 8532993 [details] [diff] [review]
> (v1.3) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces
> in nsIIccProvider.idl
> 
> Review of attachment 8532993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +44,5 @@
> > +   * @param data
> > +   *        Response's data
> > +   */
> > +  void notifyExchangeAPDUResponse(in octet sw1,
> > +                                  in octet sw2,
> 
> Since specs say sw1 and sw2 are both 1 bytes size, I am ok with octet.
> 
Ok
> @@ +137,5 @@
> > +                       in long channel,
> > +                       in octet cla,
> > +                       in octet ins,
> > +                       in octet p1,
> > +                       in octet p2,
> 
> ditto, I am fine with octet.
> 
> @@ +138,5 @@
> > +                       in octet cla,
> > +                       in octet ins,
> > +                       in octet p1,
> > +                       in octet p2,
> > +                       in unsigned long p3,
> 
> I am wondering how does idl handle the case that APDU contains only header
> (CLA, INS, P1, P2).
> From the ril.h [1], it seems it could be achieved in ril level by assigning
> p3 to a negative value.
As far as C-APDU is concerned this is Case 1: This is interpreted at T-APDU as only header i;e; Both command data ('data' & 'lc') & command response (le) are absent. Using our idl , we could do the following

iccProvider.iccExchangeAPDU(clientId, channel,cla, ins, p1, p2, -1, null , callback); Not sure if I have addressed your concern.

> Wondering should we use `short` for p3 [2] in idl, instead of `unsigned
> long`.
Agreed. Changed 'p3' to  short
>         
> [1]
> https://github.com/android/platform_hardware_ril/blob/master/include/
> telephony/ril.h#L410
> [2] From the spec,
> http://www.3gpp.org/ftp/tsg_t/TSG_T/TSGT_05/docs/PDFs/TP-99185.pdf, table
> 10.1: Contents of Command, the lc and le are both 1 bytes if present, it
> seems short is enough.

Comment 74

[Edgar Chen [:edgar]]

Comment on attachment 8534077 [details] [diff] [review]
(v1.4) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl

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

(In reply to Siddartha P from comment #73)
> As far as C-APDU is concerned this is Case 1: This is interpreted at T-APDU
> as only header i;e; Both command data ('data' & 'lc') & command response
> (le) are absent. Using our idl , we could do the following
> 
> iccProvider.iccExchangeAPDU(clientId, channel,cla, ins, p1, p2, -1, null ,
> callback); Not sure if I have addressed your concern.

Thanks for explanation, it's clear to me. :)
Since p3 could be a negative value for this case, it should not be `unsigned` in idl. Please help to address this.
And if you could add some comments for it, that will be really great.

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +138,5 @@
> +                       in octet cla,
> +                       in octet ins,
> +                       in octet p1,
> +                       in octet p2,
> +                       in unsigned short p3,

r=me with `unsigned` removed.

Comment 75

[psiddh]

Attachment: (v1.4a) Redefine IccOpenChannel, IccCloseChannel & IccExchangeApdu interfaces in nsIIccProvider.idl. r=echen

Comment 76

[psiddh]

Hi Edgar, Thanks for your comments. In order to land this patch on REL 2.1 branch, there is another dependency : https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 , we would need this change as well. (Note that this is already landed on the main line ). For SE stack development, we are based off Qualcomm's release on v2.1 branch.
Could you please let us know how we can proceed with these changes / landing ?

Comment 77

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

Thanks for catching the review up, Edgar :)

Comment 78

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Siddartha P from comment #76)
> Hi Edgar, Thanks for your comments. In order to land this patch on REL 2.1
> branch, there is another dependency :
> https://bugzilla.mozilla.org/show_bug.cgi?id=1030602 , we would need this
> change as well. (Note that this is already landed on the main line ). For SE
> stack development, we are based off Qualcomm's release on v2.1 branch.
> Could you please let us know how we can proceed with these changes / landing
> ?

Hey,
v2.1 branch is now code complete. 
We wouldn't uplift patches unless for critical fixes. (Purpose is to maintain stability.)
Can we focus the SE implementation on current master/central (v2.2)?

Comment 79

[Vincent Chang[:vchang][changyihsin]]

Hi Sid, let commit the patch to m-c first. The patch is ready to go right?

Comment 80

[psiddh]

Yes this patch is ready to go now. I will get my tryServer builds going and update the results shortly!

Comment 81

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Let's assume we can have this landed after new year holiday :)
Feel free to correct me.

Comment 82

[psiddh]

Attachment: New rebased patch for m-c

Uploading a new patch for m-c. As is the case, RILContentHelper.js and nsIIccProvider.idl are different on m-c from v2.1.
Also try server is in progress here :
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2930bf6e3e2f

Do we have to write any test cases for the try server results to be successful ?

Comment 83

[psiddh]

Hi Wesley , Vincent

Could you tell me how to go about landing these changes on m-c ?
I have also uploaded the rebased patch 'New rebased patch for m-c ' for feedback ?

Comment 84

[Vincent Chang[:vchang][changyihsin]]

Hi Sid, I think you can run that test case manually in your local environment. It looks like nsIIccChannelCallback is missing when you push the patch to try server. One of the main reason that we need to run try server is to prevent breaking m-c tree when submit a new patch.

Comment 85

[psiddh]

Attachment: New rebased patch for m-c.

Uploaded the re-based patch. I guess this needs another look by the reviewers.

Here are try results: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=04ae57c980f3 ( Mostly green)

Comment 86

[psiddh]

(In reply to Vincent Chang[:vchang] from comment #84)
> Hi Sid, I think you can run that test case manually in your local
> environment. It looks like nsIIccChannelCallback is missing when you push
> the patch to try server. One of the main reason that we need to run try
> server is to prevent breaking m-c tree when submit a new patch.

Ok. I uploaded the rebased patch for another feedback (just in case), since I have not tested this patch on m-c for the obvious reason.

Comment 87

[Edgar Chen [:edgar]]

Comment on attachment 8538931 [details] [diff] [review]
New rebased patch for m-c.

Forwarding f? to me, I can help to take a look.

Comment 88

[Edgar Chen [:edgar]]

Comment on attachment 8538931 [details] [diff] [review]
New rebased patch for m-c.

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

Looks good, thank you.

Comment 89

[psiddh]

Hi Edgar,
I assume that this patch is good to go now. Since try results are also good, should I go ahead and do 'check-in' needed for this ? Thanks!

Comment 90

[Edgar Chen [:edgar]]

(In reply to Siddartha P from comment #89)
> Hi Edgar,
> I assume that this patch is good to go now. Since try results are also good,
> should I go ahead and do 'check-in' needed for this ? Thanks!

Go ahead. Thanks for your work.

Comment 91

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

Thanks Edgar and Sid!

Comment 92

[Carsten Book [:Tomcat]]

Hi ,

this patch failed to apply:

patching file dom/icc/interfaces/nsIIccProvider.idl
Hunk #1 FAILED at 11
Hunk #2 succeeded at 161 with fuzz 2 (offset 5 lines).
1 out of 2 hunks FAILED -- saving rejects to file dom/icc/interfaces/nsIIccProvider.idl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 0001-Bug-1081789-Redefine-IccOpenChannel-IccCloseChannel.patch

could you take a look, thanks!

Comment 93

[psiddh]

Hi , I assume that the patch that is used on try is: "New rebased patch for m-c." and not the patch '(v1.4a)...' one. If yes, I will recheck and provide an update.
Thanks

Comment 94

[Carsten Book [:Tomcat]]

(In reply to Siddartha P from comment #93)
> Hi , I assume that the patch that is used on try is: "New rebased patch for
> m-c." and not the patch '(v1.4a)...' one. If yes, I will recheck and provide
> an update.
> Thanks

yes was trying to land on b2g-inbound

Comment 95

[psiddh]

Attachment: New re-based patch for Inbound

Will update try results shortly!

Comment 96

[psiddh]

Hi Edgar,

One more time, could you please quickly skim through the latest patch rebased for in-bound.
Also try results look ok for most except 'B2G ICS Emulator opt' and 'B2G ICS Emulator debug' https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc0eaa66b46c.

Could you please suggest if I am missing anything here ?

Comment 97

[Edgar Chen [:edgar]]

Comment on attachment 8542403 [details] [diff] [review]
New re-based patch for Inbound

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

I guess the try result in B2G ICS Emulator was busted due to below error.

::: dom/system/gonk/RILContentHelper.js
@@ +410,5 @@
>      });
>    },
>  
> +  iccOpenChannel: function(clientId, aid, callback) {
> +    let requestId = UUIDGenerator.generateUUID().toString();

You forget to define the lazy getter for `UUIDGenerator`.

XPCOMUtils.defineLazyServiceGetter(this, "UUIDGenerator",
                                   "@mozilla.org/uuid-generator;1",
                                   "nsIUUIDGenerator");

@@ +566,5 @@
>        if (DEBUG) debug("Unregistered listener: " + listener);
>      }
>    },
>  
> +_ addIccChannelCallback: function(requestId, channelCb) {

Should be `_addIccChangelCallback`.

Comment 98

[psiddh]

Attachment: New re-based patch for Inbound

Hi Edgar, Hopefully one last time, does these try results look promising now ?
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0f30c0bbf88

Thanks for your time

Comment 99

[Edgar Chen [:edgar]]

Comment on attachment 8543538 [details] [diff] [review]
New re-based patch for Inbound

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

Looks good to me, thank you.

And I will help to merge this patch into inbound once bug 1098210 is re-landed.

Comment 100

[Vincent Chang[:vchang][changyihsin]]

Thanks, Edgar.

Comment 101

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ab9fb3092d9f

Comment 102

[psiddh]

(In reply to Edgar Chen [:edgar][:echen] from comment #101)
> https://hg.mozilla.org/integration/b2g-inbound/rev/ab9fb3092d9f

Thanks Edgar

Comment 103

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab9fb3092d9f