Last Comment Bug 818393 - B2G RIL: Support call barring.
: B2G RIL: Support call barring.
Status: RESOLVED FIXED
[fixed-in-birch]
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla24
Assigned To: Szu-Yu Chen [:aknow] (inactive)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: b2g-ril 833754 890831
  Show dependency treegraph
 
Reported: 2012-12-05 00:06 PST by José Antonio Olivera Ortega [:jaoo]
Modified: 2013-08-22 20:31 PDT (History)
20 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[PATCH 1/3] Add interface for call barring config (5.13 KB, patch)
2013-05-07 03:05 PDT, Szu-Yu Chen [:aknow] (inactive)
mounir: superreview+
Details | Diff | Splinter Review
[PATCH 2/3] Call barring DOM implementation (1.28 KB, patch)
2013-05-07 03:06 PDT, Szu-Yu Chen [:aknow] (inactive)
htsai: review+
bugs: review+
Details | Diff | Splinter Review
[PATCH 3/3] Call barring RIL implementation (12.00 KB, patch)
2013-05-07 03:06 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Splinter Review
[PATCH 3/3 #2] Call barring RIL implementation (14.76 KB, patch)
2013-05-08 19:38 PDT, Szu-Yu Chen [:aknow] (inactive)
htsai: review+
Details | Diff | Splinter Review
Part 1: Add interface for call barring config. (5.73 KB, patch)
2013-05-15 01:40 PDT, Szu-Yu Chen [:aknow] (inactive)
htsai: review+
Details | Diff | Splinter Review
Part 2: Call barring DOM implementation. (1.27 KB, patch)
2013-05-15 01:41 PDT, Szu-Yu Chen [:aknow] (inactive)
bugs: review+
htsai: feedback+
Details | Diff | Splinter Review
Part 3: Call barring RIL implementation. (14.19 KB, patch)
2013-05-15 01:42 PDT, Szu-Yu Chen [:aknow] (inactive)
htsai: review+
Details | Diff | Splinter Review
Part 4: Add test for call barring. (3.99 KB, patch)
2013-05-15 01:43 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Splinter Review
Part 4 #2: Add test for call barring. (4.12 KB, patch)
2013-05-19 23:37 PDT, Szu-Yu Chen [:aknow] (inactive)
htsai: review+
Details | Diff | Splinter Review
Part 1 #2: Add interface for call barring config. (5.81 KB, patch)
2013-05-20 21:06 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Splinter Review
Part 2 #2: Call barring DOM implementation. (1.28 KB, patch)
2013-05-20 21:07 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Splinter Review
Part 3 #2: Call barring RIL implementation. (11.88 KB, patch)
2013-05-20 21:08 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Splinter Review
Part 4 #3: Add test for call barring. (4.13 KB, patch)
2013-05-20 21:08 PDT, Szu-Yu Chen [:aknow] (inactive)
no flags Details | Diff | Splinter Review

Description José Antonio Olivera Ortega [:jaoo] 2012-12-05 00:06:11 PST
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) 2013-05-07 03:05:20 PDT
Created attachment 746310 [details] [diff] [review]
[PATCH 1/3] Add interface for call barring config
Comment 2 Szu-Yu Chen [:aknow] (inactive) 2013-05-07 03:06:00 PDT
Created attachment 746311 [details] [diff] [review]
[PATCH 2/3] Call barring DOM implementation
Comment 3 Szu-Yu Chen [:aknow] (inactive) 2013-05-07 03:06:31 PDT
Created attachment 746312 [details] [diff] [review]
[PATCH 3/3] Call barring RIL implementation
Comment 4 Anshul 2013-05-07 12:54:38 PDT
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 [:hsinyi] 2013-05-07 20:16:27 PDT
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 [:hsinyi] 2013-05-07 21:25:13 PDT
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] 2013-05-08 00:58:16 PDT
(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) 2013-05-08 01:57:48 PDT
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 [:hsinyi] 2013-05-08 02:42:26 PDT
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 [:hsinyi] 2013-05-08 03:20:36 PDT
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) 2013-05-08 05:06:42 PDT
(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) 2013-05-08 19:38:38 PDT
Created attachment 747253 [details] [diff] [review]
[PATCH 3/3 #2] Call barring RIL implementation
Comment 13 Hsin-Yi Tsai [:hsinyi] 2013-05-08 20:51:43 PDT
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 [:hsinyi] 2013-05-09 04:16:38 PDT
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) 2013-05-10 04:18:38 PDT
(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) 2013-05-15 01:40:53 PDT
Created attachment 749745 [details] [diff] [review]
Part 1: Add interface for call barring config.

change from interface to dictionary
Comment 17 Szu-Yu Chen [:aknow] (inactive) 2013-05-15 01:41:38 PDT
Created attachment 749748 [details] [diff] [review]
Part 2: Call barring DOM implementation.
Comment 18 Szu-Yu Chen [:aknow] (inactive) 2013-05-15 01:42:28 PDT
Created attachment 749750 [details] [diff] [review]
Part 3: Call barring RIL implementation.
Comment 19 Szu-Yu Chen [:aknow] (inactive) 2013-05-15 01:43:00 PDT
Created attachment 749752 [details] [diff] [review]
Part 4: Add test for call barring.
Comment 20 Mounir Lamouri (:mounir) 2013-05-15 02:57:59 PDT
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 2013-05-15 17:02:02 PDT
(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 [:hsinyi] 2013-05-19 20:18:40 PDT
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 [:hsinyi] 2013-05-19 20:19:28 PDT
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 [:hsinyi] 2013-05-19 20:20:34 PDT
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 [:hsinyi] 2013-05-19 20:21:23 PDT
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) 2013-05-19 22:06:02 PDT
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) 2013-05-19 22:30:05 PDT
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 [:hsinyi] 2013-05-19 22:41:07 PDT
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 [:hsinyi] 2013-05-19 22:43:00 PDT
(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) 2013-05-19 23:37:35 PDT
Created attachment 751576 [details] [diff] [review]
Part 4 #2: Add test for call barring.
Comment 31 Hsin-Yi Tsai [:hsinyi] 2013-05-19 23:39:34 PDT
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) 2013-05-19 23:40:34 PDT
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) 2013-05-19 23:45:02 PDT
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 [:hsinyi] 2013-05-19 23:47:04 PDT
(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) 2013-05-20 20:51:40 PDT
https://tbpl.mozilla.org/?tree=Try&rev=81885b6d25ff
Comment 36 Szu-Yu Chen [:aknow] (inactive) 2013-05-20 21:06:58 PDT
Created attachment 751990 [details] [diff] [review]
Part 1 #2: Add interface for call barring config.
Comment 37 Szu-Yu Chen [:aknow] (inactive) 2013-05-20 21:07:32 PDT
Created attachment 751991 [details] [diff] [review]
Part 2 #2: Call barring DOM implementation.
Comment 38 Szu-Yu Chen [:aknow] (inactive) 2013-05-20 21:08:07 PDT
Created attachment 751993 [details] [diff] [review]
Part 3 #2: Call barring RIL implementation.
Comment 39 Szu-Yu Chen [:aknow] (inactive) 2013-05-20 21:08:36 PDT
Created attachment 751994 [details] [diff] [review]
Part 4 #3: Add test for call barring.
Comment 42 Sandip Kamat 2013-07-15 10:15:50 PDT
clearing flag
Comment 43 Yoshi Huang[:allstars.chh] 2013-08-19 20:21:09 PDT
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 Huang[:allstars.chh] 2013-08-19 20:24:18 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.