894871 - [User Story] [Suplementary Services] Temporary MMI codes: support calling line identification restriction (+CLIR)

Status: VERIFIED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Noemí Freire (:noemi)]

As a user I want to be able to temporary enable/disable CLIR feature through MMI codes

Acceptance Criteria:
*I'm able to activate CLIR feature for the actual/next call going to dialer and entering ""*31#<number>"", calling number will be displayed to the called party for the actual/next call

*I'm able to deactivate CLIR feature for the actual/next call going to dialer and entering ""#31#<number>"", calling number won't be displayed to the called party for the actual/next call"

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: 894871.patch

WIP v1.

This WIP shows the idea about how to implement this feature and is gonna be useful for the discussion.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Some background first. For temporary CLIR MMI commands RIL doesn't send any MMI command to modem but dials a number parsed from the MMI command string. Moreover that temporary CLIR MMI command doesn't end with '#' character. We are going to support those MMI command for v1.2 and we need to think about how to handle them. Current WIP patch is an approach that we can discuss about and I'd love to have some feedback from you guys.

It's said that we are going to merge both the sendMMI() and dial() functions into dial() in the near future so the dialer app won't check whether the dial string is an MMI command anymore and just will call dial(). Gecko will be responsible for sending the MMI command or dialing the number.

In the approach I've followed the temporary CLIR MMI command is handled by the dial function. The string for that MMI command doesn't end with '#' character so the command will be handled by the dial function (see [1] please). In the dial() function we check whether the dial strings corresponds to a MMI command and in case there is a temporary CLIR MMI command we get the CLIR mode and dial the number with the corresponding CLIR mode.

I realize this approach is a kind of dirty trick but I don't see another way and this is the simplest one.

Hsin-Yi, Anshul and Fernando, your feedback is welcome. Thanks.

BTW, Anshul, you guys already support temporary CLIR MMI commands in your commercial RIL implementation. I'm wondering what approach you guys follow. I guess it will be similar since Gaia call dial() function for temporary CLIR MMI command strings.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/dialer.js#L258

Comment 3

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

Due to having separate dial() and sendMMI() APIs, I think we do need to have _parseMMMI() within dial() in ril_worker.js right now because I really don't want gaia to maintain the whole MMI parsing algorithm. However, foreseeing we are going to unify dial() and sendMMI() in the near future, I think we also need to harden [1] for supporting both permanent and temporary cases. 

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2507

Comment 4

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

Comment on attachment 777718 [details] [diff] [review]
894871.patch

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

