1031175 - [Gaia] Unify both sendMMI() and dial() functions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Ken Chang[:kenkai]]

After fixing bug 889737, we need to do some modifications on dial app, accordingly.

Comment 1

[Ken Chang[:kenkai]]

Wesley, this is a Gaia bug for bug 889737. Can you please check if this is in the scope of 2.1.

Comment 2

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

(In reply to Ken Chang[:ken] from comment #0)
> After fixing bug 889737, we need to do some modifications on dial app,
> accordingly.

As this is for "after fixing bug 889737" I think we still need a one for "API compatility", which should support both old and new APIs and should be landed *before* bug 8889737 in order to not break existing functionality.

Comment 3

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

So far as I know this is not in 2.1 planned scope from GAIA. 
I'll keep the ni until it's firmed.

Comment 4

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> (In reply to Ken Chang[:ken] from comment #0)
> > After fixing bug 889737, we need to do some modifications on dial app,
> > accordingly.
> 
> As this is for "after fixing bug 889737" I think we still need a one for
> "API compatility", which should support both old and new APIs and should be
> landed *before* bug 8889737 in order to not break existing functionality.

Bug 1031193 created as blocking bug 889737

Comment 5

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> As this is for "after fixing bug 889737" I think we still need a one for
> "API compatility", which should support both old and new APIs and should be
> landed *before* bug 8889737 in order to not break existing functionality.

+1

I'm CC'ing more dialer people for extra-awareness on this topic.

Comment 6

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

Setting feature-b2g = 2.1 per FxOS V2.1 Release Planning and Feature Scope.

Comment 7

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

remove feature-b2g per driver's meeting that this is no longer a committed 2.1 scope. 
I'll leave this in [priority] list.

Comment 8

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

Hi Maria,
From some email discussions, looks like it's high-want in 2.1?

Comment 9

[Maria Angeles Oteo (:oteo)]

Yes, it was already agreed with Wilfred and I suppose he forgot to set it :), this should be a feature-b2g:2.1. 
Setting it right now.

Comment 10

[Doug Sherk (:drs) (inactive)]

We're taking this because we want to look into it ASAP, but there's still a lot of uncertainty due to bug 889737 not being landed yet. We don't expect that we'll be able to finish this in time.

Comment 11

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Use mozTelephony.dial() to send MMI codes

This is a WIP patch I've cooked up over the past three days. The code should be working (provided that the implementation in bug 889737 matches the previous discussions we had) however the unit-tests were only partially redone as they're quite messy. I'm not proud of this code in general and I'd like to set it aside for a couple of days to think how it could be done better. That being said there's a few issues which I simply cannot fully address right now:

- First of all I still don't have the interface of the MMICodeRequest object. I assume it contains a cancel() method for canceling the request and a reply() for following up but this is taken from our discussions as there's still no WIP patch.

- The second issue is about sending an MMI code while on a call. With my patch applied we cannot distinguish between a regular number and an MMI code before calling dial() so sending MMI codes follows the regular calling pattern. In bug 933153 we made it so that whenever we call a second number with one active call already connected we put the first call on hold. This is not needed when sending an MMI code but we've got no choice since we don't know what numbers are MMI codes beforehand. Unfortunately this seems to be incompatible with the excepted behavior in this situation, see bug 889737 comment 90 and apparently would be a certification issue too.

So we've got two ways out of here:

- Keep handling MMI codes the old way, i.e. checking if they are MMI codes using MmiManager.isMMI() before calling dial() and handling them differently than regular calls. This works but kind of defeats the purpose of having merged dial() and sendMMI().

- Modifying Gecko so we can invoke dial() even if we've already got an active call. If the active call needs to be put on hold for that to work should then be the responsibility of Gecko, not Gaia.

The first option works but really defeats the purpose of the work being done here and the second one carries a lot of risk and I'm pretty sure will take a lot more time than we have right now.

In both cases a *lot* of testing on devices will be required.

Comment 12

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

(In reply to Gabriele Svelto [:gsvelto] from comment #11)
> Created attachment 8476550 [details] [diff] [review]
> [PATCH WIP] Use mozTelephony.dial() to send MMI codes
> 
> This is a WIP patch I've cooked up over the past three days. The code should
> be working (provided that the implementation in bug 889737 matches the
> previous discussions we had) however the unit-tests were only partially
> redone as they're quite messy. I'm not proud of this code in general and I'd
> like to set it aside for a couple of days to think how it could be done
> better. That being said there's a few issues which I simply cannot fully
> address right now:
> 
> - First of all I still don't have the interface of the MMICodeRequest
> object. I assume it contains a cancel() method for canceling the request and
> a reply() for following up but this is taken from our discussions as there's
> still no WIP patch.
> 

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1031193#c8, we will have another interface, probably named UssdSession. There isn't cancel() method but providing send() to allow replying on the same ussd session.


> - The second issue is about sending an MMI code while on a call. With my
> patch applied we cannot distinguish between a regular number and an MMI code
> before calling dial() so sending MMI codes follows the regular calling
> pattern. In bug 933153 we made it so that whenever we call a second number
> with one active call already connected we put the first call on hold. This
> is not needed when sending an MMI code but we've got no choice since we
> don't know what numbers are MMI codes beforehand. Unfortunately this seems
> to be incompatible with the excepted behavior in this situation, see bug
> 889737 comment 90 and apparently would be a certification issue too.
> 
> So we've got two ways out of here:
> 
> - Keep handling MMI codes the old way, i.e. checking if they are MMI codes
> using MmiManager.isMMI() before calling dial() and handling them differently
> than regular calls. This works but kind of defeats the purpose of having
> merged dial() and sendMMI().
> 
> - Modifying Gecko so we can invoke dial() even if we've already got an
> active call. If the active call needs to be put on hold for that to work
> should then be the responsibility of Gecko, not Gaia.
> 

Aknow is working on gecko, see Bug 1056618. 

> The first option works but really defeats the purpose of the work being done
> here and the second one carries a lot of risk and I'm pretty sure will take
> a lot more time than we have right now.
> 
> In both cases a *lot* of testing on devices will be required.

One question regarding dialEmergency isn't 100% answered and needs Gaia input: please see bug 889737 comment 91

"maybe we shall only use dialEmergency in lock screen and use dial in other cases. Then in dialEmergency, we could safely reject all numbers except emergency."

How do you think, Gabriele?

Comment 13

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> Per https://bugzilla.mozilla.org/show_bug.cgi?id=1031193#c8, we will have
> another interface, probably named UssdSession. There isn't cancel() method
> but providing send() to allow replying on the same ussd session.

OK, where would the UssdSession come from? The comment mentions only ussdreceived so I'd assume that in that scenario one would have this flow:

mozTelephony.dial(<MMI code>)
-> The Promise returns an MMICodeRequest but the onsuccess/onerror callbacks do not trigger
-> We get an ussdreceived message instead carrying the UssdSession object
session.send(<MMI code used for reply>)

What about canceling interaction? Would that happen on the session or on the request?

> Aknow is working on gecko, see Bug 1056618. 

Ah, good. I must admit though that I'm a bit scared of regressions. Nevertheless I'll update the patch here accordingly.

> One question regarding dialEmergency isn't 100% answered and needs Gaia
> input: please see bug 889737 comment 91
> 
> "maybe we shall only use dialEmergency in lock screen and use dial in other
> cases. Then in dialEmergency, we could safely reject all numbers except
> emergency."
> 
> How do you think, Gabriele?

Yes, that would be better. Again having a distinction between dial() and dialEmergency() in Gaia is not really useful anyway because checking the network status and then calling dialEmergency() instead of dial() is racy.

Comment 14

[Doug Sherk (:drs) (inactive)]

We're taking this out of the current milestone for now.

Comment 15

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

Hi Sandip,
Can we remove feature-b2g for 2.1 at this moment?

Comment 16

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP v2] Use mozTelephony.dial() to send MMI codes

Second WIP. I've modified the code further according to the comments here and on the Gecko bug and made all unit tests work again (though they're not pretty and I had to remove a lot of unused stuff we still had around but which was disabled).

