The Next Action Indicator could show for SetUpMenu and SelectItem command format-> TS102.223 8.24 GCF case ID below: SET UP MENU 27.22.4.8 Expected Sequence 3.1 (SET UP MENU, next action indicator "Send SM", "Set Up Call", "Launch Browser", "Provide Local Information", successful); SELECT ITEM 27.22.4.9 Expected Sequence 2.1 (SELECT ITEM, next action indicator, successful)

Try: https://tbpl.mozilla.org/?tree=Try&rev=2cc0c7604cae

Try result: https://tbpl.mozilla.org/?tree=Try&rev=67e2a0f11e37

Comment on attachment 830565 [details] [diff] [review] Part1: STK Next Action Indicator IDL Review of attachment 830565 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/SimToolKit.idl @@ +118,5 @@ > boolean isHelpAvailable; > + > + /** > + * List of Next Action Indicators. > + * Each column should be one of STK_NAI_TYPE_* each 'column' ? ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +223,5 @@ > > /** > + * Next Action Indicator Type > + */ > + const unsigned short STK_NAI_TYPE_SET_UP_CALL = 0x10; Isn't this just the same with const unsigned short STK_CMD_SET_UP_CALL = 0x10; Why do you introduce another set of constants and the values are exactly the same?

Comment on attachment 830566 [details] [diff] [review] Part2: STK Next Action Indicator RIL handler. Review of attachment 830566 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_consts.js @@ +970,5 @@ > this.STK_BROWSER_TERMINATION_CAUSE_USER = 0x00; > this.STK_BROWSER_TERMINATION_CAUSE_ERROR = 0x01; > > +// Next Action Indicator Type > +this.STK_NAI_TYPE_SET_UP_CALL = 0x10; Why define these consts? ::: dom/system/gonk/ril_worker.js @@ +10257,5 @@ > + let naiList = []; > + for (let i = 0; i < length; i++) { > + naiList.push(GsmPDUHelper.readHexOctet()); > + } > + return naiList; From the spec, "If the value is equal to '00' or if the value is reserved (that is, value not listed), the terminal shall ignore the next action indicator type." So you aren't going to handle this? @@ +10259,5 @@ > + naiList.push(GsmPDUHelper.readHexOctet()); > + } > + return naiList; > + }, > + Not sure how you're going to name it, I already see - nextActionInd - naiList - NextActionIndList - Next Action Indicator List

