883178 - [MMI] Implement DOMMMIResult and DOMMMIError

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=879032#c26

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Blocks a blocker, so leo?

Comment 2

[Wayne Chang [:wchang]]

Triage- blocking as this bug blocks a bunch of leo+ blockers.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This patch adds the MMIResult implementation and basic usage. As you can see there are a few TODO comments associated with some bugs blocked by this one, all the patches for these bugs should be pushed together, so we don't break any MMI functionality.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 764801 [details] [diff] [review]
v1

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

I think there is still something we need to figure out in this patch. :(

::: dom/network/src/MobileConnection.cpp
@@ +282,3 @@
>  {
> +  *aRequest = nullptr;
> +

We don't really need this, do we?

@@ +298,3 @@
>  {
> +  *aRequest = nullptr;
> +

ditto.

::: dom/system/gonk/RILContentHelper.js
@@ +1541,5 @@
>  
>    handleSendCancelMMIOK: function handleSendCancelMMIOK(message) {
>      let request = this.takeRequest(message.requestId);
> +    if (!request && !message.success) {
> +      return;

When we reach here, it means we had either a "RIL:SendMMI:Return:OK" or a "RIL:CancelMMI:Return:OK", which follows |message.success| must be true.  So this condition will never take effect and we may then fireSuccess upon an invalid request object.

@@ +1562,5 @@
> +      serviceCode: message.mmiServiceCode,
> +      statusMessage: message.result
> +    };
> +
> +    switch(message.mmiServiceCode) {

nit: SP before left parenthesis.

@@ +1566,5 @@
> +    switch(message.mmiServiceCode) {
> +      case RIL.MMI_KS_SC_PIN:
> +      case RIL.MMI_KS_SC_PIN2:
> +      case RIL.MMI_KS_SC_PUK:
> +      case RIL.MMI_KS_SC_PUK2:

This patch depends on symbols in bug 879032, but those symbols are not referenced in that bug at all.  Please move them into this patch instead.

::: dom/system/gonk/ril_worker.js
@@ +2241,5 @@
>          // Call forwarding requires at least an action, given by the MMI
>          // procedure, and a reason, given by the MMI service code, but there
>          // is no way that we get this far without a valid procedure or service
>          // code.
>          options.action = MMI_PROC_TO_CF_ACTION[mmi.procedure];

nit: can we move |options.mmiServiceCode = ...| right above this line?

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Thanks Vicamo!

This patch should address your feedback.

As you can see, I've only added the MMI related consts that are actually used. I've also unified the handlers for RIL:SendMMI:Return:* and RIL:CancelMMI:Return:*.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

I don't think this belongs to Gaia.

Comment 7

[Carol Yang [:cyang]]

Comment on attachment 765429 [details] [diff] [review]
v2

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

::: dom/system/gonk/RILContentHelper.js
@@ -1537,5 @@
> -    // MMI query call forwarding options request returns a set of rules that
> -    // will be exposed in the form of an array of nsIDOMMozMobileCFInfo
> -    // instances.
> -    if (message.success && message.rules) {
> -      this._cfRulesToMobileCfInfo(message.rules);

I guess CF support is going to be moved to RIL.MMI_KS_SC_CALL_FORWARDING case below? If so, will we be sending up message.additionalInformation for CF info instead of message.rules (of course in the same form as before)?

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Yes, that's the idea :)

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

After talking with Vicamo via IRC, we decided to also implement MMIError apart from MMIResult. So MMI requests will return:

- A plain string for invalid or not recognized MMI requests, via DOMRequest.error.
- An MMIResult for successful MMI requests, via DOMRequest.result.
- An MMIError for failed MMI requests, via DOMRequest.error.

MMIError will inherit from DOMError.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (mozilla-central) Part 0: DOMError

This patch allows other components to inherit from DOMError (via WebIDL).

Jonas, Mounir, sorry for the r? broadcast, but this is quite important for the Taipei work week target milestone.

Thanks!

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (mozilla-central) Part 1: MMIResult and MMIError

Vicamo, this patch implements MMIResult and MMIError and includes its basic usage for the get IMEI and USSD functionalities.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

I'll need to write different patches for b2g18, hopefully today (CST).

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 767638 [details] [diff] [review]
(mozilla-central) Part 1: MMIResult and MMIError

I think I need to ask for sr? for the WebIDL introduction. Bugzilla only allows me to set one superreviewer, but I'm also doing a sr? broadcast for this :). Sorry for the noise in your review queues :\

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Carol, does the MMIError introduced make sense for you?

Comment 15

[Mounir Lamouri (:mounir)]

Comment on attachment 767637 [details] [diff] [review]
(mozilla-central) Part 0: DOMError

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

It's not really clear why .init() is needed here. It seems that peterv suggested it and I guess it might be because it is not possible to call the DOMError ctor from a JS child of DOMError? Instead of sr+ based on that supposition, I will let peterv do the honour.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 767638 [details] [diff] [review]
(mozilla-central) Part 1: MMIResult and MMIError

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

::: dom/system/gonk/RILContentHelper.js
@@ -1538,5 @@
> -    // will be exposed in the form of an array of nsIDOMMozMobileCFInfo
> -    // instances.
> -    if (message.success && message.rules) {
> -      this._cfRulesToMobileCfInfo(message.rules);
> -      message.result = message.rules;

It seems you removed CF result handling accidentally in the new patch.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Not really, that's going to be added again in bug 879680

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 767638 [details] [diff] [review]
(mozilla-central) Part 1: MMIResult and MMIError

So the idea is to add the basic MMIResult and MMIError support and the get IMEI and USSD cases. Other specific cases for PIN/PIN2 handling and CF will be addressed in bug 874000 and bug 879680 for Gecko and Gaia. The rest of the MMI related bugs are Gaia only since we don't support yet some MMI codes (CW, CB) that the commercial RIL does.

There's an explanation of all the required work for MMI localization at https://bugzilla.mozilla.org/show_bug.cgi?id=879032#c51

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 767638 [details] [diff] [review]
(mozilla-central) Part 1: MMIResult and MMIError

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

::: dom/system/gonk/RILContentHelper.js
@@ +1556,5 @@
>      }
>      Services.DOMRequest.fireSuccess(request, null);
>    },
>  
> +  handleSendCancelMMI: function handleSendCancelMMI(message) {

Could you still leave a "TODO: bug 879680 - ..." comment to address the missed CF handling here?  Thank you :)

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Sure! Thanks for the quick review!

Comment 21

[Carol Yang [:cyang]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #14)
> Carol, does the MMIError introduced make sense for you?

Yeah, seems quite similar to what you suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=879032#c26 so this should be fine for us.

Comment 22

[leo.bugzilla.gecko]

CLIR is also included? 
In attachment 767638 [details] [diff] [review], CLIR is not supported like below.
+      case MMI_SC_CLIR:
+        _sendMMIError(MMI_ERROR_KS_NOT_SUPPORTED);

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Not in the reference RIL, not sure about the commercial one.

Comment 24

[leo.bugzilla.gecko]

Carol, can you answer Comment 22?
CLIR is supported or not?

Comment 25

[Anshul]

Yes CLIR is supported by commercial RIL.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Anshul from comment #25)
> Yes CLIR is supported by commercial RIL.

I guess CLIP is also supported, so we'll also need to handle the localization of this requests in Gaia.

I am not familiar with this kind of requests. Anshul, could you provide an example of successful/failure for CLIR/CLIP request and what you expect to show in Gaia, please? We might be able to handle this as the general MMI use case in Gaia, so no extra work would be required.

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (b2g18) MMIResult and MMIError - WIP

Comment 28

[Gene Lian (I already quit Mozilla)]

Attachment: Experimental Patch for the Inheritance of DOMError in b2g18

Hi Fernando,

This experimental patch works for me. I don't really have much time to check what's the main difference with yours. Maybe you can try this way. :)

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Gene Lian [:gene] from comment #28)
> Created attachment 768269 [details] [diff] [review]
> Experimental Patch for the Inheritance of DOMError in b2g18
> 
> Hi Fernando,
> 
> This experimental patch works for me. I don't really have much time to check
> what's the main difference with yours. Maybe you can try this way. :)

Wow thanks Gene!

It also works for me! In fact it is the same method as the one I use in the WIP that I uploaded before. My problem was a local issue with the code that called the MMIError instantiation. I've just figured it out :(. Sorry about that.

In any case, I'm talking to mounir to check if this is the appropriate way to inherit from DOMError in b2g18 or if there is a better and cleaner way.

Thanks a lot for your help!

Comment 30

[Carol Yang [:cyang]]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #26)
> (In reply to Anshul from comment #25)
> > Yes CLIR is supported by commercial RIL.
> 
> I guess CLIP is also supported, so we'll also need to handle the
> localization of this requests in Gaia.
> 
> I am not familiar with this kind of requests. Anshul, could you provide an
> example of successful/failure for CLIR/CLIP request and what you expect to
> show in Gaia, please? We might be able to handle this as the general MMI use
> case in Gaia, so no extra work would be required.

Hi Fernando, CLIR allows restricting of out going calls (shows private number). Display in Gaia should be pretty similar to the general case for other MMI codes. For example:

Outgoing Caller ID
Caller ID defaults to not restricted. Next call: Not restricted

I would be sending this up in success case for interrogate:
{
  requestId : 'id{07db922e-b856-4248-8909-165a8ddc3bc4}',
  mmiServiceCode : 'scClir',
  result : 'smClirDefaultOffNextCallOff'
}

Error case:
{
  requestId : 'id{4b3d24e6-d93f-461e-9c3f-9b91bf6dc10b}',
  mmiServiceCode : 'scClir',
  errorMsg : 'emMmiError'
}

CLIP is for turning caller ID on/off. The Gaia display and and what we send would be the same as CLIR but with scClip as the mmiServiceCode.

Are you going to include support for CLIR/CLIP in this bug or should I open another bug to track them?

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Great! Thanks Carol!

It seems that no extra work is required as these MMISuccess/MMIError messages will be handled properly by the default handler added in bug 884349. Unless there is any extra use case that require us to handle additional information within the MMISuccess or MMIError objects.

Comment 32

[Carol Yang [:cyang]]

Perfect! I'll definitely let you know when I've ran into cases that need special handling. Thanks for the prompt responses!

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (b2g18) Part 0: DOMErrorHelper.jsm

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (b2g18) Part 1: Use DOMErrorHelper.jsm in WebApps

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (b2g18) Part 2: DOMMMIResult and DOMMMIError

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (b2g18) Part 0: Move DOMError from WebApps to RILContentHelper

This one was hard...

As happened in m-c, nsIDOMDOMError is not inheritable the way it is now in b2g18. We need a setter or an initializer for its properties so it can be called from the child components. However this setter or initializer can't be part of the public API. That can be done in WebIDL (ChromeOnly) for m-c, but not for b2g18. So for b2g18 we need to have the inheritable JS implementation in the same file as the child component. Unfortunately, it cannot be added to a JSM.

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (b2g18) Part 1: MMIResult and MMIError

Comment 38

[Fernando Jiménez Moreno [:ferjm]]

Thanks Gregor!

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Thanks Jonas! I guess I also need to thanks the jet lag this time :)

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Attachment: (b2g18) Part 1: MMIResult and MMIError

sr=sicking

Comment 41

[Peter Van der Beken [:peterv]]

Comment on attachment 767637 [details] [diff] [review]
(mozilla-central) Part 0: DOMError

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

Mounir: there's no easy way to know which arguments need to be passed through to the base class constructor when we generate the C++ binding for the inherited interface, so the JS implementation has to have a way to explicitly pass the arguments to the base class through a separate initialization function.

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Thanks Peter!

FTR I need to wait for the other MMI related bugs to be r+ before start landing the whole set of patches.

Comment 43

[Anshul]

Comment on attachment 767638 [details] [diff] [review]
(mozilla-central) Part 1: MMIResult and MMIError

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

::: dom/system/gonk/RILContentHelper.js
@@ +1244,3 @@
>        case "RIL:SendMMI:Return:KO":
>        case "RIL:CancelMMI:Return:KO":
> +        this.handleSendCancelMMI(msg.json);

Since you combined both the SendMMIOK and SendMMIKO and relying on message.success to determine if it is a success or a failure case, what's the point of having two different messages MMIOK and MMIKO?

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

Thanks Vicamo! Now we only need to get the r+ for bug 874000 patches before landing the whole stack of fixes.

(In reply to Anshul from comment #43)
> Comment on attachment 767638 [details] [diff] [review]
> (mozilla-central) Part 1: MMIResult and MMIError
> 
> Review of attachment 767638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +1244,3 @@
> >        case "RIL:SendMMI:Return:KO":
> >        case "RIL:CancelMMI:Return:KO":
> > +        this.handleSendCancelMMI(msg.json);
> 
> Since you combined both the SendMMIOK and SendMMIKO and relying on
> message.success to determine if it is a success or a failure case, what's
> the point of having two different messages MMIOK and MMIKO?

You are right! I'll remove it in a follow up bug

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

We need to land the Gaia side because of the string freeze, so even if bug 874000 is not yet reviewed, I need to push this alone. That means that we will temporary have no MMI support for CF and PIN/PUK related functionality until bug 874000 lands (hopefully tomorrow CST).

http://hg.mozilla.org/projects/birch/rev/d9ae62e6d7f3
http://hg.mozilla.org/projects/birch/rev/1fd0dd1c09f7

Comment 46

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d9ae62e6d7f3
https://hg.mozilla.org/mozilla-central/rev/1fd0dd1c09f7

Comment 47

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1996c3748df9
https://hg.mozilla.org/releases/mozilla-b2g18/rev/5a3c2e04e6c1

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

Backed out from b2g18 due to chrome tests failure.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2d9072f92b52

I won't be able to fix it until tomorrow as I am taking a 20 hours flight.

Comment 49

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

Comment on attachment 767638 [details] [diff] [review]
(mozilla-central) Part 1: MMIResult and MMIError

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

::: dom/system/gonk/ril_worker.js
@@ +2219,5 @@
>          options.ussd = mmiString;
>          this.sendUSSD(options);
>          return;
>        }
> +      _sendMMIError(MMI_ERROR_KS_ERROR);

It seems that this line cause xpcshell-test failure

Should update the test case accordingly.
dom/system/gonk/tests/test_ril_worker_mmi.js

Ex: in test_ril_worker_mmi.js
testSendMMI("**", "NO_VALID_MMI_STRING");
...
and others

Comment 50

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

(In reply to Szu-Yu Chen [:aknow] from comment #49)
> Comment on attachment 767638 [details] [diff] [review]
> (mozilla-central) Part 1: MMIResult and MMIError
> 
> Review of attachment 767638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +2219,5 @@
> >          options.ussd = mmiString;
> >          this.sendUSSD(options);
> >          return;
> >        }
> > +      _sendMMIError(MMI_ERROR_KS_ERROR);
> 
> It seems that this line cause xpcshell-test failure
> 
> Should update the test case accordingly.
> dom/system/gonk/tests/test_ril_worker_mmi.js
> 
> Ex: in test_ril_worker_mmi.js
> testSendMMI("**", "NO_VALID_MMI_STRING");
> ...
> and others

bug for test case fail => Bug 889215

Comment 51

[Fernando Jiménez Moreno [:ferjm]]

Thanks Szy-Yu!

I needed to modify "(b2g18) Part 0..." patch so we keep the DOMError implementation in Webapps.js if MOZ_B2G_RIL is not defined.

Try build looked good after that fix.

Re-pushed to b2g18:
https://hg.mozilla.org/releases/mozilla-b2g18/rev/dd85f9fd4e55
https://hg.mozilla.org/releases/mozilla-b2g18/rev/34ee4ae06093

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/dd85f9fd4e55
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/34ee4ae06093