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.

NI Wayne to set this as 1.4+ blocker.

1.4+'ing this as required to accommodate commercial ril change. refer to https://bugzilla.mozilla.org/show_bug.cgi?id=983522#c29

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.

JSON.stringify the value of |ril.cellbroadcast.searchlist| for better debugging.

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];

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.

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.

address suggestions in comment 6.

address suggestion in comment 9.

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.

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.

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.

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

address nits in comment 12.

try server result is green: https://tbpl.mozilla.org/?tree=Try&rev=41dce75191d6

https://hg.mozilla.org/integration/b2g-inbound/rev/6a55381c8812 https://hg.mozilla.org/integration/b2g-inbound/rev/d4f2542d3885 Note: I removed the empty line in Part2.

https://hg.mozilla.org/mozilla-central/rev/6a55381c8812 https://hg.mozilla.org/mozilla-central/rev/d4f2542d3885

https://hg.mozilla.org/releases/mozilla-aurora/rev/1af14a18424b https://hg.mozilla.org/releases/mozilla-aurora/rev/d92fdd3675ca