Comment on attachment 829072 [details] [diff] [review] Part3: STK Add xpcshell test for set up menu. Review of attachment 829072 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_stk.js @@ +593,5 @@ > > /** > + * Verify Proactive Command : Set Up Menu > + */ > +add_test(function test_stk_proactive_command_set_up_menu() { Can you also add a test for SELECT_ITEM?

Comment on attachment 830567 [details] [diff] [review] Part4: Modify marionette for next action indicator Review of attachment 830567 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/tests/marionette/test_stk_select_item.js @@ +18,5 @@ > is(command.options.items[index].text, expect.items[index].text, expect.name); > } > + for (let index in command.options.nextActionIndList) { > + is(command.options.nextActionIndList[index], expect.naiList[index], expect.name); > + } ditto @@ +67,5 @@ > commandQualifier: 0x00, > title: "Toolkit Select", > + items: [{identifier: 1, text: "Item 1"}, {identifier: 2, text: "Item 2"}, {identifier: 3, text: "Item 3"}], > + naiList: [0x13, 0x10, 0x26]}}, > + ditto ::: dom/icc/tests/marionette/test_stk_setup_menu.js @@ +16,5 @@ > for (let index in command.options.items) { > is(command.options.items[index].identifier, expect.items[index].identifier, expect.name); > is(command.options.items[index].text, expect.items[index].text, expect.name); > } > + for (let index in command.options.nextActionIndList) { for ( of ) @@ +17,5 @@ > is(command.options.items[index].identifier, expect.items[index].identifier, expect.name); > is(command.options.items[index].text, expect.items[index].text, expect.name); > } > + for (let index in command.options.nextActionIndList) { > + is(command.options.nextActionIndList[index], expect.naiList[index], expect.name); consistent naming. @@ +85,5 @@ > expect: {name: "setup_menu_cmd_7", > commandQualifier: 0x00, > title: "Toolkit Menu", > + items: [{identifier: 1, text: "Item 1"}, {identifier: 2, text: "Item 2"}, {identifier: 3, text: "Item 3"}, {identifier: 4, text: "Item 4"}], > + naiList: [0x13, 0x10, 0x15, 0x26]}}, use const defined in MozIccMAnager, that's why we defined it in IDL

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10) > Comment on attachment 830565 [details] [diff] [review] > Part1: STK Next Action Indicator IDL > > Review of attachment 830565 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/icc/interfaces/SimToolKit.idl > @@ +118,5 @@ > > boolean isHelpAvailable; > > + > > + /** > > + * List of Next Action Indicators. > > + * Each column should be one of STK_NAI_TYPE_* > > each 'column' ? Oh.. Maybe just "@see nsIDOMMozIccManager.STK_CMD_*" ? > > ::: dom/icc/interfaces/nsIDOMIccManager.idl > @@ +223,5 @@ > > > > /** > > + * Next Action Indicator Type > > + */ > > + const unsigned short STK_NAI_TYPE_SET_UP_CALL = 0x10; > > Isn't this just the same with > const unsigned short STK_CMD_SET_UP_CALL = 0x10; > > Why do you introduce another set of constants and the values are exactly the > same? Sorry, I did not notice that NAI using the same define as CMDs. I'll merge those define into STK_CMD_*

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #11) > Comment on attachment 830566 [details] [diff] [review] > Part2: STK Next Action Indicator RIL handler. > > Review of attachment 830566 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: dom/system/gonk/ril_worker.js > @@ +10257,5 @@ > > + let naiList = []; > > + for (let i = 0; i < length; i++) { > > + naiList.push(GsmPDUHelper.readHexOctet()); > > + } > > + return naiList; > > From the spec, > > "If the value is equal to '00' or if the value is reserved (that is, value > not listed), the terminal shall ignore > the next action indicator type." > > So you aren't going to handle this? > Dear Yoshi, The next action indicator list will map one-by-one to items. For example: item: [{1, "item 1"},{2, "item 2"},{3, "item 3"}]; naiList:[ 0x00, SET_UP_CALL, SEND_SMS ]; I think the spec means=> "If one of the NAI is 00, this NAI should be ignore by terminal". Other NAI still need to process. We should not ignore '00' when parsing next action indicator list. > @@ +10259,5 @@ > > + naiList.push(GsmPDUHelper.readHexOctet()); > > + } > > + return naiList; > > + }, > > + > > Not sure how you're going to name it, > I already see > - nextActionInd > - naiList > - NextActionIndList > - Next Action Indicator List Ok, I'll choose 'naiList' for consist naming.

Comment on attachment 831363 [details] [diff] [review] Part4: Modify marionette for next action indicator. v2 local run marionette/xpcshell test pass, will update try result later~

Comment on attachment 831402 [details] [diff] [review] Part3: STK Add xpcshell test for set up menu and select item v2 Please see this one! Sorry for the wrong file.

Comment on attachment 831360 [details] [diff] [review] Part1: STK Next Action Indicator IDL. v2 Review of attachment 831360 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/SimToolKit.idl @@ +118,5 @@ > boolean isHelpAvailable; > + > + /** > + * List of Next Action Indicators. > + * @see nsIDOMMozIccManager.STK_CMD_* Try to find some documentation from STK spec. Add an extra line before @see @@ +120,5 @@ > + /** > + * List of Next Action Indicators. > + * @see nsIDOMMozIccManager.STK_CMD_* > + */ > + jsval naiList; // unsigned short [] use longer name for those public interfaces/members. ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +63,5 @@ > + const unsigned short STK_CMD_GET_SERVICE_INFO = 0x46; > + const unsigned short STK_CMD_RETRIEVE_MMS = 0x60; > + const unsigned short STK_CMD_SUBMMIT_MMS = 0x61; > + const unsigned short STK_CMD_DISPLAY_MMS = 0x62; > + const unsigned short STK_CMD_END_PROACTIVE_SESSION = 0x63; I don't even know we already support some many STK commands now. Or if they are still not supported, what's the purpose to notify these as Next Action Indicator to Gaia app?

