After bug 1072808, we always append some boolean attribute into system message no matter the value is `true` or `false`, e.g. isAlphabet [1] ... etc. But we only append it into dom even when the value is `true` [2]. IMO, it would be nice if we could have a consistent format in both system message and dom event. And since the value comes from |commandQualifier|, always append it seems more reasonable to me. [1] https://dxr.mozilla.org/mozilla-central/source/dom/icc/gonk/StkProactiveCmdFactory.jsm#593 [2] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#11040-11043

(In reply to Edgar Chen [:edgar][:echen] from comment #0) > After bug 1072808, we always append some boolean attribute into system > message no matter the value is `true` or `false`, e.g. isAlphabet [1] ... > etc. But we only append it into dom even when the value is `true` [2]. > > IMO, it would be nice if we could have a consistent format in both system > message and dom event. And since the value comes from |commandQualifier|, > always append it seems more reasonable to me. > > [1] > https://dxr.mozilla.org/mozilla-central/source/dom/icc/gonk/ > StkProactiveCmdFactory.jsm#593 > [2] > https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker. > js#11040-11043 Totally agree! It's painful that we need to make extra efforts on the consistency b/w two kind of "notifications." Hopefully, we could eliminate this soon. But before that happens, Edgar's comment makes sense to me, too.

Comment on attachment 8557069 [details] [diff] [review] Part 1-1: Having a consistent proactive command format in both system message and dom event, v2 Review of attachment 8557069 [details] [diff] [review]: ----------------------------------------------------------------- Hi Bevis, would mind reviewing this patch? Thank you.

Comment on attachment 8557819 [details] [diff] [review] Part 2-1: Remove redundant test data in test_stk_*.js, v2 Review of attachment 8557819 [details] [diff] [review]: ----------------------------------------------------------------- Found many test data are duplicated or redundant, so remove them first.

Comment on attachment 8557823 [details] [diff] [review] Part 2-2: Wrapping and mofiying the test data in test_stk_*.js, v2 Review of attachment 8557823 [details] [diff] [review]: ----------------------------------------------------------------- - Wrapping the command into multi-line and adding comments for each tag. - Adding/modifying the test to cover more different cases.

Comment on attachment 8557827 [details] [diff] [review] Part 2-3: Add tests for stk system message, v2 Review of attachment 8557827 [details] [diff] [review]: ----------------------------------------------------------------- Just found I miss to address test_stk_launch_browser.js, cancel the review first. :( Will fix it in next version.

Address comment #14.

Comment on attachment 8557069 [details] [diff] [review] Part 1-1: Having a consistent proactive command format in both system message and dom event, v2 Review of attachment 8557069 [details] [diff] [review]: ----------------------------------------------------------------- Overall, it looks good to me with the following 2 suggestions to be addressed: 1. It's nice to retrieve info from CommandQualifier at the beginning of processing each proactive command. However, it seems that |processLaunchBrowser| and |processPlayTone| are not taken into consideration. 2. Since all DOMString with value equal to "null" or "undefined" are converted to "null" in nsIStkProactiveCmd and its sub-interfaces, we could assign these DOMString properties directly in StkCommandMessage and its sub-classes w/o null-checking.

Comment on attachment 8557070 [details] [diff] [review] Part 1-2: Throwing exception if the mandatory field isn't showed in proactive command and refactoring handler of SelectItem and SetupMenu, v1 Review of attachment 8557070 [details] [diff] [review]: ----------------------------------------------------------------- LGTM and thanks for having the documentation of these functions. I notices that the suggestion of handling CommandQualifier at the beginning for |processPlayTone| & |processLaunchBrowser|. Hence, you can skip that suggestion in Patch Part 1-1. :)

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #17) > Comment on attachment 8557070 [details] [diff] [review] > Part 1-2: Throwing exception if the mandatory field isn't showed in > proactive command and refactoring handler of SelectItem and SetupMenu, v1 > > Review of attachment 8557070 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM and thanks for having the documentation of these functions. > > I notices that the suggestion of handling CommandQualifier at the beginning > for |processPlayTone| & |processLaunchBrowser|. > Hence, you can skip that suggestion in Patch Part 1-1. :) I guess I just put them into wrong part. I will still move them into part 1-1, thank you. :)

Comment on attachment 8557819 [details] [diff] [review] Part 2-1: Remove redundant test data in test_stk_*.js, v2 Review of attachment 8557819 [details] [diff] [review]: ----------------------------------------------------------------- Please see my suggestions inline to keep the PDUs for verifying alpha_id encoded with 0x80, 0x81, 0x82. Thanks! ::: dom/icc/tests/marionette/test_stk_select_item.js @@ -204,5 @@ > - items: [{identifier: 1, text: "81ル1"}, {identifier: 2, text: "81ル2"}, {identifier: 3, text: "81ル3"}]}}, > - {command: "d0348103012400820281828508820430a03832cb308f0901820430a03832cb318f0902820430a03832cb328f0903820430a03832cb33", > - expect: {commandQualifier: 0x00, > - title: "82ル0", > - items: [{identifier: 1, text: "82ル1"}, {identifier: 2, text: "82ル2"}, {identifier: 3, text: "82ル3"}]}}, 80ル0, 81ル1, 82ル2 looks not redundant to me because the purpose is clear to verify the decoding of alpha_id in 0x80, 0x81, 0x82. We could remove "工具箱选择" and keep these 3 PDUs instead. ::: dom/icc/tests/marionette/test_stk_send_sms.js @@ -143,5 @@ > - {command: "d0348103011300820281838508820430a03832cb3286099111223344556677f88b140100099110325476f84008080038003030eb0033", > - expect: {commandQualifier: 0x00, > - text: "82ル2"}}, > - {command: "d02a81030113008202818386099111223344556677f88b140100099110325476f84008080038003030eb0033", > - expect: {commandQualifier: 0x00}} 80ル0, 81ル1, 82ル2 looks not redundant to me because the purpose is clear to verify the decoding of alpha_id in 0x80, 0x81, 0x82. We could remove "中一" and keep these 3 test PDUs instead.

