818393 - B2G RIL: Support call barring.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

A telephony feature that allows users to block certain incoming or outgoing calls. See 3GPP TS  22.088.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [PATCH 1/3] Add interface for call barring config

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [PATCH 2/3] Call barring DOM implementation

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [PATCH 3/3] Call barring RIL implementation

Comment 4

[Anshul]

Jose, this if not intended for TEF or Leo, correct? Are you guys planning to implement a settings UI for call barring? Commercial RIL already supports call barring through MMI codes.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 746310 [details] [diff] [review]
[PATCH 1/3] Add interface for call barring config

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

Nice! r=me with the comments addressed.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +16,2 @@
>  
>  [scriptable, builtinclass, uuid(bde7e16c-ff1f-4c7f-b1cd-480984cbb206)]

Remeber to update uuid whenever the interface changes.

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +72,5 @@
> +  nsIDOMDOMRequest getCallBarringOption(in nsIDOMWindow window,
> +                                        in nsIDOMMozMobileCBInfo CBInfo);
> +  nsIDOMDOMRequest setCallBarringOption(in nsIDOMWindow window,
> +                                        in nsIDOMMozMobileCBInfo CBInfo);
> +

Remeber to update uuid whenever the interface changes.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 746312 [details] [diff] [review]
[PATCH 3/3] Call barring RIL implementation

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

Thanks! Generally the patch looks great, but I would like to handle errors more carefully. Also, please provide a test for this patch. XPCShell would be fine.

::: dom/system/gonk/RILContentHelper.js
@@ +774,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (!cbInfo ||
> +        !this._isValidCBProgram(cbInfo.program)) {
> +      this.dispatchFireRequestError(requestId, "Invalid call barring program.");

Don't we need to check |cbInfo.serviceClass| & |cbInfo.password| as well?

@@ +797,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (!cbInfo ||
> +        !this._isValidCBProgram(cbInfo.program)) {
> +      this.dispatchFireRequestError(requestId, "Invalid call barring program.");

ditto.

::: dom/system/gonk/ril_worker.js
@@ +2410,5 @@
>    },
>  
> +  _barringProgramToFacility: function _barringProgramToFacility(program) {
> +    switch (program) {
> +      case 0: return ICC_CB_FACILITY_BAOC;

Let's use CONSTS to make every number express itsefl clearer. Use a mapping table between nsIDOMMozMobileCBInfo.CALL_BARRING_PROGRAM_* and ICC_CB_FACILITY_*. 

You may refer to what we have done for call forwarding.
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#2379 
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_consts.js#2400

@@ +2436,5 @@
> +      this.sendDOMMessage(options);
> +      return;
> +    }
> +    options.password = ""; // For query no need to provide it.
> +    this.queryICCFacilityLock(options);

I think we will need to check options.serviceClass before calling |queryICCFacilityLock| because if it's null, we will encouter error in 'options.serviceClass.toString().'

@@ +2459,5 @@
> +      options.success = false;
> +      this.sendDOMMessage(options);
> +      return;
> +    }
> +    this.setICCFacilityLock(options);

I think we will need to check options.serviceClass before calling |setICCFacilityLock| because if it's null, we will encouter error in 'options.serviceClass.toString().'

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Anshul from comment #4)
> Jose, this if not intended for TEF or Leo, correct? Are you guys planning to
> implement a settings UI for call barring? Commercial RIL already supports
> call barring through MMI codes.

AFAIK call baring is out of v1.0.1 and v1.1 scope, let's see Daniel's comment on this.

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 746312 [details] [diff] [review]
[PATCH 3/3] Call barring RIL implementation

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

::: dom/system/gonk/ril_worker.js
@@ +2436,5 @@
> +      this.sendDOMMessage(options);
> +      return;
> +    }
> +    options.password = ""; // For query no need to provide it.
> +    this.queryICCFacilityLock(options);

I will modify RILContentHelper.js to guarantee that serviceClass is not null.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The currently proposed API looks okay. However after having more discussion and consideration, I think I would like to re-discuss this interface. I am thinking the possibility that we introduce 'dictionary' for setCallBarringOption and getCallBarringOption.

The reasons and doubts I am holding are:
1) in nsIDOMMozMobileCBInfo, we define several constants CALL_BARRING_PROGRAM_*. But, for example, should getCallBarringOption ever return anything but CALL_BARRING_PROGRAM_ALL_OUTGOING even we are querying the result of ALL_INCOMING?
2) nsIDOMMozMobileCBInfo.enabled won't be used in getCallBarringOption() while an nsIDOMMozMobileCBInfo object is taken as input. Wouldn't it look more right that we have 'dictionary' for the input options of both getCallBarringOption and setCallBarringOption?
3) Every attribute in nsIDOMMozMobileCBInfo is readonly. What happens if user passes a nsIDOMMozMobileCBInfo object to setCallBarringOption?

