1027546 - [B2G][Emulator] Support call barring

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Emulator, defect)

Description

[Edgar Chen [:edgar]]

Enhance emulator to support call barring.

Comment 1

[Ben Hsu [:HoPang]]

Attachment: Part 1: Restore the service class (ril_worker.js)

In this patch, I restore |serviceClass| on receiving the AT response of query facility locks, which is replaced when sending the query request.

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 2: Update the existing testcases

In this patch, I removed the for loop, and use a chained promise structure to execute the test functions directly. By doing this, I think the code can be more readable. In addition to the structure changes, I also remove the cases should successfully pass, since those cases are tested massively in a new testcase.

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 3: Create a new testcase

In many places in this patch, "CBP' are used as the abbreviation for "Call Barring Program". I am not quite sure whether it makes the code hard to be read. If there is any suggestion, I'll be very appreciated.

Comment 4

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

Comment on attachment 8575141 [details] [diff] [review]
Part 2: Update the existing testcases

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

::: dom/mobileconnection/tests/marionette/test_call_barring_get_option.js
@@ +3,5 @@
>  
>  MARIONETTE_TIMEOUT = 60000;
>  MARIONETTE_HEAD_JS = "head.js";
>  
> +function testGetCallBarringOption(aExpectedError, aOptions) {

Add a log for |aOptions|. That will help us identify what we are testing now.

@@ +8,3 @@
>    return getCallBarringOption(aOptions)
>      .then(function resolve(aResult) {
> +      ok(!aExpectedError, "should be rejected");

Why not just use |false|?
Are you going to pass a falsy value for aExpectedError to test the successful case?

::: dom/mobileconnection/tests/marionette/test_call_barring_set_error.js
@@ +3,5 @@
>  
>  MARIONETTE_TIMEOUT = 60000;
>  MARIONETTE_HEAD_JS = "head.js";
>  
> +function testSetCallBarringOption(aExpectedError, aOptions) {

Add a log for |aOptions|. That will help us identify what we are testing now.

@@ +6,5 @@
>  
> +function testSetCallBarringOption(aExpectedError, aOptions) {
> +  return setCallBarringOption(aOptions)
> +    .then(function resolve() {
> +      ok(!aExpectedError, "should be rejected");

Why not just use |ok(false)|?

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 3: Create a new testcase (V2)

On finishing of this testcase, we should disable all call barring programs.

Comment 6

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

Comment on attachment 8575162 [details] [diff] [review]
Part 3: Create a new testcase (V2)

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

It's ok but I'd like to review the revised version again.

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +12,5 @@
> +const AI = MozMobileConnection.CALL_BARRING_PROGRAM_ALL_INCOMING;
> +const IR = MozMobileConnection.CALL_BARRING_PROGRAM_INCOMING_ROAMING;
> +let incomingCBPs = [null, AI, IR];
> +
> +function setCBP(aProgram, aServiceClass, aEnabled, aPassword = "0000") {

We always use |serviceClass = 1| and |passsword = "0000"| in this file.  It's better to remove them from parameter list.
function setCBP(aProgram, aEnabled) {

@@ +15,5 @@
> +
> +function setCBP(aProgram, aServiceClass, aEnabled, aPassword = "0000") {
> +  return setCallBarringOption({
> +    program: aProgram,
> +    serviceClass: aServiceClass,

serviceClass: 1,

@@ +17,5 @@
> +  return setCallBarringOption({
> +    program: aProgram,
> +    serviceClass: aServiceClass,
> +    enabled: aEnabled,
> +    password: aPassword

password: "0000"

@@ +21,5 @@
> +    password: aPassword
> +  });
> +}
> +
> +function getCBP(aProgram, aServiceClass, aEnabled) {

remove aServiceClass

@@ +22,5 @@
> +  });
> +}
> +
> +function getCBP(aProgram, aServiceClass, aEnabled) {
> +  return getCallBarringOption({program: aProgram, serviceClass: aServiceClass})

serviceClass: 1

@@ +25,5 @@
> +function getCBP(aProgram, aServiceClass, aEnabled) {
> +  return getCallBarringOption({program: aProgram, serviceClass: aServiceClass})
> +    .then(result => {
> +      is( result.program, aProgram, "Program" );
> +      is( result.serviceClass, aServiceClass, "ServiceClass");

use 1 for serviceClass

@@ +33,5 @@
> +
> +function checkCBP(aOutgoingCBP, aIncomingCBP) {
> +  return Promise.resolve()
> +    // Check outgoing call barring programs
> +    .then(() => getCBP(AO, 1, aOutgoingCBP === AO ? true : false))

|aOutgoingCBP === AO|
It's already a boolean value. You don't need |? true : flase| this part
Fix this and all the followings

@@ +42,5 @@
> +    .then(() => getCBP(AI, 1, aIncomingCBP === AI ? true : false))
> +    .then(() => getCBP(IR, 1, aIncomingCBP === IR ? true : false));
> +}
> +
> +function resetCBP(aOutgoingCBP, aIncomingCBP) {

When you are doing resetCBP, you call to setCBP.  It's kind of recursive and weird.
I mean, you are going to verify your setCBP().  However, the test relies on resetCBP and setCBP which actually does not guarantee correct.

@@ +61,5 @@
> +function test(aOutgoingCBP, aIncomingCBP) {
> +  let promise = Promise.resolve();
> +
> +  // Test outoging
> +  for (let i = 1; i < outgoingCBPs.length; ++i) {

for..of

@@ +70,5 @@
> +      .then(() => checkCBP(CBP, aIncomingCBP));
> +  }
> +
> +  // Test incoming
> +  for (let i = 1; i < incomingCBPs.length; ++i) {

for..of

@@ +85,5 @@
> +// Start tests
> +startTestCommon(function() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < outgoingCBPs.length; ++i) {

use |for..of|
for (let outgoingCBP of outgoingCBPs)

@@ +86,5 @@
> +startTestCommon(function() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < outgoingCBPs.length; ++i) {
> +    for (let j = 0; j < outgoingCBPs.length; ++j) {

for..of

@@ +88,5 @@
> +
> +  for (let i = 0; i < outgoingCBPs.length; ++i) {
> +    for (let j = 0; j < outgoingCBPs.length; ++j) {
> +      let outgoingCBP = outgoingCBPs[i];
> +      let incomingCBP = incomingCBPs[j];

then you don't need these two lines.

@@ +96,5 @@
> +
> +  return promise
> +    .then(() => resetCBP(null, null))
> +    .catch((aError) => {
> +      return resetCBP(null, null).then(() => Promise.reject(aError));

It's the last one, I don't think that you need to reject it.
Let's reorder the code to avoid two reset CBP.

return promise
.catch(aError => {
 ok(false, ...)  // just let test faile
})
.then(() => resetCBP(null, null));

Comment 7

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

Comment on attachment 8575162 [details] [diff] [review]
Part 3: Create a new testcase (V2)

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

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +15,5 @@
> +
> +function setCBP(aProgram, aServiceClass, aEnabled, aPassword = "0000") {
> +  return setCallBarringOption({
> +    program: aProgram,
> +    serviceClass: aServiceClass,

Have a descriptive constant for serviceClass and use it here.
ex:
const SERVICE_CLASS_VOICE = 1;

Comment 8

[Ben Hsu [:HoPang]]

Attachment: [external/qemu] pull request 134

Comment 9

[Ben Hsu [:HoPang]]

Attachment: Part 1: Restore the service class (ril_worker.js)

Modify the commit message.

Comment 10

[Ben Hsu [:HoPang]]

Attachment: Part 2: Update the existing testcases

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 3: Create a new testcase (V3)

Comment 12

[Ben Hsu [:HoPang]]

Comment on attachment 8579292 [details] [diff] [review]
Part 3: Create a new testcase (V3)

This testcase doesn't guarantee that setting outgoing and incoming call barring programs are independent. However, they don't and shouldn't affect each other in the real world, so I think it's okay not to test the independency here.

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8575783 [details] [review]
[external/qemu] pull request 134

I have left some comments on github, please address them and request review again. Thank you.

Comment 14

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

Comment on attachment 8579292 [details] [diff] [review]
Part 3: Create a new testcase (V3)

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

I expect it could be better.  Let's have another round of review.
Also, I'm not a fan of acronym 'CBP'.  If you really want to use it, maybe adding a cooment in the beginning to explain it is better.

Thank you.

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +46,5 @@
> +      password: "0000" // The dafault call barring password of the emulator
> +    }))
> +
> +    // Check whether the result of this operation meets our expectation
> +    .then(() => _getCBP(aProgram, aEnabled));

Looks like the check is redundant.
In the end of setCBP, you always have a thorough test for all programs.

@@ +49,5 @@
> +    // Check whether the result of this operation meets our expectation
> +    .then(() => _getCBP(aProgram, aEnabled));
> +}
> +
> +function setCBP(aCBP, aCBPs) {

Naming is not clear enough. I expect something like..
setCBP(aActiveProgram, aAllPrograms)

Also, I don't like _getCBP, _setCBP, setCBP.
IMO, we'd better change it to getCBP, setCBP, and find a better name for original setCBP.

@@ +54,5 @@
> +  let promise = Promise.resolve();
> +
> +  if (aCBP !== null) {
> +    promise = promise
> +      .then(() => _setCBP(aCBP, true));

put it in the same line if width is allowed

@@ +60,5 @@
> +    // Deactive all barring programs in |aCBPs|.
> +    for(let _CBP of aCBPs) {
> +      let CBP = _CBP;
> +      promise = promise
> +        .then(() => _setCBP(CBP, false));

put it in the same line if width is allowed

@@ +61,5 @@
> +    for(let _CBP of aCBPs) {
> +      let CBP = _CBP;
> +      promise = promise
> +        .then(() => _setCBP(CBP, false));
> +    }

I think this should work.

promise = aCBPs.reduce(function(previousPromise, program) {
  return previousPromise.then(() => _setCBP(program, false));
}, promise);

It might take you some time to figure out the code if you're not familiar with functional programming.
However, once you see this kind of code, it will be very easy for you to understand the pattern next time.
Thus, I prefer the above usage. Moreover, it avoids creating another scope variable |CBP| and have both |CBP| and |_CBP| that lead to confusion.

@@ +69,5 @@
> +  for(let _CBP of aCBPs) {
> +    let CBP = _CBP;
> +    promise = promise
> +      .then(() => _getCBP(CBP, CBP === aCBP));
> +  }

You could apply the similar trick here.

@@ +74,5 @@
> +
> +  return promise;
> +}
> +
> +function testCBP(aCBPs) {

The purpose of this function is not so clear.
I'll suggest you break it into two functions:

// add comment here if the function name is not clear enough
function testSetCBPs(originalCBP, aAllCBPs)

// also, provide a comment here
function testXXX(aAllCBPs) {
  promise = Promise.resolve();

  promise = aAllCBPs.reduce(function(previousPromise, program) {
    return previousPromise.then(() => _testSetCBPs(program, aAllCBPs));
  }, promise);

 return promise;
}

@@ +98,5 @@
> +}
> +
> +// Start tests
> +startTestCommon(function() {
> +  let promise = Promise.resolve()

return Promise.resolve()

@@ +102,5 @@
> +  let promise = Promise.resolve()
> +    .then(() => setCBP(null, outgoingCBPs))
> +    .then(() => setCBP(null, incomingCBPs));
> +
> +  promise = promise

Just chain all |.then| together

@@ +108,5 @@
> +    .then(() => testCBP(outgoingCBPs))
> +    .then(() => log("=== Test incoming call barring programs ==="))
> +    .then(() => testCBP(incomingCBPs));
> +
> +  return promise

Just chain all |.then| together

@@ +109,5 @@
> +    .then(() => log("=== Test incoming call barring programs ==="))
> +    .then(() => testCBP(incomingCBPs));
> +
> +  return promise
> +    .catch((aError) => ok(false, "promise rejects during test: " + aError))

.catch(aError => ...)
() is not needed.

Comment 15

[Ben Hsu [:HoPang]]

Attachment: Part 3: Create a new testcase (V4)

Comment 16

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

Comment on attachment 8581448 [details] [diff] [review]
Part 3: Create a new testcase (V4)

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

Good.
r=me with comment fixed or answered.

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +66,5 @@
> +
> +  return promise;
> +}
> +
> +function testSingleProgram (aOriginProgram, aOriginPrograms, aTargetPrograms) {

Second parameter looks like |aAllPrograms| that you want to verify.  Maybe put it at the third place is better.

@@ +67,5 @@
> +  return promise;
> +}
> +
> +function testSingleProgram (aOriginProgram, aOriginPrograms, aTargetPrograms) {
> +  let promise = setProgram(aOriginProgram, aOriginProgram);

What's the second parameter? should it be |true|?

@@ +83,5 @@
> +
> +  return aPrograms.reduce((previousPromise, program) => {
> +    return previousPromise
> +      .then(() => {
> +        targetPrograms.shift();

Why do we need a shift?
Why don't we test all kinds of targetPrograms?

@@ +84,5 @@
> +  return aPrograms.reduce((previousPromise, program) => {
> +    return previousPromise
> +      .then(() => {
> +        targetPrograms.shift();
> +        return testSingleProgram(program, aPrograms, targetPrograms);

In my mind, this like should be something like...
testSingleProgram(program, aPrograms, aPrograms.slice());

Comment 17

[Ben Hsu [:HoPang]]

Comment on attachment 8581448 [details] [diff] [review]
Part 3: Create a new testcase (V4)

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

::: dom/mobileconnection/tests/marionette/test_call_barring_basic_operations.js
@@ +67,5 @@
> +  return promise;
> +}
> +
> +function testSingleProgram (aOriginProgram, aOriginPrograms, aTargetPrograms) {
> +  let promise = setProgram(aOriginProgram, aOriginProgram);

Thanks for pointing this out, it's certainly a mistake.

@@ +83,5 @@
> +
> +  return aPrograms.reduce((previousPromise, program) => {
> +    return previousPromise
> +      .then(() => {
> +        targetPrograms.shift();

Since in |testSingleProgram(...)|, we test all the program changes between |aOriginProgram| and every program in |aTargetPrograms|, so |aOriginProgram| can be removed from the program list after tested by |testSingleProgram(...)|. Otherwise, when we later invoke |testSingleProgram(...)| with another program as |aOriginProgram|, the program changes with the previous |aOriginProgram| will be tested again, which are unnecessary.

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 3: Create a new testcase (V5)

Comment 19

[Ben Hsu [:HoPang]]

Comment on attachment 8575783 [details] [review]
[external/qemu] pull request 134

Hi, Edgar

I've update the pull request, do you mind reviewing it again?

Comment 20

[Edgar Chen [:edgar]]

Comment on attachment 8575783 [details] [review]
[external/qemu] pull request 134

Looks good, r=me with comments on github addressed. Thank you.

Comment 21

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fcacc8d1df0

Comment 22

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fcacc8d1df0&exclusion_profile=false

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1eb63f18209c
https://hg.mozilla.org/integration/b2g-inbound/rev/8f0c0886ac43
https://hg.mozilla.org/integration/b2g-inbound/rev/47fbd6a8b249

Master: https://github.com/mozilla-b2g/platform_external_qemu/commit/46e1877c0d88b085f7ebc5f432d5bb8f1e2d1f3b

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eb63f18209c
https://hg.mozilla.org/mozilla-central/rev/8f0c0886ac43
https://hg.mozilla.org/mozilla-central/rev/47fbd6a8b249