Comment on attachment 8557823 [details] [diff] [review] Part 2-2: Wrapping and mofiying the test data in test_stk_*.js, v2 Review of attachment 8557823 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments below and inline: 1. nit: s/identifies/identities/g for all the comments of "Device identifies". 2. for BIP proactive command, address tag is mandatory but haven't been included in these test PDUs. Thanks! ::: dom/icc/tests/marionette/test_stk_get_inkey.js @@ +109,5 @@ > + "options.isUCS2"); > + is(aCommand.options.isYesNoRequested, !!(aExpect.commandQualifier & 0x04), > + "options.isYesNoRequested"); > + is(aCommand.options.isHelpAvailable, !!(aExpect.commandQualifier & 0x80), > + "options.isHelpAvailable"); It seems that there is no test PDU here with 'commandQualifier & 0x80' to have the test coverage of |isHelpAvailable|. ::: dom/icc/tests/marionette/test_stk_get_input.js @@ +154,5 @@ > + "options.hideInput"); > + is(aCommand.options.isPacked, !!(aExpect.commandQualifier & 0x08), > + "options.isPacked"); > + is(aCommand.options.isHelpAvailable, !!(aExpect.commandQualifier & 0x80), > + "options.isHelpAvailable"); There is no test PDUs whose commandQualifier is set in bit 8(0x80) to have the test coverage of |isHelpAvailable|.

Comment on attachment 8557829 [details] [diff] [review] Part 2-4: Add tests for play tone and poll interval, v2 Review of attachment 8557829 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comment inline. Thanks! ::: dom/icc/tests/marionette/test_stk_play_tone.js @@ +80,5 @@ > + is(duration.timeUnit, aExpect.duration.timeUnit, > + "options.duration.timeUnit"); > + is(duration.timeInterval, aExpect.duration.timeInterval, > + "options.duration.timeInterval"); > + } It seems that the |duration| object is missed in the |expect| object even |duration| TLV is included in the test PDU.

Comment on attachment 8558891 [details] [diff] [review] Part 2-3: Add tests for stk system message, v3 Review of attachment 8558891 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks! :)

Attach correct one.

Comment on attachment 8559058 [details] [diff] [review] Part 1-1: Having a consistent proactive command format in both system message and dom event, v3 Review of attachment 8559058 [details] [diff] [review]: ----------------------------------------------------------------- Address review comment #16, - Moving the handling CommandQualifier for |processPlayTone| & |processLaunchBrowser| from part 1-2 to this part. - Removing the `null` checking in StkCommandMessage. Hi Bevis, would you mind reviewing again? Thank you.

