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

Blocks a blocker, so leo?

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

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 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?

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:*.

I don't think this belongs to Gaia.

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)?

Yes, that's the idea :)

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.

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!

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

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

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 :\

Carol, does the MMIError introduced make sense for you?

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 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.

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

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 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 :)

Sure! Thanks for the quick review!

(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.

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);

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

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

Yes CLIR is supported by commercial RIL.

(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, 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. :)

(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!

(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?

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.

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

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.

Thanks Gregor!

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

sr=sicking

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.

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 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?

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

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

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

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

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 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

(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

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

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