So, a new API in my mind might look like

interface nsIDOMMozMobileConnection {
  // What's alreay there.

  const long CALL_BARRING_PROGRAM_* ;
  nsIDOMDOMRequest setCallBarringOption(in jsval CBOptions);
  nsIDOMDOMRequest getCallBarringOption(in jsval CBOptions);
};

dictionary MozCBInfo {
  unsigned short program;
  attribute bool enabled;
  attribute DOMString password;
  unsigned short serviceClass;
};

Does that make more sense? May I have your comments here, Mounir? Thanks!

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 746310 [details] [diff] [review]
[PATCH 1/3] Add interface for call barring config

Cancelling the review until my quesions in comment #9 are answered.

Comment 11

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Szu-Yu Chen [:aknow] from comment #8)
> Comment on attachment 746312 [details] [diff] [review]
> [PATCH 3/3] Call barring RIL implementation
> 
> Review of attachment 746312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2436,5 @@
> > +      this.sendDOMMessage(options);
> > +      return;
> > +    }
> > +    options.password = ""; // For query no need to provide it.
> > +    this.queryICCFacilityLock(options);
> 
> I will modify RILContentHelper.js to guarantee that serviceClass is not null.

It seems that XPCOM quarantees all properties existed when receiving the parameter in RILContentHelper.js

check the experiment
http://pastebin.mozilla.org/2384571

If any of property is not specify by api user, it gets the default value as following rule:

unsigned short: 0
bool: false
DOMString: "undefined"

thus we might get

program: 0,
enabled: false,
password: "undefined",
serviceClass: 0

Just do nothing for serviceClass=0. It is valid although the response might be meaningless because you didn't set/query any class.

If password is not specified ("undefined"), it cause error from modem. The onerror event will be fired.

Comment 12

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [PATCH 3/3 #2] Call barring RIL implementation

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 747253 [details] [diff] [review]
[PATCH 3/3 #2] Call barring RIL implementation

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

Great job! Thank you.

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Mounir,
I know the proposed API already recevied your sr+ (as well as my r+). However, I have some questions (comment #9) regarding that API on second thought. Would you mind sharing more of your thoughts? :)

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Hi Mounir,
> I know the proposed API already recevied your sr+ (as well as my r+).
> However, I have some questions (comment #9) regarding that API on second
> thought. Would you mind sharing more of your thoughts? :)

If I understand correctly, you basically want to switch for an interface to a dictionary? That sounds good.

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Add interface for call barring config.

change from interface to dictionary

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Call barring DOM implementation.

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: Call barring RIL implementation.

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4: Add test for call barring.

Comment 20

[Mounir Lamouri (:mounir)]

Comment on attachment 749745 [details] [diff] [review]
Part 1: Add interface for call barring config.

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

If Hsin-Yi is okay with that patch, feel free to carry-over my sr+.

Comment 21

[Daniel Coloma:dcoloma]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #7)
> (In reply to Anshul from comment #4)
> > Jose, this if not intended for TEF or Leo, correct? Are you guys planning to
> > implement a settings UI for call barring? Commercial RIL already supports
> > call barring through MMI codes.
> 
> AFAIK call baring is out of v1.0.1 and v1.1 scope, let's see Daniel's
> comment on this.

I don't think this is part of 1.1 scope but adding ni for product team

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 749752 [details] [diff] [review]
Part 4: Add test for call barring.

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

::: dom/network/tests/marionette/test_call_barring_get_option.js
@@ +22,5 @@
> +    finish();
> +  };
> +}
> +
> +testGetCallBarringOption();

You will need a clearUp() and replace the current 'finish()' call with clearUp().

function cleanUp() {
  SpecialPowers.removePermission("mobileconnection", document);
  finish();
}

::: dom/network/tests/marionette/test_call_barring_set_error.js
@@ +10,5 @@
> +   "connection is instanceof " + connection.constructor);
> +
> +let caseId = 0;
> +let options = [
> +  buildOption(5,    true, '0000', 0),     // invalid program.

nit: remove extra whitespace

@@ +14,5 @@
> +  buildOption(5,    true, '0000', 0),     // invalid program.
> +
> +  // test null.
> +  buildOption(null, true, '0000', 0),
> +  buildOption(0,    null, '0000', 0),

ditto.

@@ +15,5 @@
> +
> +  // test null.
> +  buildOption(null, true, '0000', 0),
> +  buildOption(0,    null, '0000', 0),
> +  buildOption(0,    true, null,   0),

ditto.

@@ +16,5 @@
> +  // test null.
> +  buildOption(null, true, '0000', 0),
> +  buildOption(0,    null, '0000', 0),
> +  buildOption(0,    true, null,   0),
> +  buildOption(0,    true, '0000', null),

ditto.

@@ +19,5 @@
> +  buildOption(0,    true, null,   0),
> +  buildOption(0,    true, '0000', null),
> +
> +  // test undefined.
> +  {              'enabled': true, 'password': '0000', 'serviceClass': 0},

ditto,

@@ +20,5 @@
> +  buildOption(0,    true, '0000', null),
> +
> +  // test undefined.
> +  {              'enabled': true, 'password': '0000', 'serviceClass': 0},
> +  {'program': 0,                  'password': '0000', 'serviceClass': 0},

ditto.

@@ +21,5 @@
> +
> +  // test undefined.
> +  {              'enabled': true, 'password': '0000', 'serviceClass': 0},
> +  {'program': 0,                  'password': '0000', 'serviceClass': 0},
> +  {'program': 0, 'enabled': true,                     'serviceClass': 0},

ditto.

@@ +22,5 @@
> +  // test undefined.
> +  {              'enabled': true, 'password': '0000', 'serviceClass': 0},
> +  {'program': 0,                  'password': '0000', 'serviceClass': 0},
> +  {'program': 0, 'enabled': true,                     'serviceClass': 0},
> +  {'program': 0, 'enabled': true, 'password': '0000'                   },

ditto.

@@ +57,5 @@
> +  }
> +}
> +
> +nextTest();
> +