Comment 17

[Sandip Kamat]

OK for moving out of 2.1 for now and making it 2.2?

Comment 18

[Gabriele Svelto [:gsvelto]]

Quick update, I've got a WIP patch which works on top of bug 1058397 but I haven't found a reliable way to remove the current postMessage() based way of updating the MMI UI. I'll post the patch tomorrow with the UI part hopefully fixed.

Comment 19

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

(In reply to Gabriele Svelto [:gsvelto] from comment #18)
> Quick update, I've got a WIP patch which works on top of bug 1058397 

Wow, so excited about this :)

> but I
> haven't found a reliable way to remove the current postMessage() based way
> of updating the MMI UI. I'll post the patch tomorrow with the UI part
> hopefully fixed.

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP v3] Use mozTelephony.dial() to send MMI codes

Here's an updated patch with the postMessage() UI code ripped out and replaced with synchronous code. I've noticed that the existing code had a race which was hidden by postMessage() spinning the event-loop so I had to fix that first.

There's still some interactions with bug 1058397 that I'm not 100% sure of. Hsin-Yi, I've noticed that in some scenarios before when I got sessionEnded set to true now I get a populated session field. For example, if I type an unsupported code on my card like *123# I get an ussd-received event with the session field populated and an error message from my carrier. Before the patch I received the same error message but with sessionEnded set to true. Is this expected?

