B2G RIL: Support call barring.

RESOLVED FIXED in mozilla24

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jaoo, Assigned: aknow)

Tracking

Trunk
mozilla24
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(4 attachments, 9 obsolete attachments)

5.81 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
Details | Diff | Splinter Review
11.88 KB, patch
Details | Diff | Splinter Review
4.13 KB, patch
Details | Diff | Splinter Review
A telephony feature that allows users to block certain incoming or outgoing calls. See 3GPP TS  22.088.
(Reporter)

Updated

5 years ago
Blocks: 710489
(Reporter)

Updated

4 years ago
Blocks: 833754
(Assignee)

Updated

4 years ago
Assignee: nobody → szchen
(Assignee)

Comment 1

4 years ago
Created attachment 746310 [details] [diff] [review]
[PATCH 1/3] Add interface for call barring config
Attachment #746310 - Flags: superreview?(mounir)
Attachment #746310 - Flags: review?(htsai)
(Assignee)

Comment 2

4 years ago
Created attachment 746311 [details] [diff] [review]
[PATCH 2/3] Call barring DOM implementation
Attachment #746311 - Flags: review?(htsai)
(Assignee)

Comment 3

4 years ago
Created attachment 746312 [details] [diff] [review]
[PATCH 3/3] Call barring RIL implementation
Attachment #746312 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #746311 - Flags: review?(bugs)
Attachment #746310 - Flags: superreview?(mounir) → superreview+

Updated

4 years ago
Attachment #746311 - Flags: review?(bugs) → review+

Comment 4

4 years ago
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 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.
Attachment #746310 - Flags: review?(htsai) → review+
Attachment #746311 - Flags: review?(htsai) → review+
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().'
Attachment #746312 - Flags: review?(htsai)
(Reporter)

Comment 7

4 years ago
(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.
Flags: needinfo?(dcoloma)
(Assignee)

Comment 8

4 years ago
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.
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 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.
Attachment #746310 - Flags: review+
(Assignee)

Comment 11

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

Comment 12

4 years ago
Created attachment 747253 [details] [diff] [review]
[PATCH 3/3 #2] Call barring RIL implementation
Attachment #746312 - Attachment is obsolete: true
Attachment #747253 - Flags: review?(htsai)
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.
Attachment #747253 - Flags: review?(htsai) → review+
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? :)
Flags: needinfo?(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.
Flags: needinfo?(mounir)
(Assignee)

Comment 16

4 years ago
Created attachment 749745 [details] [diff] [review]
Part 1: Add interface for call barring config.

change from interface to dictionary
Attachment #746310 - Attachment is obsolete: true
Attachment #749745 - Flags: superreview?(mounir)
Attachment #749745 - Flags: review?(htsai)
(Assignee)

Comment 17

4 years ago
Created attachment 749748 [details] [diff] [review]
Part 2: Call barring DOM implementation.
Attachment #749748 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #746311 - Attachment is obsolete: true
(Assignee)

Comment 18

4 years ago
Created attachment 749750 [details] [diff] [review]
Part 3: Call barring RIL implementation.
Attachment #747253 - Attachment is obsolete: true
Attachment #749750 - Flags: review?(htsai)
(Assignee)

Comment 19

4 years ago
Created attachment 749752 [details] [diff] [review]
Part 4: Add test for call barring.
Attachment #749752 - Flags: review?(htsai)
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+.
Attachment #749745 - Flags: superreview?(mounir)
(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
Flags: needinfo?(dcoloma) → needinfo?(ffos-product)
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();
}
Attachment #749752 - Flags: review?(htsai)
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.
Attachment #749745 - Flags: review?(htsai) → review+
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.
Attachment #749750 - Flags: review?(htsai) → review+
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.
Attachment #749748 - Flags: review?(htsai) → feedback+
(Assignee)

Comment 26

4 years ago
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.
Attachment #749752 - Flags: review?(htsai)
(Assignee)

Comment 27

4 years ago
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?
Attachment #749750 - Flags: feedback?(htsai)
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.
Attachment #749750 - Flags: feedback?(htsai)
(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.
(Assignee)

Comment 30

4 years ago
Created attachment 751576 [details] [diff] [review]
Part 4 #2: Add test for call barring.
Attachment #749752 - Attachment is obsolete: true
Attachment #749752 - Flags: review?(htsai)
Attachment #751576 - Flags: review?(htsai)
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!
Attachment #751576 - Flags: review?(htsai) → review+
(Assignee)

Comment 32

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

need DOM peer's review
Attachment #749748 - Flags: review?(bugs)
(Assignee)

Comment 33

4 years ago
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.
(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 :)

Updated

4 years ago
Attachment #749748 - Flags: review?(bugs) → review+
(Assignee)

Comment 35

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=81885b6d25ff
(Assignee)

Comment 36

4 years ago
Created attachment 751990 [details] [diff] [review]
Part 1 #2: Add interface for call barring config.
Attachment #749745 - Attachment is obsolete: true
(Assignee)

Comment 37

4 years ago
Created attachment 751991 [details] [diff] [review]
Part 2 #2: Call barring DOM implementation.
Attachment #749748 - Attachment is obsolete: true
(Assignee)

Comment 38

4 years ago
Created attachment 751993 [details] [diff] [review]
Part 3 #2: Call barring RIL implementation.
Attachment #749750 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
Created attachment 751994 [details] [diff] [review]
Part 4 #3: Add test for call barring.
(Assignee)

Updated

4 years ago
Attachment #751576 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
Whiteboard: [fixed-in-birch]
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

4 years ago
Blocks: 890831

Comment 42

4 years ago
clearing flag
Flags: needinfo?(ffos-product)
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 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?
(Assignee)

Updated

4 years ago
Blocks: 907018
(Assignee)

Updated

4 years ago
No longer blocks: 907018
You need to log in before you can comment on or make changes to this bug.