::: dom/system/gonk/ril_worker.js
@@ +1753,5 @@
>      if (this._isEmergencyNumber(options.number)) {
>        this.dialEmergencyNumber(options, onerror);
>      } else {
> +      let mmi = this._parseMMI(options.number);
> +      if (mmi && mmi.procedure && this._isTemporaryModeCLIR()) {

Per my understanding we should use |mmi.sc == MMI_SC_CLIR| && |mmi.dialNumber| for checking if it's a temporary CLIR, right?

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Support temporary CLIR. v1

Hsin-Yi, I assume you are ok with the approach suggested in comment #2 (let me know if I¡m wrong please) so here is the patch. I've also completed the patch and addressed the comments you made in comment #4 so could you take a look please? Thanks and thanks for the feedback you gave me too!

Comment 6

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

Comment on attachment 784356 [details] [diff] [review]
Support temporary CLIR. v1

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

Thank you for the patch! Please see my comments below.

Also, kind reminder: we landed a test case for RIL code examination last week. Please make sure your patch passes the test 'test_ril_code_quality.py' which picks up some nits hardly visible to naked eyes.

::: dom/system/gonk/ril_worker.js
@@ +1500,5 @@
>    /**
> +   * Returns when to hide or show the caller id in the call.
> +   *
> +   * @param mmi
> +   *        The MMI code

nit: a period at the end.

@@ +1503,5 @@
> +   * @param mmi
> +   *        The MMI code
> +   * @return One of the CLIR_* constants.
> +   */
> +  getCLIRMode: function getCLIRMode(mmi) {

Let's rename it _getCLIRMode to represent it's for internal use.

@@ +1504,5 @@
> +   *        The MMI code
> +   * @return One of the CLIR_* constants.
> +   */
> +  getCLIRMode: function getCLIRMode(mmi) {
> +    if (!mmi || 

nit: tailing white space

@@ +1509,5 @@
> +        (mmi.serviceCode != MMI_SC_CLIR) ||
> +        (mmi.procedure != MMI_PROCEDURE_ACTIVATION &&
> +         mmi.procedure != MMI_PROCEDURE_DEACTIVATION)) {
> +      return CLIR_DEFAULT;
> +    }

nit: add a new line here.

@@ +1510,5 @@
> +        (mmi.procedure != MMI_PROCEDURE_ACTIVATION &&
> +         mmi.procedure != MMI_PROCEDURE_DEACTIVATION)) {
> +      return CLIR_DEFAULT;
> +    }
> +    if (mmi.procedure == MMI_PROCEDURE_ACTIVATION) {

We can simply write:

return mmi.procedure == MMI_PROCEDURE_ACTIVATION ? CLIR_INVOCATION : CLIR_SUPPRESSION;

@@ +3181,5 @@
>       return numbers.indexOf(number) != -1;
>     },
>  
> +   /**
> +    * Checks whether to suppress caller id for the call.

Check whether to temporarily suppress caller id for the call.

@@ +3184,5 @@
> +   /**
> +    * Checks whether to suppress caller id for the call.
> +    *
> +    * @param mmi
> +    *        MMI full object

nit: period.

@@ +3186,5 @@
> +    *
> +    * @param mmi
> +    *        MMI full object
> +    */
> +   _isTemporaryModeCLIR: function _isTemporaryModeCLIR(mmi) {

Hm... is this fully implemented the check of "temporary mode" CLIR? I don't see the difference between the check of "permanent mode."

Also, as my previous comment in discussion, I think we also need to handle temporary case in "case MMI_SC_CLIR" [1].

[1] mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2542

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)

Thanks for the review Hsin-Yi!

> Please make sure your patch passes the test 'test_ril_code_quality.py'
> which picks up some nits hardly visible to naked eyes.

Sure!

> Hm... is this fully implemented the check of "temporary mode" CLIR? I don't
> see the difference between the check of "permanent mode."

The MMI command for permanent mode doesn't bring/include the dial number. We check for the existence of that number in the dial() function. You prefer to move that check into the _isTemporaryModeCLIR() function, don't you?

> Also, as my previous comment in discussion, I think we also need to handle
> temporary case in "case MMI_SC_CLIR" [1].

Until we merge both dial() and sendMMI() functions into the dial() one Gaia calls the sendMMI() function for dealing with MMI commands except for the temporary CLIR one since the MMI command format doesn't end with '#' symbol. See [1] please. So I don't see the need for handling temporary case in "case MMI_SC_CLIR" inside the sendMMI() function. Note even once we merge both dial() and sendMMI() functions into the dial() one the flow for temporary CLIR MMI commands won't reach the switch-case statement since the flow (I guess) will be something like:

  dial: function dial(options) {

    // ...

    if (this._isEmergencyNumber(options)) {
      this.dialEmergencyNumber(options);
    } else {
      let mmi = this._getMMIFromDialString(options);
      if (!mmi) {
        this.dialNonEmergencyNumber(options);
      } else if (this._isTemporaryModeCLIR(mmi)) {
        options.number = mmi.dialNumber;
        options.clirMode = this._getCLIRMode(mmi);
        this.dialNonEmergencyNumber(options);
      } else {
        // Handle/Send MMI.
      }
    }
  },

Are you still seeing the need for adding the temporary case in the switch-case statement in the sendMMI() function?

[1] http://goo.gl/yFQ9yU

Comment 8

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> 
> Thanks for the review Hsin-Yi!
> 
> > Please make sure your patch passes the test 'test_ril_code_quality.py'
> > which picks up some nits hardly visible to naked eyes.
> 
> Sure!
> 
> > Hm... is this fully implemented the check of "temporary mode" CLIR? I don't
> > see the difference between the check of "permanent mode."
> 
> The MMI command for permanent mode doesn't bring/include the dial number. We
> check for the existence of that number in the dial() function. You prefer to
> move that check into the _isTemporaryModeCLIR() function, don't you?

Yes,  you got it! That's what I think _isTempeoraryModeCLIR should do.

> 
> > Also, as my previous comment in discussion, I think we also need to handle
> > temporary case in "case MMI_SC_CLIR" [1].
> 
> Until we merge both dial() and sendMMI() functions into the dial() one Gaia
> calls the sendMMI() function for dealing with MMI commands except for the
> temporary CLIR one since the MMI command format doesn't end with '#' symbol.
> See [1] please. So I don't see the need for handling temporary case in "case
> MMI_SC_CLIR" inside the sendMMI() function. Note even once we merge both
> dial() and sendMMI() functions into the dial() one the flow for temporary
> CLIR MMI commands won't reach the switch-case statement since the flow (I
> guess) will be something like:
> 
>   dial: function dial(options) {
> 
>     // ...
> 
>     if (this._isEmergencyNumber(options)) {
>       this.dialEmergencyNumber(options);
>     } else {
>       let mmi = this._getMMIFromDialString(options);
>       if (!mmi) {
>         this.dialNonEmergencyNumber(options);
>       } else if (this._isTemporaryModeCLIR(mmi)) {
>         options.number = mmi.dialNumber;
>         options.clirMode = this._getCLIRMode(mmi);
>         this.dialNonEmergencyNumber(options);
>       } else {
>         // Handle/Send MMI.
>       }
>     }
>   },
> 
> Are you still seeing the need for adding the temporary case in the
> switch-case statement in the sendMMI() function?
> 

Thanks for the details. Yup, I understand what gaia handles mmi code right now. That's why I think having separate paths in ril_worker.js handling CLIR MMI is acceptable for now. Your patch fixes the user story well.

I also understand that based on the current gaia code, it's not likely to use sendMMI() for temporary CLIR. However, I don't think that is convincing enough to say because gaia doesn't use sendMMI so we don't make the sendMMI API complete, especially when we claim moz RIL supports temporary CLIR MMI. 

Yup, it's kinda tricky here. :( But as handling temporary CLIR MMI in switch-case seems quite easy, I'd like to see the ril code could implement that. :)

> [1] http://goo.gl/yFQ9yU

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)