Comment 21

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

(In reply to Gabriele Svelto [:gsvelto] from comment #20)
> Created attachment 8498934 [details] [diff] [review]
> [PATCH WIP v3] Use mozTelephony.dial() to send MMI codes
> 
> Here's an updated patch with the postMessage() UI code ripped out and
> replaced with synchronous code. I've noticed that the existing code had a
> race which was hidden by postMessage() spinning the event-loop so I had to
> fix that first.
> 
> There's still some interactions with bug 1058397 that I'm not 100% sure of.
> Hsin-Yi, I've noticed that in some scenarios before when I got sessionEnded
> set to true now I get a populated session field. For example, if I type an
> unsupported code on my card like *123# I get an ussd-received event with the
> session field populated and an error message from my carrier. Before the
> patch I received the same error message but with sessionEnded set to true.
> Is this expected?

It should be a bug in the patch, I will correct it. Thank you.

Comment 22

[Gabriele Svelto [:gsvelto]]

(In reply to Szu-Yu Chen [:aknow] from comment #21)
> It should be a bug in the patch, I will correct it. Thank you.

Thanks for the quick response Szu-Yu, I'll re-check with your latest set of patches and report on bug 1058397.

Comment 23

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP v4] Use mozTelephony.dial() to send MMI codes

Yet another WIP, there's only a few unit-tests left to fix (i.e. now we're talking):

- All of the core functionality seem to work correctly barring the remaining issues with the Gecko patch-set which I haven't had time to test yet

- The UI doesn't use postMessage() anymore so it's fully synchronous. I had to squash a race condition that was previously hidden by postMessage()'s delay.

- The UI unit-tests are now synchronous too, simpler and a lot faster.

- The TelephonyHelper tests have also got some love, they all seem to work fine now.

Comment 24

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v5] Refactor the code dealing with USSD/MMI functionality to use the new, unified dial() API and fix all the associated unit-tests

This patch is large so here's a quick overview:

- First of all this works on top of bug 1031175 and there's still some issues in that patch-set.

- We now use the dial() call instead of sendMMI() and we don't explicitly put the existing call on hold when dialing out an MMI code; instead we rely on Gecko doing it for us.

- I've stripped out all the asynchronous UI updates with postMessage(). Now UI updates are synchronous. I also had to fix a race that had been previously hidden by postMessage() always spinning the event loop.

- Unit tests have been butchered to use the new interfaces, I've removed as much cruft as possible but things might be done better still.

Comment 25

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Use the new unified mozTelephony.dial() API to send MMI/USSD codes across all applications

Comment 26

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8501172 [details] [diff] [review]
[PATCH v5] Refactor the code dealing with USSD/MMI functionality to use the new, unified dial() API and fix all the associated unit-tests

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

This looks reasonable overall. The only significant problem I found was in the `_session` variable on MmiManager coupled with the removal of the session cancellation code, but I could be wrong here.

I left a few small design suggestions. I don't think I'll have anymore major ones, but I need to think about that and come back here for the next review round. Either way, I don't think this is a good bug to do more redesigning in since it's already so big.

::: apps/communications/dialer/js/dialer.js
@@ +431,5 @@
>        };
>        return;
>      }
>  
> +    if (number === '*#06#') {

I think this should be a constant stored on MmiManager. This wasn't as bad before because the `MmiManager.isMMI()` call made it fairly clear what was going on here.

@@ +480,5 @@
>        safetyId = setTimeout(releaseWakeLock, 30000);
>        document.addEventListener('visibilitychange', releaseWakeLock);
>      }
>  
> +    if (document.hidden && !evt.session) {

Let's flip the order of these calls while we're here. It makes more intuitive sense to me that the most common handler would be first.

::: apps/communications/dialer/js/mmi.js
@@ +11,2 @@
>  // USSD code used to query call waiting supplementary service status
> +var CALL_WAITING_STATUS_MMI_CODE = '*#43#';

Why these changes? `const` seems more appropriate to me.

@@ +22,5 @@
>    // ussdreceived. If the first event received is the ussdreceived one, we take
>    // the DOMRequest.onsuccess received subsequenty as a closure of the USSD
>    // session.
>    _pendingRequest: null,
> +  _session: null,

Can we not have multiple sessions? For example, we could receive an unsolicited message while sending one ourselves.

@@ -45,5 @@
> -
> -        // We cancel any active sessions if one exists to avoid sending any new
> -        // USSD message within an invalid session.
> -        conn.cancelMMI();
> -      }

