Call forwarding is a device certification requirement. Even if there are not specific test cases of call forwarding through MMI codes for the device certification that I know about, this codes allows the device to handle the call forwarding functionality, so we should probably support this too.

Is this a P1 blockers or is it a nice-to-have?

(In reply to Andrew Overholt [:overholt] from comment #1) > Is this a P1 blockers or is it a nice-to-have? Blocker because device certification requirement.

Fernando, please re-assign if you're not the correct owner here.

I consider this task as LOE:S once bug 793186 and bug 793208 lands

I am still waiting for Bug 793208 to land, but in the mean time I can upload my WIP patches.

Comment on attachment 676225 [details] [diff] [review] Part 1: RIL Review of attachment 676225 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +2384,5 @@ > + options.rilMessageType = "sendMMI"; > + options.reason = MMI_SC_TO_CF_REASON[sc]; > + options.number = mmi.sia; > + options.serviceClass = mmi.sib; > + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) { nit: would it be better to do this check before we set all the fields on |options|?

Comment on attachment 676226 [details] [diff] [review] Part 2: RIL. Query CF status Review of attachment 676226 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RILContentHelper.js @@ +998,5 @@ > + this._cfRulesToMobileCfInfo(message.rules); > + message.result = message.rules; > + } > + > + Services.DOMRequest.fireSuccess(request, message.result); Is it necessary to use |message.result| instead of |message.rules| here?

Comment on attachment 676227 [details] [diff] [review] Part 3: Tests Review of attachment 676227 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_mmi.js @@ +327,5 @@ > + > + worker.RIL.sendMMI({mmi: mmi}); > + > + do_check_eq(postedMessage.errorMsg, GECKO_ERROR_SUCCESS); > + do_check_true(postedMessage.success); nit: trailing space @@ +337,5 @@ > + run_next_test(); > +}); > + > +add_test(function test_sendMMI_call_forwarding_deactivation() { > + setCallForwardSuccess("#21*12345*99*10#"); nit: 1 space indentation should be 2 :) @@ +350,5 @@ > + }, > + postMessage: function fakePostMessage(message) { > + postedMessage = message; > + } > + }); Same as my comment in the tests for Bug 793208: The general newWorker / postedMessage pattern seems to be reused a lot here.. maybe we could separate this out into a helper function? @@ +425,5 @@ > +}); > + > +add_test(function test_sendMMI_call_forwarding_erasure() { > + setCallForwardSuccess("##21*12345*99#"); > + nit: trailing spaces

(In reply to Marshall Culpepper [:marshall_law] from comment #10) > Comment on attachment 676225 [details] [diff] [review] > Part 1: RIL > > Review of attachment 676225 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +2384,5 @@ > > + options.rilMessageType = "sendMMI"; > > + options.reason = MMI_SC_TO_CF_REASON[sc]; > > + options.number = mmi.sia; > > + options.serviceClass = mmi.sib; > > + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) { > > nit: would it be better to do this check before we set all the fields on > |options|? Thanks for the quick revision Marshall! I am afraid that the call to |_sendMMIError("CF_QUERY_STATUS_NOT_SUPPORTED");| might have lead you to a confusion here. That call is changed for a |this.queryCallForwardStatus(options)| on the second patch that makes use of the |options| object. Sorry about that.

(In reply to Marshall Culpepper [:marshall_law] from comment #11) > Comment on attachment 676226 [details] [diff] [review] > Part 2: RIL. Query CF status > > Review of attachment 676226 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/RILContentHelper.js > @@ +998,5 @@ > > + this._cfRulesToMobileCfInfo(message.rules); > > + message.result = message.rules; > > + } > > + > > + Services.DOMRequest.fireSuccess(request, message.result); > > Is it necessary to use |message.result| instead of |message.rules| here? The RadioInterfaceLayer sends |message.result| instead of |message.rules| for some MMI functionality. An alternative to that code would be: if (message.success && message.rules) { this._cfRulesToMobileCfInfo(message.rules); Services.DOMRequest.fireSuccess(request, message.rules); } Services.DOMRequest.fireSuccess(request, message.result); Is that what you meant?

I'd like to also add the Gaia part before landing this patches.

(In reply to Fernando Jiménez Moreno [:ferjm] (PTO until 5th Nov) from comment #15) > I'd like to also add the Gaia part before landing this patches. Is there a bug # for that?

No, I was planning to also attach the Gaia patch in this bug. Please, let me know if you want me to land the platform part and open a follow up bug for the Gaia patch. I am uploading a WIP patch that I wrote on the plane and I couldn't test it yet that should look pretty much like the final patch.

I had to expose the CALL_FORWARD_* from nsIDOMMozMobileConnection instead from nsIDOMMozMobileCFInfo since this last interface is not exposed to content.

Pointer to Github pull-request

Comment on attachment 678470 [details] [diff] [review] Part 1: RIL Review of attachment 678470 [details] [diff] [review]: ----------------------------------------------------------------- Looks good :) ::: dom/system/gonk/ril_worker.js @@ +2384,5 @@ > + options.rilMessageType = "sendMMI"; > + options.reason = MMI_SC_TO_CF_REASON[sc]; > + options.number = mmi.sia; > + options.serviceClass = mmi.sib; > + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) { nit: can we do this validation first before we set values on |options|?

Ack, just realized you will need to change UUIDs for these interfaces as well.. please make sure you do that. We will probably need to get an exception for this change on aurora, as well..

r=marshall_law, sr=sicking I've talked with Jonas about the required changes in the idl and he agreed to give his sr+

(In reply to Marshall Culpepper [:marshall_law] from comment #20) > Comment on attachment 678470 [details] [diff] [review] > Part 1: RIL > > Review of attachment 678470 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good :) > > ::: dom/system/gonk/ril_worker.js > @@ +2384,5 @@ > > + options.rilMessageType = "sendMMI"; > > + options.reason = MMI_SC_TO_CF_REASON[sc]; > > + options.number = mmi.sia; > > + options.serviceClass = mmi.sib; > > + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) { > > nit: can we do this validation first before we set values on |options|? Comment 13 :)

We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule). If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1feb6d4a8b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce4b53fb015 https://hg.mozilla.org/integration/mozilla-inbound/rev/85867b1963cf

https://hg.mozilla.org/mozilla-central/rev/c1feb6d4a8b5 https://hg.mozilla.org/mozilla-central/rev/8ce4b53fb015 https://hg.mozilla.org/mozilla-central/rev/85867b1963cf

https://hg.mozilla.org/releases/mozilla-aurora/rev/05b4fc4103af https://hg.mozilla.org/releases/mozilla-aurora/rev/1c2da0f31ff6 https://hg.mozilla.org/releases/mozilla-aurora/rev/c16025b84d9d