905479 - B2G RIL: Support Change Call Barring Password

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[shawn ku [:sku]]

Currently, B2G project do not support change call barring password which is a standard of 3GPP feature.

Comment 1

[shawn ku [:sku]]

Attachment: Patch for 905479

Comment 2

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

Comment on attachment 790709 [details] [diff] [review]
Patch for 905479

Hi Shawn
You need to split these huge patches into 4 parts.

1. IDL

2. DOM (MobileConnection.cpp), and this needs DOM peer to review this, not hsinyi nor I can review it.

3. RIL 

4. Tests.

Comment 3

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #2)
> Comment on attachment 790709 [details] [diff] [review]
> Patch for 905479
> 
> Hi Shawn
> You need to split these huge patches into 4 parts.
> 
> 1. IDL
> 
> 2. DOM (MobileConnection.cpp), and this needs DOM peer to review this, not
> hsinyi nor I can review it.
> 
> 3. RIL 
> 
> 4. Tests.

Hi Yoshi:
 Thanks for your comment, I will follow the rules and update patch again.
sku

Comment 4

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 1: Add interfaces for change call barring password.

Comment 5

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password.

Comment 6

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 4: Add test for change call barring password.

Comment 7

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 2: DOM implementation for change call barring password.

Comment 8

[shawn ku [:sku]]

Comment on attachment 791077 [details] [diff] [review]
Bug 905479 - Part 2: DOM implementation for change call barring password.

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

Change reviewer due to suggestion from RIL peer.

Comment 9

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

Comment on attachment 791072 [details] [diff] [review]
Bug 905479 - Part 1: Add interfaces for change call barring password.

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

This patch looks good to me,
however Hsinyi knows more about this,
so I leave the r? to her.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +335,5 @@
> +   *
> +   *   changeCallBarringPassword({pin: "...",
> +   *                              newPin: "..."});
> +   */
> +  nsIDOMDOMRequest changeCallBarringPassword(in jsval info);

Can you move this function to somewhere near Call Barring functions?
We'd like to group the similar functions together.

Comment 10

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

Comment on attachment 791073 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password.

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

It looks okay, I also leave the r? to Hsinyi.

::: dom/system/gonk/RILContentHelper.js
@@ +108,5 @@
>    "RIL:CdmaCallWaiting",
>    "RIL:ExitEmergencyCbMode",
>    "RIL:SetVoicePrivacyMode",
> +  "RIL:GetVoicePrivacyMode",
> +  "RIL:ChangeCallBarringPassword"

Same with Part1, group call barring message together.

@@ +1209,5 @@
> +      data: {
> +        requestId: requestId,
> +        pin: info.pin,
> +        newPin: info.newPin
> +      }

Seems we can reuse the info object here.

info.requestId = requestId;

...
clientId: 0,
data: info

@@ +1214,5 @@
> +    });
> +
> +    return request;
> +  },
> +

Again, move this function.

@@ +1708,1 @@
>      }

ditto.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +113,5 @@
>    "RIL:GetRoamingPreference",
>    "RIL:ExitEmergencyCbMode",
>    "RIL:SetVoicePrivacyMode",
> +  "RIL:GetVoicePrivacyMode",
> +  "RIL:ChangeCallBarringPassword"

Ditto.

@@ +1032,5 @@
>          this.workerMessenger.sendWithIPCMessage(msg, "queryVoicePrivacyMode");
>          break;
> +      case "RIL:ChangeCallBarringPassword":
> +        this.workerMessenger.sendWithIPCMessage(msg, "changeCallBarringPassword");
> +        break;

Ditto.

@@ +2440,5 @@
> +      this.debug("changeCallBarringPassword: " + JSON.stringify(message));
> +    }
> +    this.workerMessenger.send("changeCallBarringPassword",
> +                              message, (function(response) {
> +      target.sendAsyncMessage("RIL:ChangeCallBarringPassword", response);

I think you need to return false in this function,
please also help to add 'return false' in the SetCallingLineIdRestriction above.

::: dom/system/gonk/ril_worker.js
@@ +5788,5 @@
> +    this.sendChromeMessage(options);
> +    return;
> +  }
> +
> +  this.sendChromeMessage(options);

if (options.rilRequestError) {
  options.errorMsg = ...
}
this.sendChromeMessage(options);

Comment 11

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

Comment on attachment 791075 [details] [diff] [review]
Bug 905479 - Part 4: Add test for change call barring password.

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

I don't think the tests here are complete.
I'd encourage you to revise them.

::: dom/network/tests/marionette/test_call_barring_change_password_set_error.js
@@ +22,5 @@
> +let caseId = 0;
> +let options = [
> +  buildOption(  null, '0000'),
> +  buildOption('0000',   null),
> +  buildOption(  null,   null),

You don't have to do the strange alignment here. Just (param1, param2).

@@ +42,5 @@
> +  };
> +  request.onerror = function() {
> +    log('onError : ' + request.error.name);
> +    nextTest();
> +  };

I think you wrote this because emulator doesn't support this yet.

However if one day emulator supports this,
this test case has to be removed or completely rewritten.

I'd suggest you need to rewrite this test case.

Please file bug for adding support for changing call barring password on emulator.
And add TODO in this patch.

::: dom/system/gonk/tests/test_ril_worker_barring_password.js
@@ +22,5 @@
> +    },
> +    get worker() {
> +      return _worker;
> +    }
> +  };