This seems like it's what prevented my comment above from happening.

@@ +67,2 @@
>  
> +      self.openUI();

`this`

I actually prefer the `bind(this)` pattern, but it's inconsistent with the rest of the dialer.

@@ +143,4 @@
>  
>      // We always expect an MMIResult object even for USSD requests.
>      if (!mmiResult) {
> +      MmiUI.error(this._('GenericFailure'));

Please file a followup against dialer-most-wanted to switch this to using l10n data attributes.

@@ +213,5 @@
>        default:
>          // This would allow carriers and others to implement custom MMI codes
>          // with title and statusMessage only.
>          if (mmiResult.statusMessage) {
> +          message = this._(mmiResult.statusMessage) ?

`message = this._(mmiResult.statusMessage) || mmiResult.statusMessage;`

@@ +219,5 @@
>          }
>          break;
>      }
>  
> +    if (error) {

I'd rather flip these as I find it awkward to have an error handler before a success handler.

@@ +261,4 @@
>    },
>  
>    openUI: function mm_openUI() {
> +    this.init(function onInitDone(_) {

`this.init(MmiUI.loading);`

@@ +310,5 @@
>        }
>  
>        var conn = navigator.mozMobileConnections[cardIndex || 0];
>        var operator = MobileOperator.userFacingInfo(conn).operator;
> +      var title = self.prependSimNumber(operator ? operator : '', cardIndex);

`operator || ''`

@@ -390,5 @@
>        });
>      });
>    },
>  
> -  isMMI: function mm_isMMI(number, cardIndex) {

Please remove this function from the mock as well.

@@ +400,5 @@
>     *          upon successful completion or rejects upon failure.
>     */
>    _getImeiForCard: function mm_getImeiForCard(cardIndex) {
> +    return navigator.mozTelephony.dial(cardIndex, '*#06#').then(
> +      function mm_dialResolve(request) {

Naming this function is unnecessary.

@@ +410,5 @@
> +             * without the IMEI value, we throw an error message. */
> +            if ((result === null) || (result.serviceCode !== 'scImei') ||
> +                (result.statusMessage === null)) {
> +              reject(new Error('Could not retrieve the IMEI code for SIM' +
> +                     cardIndex));

I prefer how this was lined up in the past.

@@ +411,5 @@
> +            if ((result === null) || (result.serviceCode !== 'scImei') ||
> +                (result.statusMessage === null)) {
> +              reject(new Error('Could not retrieve the IMEI code for SIM' +
> +                     cardIndex));
> +              return;

I'm surprised we didn't have this before.

::: apps/communications/dialer/js/mmi_ui.js
@@ +137,5 @@
>      this.resetResponse();
>    },
>  
> +  success: function mui_success(message, title) {
> +    message = message ? message : this._('mmi-successfully-sent');

`message || this._('mmi-successfully-sent')`

@@ +145,4 @@
>    },
>  
> +  error: function mui_error(error, title) {
> +    error = error ? error : this._('mmi-error');

`error || this._('mmi-error')`

::: apps/communications/dialer/js/telephony_helper.js
@@ +52,5 @@
>      var isCdmaConnection = (cdmaTypes.indexOf(voiceType) !== -1);
>      var activeCall = telephony.active;
>  
>      if (!activeCall || isCdmaConnection) {
>        startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,

This block can be removed now, as can `isCdmaConnection` and `activeCall`.

@@ +57,5 @@
>                  ondisconnected, onerror);
>        return;
>      }
> +
> +    startDial(cardIndex, conn, sanitizedNumber, oncall, onconnected,

I assume Gecko is doing the holding for us now. Does this work for regular calls as well?

@@ +101,5 @@
>              document.removeEventListener('visibilitychange', hideDialog);
> +
> +            /* In tests this event can happen after the ConfirmDialog mock has
> +             * been removed so check that it's still there. */
> +            ConfirmDialog && ConfirmDialog.hide();

Yeah, I noticed this error as well, though I'd rather fix it another way. Perhaps we could manually dispatch a 'visibilitychange' event on `teardown()`.

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +1,3 @@
>  /* globals MockMobileconnection, MockMobileOperator, MockNavigatormozApps,
>             MockNavigatorMozIccManager, MockNavigatorMozMobileConnections,
> +           MockNavigatorMozTelephony,

Please fix the spacing here.

@@ +60,2 @@
>  
> +  function Event(result, error) {

This name is too vague and will pollute the global namespace (at least within this file), including the `Event` DOM event type.

@@ +77,5 @@
>      realMobileConnections = navigator.mozMobileConnections;
>      navigator.mozMobileConnections = MockNavigatorMozMobileConnections;
>  
> +    realMozTelephony = navigator.mozTelephony;
> +    navigator.mozTelephony = MockNavigatorMozTelephony;

We need to restore this in `suiteTeardown()`.

@@ +134,5 @@
> +      mockRequest.onsuccess(new Event({
> +        serviceCode: 'scFoo',
> +        statusMessage: null
> +      }));
> +      done();

I don't think you need `done` here.

@@ +153,5 @@
> +      mockRequest.onerror(new Event(/* result */ null, {
> +        serviceCode: 'scFoo',
> +        name: FAILED_MMI_MSG
> +      }));
> +      done();

I don't think you need `done` here.

@@ +172,5 @@
> +      mockRequest.onerror(new Event(/* result */ null, {
> +        serviceCode: 'scFoo',
> +        name: null
> +      }));
> +      done();

I don't think you need `done` here.

@@ +195,5 @@
> +      this.sinon.spy(MockMmiUI, 'received');
> +
> +      MmiManager.handleMMIReceived(MMI_MSG, {}, 0);
> +
> +      sinon.assert.calledWithMatch(MockMmiUI.received, {}, MMI_MSG,

You can switch us to `calledWith()` here.

@@ +274,5 @@
>      });
>  
> +    test('no message was received', function() {
> +      assert.isNull(MockMmiUI._message);
> +      assert.isNull(MockMmiUI._session);

This looks wrong to me. We were checking `isNull(sessionEnded)` previously, which passed.

@@ +299,2 @@
>        MmiManager._conn = mobileConn;
> +      MmiManager._session = {

I think you can write this as `this.sinon.stub(MmiManager, '_session', { /* ... */ });`, but I haven't verified this.

@@ +386,5 @@
> +            mockRequest.onsuccess(new Event({
> +              serviceCode: 'scFoo',
> +              statusMessage: SUCCESS_MMI_MSG
> +            }));
> +            done();

I don't think this `done` is needed. This is true below as well.

@@ +391,4 @@
>            });
>  
> +          test('the display is populated and the session has ended',
> +            function() {

This is tabbed in too far.

@@ +396,4 @@
>                             new RegExp('mmi-notification-title-with-sim' +
>                                        '.*sim-number.*' + (cardIndex + 1)));
> +              assert.equal(MockMmiUI._message, SUCCESS_MMI_MSG);
> +              assert.isNull(MockMmiUI._session);

This looks wrong, since we were checking `isNull(sessionEnded)` before. This is true below as well.

@@ +456,5 @@
> +              {
> +                serviceCode: 'scFoo',
> +                name: null
> +              })
> +            );

In general, we prefer to write this like so:

mockRequest.onerror(new Event(/* result */ null, {
  serviceCode: 'scFoo',	
  name: null	
}));

@@ +476,5 @@
>    });
>  
>    suite('Retrieving IMEI codes', function() {
> +    /* DOMRequest mock, to simulate asynchronous completion we trigger the
> +     * onsuccess handler on a timeout after it's being set. */

There's a MockDOMRequest in shared/test/unit/mocks/

@@ +510,5 @@
> +      });
> +
> +      this.sinon.spy(MockMmiUI, 'success');
> +      this.sinon.stub(MockNavigatorMozTelephony, 'dial');
> +      MockNavigatorMozTelephony.dial.returns(Promise.resolve(req));

Does this work the way you want it to? I think it resolves the promise immediately, and then returns the result later when `dial()` is actually invoked. I can't think of any case where it would matter, though.

@@ +515,3 @@
>  
>        MmiManager.showImei().then(function() {
> +        sinon.assert.calledWithMatch(MockMmiUI.success, imei, 'scImei');

`calledWith()`, same below.

@@ +608,3 @@
>      });
>  
> +    test('Check call forwarding rules, active voice', function() {

These tests seem like they could be written more simply using an array containing some objects with test settings and a `forEach()` loop, but it's up to you if you want to do that or not.

::: apps/communications/dialer/test/unit/mmi_ui_test.js
@@ +40,5 @@
> +    test('UI is modified properly', function() {
> +      MmiUI.success(result, title);
> +
> +      // Response form is hidden
> +      assert.isTrue(MmiUI.hideResponseForm.calledOnce);

Let's switch to newer sinon style.

`sinon.assert.calledOnce(MmiUI.hideResponseForm)`, etc.

@@ +95,5 @@
>        this.sinon.spy(MmiUI, 'showMessage');
>        this.sinon.spy(MmiUI, 'showWindow');
>      });
>  
> +    test('UI is modified properly ', function() {

Could you fix this stray space while you're here?

::: apps/communications/dialer/test/unit/mock_mmi_manager.js
@@ +20,5 @@
>    },
> +  handleMMIReceived: function(message, session, cardIndex) {
> +
> +  },
> +  handleRequest: function(conn, message, request) {

You can single-line this function and all the others that have no contents.

::: apps/communications/dialer/test/unit/mock_mmi_ui.js
@@ +24,5 @@
>  
> +  received: function mmui_received(session, message, title) {
> +    this._message = message;
> +    this._title = title;
> +    this._session = session;

I don't think the way these functions store their passed parameters is necessary. We should instead spy on them and check the passed parameters that way. We didn't used to be able to do that, but now we can.

@@ +29,4 @@
>    },
>  
> +  loading: function mmui_loading() {
> +

You can single-line this and the other empty body functions.

@@ +49,3 @@
>    },
>  
>    teardown: function mmui_mTeardown() {

This should be called `mTeardown`.

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +24,5 @@
>    'TelephonyMessages'
>  ]).init();
>  
> +// Constructor used to create a fake TelephonyCall object
> +function TelephonyCall() {}

We should use a mock for this instead.

@@ +136,5 @@
>      subject.call(dialNumber, 0);
>      sinon.assert.calledWith(navigator.mozTelephony.dialEmergency, '112');
>    });
>  
> +  test('should NOT hold the active line before dialing',

I don't think this test is necessary at all anymore, since there's no longer any code path here where we hold the call.

Comment 27

[Gabriele Svelto [:gsvelto]]

Due to bug 1080883 and the relevant discussion on dev-gaia I'll stop all further development here until the API issues raised there have been addressed.

Comment 28

[bhavana bajaj [:bajaj]]

(In reply to Gabriele Svelto [:gsvelto] from comment #27)
> Due to bug 1080883 and the relevant discussion on dev-gaia I'll stop all
> further development here until the API issues raised there have been
> addressed.

Looks like this is fixed now, what are the next steps here ? i want to make sure this is on track for 2.2?

Comment 29

[Gabriele Svelto [:gsvelto]]

The next step here is to modify the existing patch to accommodate for the API changes in Gecko. I've committed myself to do it in the current sprint so this should land by the end of next week.

Comment 30

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

(setting to feature-b2g=2.2+ as this was committed in 2.2 planning)

Comment 32

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 1/6 v6] Refactor the dialer code dealing with USSD/MMI functionality to use the new, unified mozTelephony.dial() API and fix all the associated unit-tests

Here's the updated patch that addresses all dialer changes. I've kept the changes separated in the PR but I'm not sure if it's worth looking at them instead of the full patch because they include the changes I had made for the API change which we ultimately didn't use. If you don't find those changes interesting I'll just squash them in the PR too. Here's a few comments to help with the review:

- The MmiManager doesn't use callbacks anymore for asynchronous operations, everything has been switched to promises for consistency and robustness.

- The MmiUI has also been rewritten so that it can be called directly and synchronously instead of having to go through window.postMessage().

- The changes above made it possible to make all the unit-tests rely on promises instead of manually called callbacks and timing-based setTimeout() calls (yuck). They should be a lot more robust now not to mention easier to understand.

- I've also consolidated the tests for call forwarding interactions which used to be huge.

All in all this removes over twice as much lines as it adds, which is good, effectively refactoring a lot of crummy code and making it at least bearable.

Comment 33

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8528386 [details] [diff] [review]
[PATCH 1/6 v6] Refactor the dialer code dealing with USSD/MMI functionality to use the new, unified mozTelephony.dial() API and fix all the associated unit-tests

Changed the name to reflect that this is now a patch series.

Comment 34

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/6] Do not special-case MMI codes in the contacts app anymore

This needs more testing so not asking for review just yet.

Comment 35

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 3/6] Make the uitest application use mozTelephony.dial() to retrieve IMEI codes

Fred, I saw from the log that you did most of the reviews on this test application so I'm asking for review to you. Feel free to pass it on to somebody more appropriate if I'm wrong here.

Comment 36

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 4/6] Make the system app use mozTelephony.dial() to retrieve IMEI codes

