Closed Bug 793192 Opened 7 years ago Closed 7 years ago

MMI Codes: support call forwarding

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

RESOLVED FIXED
B2G C1 (to 19nov)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

(Keywords: feature, Whiteboard: [LOE:S])

Attachments

(4 files, 5 obsolete files)

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.
Blocks: MMI
blocking-basecamp: --- → ?
Depends on: 793208
Is this a P1 blockers or is it a nice-to-have?
Whiteboard: [blocked-on-input philikon]
(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.
Whiteboard: [blocked-on-input philikon]
blocking-basecamp: ? → +
Fernando, please re-assign if you're not the correct owner here.
Assignee: nobody → ferjmoreno
Assignee: ferjmoreno → nobody
Component: DOM: Apps → DOM: Device Interfaces
Assignee: nobody → ferjmoreno
Whiteboard: [LOE:S]
I consider this task as LOE:S once bug 793186 and bug 793208 lands
Depends on: 793186
Priority: -- → P1
Attached patch Part 1: RIL. WIP (obsolete) — Splinter Review
I am still waiting for Bug 793208 to land, but in the mean time I can upload my WIP patches.
Attached patch Part 2: RIL. Query CF status (obsolete) — Splinter Review
Attachment #675617 - Flags: feedback?(marshall)
Attachment #675616 - Flags: feedback?(marshall)
Attached patch Part 1: RIL (obsolete) — Splinter Review
Attachment #676225 - Flags: review?(marshall)
Attachment #675616 - Attachment is obsolete: true
Attachment #675616 - Flags: feedback?(marshall)
Attachment #675617 - Attachment is obsolete: true
Attachment #675617 - Flags: feedback?(marshall)
Attachment #676226 - Flags: review?(marshall)
Attached patch Part 3: TestsSplinter Review
Attachment #676227 - Flags: review?(marshall)
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|?
Attachment #676225 - Flags: review?(marshall) → review+
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?
Attachment #676226 - Flags: review?(marshall) → review+
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
Attachment #676227 - Flags: review?(marshall) → review+
(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?
Attached patch Part 4: Gaia. WIP (obsolete) — Splinter Review
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.
Keywords: feature
Attached patch Part 1: RIL (obsolete) — Splinter Review
I had to expose the CALL_FORWARD_* from nsIDOMMozMobileConnection instead from nsIDOMMozMobileCFInfo since this last interface is not exposed to content.
Attachment #676225 - Attachment is obsolete: true
Attachment #678470 - Flags: review?(marshall)
Attachment #678482 - Flags: review?(francisco.jordano)
Attachment #678193 - Attachment is obsolete: true
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|?
Attachment #678470 - Flags: review?(marshall) → review+
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..
Attached patch Part 1: RILSplinter Review
r=marshall_law, sr=sicking

I've talked with Jonas about the required changes in the idl and he agreed to give his sr+
Attachment #678470 - Attachment is obsolete: true
(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 :)
Attachment #678482 - Flags: review?(francisco.jordano) → review+
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.
Target Milestone: --- → B2G C1 (to 19nov)
Depends on: 815156
Depends on: 1018341
You need to log in before you can comment on or make changes to this bug.