Closed Bug 990918 Opened 6 years ago Closed 6 years ago

[B2G][CBS] Make RIL compatible for both new/old formats of ril.cellbroadcast.searchlist

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(2 files, 7 obsolete files)

4.62 KB, patch
bevis
: review+
Details | Diff | Splinter Review
1.76 KB, patch
bevis
: review+
Details | Diff | Splinter Review
Fire this bug to make sure gecko is compatible of both new/old search list format mentioned in bug 990910.
1. For old format, the configuration will be set directly.
2. For new format, we get only 1 search list from 1st element according to current radio technology.
Blocks: 990923
NI Wayne to set this as 1.4+ blocker.
blocking-b2g: --- → 1.4?
Flags: needinfo?(wchang)
Blocks: 983522
1.4+'ing this as required to accommodate commercial ril change.

refer to https://bugzilla.mozilla.org/show_bug.cgi?id=983522#c29
Flags: needinfo?(wchang)
blocking-b2g: 1.4? → 1.4+
This is to make sure RadioInterfaceLayer/ril_worker to be compatible to both old/new formats of searchlist from gaia.
The rest behavior of setting CBS configuration remains the same.
Attachment #8401163 - Flags: review?(htsai)
JSON.stringify the value of |ril.cellbroadcast.searchlist| for better debugging.
Attachment #8401163 - Attachment is obsolete: true
Attachment #8401163 - Flags: review?(htsai)
Attachment #8401176 - Flags: review?(htsai)
Comment on attachment 8401176 [details] [diff] [review]
Patch Part 1v2: Make RIL compatible for both new/old formats of ril.cellbroadcast.searchlist. r=htsai

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

Please see my comments below. Thank you!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2513,5 @@
>    },
>  
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    // We compare search lists with JSON.stringify() because
> +    // the ORDER of the properties in newSearchList from Gaia will be the same.

Is the assumption really true? I think our code should be more careful to not heavily rely on Gaia's behaviour.

@@ +3479,2 @@
>          }
> +        // TODO: Set searchlist for Multi-SIM. See Bug 921326.

let result = Array.isArray(aResult) ? aResult[0] : aResult;

Please add the above line so that the code indicates very clearly that we don't have multi-sim format right now. Also, we just need to pass the information of a specific Client to it's ril_worker.

@@ +3481,1 @@
>          this.setCellBroadcastSearchList(aResult);

then, s/aResult/result/