> Thanks for the details. Yup, I understand what gaia handles mmi code right
> now. That's why I think having separate paths in ril_worker.js handling CLIR
> MMI is acceptable for now. Your patch fixes the user story well.
> 
> I also understand that based on the current gaia code, it's not likely to
> use sendMMI() for temporary CLIR. However, I don't think that is convincing
> enough to say because gaia doesn't use sendMMI so we don't make the sendMMI
> API complete, especially when we claim moz RIL supports temporary CLIR MMI. 

Hm, IMHO I see the sendMMI() function for handling MMI codes that send commands to the modem/network. That's not the case of the MMI code for temporary CLIR since for that MMI code we just make a call with some specific options.

> Yup, it's kinda tricky here. :( But as handling temporary CLIR MMI in
> switch-case seems quite easy, I'd like to see the ril code could implement
> that. :)

Hm, that doesn't seem quite easy to me at a first glance. I guess we need to do something like:

-      // CLIR (non-temporary ones)
+      // CLIR
       case MMI_SC_CLIR:
+        if (this._isTemporaryModeCLIR(mmi)) {
+         let onerror = (function onerror(errorMsg) {
+           // TODO or use _sendMMIError instead.
+         }).bind(this);
+
+         options.number = mmi.dialNumber;
+         options.clirMode = this._getCLIRMode(mmi);
+
+         this.dialNonEmergencyNumber(options, onerror);
+         return;
+        }

This is how I see right now how to handle temporary CLIR here but I'm a bit lost. I really appreciate your input here to move forward and finish it. Thanks Hsin-Yi.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Hsin-Yi, I don't want to bother you but please if you have a minute to comment to comment #9 I would appreciate it. Thanks!

Comment 11

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

I was writing when you pinned me ;)

So, my idea is:
==========================
             _sendMMIError(MMI_ERROR_KS_NOT_SUPPORTED, MMI_KS_SC_CLIR);
             return;
         }
-        options.rilMessageType = "setCLIR";
-        options.isSendMMI = true;
-        this.setCLIR(options);
+        if (mmi.dialNumber) {
+          this._handleTempModeCLIR(options);
+        } else {
+          options.rilMessageType = "setCLIR";
+          options.isSendMMI = true;
+          this.setCLIR(options);
+        }
         return;

