Closed Bug 921322 Opened 8 years ago Closed 8 years ago

[B2G STK] Add "searchForNextTag" in StkProactiveCmdHelper

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

(2 files, 4 obsolete files)

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.
Assignee: nobody → gwang
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..
Attachment #811818 - Flags: review?(allstars.chh)
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?
Attachment #811818 - Flags: review?(allstars.chh) → review-
Status: NEW → UNCONFIRMED
Ever confirmed: false
Blocks: b2g-stk
Status: UNCONFIRMED → NEW
Component: DOM: Device Interfaces → RIL
Ever confirmed: true
Product: Core → Boot2Gecko
Version: Trunk → unspecified
Status: NEW → ASSIGNED
Summary: [B2G STK] Add "searchForTag by position" in StkProactiveCmdHelper → [B2G STK] Add "searchForNextTag" in StkProactiveCmdHelper
Try result: pass
https://tbpl.mozilla.org/?tree=Try&rev=36c98deb10a8
Attachment #811818 - Attachment is obsolete: true
Attachment #821490 - Flags: review?(allstars.chh)
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.
Attachment #821490 - Flags: review?(allstars.chh)
Attachment #821490 - Attachment is obsolete: true
(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
Attachment #823915 - Flags: review?(allstars.chh)
Attachment #823916 - Flags: review?(allstars.chh)
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)
Attachment #823915 - Flags: review?(allstars.chh)
Attachment #823916 - Flags: review?(allstars.chh) → review+
The only change is "for (let [index, ctlv] in iter)" 

Try: https://tbpl.mozilla.org/?tree=Try&rev=522af0d6dd61
Attachment #823915 - Attachment is obsolete: true
Attachment #827142 - Flags: review?(allstars.chh)
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'
Attachment #827142 - Flags: review?(allstars.chh) → review+
Remove space.
Attachment #827142 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93c09adb48ab
https://hg.mozilla.org/mozilla-central/rev/4d3a70ce5548
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.