921322 - [B2G STK] Add "searchForNextTag" in StkProactiveCmdHelper

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Georgia Wang]

In STK spec, some proactive command (ex. OPEN_CHANNEL) contains comprehension_tlvs with same tag. The old searchForTag function can not cover such case. We need to add a search function which can indicate the searching period.

for example, function searchForTagPos(tag, ctlvs, startPoint, endPoint)
and the return value should contain the tag position if found.

Comment 1

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

FYI,
Android has two functions for this, one is searchForTag and the other is searchForNextTag, which takes an Iterator.

http://androidxref.com/4.3_r2.1/xref/frameworks/opt/telephony/src/java/com/android/internal/telephony/cat/CommandParamsFactory.java#244

http://androidxref.com/4.3_r2.1/xref/frameworks/opt/telephony/src/java/com/android/internal/telephony/cat/CommandParamsFactory.java#262

Comment 2

[Georgia Wang]

Attachment: Improve searchForTagPos in STKProactiveCmdHelper

Add new function "searchForTagPos" in STKProactiveCmdHelper.
Consider some tag locate in critical position, only searchForNext Tag may not suitable. I design the function which can indicate the detail period for search..

Comment 3

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

Comment on attachment 811818 [details] [diff] [review]
Improve searchForTagPos in STKProactiveCmdHelper

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

Please write tests as well.
I think I've said this many many times.

::: dom/system/gonk/ril_worker.js
@@ +10238,5 @@
>      return {url: s};
>    },
>  
> +  //Search for Tag by position.
> +  searchForTagPos: function searchForTagPos(tag, ctlvs, startPoint, endPoint) {

Use a better and consistent naming please.
You use 'Pos' in the function name, I think it stands for Position.
However you use 'Point' in the arguments.
Also I think when this function is called, endPoint is always be the length of the array.
So it neccesary to have two more arguments?
We are only interested in the next tag after the previous search, so Iterator seems to be a better way to do this.

@@ +10240,5 @@
>  
> +  //Search for Tag by position.
> +  searchForTagPos: function searchForTagPos(tag, ctlvs, startPoint, endPoint) {
> +    let ctlvInfo = {
> +      no: 0,

no?

@@ +10254,5 @@
> +      if ((ctlvInfo.ctlv.tag & ~COMPREHENSIONTLV_FLAG_CR) == tag) {
> +        return ctlvInfo;
> +      }
> +    }
> +    return ctlvInfo;

Is this ever correct?
Have you ever tested it?
If not found in the array, you returned the last element?

Comment 4

[Georgia Wang]

Attachment: Add searchForNextTag in STKProactiveCmdHelper

Try result: pass
https://tbpl.mozilla.org/?tree=Try&rev=36c98deb10a8

Comment 5

[Georgia Wang]

Detail try result can be find here: https://secure.pub.build.mozilla.org/buildapi/self-serve/try/rev/36c98deb10a8

Comment 6

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

Comment on attachment 821490 [details] [diff] [review]
Add searchForNextTag in STKProactiveCmdHelper

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

Can you add a test case for this?
A xpcshell testfor iterator,
a marionette with 2 AlphaIds in setUpCall.

Comment 7

[Georgia Wang]

Attachment: Part1: Add searchForNextTag in STKProactiveCmdHelper

Comment 8

[Georgia Wang]

Attachment: Part2: xpcshell test for "searchForNextTag"

Comment 9

[Georgia Wang]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #6)
> Comment on attachment 821490 [details] [diff] [review]
> Add searchForNextTag in STKProactiveCmdHelper
> 
> Review of attachment 821490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you add a test case for this?
> A xpcshell testfor iterator,
> a marionette with 2 AlphaIds in setUpCall.
Xpcshell test already add in Part2.
For marionette, there's many 2-alphaId setUpCall tests in "dom/icc/test/marionette". 

Try Result: https://tbpl.mozilla.org/?tree=Try&rev=bfe674926dee

Comment 10

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

Comment on attachment 823915 [details] [diff] [review]
Part1: Add searchForNextTag in STKProactiveCmdHelper

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

::: dom/system/gonk/ril_worker.js
@@ +10226,5 @@
> +    return this.searchForNextTag(tag, iter);
> +  },
> +
> +  searchForNextTag: function searchForNextTag(tag, iter) {
> +    for (let ctlvInfo in iter ) {

for (let [index, ctlv] in iter)

Comment 11

[Georgia Wang]

Attachment: Part1: Add searchForNextTag in STKProactiveCmdHelper v2.

The only change is "for (let [index, ctlv] in iter)" 

Try: https://tbpl.mozilla.org/?tree=Try&rev=522af0d6dd61

Comment 12

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

Comment on attachment 827142 [details] [diff] [review]
Part1: Add searchForNextTag in STKProactiveCmdHelper v2.

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

::: dom/system/gonk/ril_worker.js
@@ +10226,5 @@
> +    return this.searchForNextTag(tag, iter);
> +  },
> +
> +  searchForNextTag: function searchForNextTag(tag, iter) {
> +    for (let [index, ctlv] in iter ) {

nit, remove the extra space after 'iter'

Comment 13

[Georgia Wang]

Attachment: Part1: Add searchForNextTag in STKProactiveCmdHelper v3

Remove space.

Comment 14

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

https://hg.mozilla.org/integration/b2g-inbound/rev/93c09adb48ab
https://hg.mozilla.org/integration/b2g-inbound/rev/4d3a70ce5548

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93c09adb48ab
https://hg.mozilla.org/mozilla-central/rev/4d3a70ce5548