Closed Bug 908554 Opened 11 years ago Closed 11 years ago

B2G STK: Support "Next Action Indicator" on proactive cmd "SET UP MENU" and "SELECT ITEM"

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gwang, Assigned: gwang)

References

Details

Attachments

(4 files, 23 obsolete files)

4.51 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
7.13 KB, patch
Details | Diff | Splinter Review
8.44 KB, patch
Details | Diff | Splinter Review
2.71 KB, patch
Details | Diff | Splinter Review
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)
Assignee: nobody → gwang
Blocks: 847034
Blocks: b2g-stk
Attachment #829070 - Attachment is obsolete: true
Attachment #829071 - Flags: review?(allstars.chh)
Attachment #829072 - Flags: review?(allstars.chh)
Attachment #829107 - Flags: review?(allstars.chh)
Attachment #829107 - Attachment is obsolete: true
Attachment #829107 - Flags: review?(allstars.chh)
Attachment #829071 - Attachment is obsolete: true
Attachment #829071 - Flags: review?(allstars.chh)
Attachment #830565 - Flags: review?(allstars.chh)
Attachment #830566 - Flags: review?(allstars.chh)
Attachment #830567 - Flags: review?(allstars.chh)
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?
Attachment #830565 - Flags: review?(allstars.chh) → review-
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
Attachment #830566 - Flags: review?(allstars.chh) → review-
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?
Attachment #829072 - Flags: review?(allstars.chh)
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
Attachment #830567 - Flags: review?(allstars.chh)
(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.
Attachment #830565 - Attachment is obsolete: true
Attachment #830566 - Attachment is obsolete: true
Attachment #831360 - Attachment description: Part1: STK Next Action Indicator IDL → Part1: STK Next Action Indicator IDL. v2
Attachment #831360 - Flags: review?(allstars.chh)
Attachment #829072 - Attachment is obsolete: true
Attachment #830567 - Attachment is obsolete: true
Attachment #831361 - Flags: review?(allstars.chh)
Attachment #831362 - Flags: review?(allstars.chh)
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~
Attachment #831363 - Flags: review?(allstars.chh)
Attachment #831362 - Flags: review?(allstars.chh)
Attachment #831362 - Attachment is obsolete: true
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.
Attachment #831402 - Flags: review?(allstars.chh)
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?
Attachment #831360 - Flags: review?(allstars.chh) → review-
(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.
Attachment #831361 - Flags: review?(allstars.chh) → review-
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'?
Attachment #831363 - Flags: review?(allstars.chh) → review-
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.
Flags: needinfo?(gwang)
(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.
Flags: needinfo?(gwang)
Delete unnecessary command ID, modify naming to "nextActionList."
Attachment #831360 - Attachment is obsolete: true
Remove unnecessary command id and modify naming to "nextActionList"
Attachment #831361 - Attachment is obsolete: true
(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
Attachment #831402 - Attachment is obsolete: true
Attachment #831402 - Flags: review?(allstars.chh)
Modify naming and correct constants.
Attachment #831363 - Attachment is obsolete: true
Update UUID.
Attachment #832109 - Attachment is obsolete: true
Attachment #832123 - Flags: review?(allstars.chh)
Depends on: 938466
Attachment #832111 - Flags: review?(allstars.chh)
Attachment #832117 - Flags: review?(allstars.chh)
Comment on attachment 832118 [details] [diff] [review]
Part4: Modify marionette for next action indicator. v3

Try: https://tbpl.mozilla.org/?tree=Try&rev=1dba38e53700
Attachment #832118 - Flags: review?(allstars.chh)
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
Attachment #832123 - Flags: review?(allstars.chh) → review-
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.
Attachment #832111 - Flags: review?(allstars.chh)
Modify comment in SimToolkit.idl
Move NULL & END_PROACTIVE_SESSION out of STK_CMD in nsIDOMIccManager.idl
Attachment #832123 - Attachment is obsolete: true
Modify constant base on part1.
Attachment #832111 - Attachment is obsolete: true
Add 2 test cases for STK_NEXT_ACTION_NULL & STK_NEXT_ACTION_END_PROACTIVE_SESSION.
Attachment #832117 - Attachment is obsolete: true
Attachment #832117 - Flags: review?(allstars.chh)
Attachment #832809 - Flags: review?(allstars.chh)
Attachment #832813 - Flags: review?(allstars.chh)
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Version: Trunk → unspecified
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
Attachment #832809 - Flags: review?(htsai)
Attachment #832809 - Flags: review?(allstars.chh)
Attachment #832809 - Flags: review+
Attachment #832813 - Flags: review?(allstars.chh) → review+
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
Attachment #832118 - Flags: review?(allstars.chh) → review+
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
Attachment #832814 - Flags: review+
Modify naming base on comment 45.
Attachment #832814 - Attachment is obsolete: true
Change base on comment 44.
Attachment #832118 - Attachment is obsolete: true
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
Attachment #8333680 - Attachment is obsolete: true
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!
Attachment #8335938 - Flags: review?(allstars.chh)
Attachment #8335938 - Flags: review?(allstars.chh) → review+
,
Attachment #8335938 - Attachment is obsolete: true
Update UUID & wording~  Dear Hsin-Yi, please help review this one~ thanks a lot!
Attachment #832809 - Attachment is obsolete: true
Attachment #832809 - Flags: review?(htsai)
Attachment #8339167 - Flags: review?(htsai)
Modify marionette constant base on Bug814637.
Attachment #8338279 - Attachment is obsolete: true
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.
Attachment #8339167 - Flags: review?(htsai) → review+
Correct typo and . base on comment 56 :)
Attachment #8339167 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: