793208 - B2G RIL: Support call forwarding

Status: RESOLVED FIXED

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

Description

[Fernando Jiménez Moreno [:ferjm]]

As per device certification requirement, we should support setting call forwarding and query it state.

Comment 1

[Andrew Overholt [:overholt]]

Is this a v1 P1 requirement?

Comment 2

[Dietrich Ayala (:dietrich)]

Fernando, can you link the device certification requirements?

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Dietrich Ayala (:dietrich) from comment #2)
> Fernando, can you link the device certification requirements?

https://bugzilla.mozilla.org/show_bug.cgi?id=793178#c2

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Is this a v1 P1 requirement?

This bug is blocking Bug 793192, which is a P1 blocker.

jaoo will be working on this.

Comment 5

[Andrew Overholt [:overholt]]

Blocking based on comment #4.  Thanks, Fernando.  jaoo, can you please put a rough level-of-effort estimate into the whiteboard field?  Thanks.

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: DOM API - WIP.

DOM API proposal.

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: DOM API - WIP v1.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 2: DOM impl (C++) - WIP v1.

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 3: RIL impl - WIP v1.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: DOM API - WIP v2.

DOM API proposal.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 3: RIL impl - WIP v2.

Setting and querying forwarding rules. Current implemention returns `Generic Failure` every time I set a forwarding rule. In some cases I get the same result while querying the rules as well. I contacted our friends for getting help about that errors for the otoro device but they don't have the bandwidth to support debugging the reference implementation at this time. I'm blocked.   

Yoshi, I need to uptade the call forwarding status (CFIS record) in the ICC card but I'm not able to get that status first before updating it. Please, could you take a look at the `getCFIS` function?

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 669915 [details] [diff] [review]
Part 3: RIL impl - WIP v2.

Review of attachment 669915 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/ril_worker.js
@@ +1313,5 @@
> +   * 
> +   * @see TS 131.102, clause 4.2.64
> +   */
> +  getCFIS: function getCFIS() {
> +    function callback(options) {

Did you try to read the response here?
From my SIM I can get data here.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #12)

> Did you try to read the response here?
> From my SIM I can get data here.

Yes, I did. The callback function doesn't get called in my tests. What type (SIM or USIM) of ICC card did you use in your test?, could you please attach a full |adb logcat| output (RIL debug enabled please) to this bug?

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

oh sorry I am using an USIM sim card.
If I use a SIM, I would get error

ICC I/O Error EF id = 6fcb command = c0(94/4)

the status code: 94/4 means "not found"

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Hack to test.

Here is hack for testing how an unconditional call forwarding rule works. Need to test the remaining reasons (mobile busy, no reply, not reachable) the call is being forwarded. After this tests I'll rebase the pactches and request review.

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: DOM API - v1.

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 2: DOM impl (C++) - v1.

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 3: RIL impl - v1.

Comment 19

[Marshall Culpepper [:marshall_law]]

Comment on attachment 673219 [details] [diff] [review]
Part 1: DOM API - v1.

Review of attachment 673219 [details] [diff] [review]:
-----------------------------------------------------------------

The constants for action, reason, and status should all be defined in this API..

Comment 20

[Marshall Culpepper [:marshall_law]]

Comment on attachment 673225 [details] [diff] [review]
Part 3: RIL impl - v1.

Review of attachment 673225 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly ok. r- because I'm concerned about argument validation, and there is a place in ril_worker that looks like a placeholder comment was left to deal with an error condition. Are you also planning to add tests for this?

::: dom/system/gonk/RILContentHelper.js
@@ +497,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    cpmm.sendAsyncMessage("RIL:GetCallForwardingOption", {
> +      requestId: requestId,
> +      reason: reason});

nit: closing }) should be on the next line, aligned with cpmm.sendAsyncMessage

