1159591 - [Telephony] Move MMI logic from ril_worker to telephonyService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

MMI contains many different operations, e.g. changing lock, setting call forwarding ... etc.
Currently TelephonyService send a worker message, "sendMMI", to ril_worker and ril_worker will trigger corresponding RIL request. When receiving solicited response, ril_worker will also need to check if the RIL response is for "sendMMI" by checking |rilMessageType| in order to fill up correct information.
This flow is a bit tricky and could be error prone.

My idea is moving MMI logic to telephonyService to makes the code clearer.

Comment 1

[Edgar Chen [:edgar]]

Attachment: Part 1: Add more tests for MMI, v1

Comment 2

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 1: Add more marionette tests for MMI, v2

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 2: Call forwarding, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 3: Icc lock, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 4: IMEI, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 5: CLIP, v1

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 6: CLIR, v1

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 7: Call barring password, v1

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 8: Call barring, v1

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 9: Call waiting, v1

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 10: USSD, v1

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 11: MMI consts, v1

Comment 14

[Edgar Chen [:edgar]]

I am working on xpcshell tests now.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8601991 [details] [diff] [review]
Part 1: Add more marionette tests for MMI, v2

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

Hi Aknow, would you mind reviewing marionette tests first? Thank you.

Comment 16

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

Comment on attachment 8601991 [details] [diff] [review]
Part 1: Add more marionette tests for MMI, v2

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

Looks good. Thank you.

