MMI Codes: support call forwarding

RESOLVED FIXED in Firefox 18

Status

()

P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

({feature})

Trunk
B2G C1 (to 19nov)
feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [LOE:S])

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 793178

Updated

6 years ago
blocking-basecamp: --- → ?
(Assignee)

Updated

6 years ago
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

Updated

6 years ago
Assignee: ferjmoreno → nobody
Component: DOM: Apps → DOM: Device Interfaces

Updated

6 years ago
Assignee: nobody → ferjmoreno
(Assignee)

Updated

6 years ago
Whiteboard: [LOE:S]
(Assignee)

Comment 4

6 years ago
I consider this task as LOE:S once bug 793186 and bug 793208 lands
(Assignee)

Updated

6 years ago
Depends on: 793186

Updated

6 years ago
Priority: -- → P1
(Assignee)

Comment 5

6 years ago
Created attachment 675616 [details] [diff] [review]
Part 1: RIL. WIP

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

Comment 6

6 years ago
Created attachment 675617 [details] [diff] [review]
Part 2: RIL. Query CF status
Attachment #675617 - Flags: feedback?(marshall)
(Assignee)

Updated

6 years ago
Attachment #675616 - Flags: feedback?(marshall)
(Assignee)

Comment 7

6 years ago
Created attachment 676225 [details] [diff] [review]
Part 1: RIL
Attachment #676225 - Flags: review?(marshall)
(Assignee)

Updated

6 years ago
Attachment #675616 - Attachment is obsolete: true
Attachment #675616 - Flags: feedback?(marshall)
(Assignee)

Comment 8

6 years ago
Created attachment 676226 [details] [diff] [review]
Part 2: RIL. Query CF status
Attachment #675617 - Attachment is obsolete: true
Attachment #675617 - Flags: feedback?(marshall)
Attachment #676226 - Flags: review?(marshall)
(Assignee)

Comment 9

6 years ago
Created attachment 676227 [details] [diff] [review]
Part 3: Tests
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+
(Assignee)

Comment 13

6 years ago
(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.
(Assignee)

Comment 14

6 years ago
(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?
(Assignee)

Comment 15

6 years ago
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?
(Assignee)

Comment 17

6 years ago
Created attachment 678193 [details] [diff] [review]
Part 4: Gaia. WIP

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
(Assignee)

Comment 18

6 years ago
Created attachment 678470 [details] [diff] [review]
Part 1: RIL

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)
(Assignee)

Comment 19

6 years ago
Created attachment 678482 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6179

Pointer to Github pull-request
(Assignee)

Updated

6 years ago
Attachment #678482 - Flags: review?(francisco.jordano)
(Assignee)

Updated

6 years ago
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..
(Assignee)

Comment 22

6 years ago
Created attachment 679374 [details] [diff] [review]
Part 1: RIL

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
(Assignee)

Comment 23

6 years ago
(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)
(Assignee)

Updated

6 years ago
Depends on: 815156
Blocks: 819528
Depends on: 1018341
You need to log in before you can comment on or make changes to this bug.