Address comment #18, - Moving the handling CommandQualifier for |processPlayTone| & |processLaunchBrowser| to part 1-1. - Carrying r=bevis after r+.

Comment on attachment 8559068 [details] [diff] [review] Part 2-1: Remove redundant test data in test_stk_*.js, v3 Review of attachment 8559068 [details] [diff] [review]: ----------------------------------------------------------------- Address review comment #19, - Keeping the test data for verifying alpha_id encoded with 0x80, 0x81, 0x82.

Comment on attachment 8559072 [details] [diff] [review] Part 2-2: Wrapping and mofiying the test data in test_stk_*.js, v3 Review of attachment 8559072 [details] [diff] [review]: ----------------------------------------------------------------- Address review comment #20, - s/identifies/identities/g - Adding the mandatory tag, Address, in Open Channel test data. - Having test coverage for |isHelpAvailable|.

Carrying r=bevis after r+.

Comment on attachment 8559077 [details] [diff] [review] Part 2-4: Add tests for play tone and poll interval, v3 Review of attachment 8559077 [details] [diff] [review]: ----------------------------------------------------------------- Address review comment #21, - s/identifies/identities/g - Adding |duration| in expected result.

Hsin-Yi, do we care about making sure this change is backwards compatible with the 2.2 interfaces?

Hi Anshul, These changes shall be fine without impact to the compatibility to v2.2, IMO: For patch part 1-1, the major change is to refine our-own nsIStkProactiveCommand implmentations to ensure that an empty string "" won't be converted as null. This is just in case that empty string "" has to be distinguished from null string. So far, there is no such case in gaia. patch part 1-1 also includes some minor change without any impact to re-factor the retrieval of the attribute from CommmandQualifier of each proactive command. For patch part 1-2, we just want to make our ril_worker more robust by rejecting proactive command whose mandatory TLVs are unavailable. The rest of patches are to refactor/clean-up our own test cases for stk practive command. If you still have other concern, please let us know.

Comment on attachment 8559058 [details] [diff] [review] Part 1-1: Having a consistent proactive command format in both system message and dom event, v3 Review of attachment 8559058 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

Comment on attachment 8559068 [details] [diff] [review] Part 2-1: Remove redundant test data in test_stk_*.js, v3 Review of attachment 8559068 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

Comment on attachment 8559072 [details] [diff] [review] Part 2-2: Wrapping and mofiying the test data in test_stk_*.js, v3 Review of attachment 8559072 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

Comment on attachment 8559077 [details] [diff] [review] Part 2-4: Add tests for play tone and poll interval, v3 Review of attachment 8559077 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

Comment on attachment 8561139 [details] [diff] [review] Part 3: Fix test_ril_system_messenger.js, v1 Review of attachment 8561139 [details] [diff] [review]: ----------------------------------------------------------------- Just found the xpcshell test needs a revise, too. [1] Hi Bevis, mind reviewing this? Thank you. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e27979fe470

Comment on attachment 8561139 [details] [diff] [review] Part 3: Fix test_ril_system_messenger.js, v1 Review of attachment 8561139 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks!

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=199134bd88c8

https://hg.mozilla.org/integration/b2g-inbound/rev/291d50eee1fa https://hg.mozilla.org/integration/b2g-inbound/rev/daf5a384fc43 https://hg.mozilla.org/integration/b2g-inbound/rev/b8cb9b989866 https://hg.mozilla.org/integration/b2g-inbound/rev/45bf57830698 https://hg.mozilla.org/integration/b2g-inbound/rev/d1aa16df7afc https://hg.mozilla.org/integration/b2g-inbound/rev/ad6b607e9a7f https://hg.mozilla.org/integration/b2g-inbound/rev/05f14cef936f

https://hg.mozilla.org/mozilla-central/rev/291d50eee1fa https://hg.mozilla.org/mozilla-central/rev/daf5a384fc43 https://hg.mozilla.org/mozilla-central/rev/b8cb9b989866 https://hg.mozilla.org/mozilla-central/rev/45bf57830698 https://hg.mozilla.org/mozilla-central/rev/d1aa16df7afc https://hg.mozilla.org/mozilla-central/rev/ad6b607e9a7f https://hg.mozilla.org/mozilla-central/rev/05f14cef936f