(In reply to Georgia Wang from comment #15) > > From the spec, > > > > "If the value is equal to '00' or if the value is reserved (that is, value > > not listed), the terminal shall ignore > > the next action indicator type." > > > The next action indicator list will map one-by-one to items. > For example: > item: [{1, "item 1"},{2, "item 2"},{3, "item 3"}]; > naiList:[ 0x00, SET_UP_CALL, SEND_SMS ]; > > I think the spec means=> > "If one of the NAI is 00, this NAI should be ignore by terminal". You say "should ignore" But seems your code still parse 0x00 and post it to Content, why? > Other NAI still need to process. > We should not ignore '00' when parsing next action indicator list. > > > @@ +10259,5 @@ > > > + naiList.push(GsmPDUHelper.readHexOctet()); > > > + } > > > + return naiList; > > > + }, > > > + > > > > Not sure how you're going to name it, > > I already see > > - nextActionInd > > - naiList > > - NextActionIndList > > - Next Action Indicator List > > Ok, I'll choose 'naiList' for consist naming. No, we use longer names, unless it's a well-known abbreviation, like SMS, MMS, ICC. NAI doesn't have too much information, I guess only you will know what it means. try 'nextActionList'

Comment on attachment 831361 [details] [diff] [review] Part2: STK Next Action Indicator RIL handler. v2 Review of attachment 831361 [details] [diff] [review]: ----------------------------------------------------------------- Same comment with processing 00 and introduce some unnecessary consts before.

Comment on attachment 831363 [details] [diff] [review] Part4: Modify marionette for next action indicator. v2 Review of attachment 831363 [details] [diff] [review]: ----------------------------------------------------------------- r- for I think this patch isn't tested before send r?. ::: dom/icc/tests/marionette/test_stk_select_item.js @@ +20,5 @@ > + if (command.options.naiList) { > + let index = 0; > + for (let nai of command.options.naiList) { > + is(nai, expect.naiList[index++], expect.name); > + } since you already use index why don't you write for (let i = 0; i < ...; i++) @@ +69,5 @@ > expect: {name: "select_item_cmd_7", > commandQualifier: 0x00, > title: "Toolkit Select", > + items: [{identifier: 1, text: "Item 1"}, {identifier: 2, text: "Item 2"}, {identifier: 3, text: "Item 3"}], > + naiList: [STK_CMD_SEND_SMS, STK_CMD_SET_UP_CALL, STK_CMD_PROVIDE_LOCAL_INFO]}}, Did you try this before sending r? I wonder how your javascript engine could know where is STK_CMD_SEND_SMS defined. Maybe you need to type 'icc.STK_CMD_SEND_SMS'?

Comment on attachment 831360 [details] [diff] [review] Part1: STK Next Action Indicator IDL. v2 Review of attachment 831360 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +56,5 @@ > + const unsigned short STK_CMD_GET_READER_STATUS = 0x33; > + const unsigned short STK_CMD_OPEN_CHANNEL = 0x40; > + const unsigned short STK_CMD_CLOSE_CHANNEL = 0x41; > + const unsigned short STK_CMD_RECEIVE_DATA = 0x42; > + const unsigned short STK_CMD_SEND_DATA = 0x43; I am totally confused now, I thought you already defined these OPEN_CHANNEL/CLOSE CHANNEL/SEND DATA/RECEIVE DATA from Bug 908535, Bug 908536. Why are they gone in this patch? And they are 0x30 ~ 0x33 in those bugs Why do they become 0x40~0x43 ?

Oh, it seems 0x40~0x43 should be correct one. It seems the patches in Bug 908535, Bug 908536 are ........... wrong. It should be part of my fault didn't check all your definitions in the first place. But that doesn't explain why your patch here doesn't have those 'wrong' code I didn't find any code related to remove those wrong code. MXR still has the wrong one http://mxr.mozilla.org/mozilla-central/source/dom/icc/interfaces/nsIDOMIccManager.idl#53 53 const unsigned short STK_CMD_OPEN_CHANNEL = 0x30; 54 const unsigned short STK_CMD_CLOSE_CHANNEL = 0x31; 55 const unsigned short STK_CMD_RECEIVE_DATA = 0x32; 56 const unsigned short STK_CMD_SEND_DATA = 0x33; Please explain this.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #28) > Oh, it seems 0x40~0x43 should be correct one. > > It seems the patches in Bug 908535, Bug 908536 are ........... wrong. > > It should be part of my fault didn't check all your definitions in the first > place. > > But that doesn't explain why your patch here doesn't have those 'wrong' code > > I didn't find any code related to remove those wrong code. > MXR still has the wrong one > http://mxr.mozilla.org/mozilla-central/source/dom/icc/interfaces/ > nsIDOMIccManager.idl#53 > > 53 const unsigned short STK_CMD_OPEN_CHANNEL = 0x30; > 54 const unsigned short STK_CMD_CLOSE_CHANNEL = 0x31; > 55 const unsigned short STK_CMD_RECEIVE_DATA = 0x32; > 56 const unsigned short STK_CMD_SEND_DATA = 0x33; > > Please explain this. Per talked, that's my mistake due to not "update" local code. I already file Bug 938466 to fix those wrong constant.

