890463 - Follow up of Use MMIResult for get IMEI - default case

Status: RESOLVED FIXED

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

Description

[Fernando Jiménez Moreno [:ferjm]]

As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=888904#c6 we don't handle the default case properly.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Blocking to allow for localization.

Comment 4

[Etienne Segonzac (:etienne)]

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

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

I'll let you decide, but it may be more readable to just have something like this and don't worry about calling LazyL10n.get each time (it's pretty cheap):

```
  send: function mm_send(message) {
    if (!this.ready) {
      this.init();
    }

    LazyL10n.get((function localized(_) {
      this._ = _;
      if (this._conn) {
        var request = this._pendingRequest = this._conn.sendMMI(message);
        request.onsuccess = this.notifySuccess.bind(this);
        request.onerror = this.notifyError.bind(this);
        this.openUI();
      }
    }).bind(this));
  },

```

Either way, we need to fix the double |_send()| call :)

::: apps/communications/dialer/js/mmi.js
@@ +53,5 @@
> +    if (!self._) {
> +      LazyL10n.get((function localized(_) {
> +        self._ = _;
> +        _send();
> +      }).bind(self));

no need for the bind since you're referencing self and not this inside.

@@ +58,3 @@
>      }
> +
> +    _send();

aren't you missing an early return in the |if (!self._)|?
Looks like we're going to call |_send()| twice here...

::: apps/communications/dialer/test/unit/mmi_test.js
@@ +60,4 @@
>  
>      test('Check empty request result', function(done) {
>        setTimeout(function() {
> +        assert.equal(MmiManager._ui._messageReceived, undefined);

nit: chai has a assert.isUndefined()

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Etienne Segonzac (:etienne) from comment #4)
> 
> @@ +58,3 @@
> >      }
> > +
> > +    _send();
> 
> aren't you missing an early return in the |if (!self._)|?
> Looks like we're going to call |_send()| twice here...

You are damn right! Thanks for noticing :). Fixing it.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Comment 7

[Etienne Segonzac (:etienne)]

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

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

Perfect, thanks!

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Thanks Etienne!

https://github.com/ferjm/gaia/commit/5841c7d539f5d4d24f863a9ab3e23046dde96372

Comment 9

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #8)
> Thanks Etienne!
> 
> https://github.com/ferjm/gaia/commit/5841c7d539f5d4d24f863a9ab3e23046dde96372

Do you have a commit ID for this commit on mozilla-b2g/gaia?

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Isn't 5841c7d539f5d4d24f863a9ab3e23046dde96372 the commit ID?

Comment 11

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 5841c7d539f5d4d24f863a9ab3e23046dde96372 to:
v1-train: 7b79edb93d53f5fa956d3cbd0553d252c4e1c241

Comment 12

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 7b79edb93d53f5fa956d3cbd0553d252c4e1c241