You will need a clearUp() and replace the current 'finish()' call with clearUp().
function cleanUp() {
SpecialPowers.removePermission("mobileconnection", document);
finish();
}

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 749745 [details] [diff] [review]
Part 1: Add interface for call barring config.

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

The revision looks better, thanks.
r=me but I would like to see some improvement in the documentation.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +330,5 @@
> +   */
> +  nsIDOMDOMRequest setCallBarringOption(in jsval option);
> +
> +  /**
> +   * Queries current call barring option.

Should be 'Queries current call barring *status*'?

@@ +654,5 @@
> +   */
> +   unsigned short program;
> +
> +  /**
> +   * Enable or disable the call barring rule.

You are using call barring *rule* and *program* which seem not have the clear distinction and definition. Please make the naming and comments consistent and clearer in documentation.

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 749750 [details] [diff] [review]
Part 3: Call barring RIL implementation.

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

r=me with the comment addressed, thanks!

::: dom/system/gonk/RILContentHelper.js
@@ +802,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (DEBUG) debug("getCallBarringOption: " + JSON.stringify(option));
> +    if (!this._isValidCallBarringOption(option)) {
> +      this.dispatchFireRequestError(requestId, "GenericFailure");

"InvalidCallBarringProgram" is better.

@@ +825,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (DEBUG) debug("setCallBarringOption: " + JSON.stringify(option));
> +    if (!this._isValidCallBarringOption(option)) {
> +      this.dispatchFireRequestError(requestId, "GenericFailure");

ditto.

@@ +1474,5 @@
> +  handleGetCallBarringOption: function handleGetCallBarringOption(message) {
> +    if (!message.success) {
> +      this.fireRequestError(message.requestId, message.errorMsg);
> +    } else {
> +      this.fireRequestSuccess(message.requestId,

let option = new CallBarringOption(message);
this.fireRequestSuccess(message.requestId, option);

@@ +1563,4 @@
>    /**
>     * Helper for guarding us again invalid reason values for call forwarding.
>     */
> +  _isValidCFReason: function _isValidCFReason(reason) {

Thanks for taking care of the indention here. However, I would like to get the part out of this patch to make the change commit very clear.

You are welcoming to fire another bug to address the coding style issue for this part.

@@ +1579,5 @@
>  
>    /**
>     * Helper for guarding us again invalid action values for call forwarding.
>     */
> +  _isValidCFAction: function _isValidCFAction(action) {

ditto.

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 749748 [details] [diff] [review]
Part 2: Call barring DOM implementation.

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

Looks good to me but we still need the review from DOM peers.

Comment 26

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 749752 [details] [diff] [review]
Part 4: Add test for call barring.

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

disagree for the removing extra white space
keep them there would have better readability.

Comment 27

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 749750 [details] [diff] [review]
Part 3: Call barring RIL implementation.

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

::: dom/system/gonk/RILContentHelper.js
@@ +802,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (DEBUG) debug("getCallBarringOption: " + JSON.stringify(option));
> +    if (!this._isValidCallBarringOption(option)) {
> +      this.dispatchFireRequestError(requestId, "GenericFailure");

Every other method in mobileConnection has this kinds of error code.
ex: getNetwork, setNetwork, call forwarding, call waiting

will be either 'RadioNotAvailable', 'RequestNotSupported', 
'IllegalSIMorME', or 'GenericFailure'

Add 'InvalidCallBarringProgram' could have a better meaning but break the alignment with others. What's your opnion?

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 749750 [details] [diff] [review]
Part 3: Call barring RIL implementation.

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

I think having InvalidCallBarringProgram here provides more benefit. I still vote for that.

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #28)
> Comment on attachment 749750 [details] [diff] [review]
> Part 3: Call barring RIL implementation.
> 
> Review of attachment 749750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think having InvalidCallBarringProgram here provides more benefit. I still
> vote for that.

To be clearer, we will still keep the error type of GenericFailure, but in this case using InvalidCallBarringProgram looks better.

Comment 30

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4 #2: Add test for call barring.

Comment 31

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 751576 [details] [diff] [review]
Part 4 #2: Add test for call barring.

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

Perfect, thank you!

Comment 32

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 749748 [details] [diff] [review]
Part 2: Call barring DOM implementation.

need DOM peer's review

Comment 33

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 751576 [details] [diff] [review]
Part 4 #2: Add test for call barring.

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

::: dom/network/tests/marionette/test_call_barring_get_option.js
@@ +14,5 @@
> +  let request = connection.getCallBarringOption(option);
> +  request.onsuccess = function() {
> +    ok(request.result);
> +    ok('enabled' in request.result, 'should have "enabled" field');
> +    finish();

I forgot to change this 'finish()' to 'cleanUp()'.
The modification will be included in the final patch.

Comment 34

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #33)
> Comment on attachment 751576 [details] [diff] [review]
> Part 4 #2: Add test for call barring.
> 
> Review of attachment 751576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/tests/marionette/test_call_barring_get_option.js
> @@ +14,5 @@
> > +  let request = connection.getCallBarringOption(option);
> > +  request.onsuccess = function() {
> > +    ok(request.result);
> > +    ok('enabled' in request.result, 'should have "enabled" field');
> > +    finish();
> 
> I forgot to change this 'finish()' to 'cleanUp()'.
> The modification will be included in the final patch.
Oops... thanks for the catch :)

Comment 35

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=81885b6d25ff

Comment 36

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1 #2: Add interface for call barring config.

Comment 37

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2 #2: Call barring DOM implementation.

Comment 38

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3 #2: Call barring RIL implementation.

Comment 39

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4 #3: Add test for call barring.

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

http://hg.mozilla.org/projects/birch/rev/c5c2d7b04b97
http://hg.mozilla.org/projects/birch/rev/5300a886b3c1
http://hg.mozilla.org/projects/birch/rev/eb242dc69589
http://hg.mozilla.org/projects/birch/rev/28d35d0faa74

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5c2d7b04b97
https://hg.mozilla.org/mozilla-central/rev/5300a886b3c1
https://hg.mozilla.org/mozilla-central/rev/eb242dc69589
https://hg.mozilla.org/mozilla-central/rev/28d35d0faa74

Comment 42

[Sandip Kamat]

clearing flag

Comment 43

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

Comment on attachment 751993 [details] [diff] [review]
Part 3 #2: Call barring RIL implementation.

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

I found this patch has some style nits from other bug,
can you file another bug to fix these ?

Thanks

::: dom/system/gonk/RILContentHelper.js
@@ +313,4 @@
>    popup: null
>  };
>  
> +function CallBarringOption(option) {

should be options.
Everywhere in RIL use 'options'

@@ +810,5 @@
> +    cpmm.sendAsyncMessage("RIL:GetCallBarringOption", {
> +      requestId: requestId,
> +      program: option.program,
> +      password: option.password,
> +      serviceClass: option.serviceClass

Cannot we reuse option here ?
Please rename it to options.

@@ +815,5 @@
> +    });
> +    return request;
> +  },
> +
> +  setCallBarringOption: function setCallBarringOption(window, option) {

s/option/options/

@@ +835,5 @@
> +      program: option.program,
> +      enabled: option.enabled,
> +      password: option.password,
> +      serviceClass: option.serviceClass
> +    });

reuse options.

@@ +1613,5 @@
> +   */
> +  _isValidCallBarringOption: function _isValidCallBarringOption(option) {
> +    return (option
> +            && option.serviceClass != null
> +            && this._isValidCallBarringProgram(option.program));

Move && to the end of previous line.

Comment 44

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

Comment on attachment 751990 [details] [diff] [review]
Part 1 #2: Add interface for call barring config.

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +327,5 @@
> +   * Otherwise, the request's onerror will be called, and the request's error
> +   * will be either 'RadioNotAvailable', 'RequestNotSupported',
> +   * 'IllegalSIMorME', 'InvalidCallBarringOption' or 'GenericFailure'
> +   */
> +  nsIDOMDOMRequest setCallBarringOption(in jsval option);

s/option/options/i  ?

@@ +643,5 @@
>    readonly attribute unsigned short serviceClass;
>  };
> +
> +
> +dictionary MozCallBarringOption

It has some many options, so it should be called MozCallBarringOptions, right?