This patch modifies the system app to use the new unified API instead of mozMobileConnection.sendMMI(). Unfortunately the relevant bit of code is not DSDS-aware yet so I've left it like that; we should consider updating it for multi-SIM operation though.

Comment 37

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 5/6] Make icc_worker.js jshint-clean

And this is a quick follow-up to the previous patch that makes the file jshint-clean.

Comment 38

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 6/6] Make the settings app use mozTelephony.dial() to retrieve IMEI codes

Finally this makes the settings app use the unified API for retrieving the IMEI codes. I've also updated the unit-tests.

Comment 39

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8501175 [details] [review]
[PULL REQUEST] Use the new unified mozTelephony.dial() API to send MMI/USSD codes across all applications

Finally, let's give the PR a more sensible name.

Comment 40

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8528386 [details] [diff] [review]
[PATCH 1/6 v6] Refactor the dialer code dealing with USSD/MMI functionality to use the new, unified mozTelephony.dial() API and fix all the associated unit-tests

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

Looks generally good! This is very similar to the previous version.

Note that I skipped my usual inspection of whether or not all functionality has an added/changed test associated with it. I did this because of how similar it was to the previous version where I did go through this process. Let me know if you think I should do this and set the review? flag on me again.

::: apps/communications/dialer/js/mmi.js
@@ +37,3 @@
>  
> +    return new Promise(function(resolve, reject) {
> +      LazyLoader.load(lazyFiles, function resourcesLoaded() {

This function doesn't need a name.

::: apps/communications/dialer/js/mmi_ui.js
@@ +3,3 @@
>  'use strict';
>  
>  var MmiUI = {

Does this actually pass linting? I think we need `/* exported MmiUI */` above.

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +112,5 @@
> +      MmiManager.handleDialing(mobileConn, SUCCESS_MMI_MSG, Promise.resolve({
> +        success: true,
> +        serviceCode: 'scFoo',
> +        statusMessage: SUCCESS_MMI_MSG
> +      })).then(done);

`then(done, done)` here and below several times.

@@ +431,5 @@
>      test('for a single SIM device', function(done) {
>        var imei = '123';
> +
> +      this.sinon.stub(MockNavigatorMozTelephony, 'dial');
> +      MockNavigatorMozTelephony.dial.returns(

You can use this directly:

```js
this.sinon.stub(MockNavigatorMozTelephony, 'dial').returns(
  // ...
);
```

@@ +433,5 @@
> +
> +      this.sinon.stub(MockNavigatorMozTelephony, 'dial');
> +      MockNavigatorMozTelephony.dial.returns(
> +        Promise.resolve({
> +          result: Promise.resolve({

This nested promise is a bit confusing on first glance. I suggest commenting what's going on here, i.e. we resolve a call, then we resolve the IMEI retrieval.

@@ +487,5 @@
> +      MmiManager.notifySuccess({
> +        serviceCode: 'scCallBarring',
> +        statusMessage: 'smServiceEnabled'
> +      }, CALL_BARRING_STATUS_MMI_CODE);
> +      sinon.assert.calledWithMatch(MockMmiUI.success, 'ServiceIsEnabled');

Just `sinon.assert.calledWith()` here and below is fine.

@@ +545,5 @@
> +     * call forwarding additional information object were called. The sole
> +     * parameter is an array of additional information objects extracted from
> +     * an USSD response message.
> +     */
> +    function checkMessages(additionalInformation) {

This is a nice refactor, thanks for doing this.

::: apps/communications/dialer/test/unit/mock_mmi_manager.js
@@ +15,5 @@
>    sendNotification: function(message, cardIndex) {
>      return { then: function(callback) { callback(); } };
>    },
> +  handleEvent: function(evt) {},
> +  isImei: function(number) { return (number === '*#06#'); },

This is actually too much for a mock, in my opinion. We should just stub it where needed instead.

::: apps/communications/dialer/test/unit/mock_mmi_ui.js
@@ +9,5 @@
> +  error: function mmui_error(error, title) {},
> +  received: function mmui_received(session, message, title) {},
> +  loading: function mmui_loading() {},
> +  reply: function mmui_reply(value) {
> +    MmiManager.reply(value);

Now that this is the only mock method of this class with any side effects, it is probably better to just stub it to do this where we need it.

::: apps/communications/dialer/test/unit/telephony_helper_test.js
@@ +24,5 @@
>    'TelephonyMessages'
>  ]).init();
>  
> +// Constructor used to create a fake TelephonyCall object
> +function TelephonyCall() {}

It's not clear to me why we need this.

Comment 41

[[e-mail]]

Comment on attachment 8528393 [details] [diff] [review]
[PATCH 3/6] Make the uitest application use mozTelephony.dial() to retrieve IMEI codes

The change looks good to me. For detail connection API syntax I'd like rely on module owner's review.

Comment 42

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8528402 [details] [diff] [review]
[PATCH 4/6] Make the system app use mozTelephony.dial() to retrieve IMEI codes

Transfer review

Comment 43

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8528404 [details] [diff] [review]
[PATCH 5/6] Make icc_worker.js jshint-clean

Transfer review

Comment 44

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8528407 [details] [diff] [review]
[PATCH 6/6] Make the settings app use mozTelephony.dial() to retrieve IMEI codes

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

This looks good to me, thanks ! r+

Comment 45

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8528402 [details] [diff] [review]
[PATCH 4/6] Make the system app use mozTelephony.dial() to retrieve IMEI codes

LGTM

Comment 46

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8528404 [details] [diff] [review]
[PATCH 5/6] Make icc_worker.js jshint-clean

Great ! :)

Comment 47

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 1/6 v7] Refactor the dialer code dealing with USSD/MMI functionality to use the new, unified mozTelephony.dial() API and fix all the associated unit-tests

Addressed all the review comments and also fixed a potential race in MmiUI.init(). The mmi_ui.js file was jshint even without exporting the MmiUI variable because it called MmiUI.init() inline. However MmiUI.init() had an asynchronous part no one waited for so there was a very slim chance of MmiUI methods being called before MmiUI.init() had actually finished executing. I've fixed the issue by making MmiUI.init() synchronous, taking the result of LazyL10n.get() directly from its sole caller which is the MmiManager. As a final note, the mock for MmiUI called MmiManager.reply() but that's not needed anymore as I had already modified that unit-test not to rely on it so I've removed the call right away.

This is the complete patch but I've pushed the incremental changes on top of the previous one in the PR for ease of review as usual, see here:

https://github.com/gabrielesvelto/gaia/commit/cc6b0d1eb546d305c20618a1b6628fae5685c4b1

Comment 48

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH 2/6 v2] Remove the dependency on the MmiManager when checking for MMI codes

This is an updated patch for the contacts app. It works around not being able to dial an MMI code from outside the dialer by manually checking for codes and then using an activity. Previously we relied on MmiManager.isMMI() to check the codes but that function wasn't reliable. It could yield false positives which is one of the reasons why it's been removed. The hard-coded check that this patch puts in place covers pretty much every code that would work from a contact.

Ideally though we should remove this stuff and let mozTelephony.dial() figure out what to do with the number on its own. We should file a follow up for that.

Comment 49

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Gabriele Svelto [:gsvelto] from comment #48)
> 
> Ideally though we should remove this stuff and let mozTelephony.dial()
> figure out what to do with the number on its own. We should file a follow up
> for that.

I think we still need to differentiate in contacts cause of security reasons. If we have a number we just call mozTelephony.dial() that will initiate the whole process that will launch the attention screen and so on.

We are using a web activity when we have a mmi code cause of security reasons. The user must acknowledge the action tapping on call since some mmi codes could cost money. That's the reason to not to execute dial() directly.

Comment 50

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8529363 [details] [diff] [review]
[PATCH 2/6 v2] Remove the dependency on the MmiManager when checking for MMI codes

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

Pretty good patch, now everything is easier to read.

Tested on the phone and working for me.

::: shared/js/contacts/contacts_buttons.js
@@ +56,5 @@
> +      LazyLoader.load(['/shared/js/multi_sim_action_button.js'], function() {
> +        /* jshint nonew: false */
> +        new MultiSimActionButton(button, self._call.bind(self),
> +                                 'ril.telephony.defaultServiceId',
> +                                 function() { return number; });

Maybe here we can use the fancy fat arrow functions:

() => number;

Comment 51

[Gabriele Svelto [:gsvelto]]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #49)
> I think we still need to differentiate in contacts cause of security
> reasons. If we have a number we just call mozTelephony.dial() that will
> initiate the whole process that will launch the attention screen and so on.

OK, I'll modify the contact before landing and use the fat-arrow syntax (which I've already seen a few times but never used, I should look at the documentation on MDN; I'm a poor, old style C coder and all this fancy JS stuff makes my head spin).

Comment 52

[Gabriele Svelto [:gsvelto]]

OK, final push to my branch with all amended, squashed patches. The try run is here, let's hope it turns green:

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=11cebfd145da

Comment 53

[Gabriele Svelto [:gsvelto]]

Try is as green as the neighbor's grass:

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=11cebfd145da

Merged to gaia/master 3e117e07039187fb545455ad2fb945af6ea86bcc

https://github.com/mozilla-b2g/gaia/commit/3e117e07039187fb545455ad2fb945af6ea86bcc

Comment 54

[Michael Schwartz [:m4]]

Just to be clear, is the goal to completely deprecate the sendMMI API?

Comment 55

[Gabriele Svelto [:gsvelto]]

(In reply to Michael Schwartz [:m4] from comment #54)
> Just to be clear, is the goal to completely deprecate the sendMMI API?

Yes, after this patch landed sendMMI() is not used anymore in gaia and it can be removed from the Gecko implementation.

Comment 56

[Eric Chang [:ericcc] [:echang]]

Dialer test cases here for regression checking 
https://moztrap.mozilla.org/manage/suites/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-name=%5Bcomms%5Ddialer