Closed Bug 793208 Opened 7 years ago Closed 7 years ago

B2G RIL: Support call forwarding

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: ferjm, Assigned: jaoo)

References

Details

(Whiteboard: [LOE:M])

Attachments

(4 files, 11 obsolete files)

1.22 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.97 KB, patch
Details | Diff | Splinter Review
19.64 KB, patch
marshall
: review+
Details | Diff | Splinter Review
4.34 KB, patch
marshall
: review+
Details | Diff | Splinter Review
As per device certification requirement, we should support setting call forwarding and query it state.
Blocks: b2g-ril, 793192
blocking-basecamp: --- → ?
Is this a v1 P1 requirement?
Whiteboard: [blocked-on-input philikon]
Fernando, can you link the device certification requirements?
Whiteboard: [blocked-on-input philikon]
(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
(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.
Assignee: nobody → josea.olivera
Blocking based on comment #4.  Thanks, Fernando.  jaoo, can you please put a rough level-of-effort estimate into the whiteboard field?  Thanks.
blocking-basecamp: ? → +
Whiteboard: [LOE:M]
Attached patch Part 1: DOM API - WIP. (obsolete) — Splinter Review
DOM API proposal.
Attached patch Part 1: DOM API - WIP v1. (obsolete) — Splinter Review
Attachment #666517 - Attachment is obsolete: true
Attachment #667614 - Flags: feedback?(jonas)
Attached patch Part 1: DOM API - WIP v2. (obsolete) — Splinter Review
DOM API proposal.
Attachment #667614 - Attachment is obsolete: true
Attachment #667614 - Flags: feedback?(jonas)
Attached patch Part 3: RIL impl - WIP v2. (obsolete) — Splinter Review
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?
Attachment #667616 - Attachment is obsolete: true
Attachment #669915 - Flags: feedback?(allstars.chh)
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.
Attachment #669915 - Flags: feedback?(allstars.chh) → feedback+
(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?
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"
Priority: -- → P1
Attached patch Hack to test. (obsolete) — Splinter Review
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.
Attached patch Part 1: DOM API - v1. (obsolete) — Splinter Review
Attachment #667615 - Attachment is obsolete: true
Attachment #669914 - Attachment is obsolete: true
Attachment #669915 - Attachment is obsolete: true
Attachment #672778 - Attachment is obsolete: true
Attachment #673219 - Flags: review?(jonas)
Attached patch Part 3: RIL impl - v1. (obsolete) — Splinter Review
Attachment #673225 - Flags: review?(marshall)
Blocks: 803533
Attachment #673224 - Flags: review?(bugs) → review+
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 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?
Attachment #673225 - Flags: review?(marshall) → review-
Attached patch Part 1: DOM API - v2. (obsolete) — Splinter Review
The constants for action, reason, and status are defined in this API as Marshall suggested in comment #19.
Attachment #673219 - Attachment is obsolete: true
Attachment #673219 - Flags: review?(jonas)
Attachment #674699 - Flags: review?(jonas)
Attached patch Part 3: RIL impl - v2. (obsolete) — Splinter Review
Addressed review comment from comment #20.
Attachment #673225 - Attachment is obsolete: true
Attachment #674701 - Flags: review?(marshall)
(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?
Attachment #674699 - Attachment is patch: true
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.
Attachment #674699 - Flags: review?(jonas) → review+
(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.
Change attribute `status` to `active`.
Attachment #674699 - Attachment is obsolete: true
Minor changes due to change attributes in DOM API.
Attachment #674701 - Attachment is obsolete: true
Attachment #674701 - Flags: review?(marshall)
Attachment #675570 - Flags: review?(marshall)
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;
}
Attachment #675570 - Flags: review?(marshall) → review+
(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.
(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.
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.
Attachment #676592 - Flags: review?(marshall)
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
Attachment #676592 - Flags: review?(marshall) → review+
You need to log in before you can comment on or make changes to this bug.