::: dom/system/gonk/ril_worker.js
@@ +1828,5 @@
>      }
>    },
>  
>    setCellBroadcastSearchList: function(options) {
> +    function getSearchListStr(aIsCdma, aSearchList) {

Since this._isCdma is a global variable which can be accessed here, seems no strong need to have "aIsCdma" argument. Please remove it.

@@ +1833,5 @@
> +      if (typeof aSearchList === "string" || aSearchList instanceof String) {
> +        return aSearchList;
> +      }
> +
> +      // TODO: Set search list for CDMA/GSM individually. Bug 990910

990910 is a meta bug. I guess we will address the individual setting in bug 990926?

@@ +1835,5 @@
> +      }
> +
> +      // TODO: Set search list for CDMA/GSM individually. Bug 990910
> +      return aSearchList && aSearchList[0] &&
> +        (aSearchList[0][(aIsCdma) ? "cdma" : "gsm"]);

No this inline operation. Please,

let prop = this._isCdma ? "cdma" : "gsm";
return aSearchList && aSearchList[prop];
Attachment #8401176 - Flags: review?(htsai)
Comment on attachment 8401164 [details] [diff] [review]
Patch Part 2: Add Xpcshell Test Case for setCellBroadcastSearchList(). r=htsai

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

Nice! Thank you~

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +166,5 @@
> +
> +  test(false, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(true, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(false, searchList, "1,2,2,3,3,4,4,5");
> +  test(true, searchList, "5,6,6,7,7,8,8,9");

Please add one more test for null searchList.
Attachment #8401164 - Flags: review?(htsai) → review+
Comment on attachment 8401176 [details] [diff] [review]
Patch Part 1v2: Make RIL compatible for both new/old formats of ril.cellbroadcast.searchlist. r=htsai

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2513,5 @@
>    },
>  
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    // We compare search lists with JSON.stringify() because
> +    // the ORDER of the properties in newSearchList from Gaia will be the same.

Yes, I discussed this with Arthur offline.
But you're right, we should not rely on Gaia's behavior instead.

@@ +3479,2 @@
>          }
> +        // TODO: Set searchlist for Multi-SIM. See Bug 921326.

Sounds good! We don't have to address this until fixing bug 921326. :)

::: dom/system/gonk/ril_worker.js
@@ +1828,5 @@
>      }
>    },
>  
>    setCellBroadcastSearchList: function(options) {
> +    function getSearchListStr(aIsCdma, aSearchList) {

Will do.

@@ +1833,5 @@
> +      if (typeof aSearchList === "string" || aSearchList instanceof String) {
> +        return aSearchList;
> +      }
> +
> +      // TODO: Set search list for CDMA/GSM individually. Bug 990910

Sorry for wrong bug id.

@@ +1835,5 @@
> +      }
> +
> +      // TODO: Set search list for CDMA/GSM individually. Bug 990910
> +      return aSearchList && aSearchList[0] &&
> +        (aSearchList[0][(aIsCdma) ? "cdma" : "gsm"]);

Will do.
Comment on attachment 8401164 [details] [diff] [review]
Patch Part 2: Add Xpcshell Test Case for setCellBroadcastSearchList(). r=htsai

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

Nice! Thank you~

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +160,5 @@
> +
> +  let searchListStr = "1,2,3,4";
> +  let searchList = [
> +    { gsm: "1,2,3,4", cdma: "5,6,7,8" },
> +    { gsm: "11,12,13,14", cdma: "15,16,17,18" }

Thanks for reminding. :) 
Please also refactor this format according to the revision of Part1.

@@ +166,5 @@
> +
> +  test(false, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(true, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(false, searchList, "1,2,2,3,3,4,4,5");
> +  test(true, searchList, "5,6,6,7,7,8,8,9");

Please add one more test for null searchList.
Attachment #8401164 - Flags: review+
Attachment #8401164 - Attachment is obsolete: true
Comment on attachment 8401247 [details] [diff] [review]
Patch Part 1v3: Make RIL compatible for both new/old formats of ril.cellbroadcast.searchlist. r=htsai

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

Yah, thank you!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2513,5 @@
>    },
>  
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    if ((newSearchList == this._cellBroadcastSearchList)
> +        || (newSearchList && this._cellBroadcastSearchList

nit: move an operator to the end of a line. Just a convention style issue though I know the style isn't absolute consistent in ril_worker.js now :p

@@ +2514,5 @@
>  
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    if ((newSearchList == this._cellBroadcastSearchList)
> +        || (newSearchList && this._cellBroadcastSearchList
> +            && newSearchList.gsm == this._cellBroadcastSearchList.gsm

ditto.

@@ +2515,5 @@
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    if ((newSearchList == this._cellBroadcastSearchList)
> +        || (newSearchList && this._cellBroadcastSearchList
> +            && newSearchList.gsm == this._cellBroadcastSearchList.gsm
> +            && newSearchList.cdma == this._cellBroadcastSearchList.cdma)) {

ditto.

::: dom/system/gonk/ril_worker.js
@@ +1837,5 @@
> +      // TODO: Set search list for CDMA/GSM individually. Bug 990926
> +      let prop = this._isCdma ? "cdma" : "gsm";
> +
> +      return aSearchList && aSearchList[prop];
> +    }.bind(this);

Do we really need "bind" here? Looks it's safe to go without it. Please remove it if not necessary.
Attachment #8401247 - Flags: review?(htsai) → review+
Comment on attachment 8401248 [details] [diff] [review]
Patch Part 2 v2: Add Xpcshell Test Case for setCellBroadcastSearchList(). r=htsai

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

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +164,5 @@
> +  test(false, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(true, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(false, searchList, "1,2,2,3,3,4,4,5");
> +  test(true, searchList, "5,6,6,7,7,8,8,9");
> +  test(false, null, "null");

Does this really pass? I think it should be |test(false, null, null);|, no?

@@ +165,5 @@
> +  test(true, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(false, searchList, "1,2,2,3,3,4,4,5");
> +  test(true, searchList, "5,6,6,7,7,8,8,9");
> +  test(false, null, "null");
> +  test(true, null, "null");

ditto.
Attachment #8401248 - Flags: review?(htsai)
Comment on attachment 8401247 [details] [diff] [review]
Patch Part 1v3: Make RIL compatible for both new/old formats of ril.cellbroadcast.searchlist. r=htsai

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2513,5 @@
>    },
>  
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    if ((newSearchList == this._cellBroadcastSearchList)
> +        || (newSearchList && this._cellBroadcastSearchList

will do.

@@ +2514,5 @@
>  
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    if ((newSearchList == this._cellBroadcastSearchList)
> +        || (newSearchList && this._cellBroadcastSearchList
> +            && newSearchList.gsm == this._cellBroadcastSearchList.gsm

will do.

@@ +2515,5 @@
> +  setCellBroadcastSearchList: function(newSearchList) {
> +    if ((newSearchList == this._cellBroadcastSearchList)
> +        || (newSearchList && this._cellBroadcastSearchList
> +            && newSearchList.gsm == this._cellBroadcastSearchList.gsm
> +            && newSearchList.cdma == this._cellBroadcastSearchList.cdma)) {

will do.

::: dom/system/gonk/ril_worker.js
@@ +1837,5 @@
> +      // TODO: Set search list for CDMA/GSM individually. Bug 990926
> +      let prop = this._isCdma ? "cdma" : "gsm";
> +
> +      return aSearchList && aSearchList[prop];
> +    }.bind(this);

Yes, we need this. Otherwise, I got "this is undefined" while running the test case. :(
Comment on attachment 8401248 [details] [diff] [review]
Patch Part 2 v2: Add Xpcshell Test Case for setCellBroadcastSearchList(). r=htsai

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

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +164,5 @@
> +  test(false, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(true, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(false, searchList, "1,2,2,3,3,4,4,5");
> +  test(true, searchList, "5,6,6,7,7,8,8,9");
> +  test(false, null, "null");

Yes, it was passed.
I compare the results with string formats:
do_check_eq("" + context.RIL.cellBroadcastConfigs.MMI, aExpected);

That's why I put "null" instead of |null|. :)

@@ +165,5 @@
> +  test(true, searchListStr, "1,2,2,3,3,4,4,5");
> +  test(false, searchList, "1,2,2,3,3,4,4,5");
> +  test(true, searchList, "5,6,6,7,7,8,8,9");
> +  test(false, null, "null");
> +  test(true, null, "null");

ditto.
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #14)
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1837,5 @@
> > +      // TODO: Set search list for CDMA/GSM individually. Bug 990926
> > +      let prop = this._isCdma ? "cdma" : "gsm";
> > +
> > +      return aSearchList && aSearchList[prop];
> > +    }.bind(this);
> 
> Yes, we need this. Otherwise, I got "this is undefined" while running the
> test case. :(
Indeed, we need this bind!
Comment on attachment 8401248 [details] [diff] [review]
Patch Part 2 v2: Add Xpcshell Test Case for setCellBroadcastSearchList(). r=htsai

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

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
@@ +153,5 @@
> +    context.RIL._isCdma = aIsCdma;
> +
> +    let options = { searchList: aSearchList };
> +    context.RIL.setCellBroadcastSearchList(options);
> +    do_check_eq("" + context.RIL.cellBroadcastConfigs.MMI, aExpected);

Oh... how sharp my eyes are ORZ I missed the "" ...

Please drop a comment saying you are enforcing the aSearchList to a string in case anyone like me! Thank you.
Attachment #8401248 - Flags: review+
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #17)
> Comment on attachment 8401248 [details] [diff] [review]
> Patch Part 2 v2: Add Xpcshell Test Case for setCellBroadcastSearchList().
> r=htsai
> 
> Review of attachment 8401248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/tests/test_ril_worker_cellbroadcast_config.js
> @@ +153,5 @@
> > +    context.RIL._isCdma = aIsCdma;
> > +
> > +    let options = { searchList: aSearchList };
> > +    context.RIL.setCellBroadcastSearchList(options);
> > +    do_check_eq("" + context.RIL.cellBroadcastConfigs.MMI, aExpected);
> 
> Oh... how sharp my eyes are ORZ I missed the "" ...
> 
> Please drop a comment saying you are enforcing the aSearchList to a string
> in case anyone like me! Thank you.

Sorry! Will do. :)
You need to log in before you can comment on or make changes to this bug.