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

RESOLVED FIXED

Status

Firefox OS
RIL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Georgia Wang, Assigned: Georgia Wang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 23 obsolete attachments)

4.51 KB, patch
allstars
: 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
(Assignee)

Description

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

Updated

5 years ago
Assignee: nobody → gwang
Blocks: 847034
(Assignee)

Updated

5 years ago
Blocks: 791161
(Assignee)

Comment 1

5 years ago
Created attachment 829070 [details] [diff] [review]
Part1: STK Next Action Indicator IDL
(Assignee)

Comment 2

5 years ago
Created attachment 829071 [details] [diff] [review]
Part2: STK Next Action Indicator RIL handler.
(Assignee)

Comment 3

5 years ago
Created attachment 829072 [details] [diff] [review]
Part3: STK Add xpcshell test for set up menu.
(Assignee)

Comment 5

5 years ago
Created attachment 829107 [details] [diff] [review]
Part1: STK Next Action Indicator IDL
Attachment #829070 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #829071 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #829072 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #829107 - Flags: review?(allstars.chh)
(Assignee)

Comment 6

5 years ago
Created attachment 830565 [details] [diff] [review]
Part1: STK Next Action Indicator IDL
Attachment #829107 - Attachment is obsolete: true
Attachment #829107 - Flags: review?(allstars.chh)
(Assignee)

Comment 7

5 years ago
Created attachment 830566 [details] [diff] [review]
Part2: STK Next Action Indicator RIL handler.
Attachment #829071 - Attachment is obsolete: true
Attachment #829071 - Flags: review?(allstars.chh)
(Assignee)

Comment 8

5 years ago
Created attachment 830567 [details] [diff] [review]
Part4: Modify marionette for next action indicator
(Assignee)

Updated

5 years ago
Attachment #830565 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #830566 - Flags: review?(allstars.chh)
(Assignee)

Updated

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

Comment 14

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

Comment 15

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

Comment 16

5 years ago
Created attachment 831360 [details] [diff] [review]
Part1: STK Next Action Indicator IDL. v2
Attachment #830565 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 831361 [details] [diff] [review]
Part2: STK Next Action Indicator RIL handler. v2
Attachment #830566 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #831360 - Attachment description: Part1: STK Next Action Indicator IDL → Part1: STK Next Action Indicator IDL. v2
Attachment #831360 - Flags: review?(allstars.chh)
(Assignee)

Comment 18

5 years ago
Created attachment 831362 [details] [diff] [review]
Part3: STK Add xpcshell test for set up menu and select item v2
Attachment #829072 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 831363 [details] [diff] [review]
Part4: Modify marionette for next action indicator. v2
Attachment #830567 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #831361 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #831362 - Flags: review?(allstars.chh)
(Assignee)

Comment 20

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

Updated

5 years ago
Attachment #831362 - Flags: review?(allstars.chh)
(Assignee)

Comment 21

5 years ago
Created attachment 831402 [details] [diff] [review]
Part3: STK Add xpcshell test for set up menu and select item v2
Attachment #831362 - Attachment is obsolete: true
(Assignee)

Comment 22

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

Comment 29

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

Comment 30

5 years ago
Created attachment 832109 [details] [diff] [review]
Part1: STK Next Action Indicator IDL v3

Delete unnecessary command ID, modify naming to "nextActionList."
Attachment #831360 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
Created attachment 832111 [details] [diff] [review]
Part2: STK Next Action Indicator RIL handler. v3

Remove unnecessary command id and modify naming to "nextActionList"
Attachment #831361 - Attachment is obsolete: true
(Assignee)

Comment 32

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

Comment 33

5 years ago
Created attachment 832117 [details] [diff] [review]
Part3: STK Add xpcshell test for set up menu and select item v3

Modify naiList => nextActionList
Attachment #831402 - Attachment is obsolete: true
Attachment #831402 - Flags: review?(allstars.chh)
(Assignee)

Comment 34

5 years ago
Created attachment 832118 [details] [diff] [review]
Part4: Modify marionette for next action indicator. v3

Modify naming and correct constants.
Attachment #831363 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
Created attachment 832123 [details] [diff] [review]
Part1: STK Next Action Indicator IDL v3

Update UUID.
Attachment #832109 - Attachment is obsolete: true
Attachment #832123 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Depends on: 938466
(Assignee)

Updated

5 years ago
Attachment #832111 - Flags: review?(allstars.chh)
(Assignee)

Updated

5 years ago
Attachment #832117 - Flags: review?(allstars.chh)
(Assignee)

Comment 37

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

Comment 40

5 years ago
Created attachment 832809 [details] [diff] [review]
Part1: STK Next Action Indicator IDL v4

Modify comment in SimToolkit.idl
Move NULL & END_PROACTIVE_SESSION out of STK_CMD in nsIDOMIccManager.idl
Attachment #832123 - Attachment is obsolete: true
(Assignee)

Comment 41

5 years ago
Created attachment 832813 [details] [diff] [review]
Part2: STK Next Action Indicator RIL handler. v4

Modify constant base on part1.
Attachment #832111 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 832814 [details] [diff] [review]
Part3: STK Add xpcshell test for set up menu and select item v4

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)
(Assignee)

Updated

5 years ago
Attachment #832809 - Flags: review?(allstars.chh)
(Assignee)

Updated

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

Comment 46

5 years ago
Created attachment 8333679 [details] [diff] [review]
Part3: STK Add xpcshell test for set up menu and select item v5

Modify naming base on comment 45.
Attachment #832814 - Attachment is obsolete: true
(Assignee)

Comment 47

5 years ago
Created attachment 8333680 [details] [diff] [review]
Part4: Modify marionette for next action indicator. v4.

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

Comment 50

5 years ago
Created attachment 8335938 [details] [diff] [review]
Part4: Modify marionette for next action indicator. v5

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
(Assignee)

Comment 51

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

Comment 52

5 years ago
Created attachment 8338279 [details] [diff] [review]
Part4: Modify marionette for next action indicator. v6

,
Attachment #8335938 - Attachment is obsolete: true
(Assignee)

Comment 53

5 years ago
Created attachment 8339167 [details] [diff] [review]
Part1: STK Next Action Indicator IDL v5.

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)
(Assignee)

Comment 54

5 years ago
Created attachment 8339169 [details] [diff] [review]
Part4: Modify marionette for next action indicator. v6

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+
(Assignee)

Comment 57

5 years ago
Created attachment 8340272 [details] [diff] [review]
Part1: STK Next Action Indicator IDL v6

Correct typo and . base on comment 56 :)
Attachment #8339167 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.