TS 102.223 6.4.27 When ME receive this command, open a channel for data transmission

This clause applies if class "e" is supported.

Local test pass, here's the try server link https://tbpl.mozilla.org/?tree=Try&rev=2a1b7868d417

Comment on attachment 813034 [details] [diff] [review] Part1: BIP "Open Channel" proactive command IDL I'll like to split this patch, as the modem may already did the work for us. So please parse only the mandatory and text message first. And move other functionalities to other bugs.

Comment on attachment 816464 [details] [diff] [review] Part1: BIP "Open Channel" proactive command IDL v.2 Delete the optional part. Now the patch only contain Open channel: default bearer, uicc/terminal server command's mandatory part & alphaId.

Comment on attachment 816464 [details] [diff] [review] Part1: BIP "Open Channel" proactive command IDL v.2 Review of attachment 816464 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/SimToolKit.idl @@ +414,5 @@ > + * For the only support bearerType: nsIDOMMozIccManager.STK_BEARER_TYPE_DEFAULT, > + * this parameter is no need. > + */ > + jsval bearerParameters; > +}; I don't know why this dict is needed. only support 'default', and when the type is 'default', parameter is no need. @@ +416,5 @@ > + */ > + jsval bearerParameters; > +}; > + > +dictionary MozStkTransportLevel TransportLevel, is optional, remove. @@ +435,5 @@ > + */ > + unsigned short portNumber; > +}; > + > +dictionary MozStkDefaultBearer If modem has done this for us, then we don't need Bearer description as well. Remove this. @@ +469,5 @@ > + */ > + unsigned short bufferSize; > +}; > + > +dictionary MozStkServer We don't need Server, either Remove this. @@ +472,5 @@ > + > +dictionary MozStkServer > +{ > + /** > + * Text String, contain alphaId. "contain alphaId" doesn't make any sense here. And should be "contains" @@ +489,5 @@ > + jsval transportLevel; > +}; > + > +dictionary MozStkOpenChannel > +{ We should move text and bufferSize (if needed) to here. ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +218,5 @@ > const unsigned short STK_TIMER_GET_CURRENT_VALUE = 0x02; > > /** > + * Bearer Type > + */ We don't need this, and below.

(The test result of patches) Try server link: https://tbpl.mozilla.org/?tree=Try&rev=ca80f4bd3d68 I'll update new patches in this bug later.

Comment on attachment 816965 [details] [diff] [review] Part1: BIP "Open Channel" proactive command IDL v.3 Review of attachment 816965 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Forward r? to Hsinyi. ::: dom/icc/interfaces/SimToolKit.idl @@ +400,5 @@ > */ > unsigned short timerAction; > }; > > +dictionary MozStkBIPCmd MozStkBipMessage, or please specify what does Cmd mean here.

Comment on attachment 816966 [details] [diff] [review] Part2: "Open Channel" proactive command implement in ril v.3 Review of attachment 816966 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_consts.js @@ +1053,5 @@ > +this.STK_TERMINAL_SUPPORT_BIP_BEARER_TIA_EIA_IS_820 = 0; > +this.STK_TERMINAL_SUPPORT_BIP_BEARER_USB = 0; > + > +this.STK_TERMINAL_SUPPORT_BIP_UDP_CLIENT_REMOTE = 1; > +this.STK_TERMINAL_SUPPORT_BIP_TCP_CLIENT_REMOTE = 1; What does these two mean here? @@ +1139,5 @@ > (STK_TERMINAL_SUPPORT_PROACTIVE_LANGUAGE_NOTIFICATION << 5) | > (STK_TERMINAL_SUPPORT_PROACTIVE_LAUNCH_BROWSER << 6) | > (STK_TERMINAL_SUPPORT_PROACTIVE_LOCAL_INFO_ACCESS_TECH << 7); > > +this.STK_TERMINAL_PROFILE_PROACTIVE_5 = Better naming, for example, STK_TERMINAL_PROFILE_BIP_COMMAND @@ +1174,5 @@ > STK_TERMINAL_PROFILE_PROACTIVE_3, > STK_TERMINAL_PROFILE_PROACTIVE_4, > 0x00, // Softkey support > 0x00, // Softkey information > + STK_TERMINAL_PROFILE_PROACTIVE_5, rename this ::: dom/system/gonk/ril_worker.js @@ +9851,5 @@ > + * The value object of CommandDetails TLV. > + * @param ctlvs > + * The all TLVs in this proactive command. > + */ > + processBIPCmd: function processBIPCmd(cmdDetails, ctlvs) { Bip And replace Cmd to the one in the IDL.

Comment on attachment 816967 [details] [diff] [review] Part3: Xpcshell test for "Open Channel" proactive cmd. v.3 Review of attachment 816967 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_stk.js @@ +753,5 @@ > + 0xD0, > + 0x0F, > + 0x81, 0x03, 0x01, 0x16, 0x00, > + 0x82, 0x02, 0x81, 0x82, > + 0x85, 0x04, 0x54, 0x65, 0x78, 0x74//alpha id: "Text" nit, space before //

Add space before //

Change name and remove unnecessary SAT profile element.

Dears, Please just review the new patch, thank you!

I put the new patches to try server. See below link: https://tbpl.mozilla.org/?tree=Try&rev=5c2c9879155e Result pass!

Comment on attachment 817692 [details] [diff] [review] Part2: "Open Channel" proactive command implement in ril v.5 Dear Yoshi, This patch should be correct one~ sorry for boring you again.

Comment on attachment 817694 [details] [diff] [review] Part1: BIP "Open Channel" proactive command IDL v.4 Due to I delete previous Part1 patch by accident...re-upload and set review? flag. Sorry for bothering you again.

Comment on attachment 817692 [details] [diff] [review] Part2: "Open Channel" proactive command implement in ril v.5 Review of attachment 817692 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9858,5 @@ > + let ctlv = StkProactiveCmdHelper.searchForTag( > + COMPREHENSIONTLV_TAG_ALPHA_ID, ctlvs); > + if (ctlv) { > + bipMsg.text = ctlv.value.identifier; > + } What happened if ALPHA_ID is not in the TLVs?

Comment on attachment 817692 [details] [diff] [review] Part2: "Open Channel" proactive command implement in ril v.5 Review of attachment 817692 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9858,5 @@ > + let ctlv = StkProactiveCmdHelper.searchForTag( > + COMPREHENSIONTLV_TAG_ALPHA_ID, ctlvs); > + if (ctlv) { > + bipMsg.text = ctlv.value.identifier; > + } AlphaId is optional so it's okay if text is not included in BipMessage.

Comment on attachment 817694 [details] [diff] [review] Part1: BIP "Open Channel" proactive command IDL v.4 Review of attachment 817694 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. It looks good to me!

Due to change same file, the patches need to rebase after 908083 landed. I'll update on bug.

re-patch base on Bug908083

rebase.

rebase.

https://hg.mozilla.org/integration/b2g-inbound/rev/c6d40dd918c1 https://hg.mozilla.org/integration/b2g-inbound/rev/3cf6563fe890 https://hg.mozilla.org/integration/b2g-inbound/rev/bab8792e66e8

CCing Anshul for IDL change

https://hg.mozilla.org/mozilla-central/rev/c6d40dd918c1 https://hg.mozilla.org/mozilla-central/rev/3cf6563fe890 https://hg.mozilla.org/mozilla-central/rev/bab8792e66e8

The constants here are wrong, file Bug 938466 to fix this.