1060451 - Refactor copied-and-pasted displayMessage() code in emergency-call/dialer.js taken from dialer/telephony_helper.js

Status: RESOLVED FIXED

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

Description

[Doug Sherk (:drs) (inactive)]

In bug 1038701, we copied-and-pasted some code from dialer/telephony_helper.js into emergency-call/dialer.js. We should investigate sharing this properly. See the discussion at https://github.com/mozilla-b2g/gaia/pull/23322#discussion_r16732484

Comment 1

[Anthony Ricaud (:rik)]

When thinking about that, we should consider bug 1032273 to make it easy to share the error handling with SMS too.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Refactor TelephonyHelper code to be shared with dialer and emergency-call.

PR: https://github.com/mozilla-b2g/gaia/pull/24592

The basic structure here is to take the displayMessage(), handleError(), and notifyBusyLine() functions out of TelephonyHelper and stick them in a shared TelephonyMessages class.

I didn't put serious thought into how to structure this so that we have optimal results in bug 1032273 and bug 1062122. I believe that this will give us gains in bug 1032273 for free, and I'm not sure about bug 1062122, though that will need more work anyways.

Francisco, could you review just the contacts portions?

Comment 3

[Anthony Ricaud (:rik)]

Comment on attachment 8497944 [details] [diff] [review]
Refactor TelephonyHelper code to be shared with dialer and emergency-call.

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

I like the split!

We're losing one error message though so r-.

::: apps/communications/contacts/js/contacts.js
@@ +615,5 @@
>          var simPickerNode = document.getElementById('sim-picker');
>          LazyLoader.load(
>            [SHARED_UTILS_PATH + '/misc.js',
>             '/dialer/js/telephony_helper.js',
> +           '/shared/js/dialer/telephony_messages.js',

Instead of making apps load this, I feel like telephony_helper should do it.

::: apps/emergency-call/js/dialer.js
@@ -20,5 @@
> -  _throwEmergencyError: function(sanitizedNumber) {
> -    LazyLoader.load(['/shared/style/confirm.css'], function() {
> -      navigator.mozL10n.once(function() {
> -        this._emergencyMsg.textContent =
> -          this.l10n.get('emergency-call-error',

We are losing this specific error message.

Comment 4

[Doug Sherk (:drs) (inactive)]

Attachment: Refactor TelephonyHelper code to be shared with dialer and emergency-call.

PR: https://github.com/mozilla-b2g/gaia/pull/24644

New PR since I don't want for Gaia to reopen before I can reopen the old PR.

Each of the code review comments got their own commit on the PR. They're folded into one patch here.

Comment 5

[Doug Sherk (:drs) (inactive)]

Commit https://github.com/DouglasSherk/gaia/commit/fa0a5a180f54f8b21fdc09a2f2480c2ca8e69ba0 has a couple of additional fixes for other problems that I noticed and improvements that I thought could be made.

Comment 6

[Anthony Ricaud (:rik)]

Forgot to mention there is a console.log that needs to disappear.

Comment 7

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

Comment on attachment 8498626 [details] [diff] [review]
Refactor TelephonyHelper code to be shared with dialer and emergency-call.

r+ to the changes in contacts. Didn't put much attention to the refactor itself, left Antony that.

Comment 8

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/e678ab4e40bf23bfbb9beb932ce5a048b6523ea9