It looks to me that you could just override postMessage method in your test,
without having to construct these functions/objects.

Like
add_test(function ..() {
  worker = new Worker();
  worker.postMessage = function fakePostMessage(message) {
    do_check_eq(...);
  }
})

@@ +65,5 @@
> +
> +  do_check_eq(postedMessage.errorMsg, GECKO_ERROR_PASSWORD_INCORRECT);
> +  do_check_false(postedMessage.success);
> +  run_next_test();
> +});

These two tests don't make too much sense to me.

You also need to test those arguments, like facility, pin, newPin.

Comment 12

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #11)
> Comment on attachment 791075 [details] [diff] [review]
> Bug 905479 - Part 4: Add test for change call barring password.
> 
> Review of attachment 791075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think the tests here are complete.
> I'd encourage you to revise them.
> 
> :::
> dom/network/tests/marionette/test_call_barring_change_password_set_error.js
> @@ +22,5 @@
> > +let caseId = 0;
> > +let options = [
> > +  buildOption(  null, '0000'),
> > +  buildOption('0000',   null),
> > +  buildOption(  null,   null),
> 
> You don't have to do the strange alignment here. Just (param1, param2).
> 
> @@ +42,5 @@
> > +  };
> > +  request.onerror = function() {
> > +    log('onError : ' + request.error.name);
> > +    nextTest();
> > +  };
> 
> I think you wrote this because emulator doesn't support this yet.
> 
> However if one day emulator supports this,
> this test case has to be removed or completely rewritten.
> 
> I'd suggest you need to rewrite this test case.
> 
> Please file bug for adding support for changing call barring password on
> emulator.
> And add TODO in this patch.
> 
> ::: dom/system/gonk/tests/test_ril_worker_barring_password.js
> @@ +22,5 @@
> > +    },
> > +    get worker() {
> > +      return _worker;
> > +    }
> > +  };
> 
> It looks to me that you could just override postMessage method in your test,
> without having to construct these functions/objects.
> 
> Like
> add_test(function ..() {
>   worker = new Worker();
>   worker.postMessage = function fakePostMessage(message) {
>     do_check_eq(...);
>   }
> })
> 
> @@ +65,5 @@
> > +
> > +  do_check_eq(postedMessage.errorMsg, GECKO_ERROR_PASSWORD_INCORRECT);
> > +  do_check_false(postedMessage.success);
> > +  run_next_test();
> > +});
> 
> These two tests don't make too much sense to me.
> 
> You also need to test those arguments, like facility, pin, newPin.

Hi Yoshi:
 Thanks for your comment. I will check if any updated patch can be provided soon.

BTW,

> These two tests don't make too much sense to me.
> 
> You also need to test those arguments, like facility, pin, newPin.

IMHO, we only create an API with two parameters which are pin and newPin. For facility part, it is set to ICC_CB_FACILITY_BA_ALL on purpose. (Please check the reason below.), That means it is not necessary to check facility for changeCallBarringPassword request. Besides, I will check if any further check for pin/newPin is available/necessary.

Plesae correct me if anything is wrong.
Thanks!!
sku

  /**
   * Change call barring facility password.
   *
   * @param pin
   *        old password.
   * @param newPin
   *        new password.
   */
  changeCallBarringPassword: function changeCallBarringPassword(options) {
    Buf.newParcel(REQUEST_CHANGE_BARRING_PASSWORD, options);
    Buf.writeUint32(3);
    // As defined in TS.22.088 clause 0, one password per mobile subscriber is
    // supported by the network for all barring services, and, for Barring
    // Services MMI, ZZ = 330 (all Barring Serv.) is defined in TS.22.030 clause
    // 6.5.4, hence, the facility string is set to ICC_CB_FACILITY_BA_ALL when
    // changing call barring password.
    Buf.writeString(ICC_CB_FACILITY_BA_ALL);
    Buf.writeString(options.pin);
    Buf.writeString(options.newPin);
    Buf.sendParcel();
  },

Comment 13

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