Delete unnecessary command ID, modify naming to "nextActionList."

Remove unnecessary command id and modify naming to "nextActionList"

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #24) > (In reply to Georgia Wang from comment #15) > > > From the spec, > > > > > > "If the value is equal to '00' or if the value is reserved (that is, value > > > not listed), the terminal shall ignore > > > the next action indicator type." > > > > > The next action indicator list will map one-by-one to items. > > For example: > > item: [{1, "item 1"},{2, "item 2"},{3, "item 3"}]; > > naiList:[ 0x00, SET_UP_CALL, SEND_SMS ]; > > > > I think the spec means=> > > "If one of the NAI is 00, this NAI should be ignore by terminal". > You say "should ignore" > > But seems your code still parse 0x00 and post it to Content, why? > Per talked, length of next action indicator list should equal to item list. We still need to post 0x00. I add STK_CMD_NULL = 0x00 to indicate the "no-need-action" state.

Modify naiList => nextActionList

Modify naming and correct constants.

Try: https://tbpl.mozilla.org/?tree=Try&rev=1dba38e53700

Update UUID.

Comment on attachment 832118 [details] [diff] [review] Part4: Modify marionette for next action indicator. v3 Try: https://tbpl.mozilla.org/?tree=Try&rev=1dba38e53700

Comment on attachment 832123 [details] [diff] [review] Part1: STK Next Action Indicator IDL v3 Review of attachment 832123 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/SimToolKit.idl @@ +119,5 @@ > + > + /** > + * List of Next Action Indicators. > + * Each element should be one of nsIDOMMozIccManager.STK_CMD_* > + * If STK_CMD_NULL, terminal should ignore this action in correspond item. If it's ..., the terminal ... ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +54,5 @@ > const unsigned short STK_CMD_OPEN_CHANNEL = 0x40; > const unsigned short STK_CMD_CLOSE_CHANNEL = 0x41; > const unsigned short STK_CMD_RECEIVE_DATA = 0x42; > const unsigned short STK_CMD_SEND_DATA = 0x43; > + const unsigned short STK_CMD_END_PROACTIVE_SESSION = 0x63; 0x81

Comment on attachment 832111 [details] [diff] [review] Part2: STK Next Action Indicator RIL handler. v3 Review of attachment 832111 [details] [diff] [review]: ----------------------------------------------------------------- please update the constants according to part 1.

Modify comment in SimToolkit.idl Move NULL & END_PROACTIVE_SESSION out of STK_CMD in nsIDOMIccManager.idl

Modify constant base on part1.

Add 2 test cases for STK_NEXT_ACTION_NULL & STK_NEXT_ACTION_END_PROACTIVE_SESSION.

Comment on attachment 832809 [details] [diff] [review] Part1: STK Next Action Indicator IDL v4 Review of attachment 832809 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/interfaces/SimToolKit.idl @@ +121,5 @@ > + * List of Next Action Indicators. > + * Each element should be one of nsIDOMMozIccManager.STK_CMD_* > + * or nsIDOMMozIccManager.STK_NEXT_ACTION_* > + * If it's STK_NEXT_ACTION_NULL, the terminal should ignore this action > + * in correspond item. in the corresponding item

Comment on attachment 832118 [details] [diff] [review] Part4: Modify marionette for next action indicator. v3 Review of attachment 832118 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/tests/marionette/test_stk_select_item.js @@ +7,5 @@ > log("STK CMD " + JSON.stringify(command)); > is(command.typeOfCommand, icc.STK_CMD_SELECT_ITEM, expect.name); > is(command.commandQualifier, expect.commandQualifier, expect.name); > is(command.options.title, expect.title, expect.name); > for (let index in command.options.items) { I spot some code here use for (in), and understand why you use for (in) in the previous version. items here should be of array, we shouldn't use for(in) to enumerate it. Please file another bug to fix this. @@ +11,5 @@ > for (let index in command.options.items) { > is(command.options.items[index].identifier, expect.items[index].identifier, expect.name); > is(command.options.items[index].text, expect.items[index].text, expect.name); > } > + if (command.options.nextActionList) { let length = command.options.nextActionList ? command.options.nextActionList.length : 0; for (let i = 0; i < length; i++) { ::: dom/icc/tests/marionette/test_stk_setup_menu.js @@ +15,5 @@ > + if (command.options.nextActionList) { > + for (let i = 0; i < command.options.nextActionList.length; i++) { > + is(command.options.nextActionList[i], expect.nextActionList[i], expect.name); > + } > + } ditto

Comment on attachment 832814 [details] [diff] [review] Part3: STK Add xpcshell test for set up menu and select item v4 Review of attachment 832814 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_stk.js @@ +664,5 @@ > + let worker = newUint8Worker(); > + let pduHelper = worker.GsmPDUHelper; > + let berHelper = worker.BerTlvHelper; > + let stkHelper = worker.StkProactiveCmdHelper; > + let stkCmdHelper = worker.StkCommandParamsFactory; stkFactory, cmdHelper seems to be a StkProactiveCmdHelper. @@ +751,5 @@ > + let worker = newUint8Worker(); > + let pduHelper = worker.GsmPDUHelper; > + let berHelper = worker.BerTlvHelper; > + let stkHelper = worker.StkProactiveCmdHelper; > + let stkCmdHelper = worker.StkCommandParamsFactory; ditto

Modify naming base on comment 45.

Change base on comment 44.

TRY: https://tbpl.mozilla.org/?tree=Try&rev=92d636874add

Hi, Georgia Since youd add two consts in this bug, please also write a marionette to test STK_NEXT_ACTION_NULL and STK_NEXT_ACTION_END_PROACTIVE_SESSION.

Add 2 test about STK_NEXT_ACTION_END_PROACTIVE_SESSION\NULL TRY:: https://tbpl.mozilla.org/?tree=Try&rev=6bc7e00265fc

Comment on attachment 8335938 [details] [diff] [review] Part4: Modify marionette for next action indicator. v5 I've added tests base on previous comment~Could you kindly check it? Thank you!

,

Update UUID & wording~ Dear Hsin-Yi, please help review this one~ thanks a lot!

Modify marionette constant base on Bug814637.

Rebase and TRY: https://tbpl.mozilla.org/?tree=Try&rev=f1113e33d14c

Comment on attachment 8339167 [details] [diff] [review] Part1: STK Next Action Indicator IDL v5. Review of attachment 8339167 [details] [diff] [review]: ----------------------------------------------------------------- r=me but please don't forget to address those minor comments. Thank you :) ::: dom/icc/interfaces/SimToolKit.idl @@ +123,5 @@ > + * or nsIDOMMozIccManager.STK_NEXT_ACTION_* > + * If it's STK_NEXT_ACTION_NULL, the terminal should ignore this action > + * in corresponding item. > + * > + * @see TS 11.14, clase 12.24, Items Next Action Indicator. s/clase/clause/ ::: dom/icc/interfaces/nsIDOMIccManager.idl @@ +223,5 @@ > const unsigned short STK_BROWSER_TERMINATION_CAUSE_ERROR = 0x01; > > /** > + * Next Action Indicator > + */ nit: please place a period at the end of the line.

Correct typo and . base on comment 56 :)

https://hg.mozilla.org/integration/b2g-inbound/rev/472b2ae327e1 https://hg.mozilla.org/integration/b2g-inbound/rev/34e1cbfc6b64 https://hg.mozilla.org/integration/b2g-inbound/rev/fa30b62f33c3 https://hg.mozilla.org/integration/b2g-inbound/rev/d1b79eede8cd

https://hg.mozilla.org/mozilla-central/rev/472b2ae327e1 https://hg.mozilla.org/mozilla-central/rev/34e1cbfc6b64 https://hg.mozilla.org/mozilla-central/rev/fa30b62f33c3 https://hg.mozilla.org/mozilla-central/rev/d1b79eede8cd