@@ +502,5 @@
> +
> +   return request;
> +  },
> +
> +  setCallForwardingOption: function setCallForwardingOption(window, CFInfo) {

nit: function parameters should be lower camel-case (i.e. cfInfo)

@@ +507,5 @@
> +    if (window == null) {
> +      throw Components.Exception("Can't get window object",
> +                                  Cr.NS_ERROR_UNEXPECTED);
> +    }
> +

AFAICT, it's possible for CFInfo to also be null here, so we should deal with that. We should also move action and reason validation here, and add status validation.

@@ +904,5 @@
>      }
>    },
>  
> +  handleGetCallForwardingOption: function handleGetCallForwardingOption(message) {
> +    debug("handleGetCallForwardingOption: " + JSON.stringify(message));

nit: any calls to debug() that are in the main code path should be surrounded with |if (DEBUG)|. It isn't clear to me if these function calls (even though they are no-op) don't have runtime overhead when stringifying / appending arguments..

::: dom/system/gonk/ril_worker.js
@@ +2434,5 @@
> +   *        One of ICC_SERVICE_CLASS_* constants.
> +   * @param number
> +   *        Phone number of forwarding address.
> +   */
> +  queryCallForwardStatus: function queryCallForwardStatus(options) {

nit: indent by two spaces here, and below in setCallForward

@@ +2436,5 @@
> +   *        Phone number of forwarding address.
> +   */
> +  queryCallForwardStatus: function queryCallForwardStatus(options) {
> +     if (!this._isValidCFReason(options.reason)){
> +        options.success = false;

The reason and action validation should be moved to RILContentHelper. This way, we can avoid sending four(!) worker messages just to get that error condition back to the DOMRequest.onerror

@@ +3534,5 @@
> +       case CALL_FORWARD_REASON_NO_REPLY:
> +       case CALL_FORWARD_REASON_NOT_REACHABLE:
> +       case CALL_FORWARD_REASON_ALL_CALL_FORWARDING:
> +       case CALL_FORWARD_REASON_ALL_CONDITIONAL_CALL_FORWARDING:
> +           return true;

nit: indent by two spaces here, and the default case (also below in _isValidCFAction)

@@ +4519,5 @@
>  RIL[REQUEST_GET_CLIR] = null;
>  RIL[REQUEST_SET_CLIR] = null;
> +RIL[REQUEST_QUERY_CALL_FORWARD_STATUS] = 
> +  function REQUEST_QUERY_CALL_FORWARD_STATUS(length, options) {
> +    options.success = options.rilRequestError == 0 ? true : false;

nit: the tertiary is unnecessary here (and below on line 4554)

@@ +4526,5 @@
> +      this.sendDOMMessage(options);
> +      return;
> +    }
> +
> +    let rules_length = 0;

nit: this should be camelCase

@@ +4531,5 @@
> +    if (length) {
> +      rules_length = Buf.readUint32();
> +    }
> +    if (!rules_length) {
> +      // XXXX

Does options.errorMsg need to be set, and the error sent back to the RadioInterfaceLayer here?

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: DOM API - v2.

The constants for action, reason, and status are defined in this API as Marshall suggested in comment #19.

Comment 22

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 3: RIL impl - v2.

Addressed review comment from comment #20.

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Marshall Culpepper [:marshall_law] from comment #20)
> Comment on attachment 673225 [details] [diff] [review]
> Part 3: RIL impl - v1.
> 
> Review of attachment 673225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks mostly ok. r- because I'm concerned about argument validation, and
> there is a place in ril_worker that looks like a placeholder comment was
> left to deal with an error condition. Are you also planning to add tests for
> this?

Sure, AFAIK the emulator doesn't have support for forwarding calls so my plan is to have some xpcshell tests. Should I add them here or file a separate follow-up bug for that?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 674699 [details] [diff] [review]
Part 1: DOM API - v2.

Review of attachment 674699 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have particularly strong opinions here since this API is so internal already. So all of the below comments are optional.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +453,5 @@
> +   *       configured.
> +   *
> +   * @see 3GPP TS 27.007 7.11 "status".
> +   */
> +  readonly attribute unsigned short status;

Maybe change this to be a simple bool and call it "active"?

@@ +469,5 @@
> +   * enable (1), query status (2), registration (3), or erasure (4).
> +   *
> +   * @see 3GPP TS 27.007 7.11 "mode".
> +   */
> +  readonly attribute unsigned short action;

I don't really understand how this works.

Can getCallForwardingOption ever return anything but QUERY_STATUS here?

What happens if you pass a nsIDOMMozMobileCFInfo object to setCallForwardingOption?

What happens if you call setCallForwardingOption and set "action" to DISABLE and "status" to ACTIVE?

What is the difference between ENABLE and REGISTRATION, and what is the difference between DISABLE and ERASURE?

There seems to be a lot of combinations here that doesn't make sense. I suspect something much cleaner could be created here.

Comment 25

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jonas Sicking (:sicking) from comment #24)
> Comment on attachment 674699 [details] [diff] [review]
> Part 1: DOM API - v2.
> 
> Review of attachment 674699 [details] [diff] [review]:

> > +  readonly attribute unsigned short status;
> 
> Maybe change this to be a simple bool and call it "active"?

SGTM, I'll change it.

> @@ +469,5 @@
> > +   * enable (1), query status (2), registration (3), or erasure (4).
> > +   *
> > +   * @see 3GPP TS 27.007 7.11 "mode".
> > +   */
> > +  readonly attribute unsigned short action;
> 
> I don't really understand how this works.

> What happens if you call setCallForwardingOption and set "action" to DISABLE
> and "status" to ACTIVE?

`status` is unused for `setCallForwardingOption` function.

> What is the difference between ENABLE and REGISTRATION, and what is the
> difference between DISABLE and ERASURE?
> 
> There seems to be a lot of combinations here that doesn't make sense. I
> suspect something much cleaner could be created here.

That's a bit confusing somehow. We inherit that complexity from how the RIL interface is. I agree with you that there are lot of combinations that seems not make sense but I don't see a normal user dealing with this API but a skilled user (e.g. developing a setting UI) considering what 3GPP TS 27.007 7.11 specs says.

Comment 26

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: DOM API - v3. r=sicking

Change attribute `status` to `active`.

Comment 27

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 3: RIL impl - v3.

Minor changes due to change attributes in DOM API.

Comment 28

[Marshall Culpepper [:marshall_law]]

Comment on attachment 675570 [details] [diff] [review]
Part 3: RIL impl - v3.

Review of attachment 675570 [details] [diff] [review]:
-----------------------------------------------------------------

Woot, looks good :) r+ with just a few minor nits

::: dom/system/gonk/ril_worker.js
@@ +2521,5 @@
> +   *        Phone number of forwarding address.
> +   */
> +  queryCallForwardStatus: function queryCallForwardStatus(options) {
> +    Buf.newParcel(REQUEST_QUERY_CALL_FORWARD_STATUS, options);
> +    Buf.writeUint32(2); // Query status action.

nit: should this be declared as a constant in ril_consts.js?

@@ +4567,5 @@
> +      rulesLength = Buf.readUint32();
> +    }
> +    if (!rulesLength) {
> +      options.success = false;
> +      options.errorMsg = 

nit: trailing space

@@ +4572,5 @@
> +        "Invalid rule length while querying call forwarding status.";
> +      this.sendDOMMessage(options);
> +      return;
> +    }
> +    let rules = [];

nit: since you have the length up front, it might be slightly more performant to do:

let rules = new Array(rulesLength);
for (..) {
  ... initialize rule ..
  rules[i] = rule;
}

Comment 29

[Marshall Culpepper [:marshall_law]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #23)
> (In reply to Marshall Culpepper [:marshall_law] from comment #20)
> > Are you also planning to add tests for this?
> 
> Sure, AFAIK the emulator doesn't have support for forwarding calls so my
> plan is to have some xpcshell tests. Should I add them here or file a
> separate follow-up bug for that?

xpcshell tests would be fine, but I would prefer if the tests landed with the feature. At this stage, we should be as careful as possible and only land features with tests IMO.

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Marshall Culpepper [:marshall_law] from comment #28)
> Comment on attachment 675570 [details] [diff] [review]
> Part 3: RIL impl - v3.
> 
> Review of attachment 675570 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Woot, looks good :) r+ with just a few minor nits
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2521,5 @@
> > +   *        Phone number of forwarding address.
> > +   */
> > +  queryCallForwardStatus: function queryCallForwardStatus(options) {
> > +    Buf.newParcel(REQUEST_QUERY_CALL_FORWARD_STATUS, options);
> > +    Buf.writeUint32(2); // Query status action.
> 
> nit: should this be declared as a constant in ril_consts.js?
>

I need to declare in the ril_consts.js all the consts that are declared in the idl regarding CF for Bug 793192. So we can do it in this patch or later.

Comment 31

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 4: Tests - V1.

Adding some test about call forwarding. I recognize these tests are not the best ones I've ever seen to be honest but I expect it looks good to you. Thanks to ferjm since I've cannibalized the test he's implementing for MMI codes regarding call forwarding.

Comment 32

[Marshall Culpepper [:marshall_law]]

Comment on attachment 676592 [details] [diff] [review]
Part 4: Tests - V1.

Review of attachment 676592 [details] [diff] [review]:
-----------------------------------------------------------------

Nice :) I'd like to see more tests for fail cases, but I'd be ok filing a follow up for that at this point..

::: dom/system/gonk/tests/test_ril_worker_cf.js
@@ +58,5 @@
> +    },
> +    postMessage: function fakePostMessage(message) {
> +      postedMessage = message;
> +    }
> +  });

nit: can you factor the code from line 55-62 into a common function? It looks like the same code is being used by test_queryCallForwardStatus_unconditional below

Comment 33

[José Antonio Olivera Ortega [:jaoo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97f89086d9c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/929a426a89f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cfb5d3bd778
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d9388d7a2c

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/97f89086d9c7
https://hg.mozilla.org/mozilla-central/rev/929a426a89f9
https://hg.mozilla.org/mozilla-central/rev/3cfb5d3bd778
https://hg.mozilla.org/mozilla-central/rev/a5d9388d7a2c

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66483e9ebc4c
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbcc0cd4bd8e
https://hg.mozilla.org/releases/mozilla-aurora/rev/30c107f5a13b
https://hg.mozilla.org/releases/mozilla-aurora/rev/25237cf5a928