(In reply to shawn ku [:sku] from comment #12)
> IMHO, we only create an API with two parameters which are pin and newPin.
> For facility part, it is set to ICC_CB_FACILITY_BA_ALL on purpose. (Please
> check the reason below.), That means it is not necessary to check facility
> for changeCallBarringPassword request. 

In that case, you're just writing dummy test.

When you implement the function, you implement it by reading specification.

But when you're going to write the tests, you also need to do the same thing, you need to read the specification.
You cannot write your tests by reading your implementation.

So there's why I said your test should do something like checking there is a string array of length 3, in the array the 1st element should be facility string , which should be XXX, and the 2nd should be ...., ..etc

When you're writing tests, you need to forget what you've done in the implementation. You have to pretend yourself a different people, and write tests according to the specification.

Comment 14

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #13)
> (In reply to shawn ku [:sku] from comment #12)
> > IMHO, we only create an API with two parameters which are pin and newPin.
> > For facility part, it is set to ICC_CB_FACILITY_BA_ALL on purpose. (Please
> > check the reason below.), That means it is not necessary to check facility
> > for changeCallBarringPassword request. 
> 
> In that case, you're just writing dummy test.
> 
> When you implement the function, you implement it by reading specification.
> 
> But when you're going to write the tests, you also need to do the same
> thing, you need to read the specification.
> You cannot write your tests by reading your implementation.
> 
> So there's why I said your test should do something like checking there is a
> string array of length 3, in the array the 1st element should be facility
> string , which should be XXX, and the 2nd should be ...., ..etc
> 
> When you're writing tests, you need to forget what you've done in the
> implementation. You have to pretend yourself a different people, and write
> tests according to the specification.

Roger that, Thanks!!
sku

Comment 15

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

Comment on attachment 791072 [details] [diff] [review]
Bug 905479 - Part 1: Add interfaces for change call barring password.

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +332,5 @@
> +   *
> +   * @param info
> +   *        An object containing information about oldPin and newPin.
> +   *
> +   *   changeCallBarringPassword({pin: "...",

Please remove this.

@@ +335,5 @@
> +   *
> +   *   changeCallBarringPassword({pin: "...",
> +   *                              newPin: "..."});
> +   */
> +  nsIDOMDOMRequest changeCallBarringPassword(in jsval info);

Agree with Yoshi's comment about moving this function to a more appropriate place.

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +81,5 @@
>    nsIDOMDOMRequest getCallingLineIdRestriction(in nsIDOMWindow   window);
>  
>    nsIDOMDOMRequest exitEmergencyCbMode(in nsIDOMWindow window);
> +
> +  nsIDOMDOMRequest changeCallBarringPassword(in nsIDOMWindow window,

Same here.

Comment 16

[shawn ku [:sku]]

Comment on attachment 791073 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password.

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

Cancel review request, revise based on Yoshi comment first.

Comment 17

[shawn ku [:sku]]

Comment on attachment 791075 [details] [diff] [review]
Bug 905479 - Part 4: Add test for change call barring password.

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

Cancel review request, revise based on Yoshi comment first.

Comment 18

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 1: Add interfaces for change call barring password.

1. revise comment
2. group barring related features

Comment 19

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 2: DOM implementation for change call barring password. r=bugs

Comment 20

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password.

1. gorup the barring features
2. issue proper return false (also on CLIR part)
3. revise according to Yoshi's suggesion on ril_worker.js
4. add pin/newPin valid check according to hsinyi's suggestion on RILContentHelper.js

Comment 21

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 4: Add test for change call barring password.

1. fire Bug 906603 for supporting change barring password on emulator, left TODO: on "dom/network/tests/marionette/test_call_barring_change_password.js"
2. add parcel check test case in dom/system/gonk/tests/test_ril_worker_barring_password.js to make sure we comply the definition on ril.h

Comment 22

[shawn ku [:sku]]

Comment on attachment 792099 [details] [diff] [review]
Bug 905479 - Part 4: Add test for change call barring password.

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

will re-upload patch later today.

Comment 23

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 4: Add test for change call barring password.

add one more test case

Comment 24

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

Comment on attachment 792089 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password.

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

::: dom/system/gonk/RILContentHelper.js
@@ +1111,5 @@
> +    let requestId = this.getRequestId(request);
> +
> +    if (DEBUG) debug("changeCallBarringPassword: " + JSON.stringify(info));
> +    if (!this._isValidSupplementaryServicesPin(info.pin)
> +        || !this._isValidSupplementaryServicesPin(info.newPin)) {

if (A ||
    B)
in that way you can align A expression and B expression.

Also we can move _isValidSupplementaryServicesPin to a local function.

@@ +1122,5 @@
> +      data: {
> +        requestId: requestId,
> +        pin: info.pin,
> +        newPin: info.newPin
> +      }

Can't we reuse info?
I think I have pointed this out before.

@@ +2042,5 @@
> +
> +  /**
> +   * Helper for checking valid supplementary services PIN
> +   */
> +  _isValidSupplementaryServicesPin: function

Move to a local function

@@ +2048,5 @@
> +    // As defined in TS.22.004 clause 5.2, the password will consist of four
> +    // digits in the range 0000 to 9999.
> +    return (pin
> +            && pin !== null
> +            && pin.length === 4);

return pin != null && pin.length === 4;

Comment 25

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

Comment on attachment 792566 [details] [diff] [review]
Bug 905479 - Part 4: Add test for change call barring password.

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

::: dom/network/tests/marionette/test_call_barring_change_password.js
@@ +38,5 @@
> +
> +function testChangeCallBarringPassword(options) {
> +  let request = connection.changeCallBarringPassword(options);
> +  request.onsuccess = function() {
> +    ok(option.successResult,

You are checking an object which is not returned from DOMRequest.
Given that you have updated RILContentHelper,
I think for some test data (empty pin), you should expect your onerror will be called, instead of onsuccess.

::: dom/system/gonk/tests/test_ril_worker_barring_password.js
@@ +48,5 @@
> +    get worker() {
> +      return _worker;
> +    }
> +  };
> +}

Have you addressed my previous comment?
I still don't understand why you spent 17 lines,
which can be just
worker.postMessage = function (message) {
..
}

@@ +118,5 @@
> +  do_check_false(postedMessage.success);
> +  run_next_test();
> +});
> +
> +add_test(function test_change_call_barring_password_invalid() {

It looks to me all these 3 functions are testing for the same thing, errorMsg.
In that case you could just move it into one function.

Comment 26

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password. (3rd revised))

Hi Yoshi:
 Thanks for your comment/suggestion, pelase let me know if I still miss anything.

1. revise format.
2. re-use info instead of creating new object.
3. keep _isValidSupplementaryServicesPin, use it on both changeCallBarringPassword and setCallBarringOption.

Comment 27

[shawn ku [:sku]]

Comment on attachment 792650 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (3rd revised))

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

The patch seems not the same as I modified. check it at local first.

Comment 28

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password. (3rd patch)

Comment 29

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password. (3rd revised patch))

Comment 30

[shawn ku [:sku]]

Comment on attachment 792655 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (3rd revised patch))

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

Hi Yoshi:
 Thanks for your comment/suggestion, pelase let me know if anything I missed.

Change scope:
1. revise format.
2. re-use info instead of creating new object.
3. keep _isValidSupplementaryServicesPin, and, use it on both changeCallBarringPassword and setCallBarringOption.

Comment 31

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

Comment on attachment 792655 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (3rd revised patch))

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

This patch looks better now,
but I would like to remove the pin !== null check,
otherwise it would throw TypeError when pin is undefined.
Cancelling review for this reason.

::: dom/system/gonk/RILContentHelper.js
@@ +2044,5 @@
> +
> +  /**
> +   * Helper for checking valid supplementary services PIN.
> +   */
> +  _isValidSupplementaryServicesPin: function

The grammer seems strange to me,
you could ask Hsinyi for this.
I think it should be _isValidPinForSupplementaryService,
or _isValidPinForSS is fine by me.

@@ +2048,5 @@
> +  _isValidSupplementaryServicesPin: function
> +    _isValidSupplementaryServicesPin(pin) {
> +    // As defined in TS.22.004 clause 5.2, the password will consist of four
> +    // digits in the range 0000 to 9999.
> +    return (pin !== null && pin.length === 4);

I think I wrote
pin != null 
in my previous comment.

You could test your code, and find out what will happen when the pin is undefined.

Comment 32

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password. (4th revision)

Comment 33

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 4: Add test for change call barring password. (4th revision)

Comment 34

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

Comment on attachment 793863 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (4th revision)

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

::: dom/system/gonk/RILContentHelper.js
@@ +1089,5 @@
>        this.dispatchFireRequestError(requestId, "InvalidCallBarringOption");
>        return request;
>      }
>  
> +    if (!this._isValidPinForSupplementaryService(option.password)) {

Why here? password isn't necessary for setting call barring options, no?

@@ +2080,5 @@
> +
> +  /**
> +   * Helper for checking valid PIN for supplementary services.
> +   */
> +  _isValidPinForSupplementaryService: function

Really long name here... 

A stupid question: I think PIN is PIN. I meant, do we need to distinguish if this is a PIN for "supplementary services"? Can't the function name be _isValidPin?

@@ +2082,5 @@
> +   * Helper for checking valid PIN for supplementary services.
> +   */
> +  _isValidPinForSupplementaryService: function
> +    _isValidPinForSupplementaryService(pin) {
> +    // As defined in TS.22.004 clause 5.2, the password will consist of four

nit: I would like to have the comment brief. |See TS 22.004 clause 5.2.| looks enough to me.

@@ +2084,5 @@
> +  _isValidPinForSupplementaryService: function
> +    _isValidPinForSupplementaryService(pin) {
> +    // As defined in TS.22.004 clause 5.2, the password will consist of four
> +    // digits in the range 0000 to 9999.
> +    return (pin != null && pin.length === 4);

If we want to completely follow the spec, 'pin.length === 4' seems not enough for the definition '0000 to 9999'.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2488,5 @@
>          this._updateCallingLineIdRestrictionPref(response.clirMode);
>        }
>        target.sendAsyncMessage("RIL:SetCallingLineIdRestriction", response);
> +      return false;
> +    }).bind(this));

Why this?

@@ +2491,5 @@
> +      return false;
> +    }).bind(this));
> +  },
> +
> +  changeCallBarringPassword: function changeCallBarringPassword(target,

You don't need this function. 

|this.workerMessenger.sendWithIPCMessage(msg, "changeCallBarringPassword");| already does what you need.

::: dom/system/gonk/ril_worker.js
@@ +2938,5 @@
>    /**
> +   * Change call barring facility password.
> +   *
> +   * @param pin
> +   *        old password.

s/old/Old

@@ +2940,5 @@
> +   *
> +   * @param pin
> +   *        old password.
> +   * @param newPin
> +   *        new password.

s/new/New

@@ +2945,5 @@
> +   */
> +  changeCallBarringPassword: function changeCallBarringPassword(options) {
> +    Buf.newParcel(REQUEST_CHANGE_BARRING_PASSWORD, options);
> +    Buf.writeUint32(3);
> +    // As defined in TS.22.088 clause 0, one password per mobile subscriber is

I would like to move these explanations to line #2945. And please try to make it briefer if possible. :)

@@ +2947,5 @@
> +    Buf.newParcel(REQUEST_CHANGE_BARRING_PASSWORD, options);
> +    Buf.writeUint32(3);
> +    // As defined in TS.22.088 clause 0, one password per mobile subscriber is
> +    // supported by the network for all barring services, and, for Barring
> +    // Services MMI, ZZ = 330 (all Barring Serv.) is defined in TS.22.030 clause

Hm, what does ZZ mean?

s/Serv./Service

@@ +2948,5 @@
> +    Buf.writeUint32(3);
> +    // As defined in TS.22.088 clause 0, one password per mobile subscriber is
> +    // supported by the network for all barring services, and, for Barring
> +    // Services MMI, ZZ = 330 (all Barring Serv.) is defined in TS.22.030 clause
> +    // 6.5.4, hence, the facility string is set to ICC_CB_FACILITY_BA_ALL when

grammar: s/, hence/. Hence

@@ +5948,5 @@
>    this.sendChromeMessage(options);
>  };
> +RIL[REQUEST_CHANGE_BARRING_PASSWORD] =
> +  function REQUEST_CHANGE_BARRING_PASSWORD(length, options) {
> +  options.success = (options.rilRequestError === 0);

Having options.rilRequestError seems enough. We don't need to have options.success.

Comment 35

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

Comment on attachment 793863 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (4th revision)

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

::: dom/system/gonk/RILContentHelper.js
@@ +1089,5 @@
>        this.dispatchFireRequestError(requestId, "InvalidCallBarringOption");
>        return request;
>      }
>  
> +    if (!this._isValidPinForSupplementaryService(option.password)) {

Why here? password isn't necessary for setting call barring options, no? And I would prefer filing another bug to correct setCallBarringOptions if there's an error. Thank you.

Comment 36

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

Comment on attachment 793863 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (4th revision)

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

::: dom/system/gonk/RILContentHelper.js
@@ +2084,5 @@
> +  _isValidPinForSupplementaryService: function
> +    _isValidPinForSupplementaryService(pin) {
> +    // As defined in TS.22.004 clause 5.2, the password will consist of four
> +    // digits in the range 0000 to 9999.
> +    return (pin != null && pin.length === 4);

You could use regex: pin.match(/^\d{4}$/)

Comment 37

[shawn ku [:sku]]

Hi Hsinyi:
  Thanks for your reply. please check my inline reply.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #34)
> Comment on attachment 793863 [details] [diff] [review]
> Bug 905479 - Part 3: RIL implementation on change call barring password.
> (4th revision)
> 
> Review of attachment 793863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1089,5 @@
> >        this.dispatchFireRequestError(requestId, "InvalidCallBarringOption");
> >        return request;
> >      }
> >  
> > +    if (!this._isValidPinForSupplementaryService(option.password)) {
> 
> Why here? password isn't necessary for setting call barring options, no?

Yoshi said, I only need to use private function to do the validation check, but, if more than one place need it, I can let _isValidPinForSupplementaryService to be a member of RILContentHelper.js. To be more consist, I let set call barring options to share the same check as change call barring password.


> 
> @@ +2080,5 @@
> > +
> > +  /**
> > +   * Helper for checking valid PIN for supplementary services.
> > +   */
> > +  _isValidPinForSupplementaryService: function
> 
> Really long name here... 
> 
> A stupid question: I think PIN is PIN. I meant, do we need to distinguish if
> this is a PIN for "supplementary services"? Can't the function name be
> _isValidPin?
> 
For SIM PIN, 4~8 digits are all valid one.
For SS PIN, spec. define 4 digits only (ranged between 0000 ~ 9999).
That's why I name it as supplementray service pin.

> @@ +2082,5 @@
> > +   * Helper for checking valid PIN for supplementary services.
> > +   */
> > +  _isValidPinForSupplementaryService: function
> > +    _isValidPinForSupplementaryService(pin) {
> > +    // As defined in TS.22.004 clause 5.2, the password will consist of four
> 
> nit: I would like to have the comment brief. |See TS 22.004 clause 5.2.|
> looks enough to me.
> 
Roger that!! Thanks!!

> @@ +2084,5 @@
> > +  _isValidPinForSupplementaryService: function
> > +    _isValidPinForSupplementaryService(pin) {
> > +    // As defined in TS.22.004 clause 5.2, the password will consist of four
> > +    // digits in the range 0000 to 9999.
> > +    return (pin != null && pin.length === 4);
> 
> If we want to completely follow the spec, 'pin.length === 4' seems not
> enough for the definition '0000 to 9999'.
Well, I will check if any better way to approach.

> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +2488,5 @@
> >          this._updateCallingLineIdRestrictionPref(response.clirMode);
> >        }
> >        target.sendAsyncMessage("RIL:SetCallingLineIdRestriction", response);
> > +      return false;
> > +    }).bind(this));
> 
> Why this?
please check Comment 10.

> 
> @@ +2491,5 @@
> > +      return false;
> > +    }).bind(this));
> > +  },
> > +
> > +  changeCallBarringPassword: function changeCallBarringPassword(target,
> 
> You don't need this function. 
> 
> |this.workerMessenger.sendWithIPCMessage(msg, "changeCallBarringPassword");|
> already does what you need.
Will check.

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2938,5 @@
> >    /**
> > +   * Change call barring facility password.
> > +   *
> > +   * @param pin
> > +   *        old password.
> 
> s/old/Old
Roger that, will do!!

> 
> @@ +2940,5 @@
> > +   *
> > +   * @param pin
> > +   *        old password.
> > +   * @param newPin
> > +   *        new password.
> 
> s/new/New
Roger that, will do!!

> 
> @@ +2945,5 @@
> > +   */
> > +  changeCallBarringPassword: function changeCallBarringPassword(options) {
> > +    Buf.newParcel(REQUEST_CHANGE_BARRING_PASSWORD, options);
> > +    Buf.writeUint32(3);
> > +    // As defined in TS.22.088 clause 0, one password per mobile subscriber is
> 
> I would like to move these explanations to line #2945. And please try to
> make it briefer if possible. :)
Roger that, will do!!

> 
> @@ +2947,5 @@
> > +    Buf.newParcel(REQUEST_CHANGE_BARRING_PASSWORD, options);
> > +    Buf.writeUint32(3);
> > +    // As defined in TS.22.088 clause 0, one password per mobile subscriber is
> > +    // supported by the network for all barring services, and, for Barring
> > +    // Services MMI, ZZ = 330 (all Barring Serv.) is defined in TS.22.030 clause
> 
> Hm, what does ZZ mean?
> 
> s/Serv./Service
TS 22.030 clause 6.5.4 Registration of new password
Procedure:
* 03 * ZZ * OLD_PASSWORD * NEW_PASSWORD * NEW_PASSWORD #
The UE shall also support the alternative procedure:
** 03 * ZZ * OLD_PASSWORD * NEW_PASSWORD * NEW_PASSWORD #
where, for Barring Services, ZZ = 330;

> 
> @@ +2948,5 @@
> > +    Buf.writeUint32(3);
> > +    // As defined in TS.22.088 clause 0, one password per mobile subscriber is
> > +    // supported by the network for all barring services, and, for Barring
> > +    // Services MMI, ZZ = 330 (all Barring Serv.) is defined in TS.22.030 clause
> > +    // 6.5.4, hence, the facility string is set to ICC_CB_FACILITY_BA_ALL when
> 
> grammar: s/, hence/. Hence
Roger that, will do!!

> 
> @@ +5948,5 @@
> >    this.sendChromeMessage(options);
> >  };
> > +RIL[REQUEST_CHANGE_BARRING_PASSWORD] =
> > +  function REQUEST_CHANGE_BARRING_PASSWORD(length, options) {
> > +  options.success = (options.rilRequestError === 0);
> 
> Having options.rilRequestError seems enough. We don't need to have
> options.success.
Roger that, will do!!

Comment 38

[shawn ku [:sku]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #35)
> Comment on attachment 793863 [details] [diff] [review]
> Bug 905479 - Part 3: RIL implementation on change call barring password.
> (4th revision)
> 
> Review of attachment 793863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1089,5 @@
> >        this.dispatchFireRequestError(requestId, "InvalidCallBarringOption");
> >        return request;
> >      }
> >  
> > +    if (!this._isValidPinForSupplementaryService(option.password)) {
> 
> Why here? password isn't necessary for setting call barring options, no? And
> I would prefer filing another bug to correct setCallBarringOptions if
> there's an error. Thank you.

we need SS PIN to setting call barring options, 
for query, there is no SS PIN needed.

Comment 39

[shawn ku [:sku]]

(In reply to Szu-Yu Chen [:aknow] from comment #36)
> Comment on attachment 793863 [details] [diff] [review]
> Bug 905479 - Part 3: RIL implementation on change call barring password.
> (4th revision)
> 
> Review of attachment 793863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +2084,5 @@
> > +  _isValidPinForSupplementaryService: function
> > +    _isValidPinForSupplementaryService(pin) {
> > +    // As defined in TS.22.004 clause 5.2, the password will consist of four
> > +    // digits in the range 0000 to 9999.
> > +    return (pin != null && pin.length === 4);
> 
> You could use regex: pin.match(/^\d{4}$/)

Thanks, aknow, will try to apply it on patch.

Comment 40

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

Comment on attachment 793864 [details] [diff] [review]
Bug 905479 - Part 4: Add test for change call barring password. (4th revision)

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

::: dom/network/tests/marionette/test_call_barring_change_password.js
@@ +24,5 @@
> +  // Incorrect parameters, expect onerror callback.
> +  let test_items = [
> +    {pin: null, newPin: '0000'},
> +    {pin: '0000', newPin: null},
> +    {pin: null, newPin: null},

I would like to see cases of passwords with non-numeric digits.

@@ +47,5 @@
> +      }
> +    };
> +  }
> +
> +  do_test(test_items[caseId++]);

Can't we do |caseId++| in only one place?
Doing that in do_test makes more sense to me.

And, no inline incremental operation.

@@ +51,5 @@
> +  do_test(test_items[caseId++]);
> +}
> +
> +function testChangeCallBarringPasswordWithSuccess() {
> +  // TODO: Bug 906603

Bug 906603 - B2G RIL: Support Change Call Barring Password on Emulator.

::: dom/system/gonk/tests/test_ril_worker_barring_password.js
@@ +70,5 @@
> +      do_print(JSON.stringify(barringPasswordOptions));
> +      do_check_eq(barringPasswordOptions.pin, "0000");
> +      do_check_eq(barringPasswordOptions.newPin, "1234");
> +      do_check_eq(message.errorMsg, GECKO_ERROR_SUCCESS);
> +      do_check_true(message.success);

I was suggesting removing options.success in ril_worker.js. Corresponding modification is required here. Thank you.

@@ +76,5 @@
> +  });
> +
> +  worker.RIL.changeCallBarringPassword =
> +    function fakeChangeCallBarringPassword(options) {
> +    barringPasswordOptions = options;

nit: indention

@@ +77,5 @@
> +
> +  worker.RIL.changeCallBarringPassword =
> +    function fakeChangeCallBarringPassword(options) {
> +    barringPasswordOptions = options;
> +    worker.RIL[REQUEST_CHANGE_BARRING_PASSWORD](0, {

ditto.

Comment 41

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

(In reply to shawn ku [:sku] from comment #37)
> Hi Hsinyi:
>   Thanks for your reply. please check my inline reply.
> 
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #34)
> > Comment on attachment 793863 [details] [diff] [review]
> > Bug 905479 - Part 3: RIL implementation on change call barring password.
> > (4th revision)
> > 
> > Review of attachment 793863 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RILContentHelper.js
> > @@ +1089,5 @@
> > >        this.dispatchFireRequestError(requestId, "InvalidCallBarringOption");
> > >        return request;
> > >      }
> > >  
> > > +    if (!this._isValidPinForSupplementaryService(option.password)) {
> > 
> > Why here? password isn't necessary for setting call barring options, no?
> 
> Yoshi said, I only need to use private function to do the validation check,
> but, if more than one place need it, I can let
> _isValidPinForSupplementaryService to be a member of RILContentHelper.js. To
> be more consist, I let set call barring options to share the same check as
> change call barring password.
> 

> 
> > 
> > @@ +2080,5 @@
> > > +
> > > +  /**
> > > +   * Helper for checking valid PIN for supplementary services.
> > > +   */
> > > +  _isValidPinForSupplementaryService: function
> > 
> > Really long name here... 
> > 
> > A stupid question: I think PIN is PIN. I meant, do we need to distinguish if
> > this is a PIN for "supplementary services"? Can't the function name be
> > _isValidPin?
> > 
> For SIM PIN, 4~8 digits are all valid one.
> For SS PIN, spec. define 4 digits only (ranged between 0000 ~ 9999).
> That's why I name it as supplementray service pin.
> 

Got it. Then how about _isValidPinFor SuppSvc? Thanks.


> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +2488,5 @@
> > >          this._updateCallingLineIdRestrictionPref(response.clirMode);
> > >        }
> > >        target.sendAsyncMessage("RIL:SetCallingLineIdRestriction", response);
> > > +      return false;
> > > +    }).bind(this));
> > 
> > Why this?
> please check Comment 10.

I see now!

> 
> > 
> > @@ +2491,5 @@
> > > +      return false;
> > > +    }).bind(this));
> > > +  },
> > > +
> > > +  changeCallBarringPassword: function changeCallBarringPassword(target,
> > 
> > You don't need this function. 
> > 
> > |this.workerMessenger.sendWithIPCMessage(msg, "changeCallBarringPassword");|
> > already does what you need.
> Will check.
> 
> > 

> > 
> > @@ +2947,5 @@
> > > +    Buf.newParcel(REQUEST_CHANGE_BARRING_PASSWORD, options);
> > > +    Buf.writeUint32(3);
> > > +    // As defined in TS.22.088 clause 0, one password per mobile subscriber is
> > > +    // supported by the network for all barring services, and, for Barring
> > > +    // Services MMI, ZZ = 330 (all Barring Serv.) is defined in TS.22.030 clause
> > 
> > Hm, what does ZZ mean?
> > 
> > s/Serv./Service
> TS 22.030 clause 6.5.4 Registration of new password
> Procedure:
> * 03 * ZZ * OLD_PASSWORD * NEW_PASSWORD * NEW_PASSWORD #
> The UE shall also support the alternative procedure:
> ** 03 * ZZ * OLD_PASSWORD * NEW_PASSWORD * NEW_PASSWORD #
> where, for Barring Services, ZZ = 330;
> 

ZZ is really not friendly for comment readers. And I feel that this sentence, |As defined in TS.22.088 clause 0, one password per mobile subscriber is supported by the network for all barring services|, explains the thing? Could you please try to rewrite the explanation? 

Thanks again :)