::: dom/telephony/test/marionette/test_mmi_call_barring.js
@@ +32,5 @@
> +function testCallBarring(aEnabled) {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < CB_TYPES.length; i++) {
> +    let type = CB_TYPES[i];

you can use for .. of
for (let type of CB_TYPES)

@@ +41,5 @@
> +      // Test getting call barring.
> +      .then(() => sendCbMMI("*#", type, true,
> +                            aEnabled ? "smServiceEnabledFor": "smServiceDisabled"))
> +      .then(aResult => {
> +        if (aEnabled) {

Let's check the length here.
is(aResult.additionalInformation.length, MMI_SERVICE_CLASS.length);

@@ +56,5 @@
> +
> +function testUnsupportType() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < CB_TYPES_UNSUPPORTED.length; i++) {

for .. of

@@ +72,5 @@
> +function testUnsupportedOperation() {
> +  let promise = Promise.resolve();
> +
> +  let types = CB_TYPES.concat(CB_TYPES_UNSUPPORTED);
> +  for (let i = 0; i < types.length; i++) {

for .. of

@@ +73,5 @@
> +  let promise = Promise.resolve();
> +
> +  let types = CB_TYPES.concat(CB_TYPES_UNSUPPORTED);
> +  for (let i = 0; i < types.length; i++) {
> +    for (let j = 0; j < OPERATION_UNSUPPORTED.length; j++) {

for .. of

@@ +91,5 @@
> +    // Deactivate call barring service.
> +    .then(() => testCallBarring(false))
> +    // Test unsupported call barring types.
> +    .then(() => testUnsupportType())
> +    // Test unsupported operation.

Some comments here are redundant.
The function names clearly convey the meaning.

::: dom/telephony/test/marionette/test_mmi_call_waiting.js
@@ +5,5 @@
> +MARIONETTE_HEAD_JS = "head.js";
> +
> +const TEST_DATA = [
> +  // [mmi, expectedError]
> +  // Currently emulator doesn't support REQUEST_GET_CLIR, so we expect to get a

wrong comment, not CLIR

@@ +8,5 @@
> +  // [mmi, expectedError]
> +  // Currently emulator doesn't support REQUEST_GET_CLIR, so we expect to get a
> +  // 'RequestNotSupported' error here.
> +  ["*#43*10#", "RequestNotSupported"],
> +  // Currently emulator doesn't support REQUEST_SET_CLIR, so we expect to get a

wrong comment, not CLIR

@@ +21,5 @@
> +function testCallWaiting(aMmi, aExpectedError) {
> +  log("Test " + aMmi + " ...");
> +
> +  return gSendMMI(aMmi).then(aResult => {
> +    ok(!aResult.success, "Check success");

It's better to add a comment that explains why all the operations are failed.

@@ +31,5 @@
> +// Start test
> +startTest(function() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < TEST_DATA.length; i++) {

for .. of

::: dom/telephony/test/marionette/test_mmi_clip.js
@@ +19,5 @@
> +function testCLIP(aMmi, aExpectedError) {
> +  log("Test " + aMmi + " ...");
> +
> +  return gSendMMI(aMmi).then(aResult => {
> +    ok(!aResult.success, "Check success");

It's better to add a comment that explains why all the operations are failed.

@@ +29,5 @@
> +// Start test
> +startTest(function() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < TEST_DATA.length; i++) {

for .. of

::: dom/telephony/test/marionette/test_mmi_clir.js
@@ +21,5 @@
> +function testCLIR(aMmi, aExpectedError) {
> +  log("Test " + aMmi + " ...");
> +
> +  return gSendMMI(aMmi).then(aResult => {
> +    ok(!aResult.success, "Check success");

It's better to add a comment that explains why all the operations are failed.

@@ +31,5 @@
> +// Start test
> +startTest(function() {
> +  let promise = Promise.resolve();
> +
> +  for (let i = 0; i < TEST_DATA.length; i++) {

for .. of

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 1: Add more marionette tests for MMI, v3, r=aknow

Address review comment #16,
- Use for ... of
- Revise some comments

Thank you, Aknow.

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 2: Call forwarding, v2

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 3: Icc lock, v2

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 4: IMEI, v2

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 8: Call barring, v2

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 9: Call waiting, v2

Comment 23

[Edgar Chen [:edgar]]

Attachment: Part 10: USSD, v2

Comment 24

[Edgar Chen [:edgar]]

Attachment: Part 11: MMI consts, v2

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 8608059 [details] [diff] [review]
Part 2: Call forwarding, v2

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

Hi Aknow, I send review request for part2 first. If you are ok with the changes in part2, I will send review requset for the remaining parts then. Thank you.

Comment 26

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

Comment on attachment 8608059 [details] [diff] [review]
Part 2: Call forwarding, v2

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

::: dom/telephony/gonk/TelephonyService.js
@@ +931,5 @@
> +      number: aMmi.sia,
> +      serviceClass: this._siToServiceClass(aMmi.sib),
> +    };
> +
> +    if (options.action === RIL.CALL_FORWARD_ACTION_QUERY_STATUS) {

Could we redirect the request to mobileConnectionService.getCallForwarding and setCallForwarding instead of doing so by telephonyService?  Although it may not reduce the code, I still prefer to have a central point for all the logic. The benefits for this case is that we don't need to have duplicate code for something like:
- _rulesToCallForwardingOptions
- notifyCFStateChanged

Furthermore, I will expect we use the similar way for all MMI codes if we have an explicit API for them.

Comment 27

[Edgar Chen [:edgar]]

(In reply to Szu-Yu Chen [:aknow] from comment #26)
> Comment on attachment 8608059 [details] [diff] [review]
> Part 2: Call forwarding, v2
> 
> Review of attachment 8608059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +931,5 @@
> > +      number: aMmi.sia,
> > +      serviceClass: this._siToServiceClass(aMmi.sib),
> > +    };
> > +
> > +    if (options.action === RIL.CALL_FORWARD_ACTION_QUERY_STATUS) {
> 
> Could we redirect the request to mobileConnectionService.getCallForwarding
> and setCallForwarding instead of doing so by telephonyService?  Although it
> may not reduce the code, I still prefer to have a central point for all the
> logic. The benefits for this case is that we don't need to have duplicate
> code for something like:
> - _rulesToCallForwardingOptions
> - notifyCFStateChanged
> 
> Furthermore, I will expect we use the similar way for all MMI codes if we
> have an explicit API for them.

I will try using explicit API, thank you.

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 2: Call forwarding, v3

Address comment #26:
- Using MobileConnectionService API instead.

Comment 29

[Edgar Chen [:edgar]]

Comment on attachment 8609304 [details] [diff] [review]
Part 2: Call forwarding, v3

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

I am still working on other parts, but I would like to have your feedback for this part first. Thank you.

Comment 30

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

Comment on attachment 8609304 [details] [diff] [review]
Part 2: Call forwarding, v3

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

Looks very good. Thank you.

::: dom/telephony/gonk/TelephonyService.js
@@ +924,5 @@
> +        serviceClass, {
> +          QueryInterface: XPCOMUtils.generateQI([Ci.nsIMobileConnectionCallback]),
> +          notifySuccess: function() {
> +            let statusMessage;
> +            switch (action) {

How about having a mapping table from 'action' to 'statusMessage'? We could define it in ril_consts.js.

Comment 31

[Edgar Chen [:edgar]]

Attachment: Part 2: Call forwarding, v4

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 3: Icc lock, v3

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 4: IMEI, v3

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 6: CLIR, v2

Comment 35

[Edgar Chen [:edgar]]

Attachment: Part 1: Add more marionette tests for MMI, v4, r=aknow

I found some problems on using for .. of, so I change to use forEach.

Comment 36

[Edgar Chen [:edgar]]

Attachment: Part 5: CLIP, v2

Comment 37

[Edgar Chen [:edgar]]

Attachment: Part 7: Call barring password, v2

Comment 38

[Edgar Chen [:edgar]]

Attachment: Part 8: Call barring, v3

Comment 39

[Edgar Chen [:edgar]]

Attachment: Part 9: Call waiting, v3

Comment 40

[Edgar Chen [:edgar]]

Attachment: Part 6: CLIR, v3

Comment 41

[Edgar Chen [:edgar]]

Attachment: Part 6: CLIR, v4

Comment 42

[Edgar Chen [:edgar]]

Attachment: Part 11: MMI consts, v3

Comment 43

[Edgar Chen [:edgar]]

Attachment: Part 9: Call waiting, v4

Comment 44

[Edgar Chen [:edgar]]

Comment on attachment 8610586 [details] [diff] [review]
Part 2: Call forwarding, v4

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

I put the MMI consts in TelephonyService, since they are only being used in TelephonyService.
And in Part 11, I also move all mmi consts from ril_consts.js to here.

Aknow, may I have your review? Thank you.

Comment 45

[Edgar Chen [:edgar]]

Comment on attachment 8610588 [details] [diff] [review]
Part 4: IMEI, v3

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

There is no explicit API for getting imei, so use `sendWorkerMessage`.

Comment 46

[Edgar Chen [:edgar]]

Comment on attachment 8615201 [details] [diff] [review]
Part 5: CLIP, v2

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

There is no explicit API for getting imei, so use `sendWorkerMessage`.

Comment 47

[Edgar Chen [:edgar]]

Comment on attachment 8608064 [details] [diff] [review]
Part 10: USSD, v2

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

There is no explicit API for ussd, so use `sendWorkerMessage`.

Comment 48

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

Comment on attachment 8610587 [details] [diff] [review]
Part 3: Icc lock, v3

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

Edgar,
Some of the tests in test_ril_worker_mmi.js are removed in your patch. Do we already have the cases that cover the same part?
I can see pin and puk related test cases in telephony's marionette cases. However, I can't find anything about pin2 and puk2.

Comment 49

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

Comment on attachment 8610588 [details] [diff] [review]
Part 4: IMEI, v3

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

r- for test cases question

::: dom/system/gonk/ril_worker.js
@@ +4860,1 @@
>    // So far we only send the IMEI back to chrome if it was requested via MMI.

The comment doesn't really match the code.

::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +67,5 @@
>    ok(context.RIL._ussdSession);
>  
>    run_next_test();
>  });
>  

Since you remove some tests here, do we have to add the related ones in other place?

::: dom/telephony/gonk/TelephonyService.js
@@ +1069,5 @@
> +   *        Parsed MMI structure.
> +   * @param aCallback
> +   *        A nsITelephonyDialCallback object.
> +   */
> +  _getImeiMMI: function(aClientId, aMmi, aCallback) {

Imei in lower case looks weird... but I don't have a better idea now.

Comment 50

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

Comment on attachment 8615938 [details] [diff] [review]
Part 6: CLIR, v4

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

::: dom/telephony/gonk/TelephonyService.js
@@ +60,5 @@
>  MMI_SC_TO_LOCK_TYPE[RIL.MMI_SC_PUK2] = Ci.nsIIcc.CARD_LOCK_TYPE_PUK2;
>  
> +const MMI_PROC_TO_CLIR_ACTION = {};
> +MMI_PROC_TO_CLIR_ACTION[RIL.MMI_PROCEDURE_ACTIVATION] = Ci.nsIMobileConnection.CLIR_INVOCATION;
> +MMI_PROC_TO_CLIR_ACTION[RIL.MMI_PROCEDURE_DEACTIVATION] =  Ci.nsIMobileConnection.CLIR_SUPPRESSION;

I am thinking that why don't we just write the object literal here.

const MMI_PROC_TO_CLIR_ACTION = {
  RIL.MMI_PROCEDURE_ACTIVATION: Ci.nsIMobileConnection.CLIR_INVOCATION,
  RIL.MMI_PROCEDURE_DEACTIVATION: Ci.nsIMobileConnection.CLIR_SUPPRESSION
};

How do you think? Will it be more readable? If you agree with me, we could also write other object mappings in this way.

Comment 51

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

Comment on attachment 8615255 [details] [diff] [review]
Part 8: Call barring, v3

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

r- for the test coverage. We will have no test cases for call barring mmi code after this patch.

::: dom/telephony/gonk/TelephonyService.js
@@ +1356,5 @@
> +            let services = [];
> +            for (let mask = 1;
> +                 mask <= Ci.nsIMobileConnection.ICC_SERVICE_CLASS_MAX;
> +                 mask <<= 1) {
> +              if ((mask & aServiceClass) !== 0) {

We could remove '!== 0' and '()'

Comment 52

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

Comment on attachment 8620785 [details] [diff] [review]
Part 9: Call waiting, v4

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

r- for the test coverage issue. Some test cases are removed and we have to add them back in somewhere to ensure the quality.

::: dom/telephony/gonk/TelephonyService.js
@@ +1411,5 @@
> +  _callWaitingMMI: function(aClientId, aMmi, aCallback) {
> +    if (!this._isRadioOn(aClientId)) {
> +      aCallback.notifyDialMMIError(RIL.GECKO_ERROR_RADIO_NOT_AVAILABLE);
> +      return;
> +    }

Is it the case that only IMEI mmi code doesn't require the radio?

Could we move the radio check to the beginning of all mmi code so that we don't have to write it everywhere?

in _dialMMI():
  if (procedure !== RIL.MMI_KS_SC_IMEI && !this._isRadioOn(aClientId)) {
    // error
  }

@@ +1425,5 @@
> +              aCallback.notifyDialMMISuccess(RIL.MMI_SM_KS_SERVICE_DISABLED);
> +              return;
> +            }
> +
> +            let services = [];

The following part of code is duplicated in _callBarringMMI in part 8. It's better to have an utility function for that.

@@ +1430,5 @@
> +            for (let mask = Ci.nsIMobileConnection.ICC_SERVICE_CLASS_VOICE;
> +                 mask <= Ci.nsIMobileConnection.ICC_SERVICE_CLASS_MAX;
> +                 mask <<= 1) {
> +              if ((mask & aServiceClass) !== 0) {
> +                services.push(RIL.MMI_KS_SERVICE_CLASS_MAPPING[mask]);

I think MMI_KS_SERVICE_CLASS_MAPPING is no longer used in ril_worker.js. If it is, we could move it to telephonyService.js

Comment 53

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

Comment on attachment 8608064 [details] [diff] [review]
Part 10: USSD, v2

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

r- for test cases coverage and some of the logics could be moved from ril_worker to telephonyService.

::: dom/system/gonk/ril_worker.js
@@ +1883,5 @@
> +  /**
> +   * Cache the request for send out a new ussd when there is an existing
> +   * session. We should do cancelUSSD first.
> +   */
> +  cachedUSSDRequest : null,

I'd prefer to store cachedUSSDRequest and ussdSession in telephonyService and let ril_worker as simple as possible. Also, maybe we don't need the |cachedUSSDRequest|. We could send out the new quest in the callback of cancelUSSD.

::: dom/telephony/gonk/TelephonyService.js
@@ +930,5 @@
> +          aCallback.notifyDialMMIError(RIL.GECKO_ERROR_RADIO_NOT_AVAILABLE);
> +          return;
> +        }
> +
> +        this._sendToRilWorker(aClientId, "sendUSSD",

We have sendUSSD function. How about calling it instead of directly sending the message to ril_worker.

Comment 54

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

Comment on attachment 8615939 [details] [diff] [review]
Part 11: MMI consts, v3

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

Thanks for the huge effort for this bug.

Comment 55

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Add MMI tests for pin2 and puk2, v1

Comment 56

[Edgar Chen [:edgar]]

Attachment: Part 4: IMEI, v4

Address review comment 49,
- Revise the comments in order to match the code.

Comment 57

[Edgar Chen [:edgar]]

Attachment: Part 5: CLIP, v3, r=aknow

Rebase only.

Comment 58

[Edgar Chen [:edgar]]

Attachment: Part 8: Call barring, v4

Address review comment #51,
- if (mask & aServiceClass) ...

Comment 59

[Edgar Chen [:edgar]]

Attachment: Part 9: Call waiting, v5

Address review comment #52,
- Having an utility function for service class parsing.

Comment 60

[Edgar Chen [:edgar]]

Attachment: Part 10: USSD, v3

Address review comment #53,
- Move cachedUSSDRequest and _ussdSession from ril_worker to telephonyService.

Comment 61

[Edgar Chen [:edgar]]

Attachment: Part 11: MMI consts, v4, r=aknow

Rebase only.

Comment 62

[Edgar Chen [:edgar]]

Attachment: Part 12: Radio check, v1

Comment 63

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Add MMI tests for pin2, puk2 and ussd, v2

Comment 64

[Edgar Chen [:edgar]]

Comment on attachment 8632407 [details] [diff] [review]
Part 1-2: Add MMI tests for pin2, puk2 and ussd, v2

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

Add marionette tests for pin2, puk2 and also ussd (will squash with part 1 after review+).

Comment 65

[Edgar Chen [:edgar]]

Comment on attachment 8610587 [details] [diff] [review]
Part 3: Icc lock, v3

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

(In reply to Szu-Yu Chen [:aknow] from comment #48)
> Comment on attachment 8610587 [details] [diff] [review]
> Part 3: Icc lock, v3
> 
> Review of attachment 8610587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Edgar,
> Some of the tests in test_ril_worker_mmi.js are removed in your patch. Do we
> already have the cases that cover the same part?
> I can see pin and puk related test cases in telephony's marionette cases.
> However, I can't find anything about pin2 and puk2.

The tests for pin2 and puk2 are in part1-2 (see comment #64).

Comment 66

[Edgar Chen [:edgar]]

Comment on attachment 8631998 [details] [diff] [review]
Part 4: IMEI, v4

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

(In reply to Szu-Yu Chen [:aknow] from comment #49)
> Comment on attachment 8610588 [details] [diff] [review]
> Part 4: IMEI, v3
> 
> Review of attachment 8610588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +4860,1 @@
> >    // So far we only send the IMEI back to chrome if it was requested via MMI.
> 
> The comment doesn't really match the code.

Have addressed in this version.

> ::: dom/system/gonk/tests/test_ril_worker_mmi.js
> @@ +67,5 @@
> 
> Since you remove some tests here, do we have to add the related ones in
> other place?

There is a marionette test for getting IMSI via MMI, https://dxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_mmi.js.

Comment 67

[Edgar Chen [:edgar]]

Comment on attachment 8632001 [details] [diff] [review]
Part 8: Call barring, v4

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

(In reply to Szu-Yu Chen [:aknow] from comment #51)
> Comment on attachment 8615255 [details] [diff] [review]
> Part 8: Call barring, v3
> 
> Review of attachment 8615255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the test coverage. We will have no test cases for call barring mmi
> code after this patch.

I have added a marionette test for call barring in part 1.

> ::: dom/telephony/gonk/TelephonyService.js
> @@ +1356,5 @@
> > +            let services = [];
> > +            for (let mask = 1;
> > +                 mask <= Ci.nsIMobileConnection.ICC_SERVICE_CLASS_MAX;
> > +                 mask <<= 1) {
> > +              if ((mask & aServiceClass) !== 0) {
> 
> We could remove '!== 0' and '()'

Have addressed in this version.

Comment 68

[Edgar Chen [:edgar]]

Comment on attachment 8632015 [details] [diff] [review]
Part 9: Call waiting, v5

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

(In reply to Szu-Yu Chen [:aknow] from comment #52)
> Comment on attachment 8620785 [details] [diff] [review]
> Part 9: Call waiting, v4
> 
> Review of attachment 8620785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the test coverage issue. Some test cases are removed and we have to
> add them back in somewhere to ensure the quality.

I have added a marionette test for call waiting in part 1.

> ::: dom/telephony/gonk/TelephonyService.js
> @@ +1411,5 @@
> > +  _callWaitingMMI: function(aClientId, aMmi, aCallback) {
> > +    if (!this._isRadioOn(aClientId)) {
> > +      aCallback.notifyDialMMIError(RIL.GECKO_ERROR_RADIO_NOT_AVAILABLE);
> > +      return;
> > +    }
> 
> Is it the case that only IMEI mmi code doesn't require the radio?
> 
> Could we move the radio check to the beginning of all mmi code so that we
> don't have to write it everywhere?

Thank for the suggestion, I addressed this in part 12.
 
> @@ +1425,5 @@
> > +              aCallback.notifyDialMMISuccess(RIL.MMI_SM_KS_SERVICE_DISABLED);
> > +              return;
> > +            }
> > +
> > +            let services = [];
> 
> The following part of code is duplicated in _callBarringMMI in part 8. It's
> better to have an utility function for that.

Have addressed in this version.
 
> @@ +1430,5 @@
> > +            for (let mask = Ci.nsIMobileConnection.ICC_SERVICE_CLASS_VOICE;
> > +                 mask <= Ci.nsIMobileConnection.ICC_SERVICE_CLASS_MAX;
> > +                 mask <<= 1) {
> > +              if ((mask & aServiceClass) !== 0) {
> > +                services.push(RIL.MMI_KS_SERVICE_CLASS_MAPPING[mask]);
> 
> I think MMI_KS_SERVICE_CLASS_MAPPING is no longer used in ril_worker.js. If
> it is, we could move it to telephonyService.js

Already handle this in part 11.

Comment 69

[Edgar Chen [:edgar]]

Comment on attachment 8632051 [details] [diff] [review]
Part 10: USSD, v3

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

(In reply to Szu-Yu Chen [:aknow] from comment #53)
> Comment on attachment 8608064 [details] [diff] [review]
> Part 10: USSD, v2
> 
> Review of attachment 8608064 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for test cases coverage and some of the logics could be moved from
> ril_worker to telephonyService.

The test for ussd is added in part1-2 (see comment #64).

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1883,5 @@
> 
> I'd prefer to store cachedUSSDRequest and ussdSession in telephonyService
> and let ril_worker as simple as possible. Also, maybe we don't need the
> |cachedUSSDRequest|. We could send out the new quest in the callback of
> cancelUSSD.

Have addressed in this version.

> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +930,5 @@
> > +          aCallback.notifyDialMMIError(RIL.GECKO_ERROR_RADIO_NOT_AVAILABLE);
> > +          return;
> > +        }
> > +
> > +        this._sendToRilWorker(aClientId, "sendUSSD",
> 
> We have sendUSSD function. How about calling it instead of directly sending
> the message to ril_worker.

We can not reuse sendUSSD directly because the MMI operation has different callback.
So I introduce new utility functions, _sendUSSDInternal and _cancelUSSDInternal, for the common code.

Comment 70

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

Comment on attachment 8632407 [details] [diff] [review]
Part 1-2: Add MMI tests for pin2, puk2 and ussd, v2

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

The style of test_mmi_unlock_puk.js and test_mmi_unlick_puk2.js are quite different. Would you like to unify them in this patch?

Comment 71

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

Comment on attachment 8631998 [details] [diff] [review]
Part 4: IMEI, v4

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

Please also rename the test case "test_mmi.js" to "test_mmi_imei.js" in this patch.

Comment 72

[Edgar Chen [:edgar]]

(In reply to Szu-Yu Chen [:aknow] from comment #70)
> Comment on attachment 8632407 [details] [diff] [review]
> Part 1-2: Add MMI tests for pin2, puk2 and ussd, v2
> 
> Review of attachment 8632407 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The style of test_mmi_unlock_puk.js and test_mmi_unlick_puk2.js are quite
> different. Would you like to unify them in this patch?

Okay, will do that. Thank you.

Comment 73

[Edgar Chen [:edgar]]

(In reply to Szu-Yu Chen [:aknow] from comment #71)
> Comment on attachment 8631998 [details] [diff] [review]
> Part 4: IMEI, v4
> 
> Review of attachment 8631998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please also rename the test case "test_mmi.js" to "test_mmi_imei.js" in this
> patch.

Okay, will do that. Thank you.

Comment 74

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

Comment on attachment 8632001 [details] [diff] [review]
Part 8: Call barring, v4

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

Sorry for not notice the test case in part 1.  The patch looks good to me.  Thank you.

Comment 75

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

Comment on attachment 8632051 [details] [diff] [review]
Part 10: USSD, v3

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

::: dom/system/gonk/ril_worker.js
@@ +5093,3 @@
>    this.sendChromeMessage({rilMessageType: "ussdreceived",
>                            message: message,
> +                          sessionEnded: typeCode !== "1"});

It will be better to have a comment here for the magic "1" so that we don't have to refer to spec or ril.h.

::: dom/telephony/gonk/TelephonyService.js
@@ +271,5 @@
> +   * to exist until:
> +   *    a) There's a call to cancelUSSD()
> +   *    b) Receiving a session end unsolicited event.
> +   */
> +  _ussdSession: false,

Just realized that we should make it multi-sim-compatible. I am sorry that I am not aware of this in the previous review. Maybe we should change it to an array because each client could have an ussd session.

Comment 76

[Edgar Chen [:edgar]]

(In reply to Szu-Yu Chen [:aknow] from comment #75)
> Comment on attachment 8632051 [details] [diff] [review]
> Part 10: USSD, v3
> 
> Review of attachment 8632051 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +5093,3 @@
> >    this.sendChromeMessage({rilMessageType: "ussdreceived",
> >                            message: message,
> > +                          sessionEnded: typeCode !== "1"});
> 
> It will be better to have a comment here for the magic "1" so that we don't
> have to refer to spec or ril.h.

Will do.

> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +271,5 @@
> > +   * to exist until:
> > +   *    a) There's a call to cancelUSSD()
> > +   *    b) Receiving a session end unsolicited event.
> > +   */
> > +  _ussdSession: false,
> 
> Just realized that we should make it multi-sim-compatible. I am sorry that I
> am not aware of this in the previous review. Maybe we should change it to an
> array because each client could have an ussd session.

Thanks for catching this. I didn't notice it, either.
Will address it in next version.

Comment 77

[Edgar Chen [:edgar]]

Attachment: Part 1: Add more marionette tests for MMI, v5, r=aknow

1. Squash with part 1-2.
2. Address review comment #70.

Comment 78

[Edgar Chen [:edgar]]

Attachment: Part 4: IMEI, v5, r=aknow

Address review comment #71.

Comment 79

[Edgar Chen [:edgar]]

Attachment: Part 10: USSD, v4

Address review comment #75.

Comment 80

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

Comment on attachment 8633939 [details] [diff] [review]
Part 10: USSD, v4

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

Looks good. Thank you.

Comment 81

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8ddbdaa16bf&exclusion_profile=false

Comment 82

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/b525be72c3e9
https://hg.mozilla.org/integration/b2g-inbound/rev/e04392ce8091
https://hg.mozilla.org/integration/b2g-inbound/rev/f57a3016d1db
https://hg.mozilla.org/integration/b2g-inbound/rev/eef54a4daac3
https://hg.mozilla.org/integration/b2g-inbound/rev/79638154f65c
https://hg.mozilla.org/integration/b2g-inbound/rev/62b70bfd0b1a
https://hg.mozilla.org/integration/b2g-inbound/rev/78b31f53cf9e
https://hg.mozilla.org/integration/b2g-inbound/rev/79740627c609
https://hg.mozilla.org/integration/b2g-inbound/rev/ecbc3355b2ff
https://hg.mozilla.org/integration/b2g-inbound/rev/6d19921212c4
https://hg.mozilla.org/integration/b2g-inbound/rev/516652699d4f
https://hg.mozilla.org/integration/b2g-inbound/rev/ac1cfdfcdd29

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b525be72c3e9
https://hg.mozilla.org/mozilla-central/rev/e04392ce8091
https://hg.mozilla.org/mozilla-central/rev/f57a3016d1db
https://hg.mozilla.org/mozilla-central/rev/eef54a4daac3
https://hg.mozilla.org/mozilla-central/rev/79638154f65c
https://hg.mozilla.org/mozilla-central/rev/62b70bfd0b1a
https://hg.mozilla.org/mozilla-central/rev/78b31f53cf9e
https://hg.mozilla.org/mozilla-central/rev/79740627c609
https://hg.mozilla.org/mozilla-central/rev/ecbc3355b2ff
https://hg.mozilla.org/mozilla-central/rev/6d19921212c4
https://hg.mozilla.org/mozilla-central/rev/516652699d4f
https://hg.mozilla.org/mozilla-central/rev/ac1cfdfcdd29