=========================
+  _handleTempModeCLIR: function _handleTempModeCLIR(options) {
+    function callback(options) {
+      if (options.errorMsg) {
+        options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
+      }
+      delete options.callback;
+      this.sendChromeMEssage(options);
+    };
+
+    options.number = mmi.dialNumber;
+    options.request = REQUEST_DIAL;
+    this.sendDialRequest(options);
+  },
========================
 RIL[REQUEST_DIAL] = function REQUEST_DIAL(length, options) {
+  if (options.callback) {
+    options.callback.call(this, options);
+    return;
+  }
+
=======================

I think you and I are in the same direction but I guess we don't need the whole _isTemporaryModeCLIR and _getCLIRMode here. We have that information already in switch-case and can simply reuse them.

Above are the things that I guess we need to address for now and even for the day we have the unified API. Please take a look at it and kindly note that I haven't really tested it. :)

Comment 12

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #9)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> 
> > Thanks for the details. Yup, I understand what gaia handles mmi code right
> > now. That's why I think having separate paths in ril_worker.js handling CLIR
> > MMI is acceptable for now. Your patch fixes the user story well.
> > 
> > I also understand that based on the current gaia code, it's not likely to
> > use sendMMI() for temporary CLIR. However, I don't think that is convincing
> > enough to say because gaia doesn't use sendMMI so we don't make the sendMMI
> > API complete, especially when we claim moz RIL supports temporary CLIR MMI. 
> 
> Hm, IMHO I see the sendMMI() function for handling MMI codes that send
> commands to the modem/network. That's not the case of the MMI code for
> temporary CLIR since for that MMI code we just make a call with some
> specific options.
> 

Hmm, isn't that still the case that we send commands to modem or network?

Comment 13

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #11)
> I was writing when you pinned me ;)
> 
> So, my idea is:
> ==========================
>              _sendMMIError(MMI_ERROR_KS_NOT_SUPPORTED, MMI_KS_SC_CLIR);
>              return;
>          }
> -        options.rilMessageType = "setCLIR";
> -        options.isSendMMI = true;
> -        this.setCLIR(options);
> +        if (mmi.dialNumber) {
> +          this._handleTempModeCLIR(options);

Aha, clearly some errors here. But you can still get my points I guess ;)

> +        } else {
> +          options.rilMessageType = "setCLIR";
> +          options.isSendMMI = true;
> +          this.setCLIR(options);
> +        }
>          return;
> 
> =========================
> +  _handleTempModeCLIR: function _handleTempModeCLIR(options) {
> +    function callback(options) {
> +      if (options.errorMsg) {
> +        options.errorMsg =
> RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +      }
> +      delete options.callback;
> +      this.sendChromeMEssage(options);
> +    };
> +
> +    options.number = mmi.dialNumber;
> +    options.request = REQUEST_DIAL;
> +    this.sendDialRequest(options);
> +  },
> ========================
>  RIL[REQUEST_DIAL] = function REQUEST_DIAL(length, options) {
> +  if (options.callback) {
> +    options.callback.call(this, options);
> +    return;
> +  }
> +
> =======================
> 
> I think you and I are in the same direction but I guess we don't need the
> whole _isTemporaryModeCLIR and _getCLIRMode here. We have that information
> already in switch-case and can simply reuse them.
> 
> Above are the things that I guess we need to address for now and even for
> the day we have the unified API. Please take a look at it and kindly note
> that I haven't really tested it. :)

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Support temporary CLIR. v2

Version 1 updated with the nits addressed. It keeps the original approach of handling temporary CLIR MMI commands through the dial() function. Hsin-Yi, could you take a look please? Thanks!

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Support temporary CLIR. v2

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

Oh, forgot to mention that I pushed to try.

https://tbpl.mozilla.org/?tree=Try&rev=ed83209fc54a

Comment 17

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

Comment on attachment 786340 [details] [diff] [review]
Support temporary CLIR. v2

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

Looks good. Thank you.

Comment 18

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

As the comment in the patch, the current separate APIs lead us to some obstructive problems with implementation, either in gecko or in gaia. In the current situation, it cannot be that perfect without a unifying API. We let this patch simple and definitely need keeping discussing in bug 889737.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

https://hg.mozilla.org/integration/b2g-inbound/rev/400f307a9490

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/400f307a9490

Comment 21

[rafael.marquez]

The user story has been tested with satisfactory results. We could not run the test when the devices is configured as network status and the network status is hide number. For QA team is impossible reproduce this case.

The tests have been executed with a SIM Tuenti. We tested several movistar SIMS, but we could not activating the CLIR to this type of SIM.