Comment 42

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

Comment on attachment 793863 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (4th revision)

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

::: dom/system/gonk/RILContentHelper.js
@@ +1119,5 @@
> +
> +    if (!this._isValidPinForSupplementaryService(info.pin) ||
> +        !this._isValidPinForSupplementaryService(info.newPin)) {
> +      this.dispatchFireRequestError(requestId,
> +                                    RIL.GECKO_ERROR_PASSWORD_INCORRECT);

The error message doesn't look right to me.

Comment 43

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

Comment on attachment 793864 [details] [diff] [review]
Bug 905479 - Part 4: Add test for change call barring password. (4th revision)

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

::: dom/network/tests/marionette/test_call_barring_change_password.js
@@ +38,5 @@
> +
> +    request.onerror = function() {
> +      is(request.error.name,
> +         'IncorrectPassword',
> +         'IncorrectPassword');

Again the error message doesn't quite fit into this.
Some API uses Throw, some return error Message.

You should choose one that makes sense.

@@ +43,5 @@
> +      if (caseId < test_items.length) {
> +        do_test(test_items[caseId++]);
> +      } else {
> +        setTimeout(testChangeCallBarringPasswordWithSuccess, 0);
> +      }

Oh you're calling itself recursively.

I think for-loop is easier.

for (let i =..) 
  do_test(..[i])

@@ +62,5 @@
> +
> +  // request.onerror = function() {
> +  //   ok(false, request.error.name);
> +  //   setTimeout(cleanUp , 0);
> +  // };

You don't have to add these comment-off code.

::: dom/system/gonk/tests/test_ril_worker_barring_password.js
@@ +64,5 @@
> +  let barringPasswordOptions;
> +  let worker = newWorker({
> +    postRILMessage: function fakePostRILMessage(data) {
> +      // Do nothing
> +    },

remove this method, unless you really need it.

@@ +66,5 @@
> +    postRILMessage: function fakePostRILMessage(data) {
> +      // Do nothing
> +    },
> +    postMessage: function fakePostMessage(message) {
> +      do_print(JSON.stringify(barringPasswordOptions));

Remove this

@@ +68,5 @@
> +    },
> +    postMessage: function fakePostMessage(message) {
> +      do_print(JSON.stringify(barringPasswordOptions));
> +      do_check_eq(barringPasswordOptions.pin, "0000");
> +      do_check_eq(barringPasswordOptions.newPin, "1234");

You can const "0000" and "1234"

Comment 44

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password. (5th revision)

Comment 45

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 4: Add test for change call barring password. r=yoshi.

Comment 46

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

Comment on attachment 794417 [details] [diff] [review]
Bug 905479 - Part 3: RIL implementation on change call barring password. (5th revision)

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

::: dom/system/gonk/RILContentHelper.js
@@ +1113,5 @@
> +
> +    // Checking valid PIN for supplementary services. See TS.22.004 clause 5.2.
> +    if (!(info.pin != null && info.pin.match(/^\d{4}$/)) ||
> +        !(info.newPin != null && info.newPin.match(/^\d{4}$/))) {
> +      this.dispatchFireRequestError(requestId, "InvalidPassword");

Could we express the condition slightly different?  
That said, 
if (info.pin == null || !info.pin.match(/^\d{4}$/)) || info.newPin == null || !info.pin.match(/^\d{4}$/))) 

Thanks!

::: dom/system/gonk/ril_worker.js
@@ +2946,5 @@
> +  changeCallBarringPassword: function changeCallBarringPassword(options) {
> +    Buf.newParcel(REQUEST_CHANGE_BARRING_PASSWORD, options);
> +    Buf.writeUint32(3);
> +    // Set facility to ICC_CB_FACILITY_BA_ALL by following TS.22.030 clause
> +    // 6.5.4 and Table B.1.

Much better now!

Comment 47

[shawn ku [:sku]]

Attachment: [Final] Bug 905479 - Part 1: Add interfaces for change call barring password. r=hsinyi, yoshi. sr=smaug

Comment 48

[shawn ku [:sku]]

Attachment: [Final] Bug 905479 - Part 2: DOM implementation for change call barring password. r=smaug

Comment 49

[shawn ku [:sku]]

Attachment: Bug 905479 - Part 3: RIL implementation on change call barring password. r=hsinyi, yoshi

Comment 50

[shawn ku [:sku]]

Attachment: [Final] Bug 905479 - Part 3: RIL implementation on change call barring password. r=hsinyi, yoshi

Comment 51

[shawn ku [:sku]]

Attachment: [Final] Bug 905479 - Part 4: Add test for change call barring password. r=yoshi

Comment 52

[shawn ku [:sku]]

Comment on attachment 795785 [details] [diff] [review]
[Final] Bug 905479 - Part 4: Add test for change call barring password. r=yoshi

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

Miss one grant from hsinyi for test case part. remove checkin needed tag, and, wait for hisiyi's review.

Comment 53

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

Comment on attachment 795785 [details] [diff] [review]
[Final] Bug 905479 - Part 4: Add test for change call barring password. r=yoshi

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

Thank you!

::: dom/system/gonk/tests/test_ril_worker_barring_password.js
@@ +7,5 @@
> +  run_next_test();
> +}
> +
> +var PIN = "0000";
> +var NEW_PIN = "1234";

Use 'const'

Comment 54

[shawn ku [:sku]]

Attachment: [Final] Bug 905479 - Part 4: Add test for change call barring password. r=yoshi, hsinyi

Comment 55

[shawn ku [:sku]]

need try service link first. remove checkedin-need.

Comment 56

[shawn ku [:sku]]

need try server link first.

Comment 57

[shawn ku [:sku]]

update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=8e5043eab364

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/69d86bea5167
https://hg.mozilla.org/integration/b2g-inbound/rev/379ed65742f6
https://hg.mozilla.org/integration/b2g-inbound/rev/3a40d7298f10
https://hg.mozilla.org/integration/b2g-inbound/rev/a15eb6b9e7a6

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69d86bea5167
https://hg.mozilla.org/mozilla-central/rev/379ed65742f6
https://hg.mozilla.org/mozilla-central/rev/3a40d7298f10
https://hg.mozilla.org/mozilla-central/rev/a15eb6b9e7a6