Steps: 0. Open the dialer app. 1. Send Call Waiting MMI request (ex: *#43#, *43#, #43# etc...), then press call button. Expected: Device will send out call waiting command to network. Current: Device will return failure in ril_worker.js,

update patch with hg header

Comment on attachment 783070 [details] [diff] [review] 899400.patch Review of attachment 783070 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch, Shawn. I have some ideas to refactor the code. Please see my comments below. ::: dom/system/gonk/ril_worker.js @@ +2590,5 @@ > + } > + options.mmiServiceCode = MMI_KS_SC_CALL_WAITING; > + > + if (mmi.procedure === MMI_PROCEDURE_INTERROGATION) { > + this.queryCallWaiting(options); I would like to introduce an additional callback here. In this way, when we get responses from REQUEST_QUERY_CALL_WAITING, we can call that callback to do what we need for the original sendMMI request. That is, doing the things from the newly added lines, from #5280 to #5295. I believe the solution should help maintain the original query call waiting logic and make it clear in support of MMI. @@ +5300,4 @@ > this.sendChromeMessage(options); > }; > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { Don't we need to update "options.statusMessage" here?

Hi HsinYi: Please check my inline reply. (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3) > Comment on attachment 783070 [details] [diff] [review] > 899400.patch > > Review of attachment 783070 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you for the patch, Shawn. I have some ideas to refactor the code. > Please see my comments below. > > ::: dom/system/gonk/ril_worker.js > @@ +2590,5 @@ > > + } > > + options.mmiServiceCode = MMI_KS_SC_CALL_WAITING; > > + > > + if (mmi.procedure === MMI_PROCEDURE_INTERROGATION) { > > + this.queryCallWaiting(options); > > I would like to introduce an additional callback here. In this way, when we > get responses from REQUEST_QUERY_CALL_WAITING, we can call that callback to > do what we need for the original sendMMI request. That is, doing the things > from the newly added lines, from #5280 to #5295. I believe the solution > should help maintain the original query call waiting logic and make it clear > in support of MMI. > If we implement a new function to handle the code between #5280 and #5295, it may only be used for CallWaiting, the reason is different commands response may variant. Ex: there is only one return value for REQUEST_QUERY_FACILITY_LOCK which indicate both on/off and serviceClass, however, there are two return values for REQUEST_QUERY_CALL_WAITING. the 1st is for enable or not, and, the 2nd is for serviceClass. In my personal opinion, I will suggest to creat an additional function to just handle serviceClass (ex: line between #5285 and #5291), then assign it to options.additionalInformation, is that okay for me do so? > @@ +5300,4 @@ > > this.sendChromeMessage(options); > > }; > > > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > > Don't we need to update "options.statusMessage" here? for both line #5283 and #5294, the options.statusMessage has been filled up, and, this options.statusMessage is only used for MMI request by checking the code around. I am not sure if I got your point. Finally, thanks for your suggestion and comment. sku

(In reply to shawn ku [:sku] from comment #4) > Hi HsinYi: > Please check my inline reply. > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3) > > Comment on attachment 783070 [details] [diff] [review] > > 899400.patch > > > > Review of attachment 783070 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Thank you for the patch, Shawn. I have some ideas to refactor the code. > > Please see my comments below. > > > > ::: dom/system/gonk/ril_worker.js > > @@ +2590,5 @@ > > > + } > > > + options.mmiServiceCode = MMI_KS_SC_CALL_WAITING; > > > + > > > + if (mmi.procedure === MMI_PROCEDURE_INTERROGATION) { > > > + this.queryCallWaiting(options); > > > > I would like to introduce an additional callback here. In this way, when we > > get responses from REQUEST_QUERY_CALL_WAITING, we can call that callback to > > do what we need for the original sendMMI request. That is, doing the things > > from the newly added lines, from #5280 to #5295. I believe the solution > > should help maintain the original query call waiting logic and make it clear > > in support of MMI. > > > Seems I didn't express myself clear enough :( > If we implement a new function to handle the code between #5280 and #5295, > it may only be used for CallWaiting, Yes, my idea is that we should have a specific callback function for this sendMMI request, and this callback is surely for CallWaiting querying only. > the reason is different commands > response may variant. That's true. > Ex: there is only one return value for REQUEST_QUERY_FACILITY_LOCK which > indicate both on/off and serviceClass, Then for this example, we might need another callback here. > however, there are two return values > for REQUEST_QUERY_CALL_WAITING. the 1st is for enable or not, and, the 2nd > is for serviceClass. > > In my personal opinion, I will suggest to creat an additional function to > just handle serviceClass (ex: line between #5285 and #5291), then assign it > to options.additionalInformation, > > is that okay for me do so? > > > > @@ +5300,4 @@ > > > this.sendChromeMessage(options); > > > }; > > > > > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > > > > Don't we need to update "options.statusMessage" here? > > for both line #5283 and #5294, the options.statusMessage has been filled up, > and, this options.statusMessage is only used for MMI request by checking the > code around. I am not sure if I got your point. > > Finally, thanks for your suggestion and comment. > sku

update callback method to handle MMI call waiting.

Comment on attachment 783641 [details] [diff] [review] 2nd patch for 899400 Review of attachment 783641 [details] [diff] [review]: ----------------------------------------------------------------- test case part is not merged. cancel the review request first

update callback method to handle MMI call waiting.

Comment on attachment 783653 [details] [diff] [review] 2nd patch for 899400 Review of attachment 783653 [details] [diff] [review]: ----------------------------------------------------------------- Closer! Let's keep moving on :P ::: dom/system/gonk/ril_worker.js @@ +1451,5 @@ > * Query call waiting status. > * > */ > queryCallWaiting: function queryCallWaiting(options) { > + function callback(options) { This is the callback for sendMMI. It should be in "switch-case MMI_SC_CALL_WAITING," not here. And we will need one more line "this.sendChromeMessage(options);" at the end of the callback function. @@ +5299,5 @@ > this.sendChromeMessage(options); > return; > } > options.length = Buf.readUint32(); > + if (options.rilMessageType === "sendMMI") { We should do if (options.callback) { options.callback.call(this, options); return; }

Comment on attachment 783653 [details] [diff] [review] 2nd patch for 899400 Review of attachment 783653 [details] [diff] [review]: ----------------------------------------------------------------- Let me provide (with Hsin-Yi's permission) some feedback here guys since I've been involved on other MMI command developments. ::: dom/system/gonk/ril_worker.js @@ +2623,5 @@ > + mmi.procedure === MMI_PROCEDURE_REGISTRATION) { > + options.enabled = true; > + } else { > + options.enabled = false; > + } You should take care of the rest MMI procedures as well. @@ +5299,5 @@ > this.sendChromeMessage(options); > return; > } > options.length = Buf.readUint32(); > + if (options.rilMessageType === "sendMMI") { We have not used callback functions for handling sendMMI results in the handlers. IMHO we should not brake the design about how we have implemented other MMI commands. I'd suggest to take a look and follow the same approach implemented in bug 833754 for example. @@ +5314,5 @@ > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > options.success = (options.rilRequestError === 0); > if (!options.success) { > options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]; > } You must handle the result of setting call waiting preferences through MMI codes as well. I mean you must return a value for `options.statusMessage` property here.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9) > Comment on attachment 783653 [details] [diff] [review] > 2nd patch for 899400 > > Review of attachment 783653 [details] [diff] [review]: > ----------------------------------------------------------------- > > Closer! Let's keep moving on :P > > ::: dom/system/gonk/ril_worker.js > @@ +1451,5 @@ > > * Query call waiting status. > > * > > */ > > queryCallWaiting: function queryCallWaiting(options) { > > + function callback(options) { > > This is the callback for sendMMI. It should be in "switch-case > MMI_SC_CALL_WAITING," not here. And we will need one more line > "this.sendChromeMessage(options);" at the end of the callback function. > > @@ +5299,5 @@ > > this.sendChromeMessage(options); > > return; > > } > > options.length = Buf.readUint32(); > > + if (options.rilMessageType === "sendMMI") { > > We should do > > if (options.callback) { > options.callback.call(this, options); > return; > } Hi HsinYi: Thanks for your feedback, will update this, and, have a discussion with you later. (In reply to José Antonio Olivera Ortega [:jaoo] from comment #10) > Comment on attachment 783653 [details] [diff] [review] > 2nd patch for 899400 > > Review of attachment 783653 [details] [diff] [review]: > ----------------------------------------------------------------- > > Let me provide (with Hsin-Yi's permission) some feedback here guys since > I've been involved on other MMI command developments. > > ::: dom/system/gonk/ril_worker.js > @@ +2623,5 @@ > > + mmi.procedure === MMI_PROCEDURE_REGISTRATION) { > > + options.enabled = true; > > + } else { > > + options.enabled = false; > > + } > > You should take care of the rest MMI procedures as well. > > @@ +5299,5 @@ > > this.sendChromeMessage(options); > > return; > > } > > options.length = Buf.readUint32(); > > + if (options.rilMessageType === "sendMMI") { > > We have not used callback functions for handling sendMMI results in the > handlers. IMHO we should not brake the design about how we have implemented > other MMI commands. I'd suggest to take a look and follow the same approach > implemented in bug 833754 for example. > > @@ +5314,5 @@ > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > > options.success = (options.rilRequestError === 0); > > if (!options.success) { > > options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]; > > } > > You must handle the result of setting call waiting preferences through MMI > codes as well. I mean you must return a value for `options.statusMessage` > property here. Hi José: Thanks for your comment. will check how to adopt it in new patch. Besides, For the REQUEST_SET_CALL_WAITING request, I think there should be *no* options.statusMessage filled-up, but simply return ok/ko. The reason is we can only make sure this REQUEST_SET_CALL_WAITING request ok or not in gecko/gaia layer, but, it may still associate with other SS results. For the final result, *#43# should be used to know the exact status (which is stored in MSC side) if user would like to be updated for real status. cheers, sku

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #10) > Comment on attachment 783653 [details] [diff] [review] > 2nd patch for 899400 > > Review of attachment 783653 [details] [diff] [review]: > ----------------------------------------------------------------- > > Let me provide (with Hsin-Yi's permission) some feedback here guys since > I've been involved on other MMI command developments. > > ::: dom/system/gonk/ril_worker.js > @@ +2623,5 @@ > > + mmi.procedure === MMI_PROCEDURE_REGISTRATION) { > > + options.enabled = true; > > + } else { > > + options.enabled = false; > > + } > > You should take care of the rest MMI procedures as well. Hi José: Plesae correct me if anything I said is wrong. For defined SS, there are only five types of format, that is - (*, #, **, ## or *#.) , and, ril_consts.js has below definition. // MMI procedure as defined in TS.22.030 6.5.2 this.MMI_PROCEDURE_ACTIVATION = "*"; this.MMI_PROCEDURE_DEACTIVATION = "#"; this.MMI_PROCEDURE_INTERROGATION = "*#"; this.MMI_PROCEDURE_REGISTRATION = "**"; this.MMI_PROCEDURE_ERASURE = "##"; From my code, we already handle MMI_PROCEDURE_INTERROGATION/MMI_PROCEDURE_ACTIVATION/MMI_PROCEDURE_REGISTRATION, the rest of MMI_PROCEDURE_DEACTIVATION/MMI_PROCEDURE_ERASURE will be handled in "else" case. Besides, if mmi code without proper prefix (ex: *, #, **, ## or *#), we will got null result from _parseMMI, that means, there is no way for code to run into any defined SS switch case. if (mmi.procedure === MMI_PROCEDURE_INTERROGATION) { this.queryCallWaiting(options); return; } if (mmi.procedure === MMI_PROCEDURE_ACTIVATION || mmi.procedure === MMI_PROCEDURE_REGISTRATION) { options.enabled = true; } else { options.enabled = false; } cheers, sku > > @@ +5299,5 @@ > > this.sendChromeMessage(options); > > return; > > } > > options.length = Buf.readUint32(); > > + if (options.rilMessageType === "sendMMI") { > > We have not used callback functions for handling sendMMI results in the > handlers. IMHO we should not brake the design about how we have implemented > other MMI commands. I'd suggest to take a look and follow the same approach > implemented in bug 833754 for example. > > @@ +5314,5 @@ > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > > options.success = (options.rilRequestError === 0); > > if (!options.success) { > > options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]; > > } > > You must handle the result of setting call waiting preferences through MMI > codes as well. I mean you must return a value for `options.statusMessage` > property here.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #10) > Comment on attachment 783653 [details] [diff] [review] > 2nd patch for 899400 > > Review of attachment 783653 [details] [diff] [review]: > ----------------------------------------------------------------- > > Let me provide (with Hsin-Yi's permission) some feedback here guys since > I've been involved on other MMI command developments. > Jose, Thank you very much for the feedback! You are always welcome :) > ::: dom/system/gonk/ril_worker.js > @@ +2623,5 @@ > > + mmi.procedure === MMI_PROCEDURE_REGISTRATION) { > > + options.enabled = true; > > + } else { > > + options.enabled = false; > > + } > > You should take care of the rest MMI procedures as well. Shawn and I had discussion regarding your comment here. Were you suggesting we doing the following? if (mmi.procedure === MMI_PROCEDURE_ACTIVATION) { options.enabled = true; } else if (mmi.procedure === MMI_PROCEDURE_DEACTIVATION) { options.enabled = false; } else { _sendMMIError(MMI_ERROR_KS_NOT_SUPPORTED, ...); return; } > > @@ +5299,5 @@ > > this.sendChromeMessage(options); > > return; > > } > > options.length = Buf.readUint32(); > > + if (options.rilMessageType === "sendMMI") { > > We have not used callback functions for handling sendMMI results in the > handlers. IMHO we should not brake the design about how we have implemented > other MMI commands. I'd suggest to take a look and follow the same approach > implemented in bug 833754 for example. > It's actually my suggestion. I was thinking how to help modulize ril_worker.js as you can see its code length grows so fast. And also, as we would like to support more and more features, that said, a single RIL_REQUEST_* might be used to achieve variant users request, introducing individual callback functions looks the way to me. Do you think the proposal works? Or please let me know if you foresee any problems that I might be missing. Maintaining consistent coding style is also what we keep in mind. I didn't attempt to break the design. I am sorry if it made you feel that way. If you don't think the callback method is good, we are willing to change our method. If you feel the way is gonna work, then we are also willing to have more discussion with you to see how we maintain the consistent mmi code style. :) How does that sound to you? Looking forward to hearing your feedback, and thank you so much! > @@ +5314,5 @@ > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > > options.success = (options.rilRequestError === 0); > > if (!options.success) { > > options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]; > > } > > You must handle the result of setting call waiting preferences through MMI > codes as well. I mean you must return a value for `options.statusMessage` > property here. We noticed that in one previous comment. Thank you for pointing this out again!

correct typo for Description

Comment on attachment 784219 [details] [diff] [review] 3rd patch for 899400, with callback version Review of attachment 784219 [details] [diff] [review]: ----------------------------------------------------------------- Empty patches XD

Weird empty patch, re-upload again.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #13) > Shawn and I had discussion regarding your comment here. Were you suggesting > we doing the following? > > if (mmi.procedure === MMI_PROCEDURE_ACTIVATION) { > options.enabled = true; > } else if (mmi.procedure === MMI_PROCEDURE_DEACTIVATION) { > options.enabled = false; > } else { > _sendMMIError(MMI_ERROR_KS_NOT_SUPPORTED, ...); > return; > } Yes, that's the idea. That way the RIL plumbing reports the error for registration and erasure procedures since they are not used for this MMI code. > > We have not used callback functions for handling sendMMI results in the > > handlers. IMHO we should not brake the design about how we have implemented > > other MMI commands. I'd suggest to take a look and follow the same approach > > implemented in bug 833754 for example. > > > > It's actually my suggestion. I was thinking how to help modulize > ril_worker.js as you can see its code length grows so fast. Oh, I see. Good point then. > If you > don't think the callback method is good, we are willing to change our > method. If you feel the way is gonna work, then we are also willing to have > more discussion with you to see how we maintain the consistent mmi code > style. :) The callback approach works for me too. It helps to keep the RIL request handlers cleaner. I guess we need to move to this approach for the rest of MMI commands already implemented.

Comment on attachment 784249 [details] [diff] [review] 3rd patch for 899400, with callback version Review of attachment 784249 [details] [diff] [review]: ----------------------------------------------------------------- Shawn, I'm not within the RIL peer list so I cannot give formals r+ but I'll be happy to help you and provide some feedback if needed. ::: dom/system/gonk/ril_worker.js @@ +5323,4 @@ > this.sendChromeMessage(options); > }; > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { Still need to handle the result of setting call waiting preferences through MMI codes as well. This is needed since the `options.statusMessage` property is used in Gaia for localization stuff.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #20) > Comment on attachment 784249 [details] [diff] [review] > 3rd patch for 899400, with callback version > > Review of attachment 784249 [details] [diff] [review]: > ----------------------------------------------------------------- > > Shawn, I'm not within the RIL peer list so I cannot give formals r+ but I'll > be happy to help you and provide some feedback if needed. > > ::: dom/system/gonk/ril_worker.js > @@ +5323,4 @@ > > this.sendChromeMessage(options); > > }; > > > > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > > Still need to handle the result of setting call waiting preferences through > MMI codes as well. This is needed since the `options.statusMessage` property > is used in Gaia for localization stuff. Hi José: Due to the response of REQUEST_SET_CALL_WAITING may involves serviceClass, if we simply fill up result with MMI_SM_KS_SERVICE_ENABLED_FOR or MMI_SM_KS_SERVICE_DISABLED. it may confuse user. ex: device show enabled, and, it is just for Fax type. After user go to setting menu, it still show disabled. hecnce, I still think it might be enough to show command success or failure to user (you can refer to the attached picture in Comment 21). If you still think it is necessary for ril_worker.js to fill up options.statusMessage for REQUEST_SET_CALL_WAITING request, I will discuss with HsinYi, and do so then. Thanks!! sku

(In reply to shawn ku [:sku] from comment #22) > Due to the response of REQUEST_SET_CALL_WAITING may involves serviceClass, > if we simply fill up result with MMI_SM_KS_SERVICE_ENABLED_FOR or > MMI_SM_KS_SERVICE_DISABLED. it may confuse user. ex: device show enabled, > and, it is just for Fax type. After user go to setting menu, it still show > disabled. hecnce, Yeah, this is true and it might be confusing. The absence of error in the RIL response has been considered as the request was successfully and the service has been enabled/disabled but that assumption might not be true because the network side is not fully reliable. > I still think it might be enough to show command success > or failure to user (you can refer to the attached picture in Comment 21). > If you still think it is necessary for ril_worker.js to fill up > options.statusMessage for REQUEST_SET_CALL_WAITING request, I will discuss > with HsinYi, and do so then. The reason is because the commercial RIL development already includes what I'm proposing you and IMHO we should provide the same response to users.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #23) > (In reply to shawn ku [:sku] from comment #22) > > Due to the response of REQUEST_SET_CALL_WAITING may involves serviceClass, > > if we simply fill up result with MMI_SM_KS_SERVICE_ENABLED_FOR or > > MMI_SM_KS_SERVICE_DISABLED. it may confuse user. ex: device show enabled, > > and, it is just for Fax type. After user go to setting menu, it still show > > disabled. hecnce, > > Yeah, this is true and it might be confusing. The absence of error in the > RIL response has been considered as the request was successfully and the > service has been enabled/disabled but that assumption might not be true > because the network side is not fully reliable. > > > I still think it might be enough to show command success > > or failure to user (you can refer to the attached picture in Comment 21). > > If you still think it is necessary for ril_worker.js to fill up > > options.statusMessage for REQUEST_SET_CALL_WAITING request, I will discuss > > with HsinYi, and do so then. > > The reason is because the commercial RIL development already includes what > I'm proposing you and IMHO we should provide the same response to users. Understood. Will provide an updated patch for review, thanks again.

1. Add callback methods for both MMI queryCallWaiting/setCallWaiting. 2. Handle MMI_PROCEDURE_ACTIVATION/MMI_PROCEDURE_DEACTIVATION/MMI_PROCEDURE_INTERROGATION only, return error for rest of MMI CW requests. 3. remove invalid test case - error on *43# request, and add 5 test cases for MMI CW.

Comment on attachment 784766 [details] [diff] [review] the 4th patch for 899400 Review of attachment 784766 [details] [diff] [review]: ----------------------------------------------------------------- Thank you and thanks for jaoo's feedbacks. The patch looks quite good to me. Some minor things below. Also, I would like to invite ferjmoreno for the review as well. He and jaoo have been involved in MMI implementation and review for a while. Would be good to have both their comments. Kind reminder: we landed a test case for RIL code examination last week. Please make sure your patch passes the test 'test_ril_code_quality.py' which picks up some nits hardly visible to naked eyes. ::: dom/system/gonk/ril_worker.js @@ +1453,5 @@ > }, > > /** > + * Query call waiting status via MMI. > + * nit: remove this line. @@ +1465,5 @@ > + let serviceClass = []; > + for (let serviceClassMask = 1; > + serviceClassMask <= ICC_SERVICE_CLASS_MAX; > + serviceClassMask <<= 1) { > + if ((serviceClassMask & services) != 0) { s/!= 0/!== 0 @@ +1472,5 @@ > + } > + options.additionalInformation = serviceClass; > + } else { > + options.statusMessage = MMI_SM_KS_SERVICE_DISABLED; > + } nit: put a new line here. @@ +1473,5 @@ > + options.additionalInformation = serviceClass; > + } else { > + options.statusMessage = MMI_SM_KS_SERVICE_DISABLED; > + } > + // To prevent DataCloneError when sending parcels. The comment isn't so right. " Prevent DataCloneError when sending chrome messages." @@ +1477,5 @@ > + // To prevent DataCloneError when sending parcels. > + delete options.callback; > + } > + > + if (options.mmiServiceCode) { Hm, I don't think we need this check. Let's remove it. @@ +1485,5 @@ > + }, > + > + /** > + * Set call waiting status via MMI. > + * nit: remove this line. @@ +1494,5 @@ > + options.statusMessage = MMI_SM_KS_SERVICE_ENABLED; > + } else { > + options.statusMessage = MMI_SM_KS_SERVICE_DISABLED; > + } > + // To prevent DataCloneError when sending parcels. The comment isn't so right. " Prevent DataCloneError when sending chrome messages." @@ +1498,5 @@ > + // To prevent DataCloneError when sending parcels. > + delete options.callback; > + } > + > + if (options.mmiServiceCode) { Hm, I don't think we need this check. Let's remove it. @@ +2648,5 @@ > // Call waiting > case MMI_SC_CALL_WAITING: > + if (!_isRadioAvailable(MMI_KS_SC_CALL_WAITING)) { > + return; > + } nit: add a new line here. @@ +2663,5 @@ > + options.enabled = false; > + } else { > + _sendMMIError(MMI_ERROR_KS_NOT_SUPPORTED, MMI_KS_SC_CALL_WAITING); > + return; > + } nit: add a new line here. @@ +5347,5 @@ > options.length = Buf.readUint32(); > + if (options.rilMessageType === "sendMMI") { > + if (options.callback) { > + options.callback.call(this, options); > + this.sendChromeMessage(options); "this.sendChromeMessage(options); is part of the callback. Please move it into that. And we don't check options.rilMessageType here. So here becomes: if (options.callback) { options.callback.call(this, options); return; } @@ +5350,5 @@ > + options.callback.call(this, options); > + this.sendChromeMessage(options); > + return; > + } > + } else { No else since we have return above. @@ +5361,5 @@ > RIL[REQUEST_SET_CALL_WAITING] = function REQUEST_SET_CALL_WAITING(length, options) { > options.success = (options.rilRequestError === 0); > if (!options.success) { > options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]; > } Put a new line here. @@ +5362,5 @@ > options.success = (options.rilRequestError === 0); > if (!options.success) { > options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]; > } > + if (options.rilMessageType === "sendMMI") { Same. No need to check rilMessageType. @@ +5365,5 @@ > } > + if (options.rilMessageType === "sendMMI") { > + if (options.callback) { > + options.callback.call(this, options); > + this.sendChromeMessage(options); "this.sendChromeMessage(options); is part of the callback. Please move it into that. And we don't check options.rilMessageType here. @@ +5368,5 @@ > + options.callback.call(this, options); > + this.sendChromeMessage(options); > + return; > + } > + } Put a new line here.

1. modified patch based on HsinYi's suggestion. 2. pass all TCs under /dom/system/gonk/tests. 3. pass test_ril_code_quality.py regarding to this patch scope. Thanks!! sku

Comment on attachment 785687 [details] [diff] [review] the 5th patch for 899400 Review of attachment 785687 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Just a minor comment below. Thanks! ::: dom/system/gonk/ril_worker.js @@ +1525,5 @@ > setCallWaiting: function setCallWaiting(options) { > Buf.newParcel(REQUEST_SET_CALL_WAITING, options); > Buf.writeUint32(2); > Buf.writeUint32(options.enabled ? 1 : 0); > + if (options.mmiServiceCode) { Checking for something like `options.sendMMI` property is much more clear IMHO, we have used that property in the code for handling other MMI commands. If you like it you have to define it on the options object in the _handleSetMMICallWaiting() function and set to true.

Comment on attachment 785687 [details] [diff] [review] the 5th patch for 899400 Review of attachment 785687 [details] [diff] [review]: ----------------------------------------------------------------- Test case looks good to me. ::: dom/system/gonk/ril_worker.js @@ +1529,5 @@ > + if (options.mmiServiceCode) { > + Buf.writeUint32(options.serviceClass); > + } else { > + Buf.writeUint32(ICC_SERVICE_CLASS_VOICE); > + } Can't we just use Buf.writeUint32( options.serviceClass? options.serviceClass : ICC_SERVICE_CLASS_VOICE)? I don't think we should check who calls setCallWaiting(). Instead, it depends on whether the option is provided. If not, using default value, i.e. ICC_SERVICE_CLASS_VOICE, should be fine. @@ +5408,1 @@ > options.length = Buf.readUint32(); options.callback should contain this line, so that we can have a clear cut line between the introduced options.callback and the default handler.

Comment on attachment 785687 [details] [diff] [review] the 5th patch for 899400 Review of attachment 785687 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1529,5 @@ > + if (options.mmiServiceCode) { > + Buf.writeUint32(options.serviceClass); > + } else { > + Buf.writeUint32(ICC_SERVICE_CLASS_VOICE); > + } Buf.writeUint32(optins.serviceClass || ICC_SERVICE_CLASS_VOICE) should be enough.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #30) > Comment on attachment 785687 [details] [diff] [review] > the 5th patch for 899400 > > Review of attachment 785687 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +1529,5 @@ > > + if (options.mmiServiceCode) { > > + Buf.writeUint32(options.serviceClass); > > + } else { > > + Buf.writeUint32(ICC_SERVICE_CLASS_VOICE); > > + } > > Buf.writeUint32(optins.serviceClass || ICC_SERVICE_CLASS_VOICE) should be > enough. Hi HsinYi: We can *not* just simply use "Buf.writeUint32(optins.serviceClass || ICC_SERVICE_CLASS_VOICE)", the reason is _siToServiceClass will return ICC_SERVICE_CLASS_NONE if mmi string do not contain serviceClass information, the (optins.serviceClass || ICC_SERVICE_CLASS_VOICE) will always return ICC_SERVICE_CLASS_VOICE because optins.serviceClass = 0. then, the request do not refeclt to user's request. Hence, I would like to change the check condition to Buf.writeUint32(options.serviceClass !== undefined ? options.serviceClass : ICC_SERVICE_CLASS_VOICE); Thanks! sku

(In reply to shawn ku [:sku] from comment #31) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #30) > > Comment on attachment 785687 [details] [diff] [review] > > the 5th patch for 899400 > > > > Review of attachment 785687 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/system/gonk/ril_worker.js > > @@ +1529,5 @@ > > > + if (options.mmiServiceCode) { > > > + Buf.writeUint32(options.serviceClass); > > > + } else { > > > + Buf.writeUint32(ICC_SERVICE_CLASS_VOICE); > > > + } > > > > Buf.writeUint32(optins.serviceClass || ICC_SERVICE_CLASS_VOICE) should be > > enough. > Hi HsinYi: > We can *not* just simply use "Buf.writeUint32(optins.serviceClass || > ICC_SERVICE_CLASS_VOICE)", the reason is _siToServiceClass will return > ICC_SERVICE_CLASS_NONE if mmi string do not contain serviceClass > information, the (optins.serviceClass || ICC_SERVICE_CLASS_VOICE) will > always return ICC_SERVICE_CLASS_VOICE because optins.serviceClass = 0. then, > the request do not refeclt to user's request. > > Hence, I would like to change the check condition to > Buf.writeUint32(options.serviceClass !== undefined ? > options.serviceClass : > ICC_SERVICE_CLASS_VOICE); > > Thanks! > sku Nice catch. The change looks good to me:)

revise scope: 1. Buf.writeUint32(options.serviceClass !== undefined ? options.serviceClass : ICC_SERVICE_CLASS_VOICE); 2. move options.length = Buf.readUint32(); to the body of callback@_handleQueryMMICallWaiting

Comment on attachment 790032 [details] [diff] [review] the 6th patch for 899400 Review of attachment 790032 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Thank you for the patch! ::: dom/system/gonk/ril_worker.js @@ +1525,5 @@ > Buf.newParcel(REQUEST_SET_CALL_WAITING, options); > Buf.writeUint32(2); > Buf.writeUint32(options.enabled ? 1 : 0); > + Buf.writeUint32(options.serviceClass !== undefined ? options.serviceClass : > + ICC_SERVICE_CLASS_VOICE); nit: make ICC_SERVICE_CLASS_VOICE align with options.srviceClass.

Try server link: https://tbpl.mozilla.org/?tree=Try&rev=fc35afad5bdf

(Just use the bug keyword next time instead of using the whiteboard :)...)

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38) > (Just use the bug keyword next time instead of using the whiteboard :)...) Roget that, thanks!!! sku

(In reply to shawn ku [:sku] from comment #40) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38) > > (Just use the bug keyword next time instead of using the whiteboard :)...) > > Roget that, thanks!!! > sku Roger that, thanks!!! Sorry for typo..... sku

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #26) > Comment on attachment 784766 [details] [diff] [review] > the 4th patch for 899400 > > Review of attachment 784766 [details] [diff] [review]: > ----------------------------------------------------------------- > > Also, I would like to invite ferjmoreno for the review as well. He and jaoo > have been involved in MMI implementation and review for a while. Would be > good to have both their comments. Sorry Hsin-Yi, I was on PTO during August.

(In reply to Fernando Jiménez Moreno [:ferjm] (PTO from 30/7 to 26/8) from comment #42) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #26) > > Comment on attachment 784766 [details] [diff] [review] > > the 4th patch for 899400 > > > > Review of attachment 784766 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Also, I would like to invite ferjmoreno for the review as well. He and jaoo > > have been involved in MMI implementation and review for a while. Would be > > good to have both their comments. > > Sorry Hsin-Yi, I was on PTO during August. No problem! :)