Overview

This document serves as a guide for the technical direction of the Gaia Dialer. It is a set of code standards, refactors, and patterns that we will be working towards. It is based heavily on standards and patterns from Gecko. There will be no big push or work done just to align us more with this document. Instead, work will be done incrementally as we work on features and bug fixes. This document is entirely up for discussion. The recommendations in it are based more on a desire for consistency rather than being an assertion that one way of doing things is better than another.

Bug Threads on Bugzilla

temp

Promises

Promises should be used anytime we're doing something async that either multiple points in code need to know about, or the async operation can fail. Otherwise, we should just use callbacks.

Upcoming Features

bug 1099398 - (dialer-favorite-contacts) (meta) Display favorite contacts at the top of the call log

• Create a FavoriteContactsHandler (favorite_contacts_handler.js) class to manage the logic behind loading and maintaining favorite contacts. This will contain:
  • Saving and loading
• Create a FavoriteContactsUI (favorite_contacts_ui.js) class to manage the

Design Patterns

Class Hierarchy

The pattern that we use is a loose interpretation of Model-view-controller (MVC). We don't think that a total redesign to be true MVC is either beneficial or necessary. We have chosen to focus on the separation of UI and logic code as this is one of our weak areas as of writing. To this end, we have crafted two common hierarchies:

View with subview(s)

--------------------
|   Master---------|---> outside world
|   ^    ^         |
|   |    |         |
|   v    v         |
| UI <--> Handler--|---> outside world
--------------------

(note that UI has no connection to the outside world)
This scenario is the pattern used when there is a master view with subcomponents that function independently. An example of this is the callscreen view and its conference group sub-functionality.
In this scenario, the following are analogous:

• Handler to (Inner) Controller
• UI to View
• Master to (Outer) Model and Controller is this too vague and poorly defined to be useful? this seems to work well for the callscreen

Comments

Classes should clearly delineate the following:

• Their purpose from a high level.
• Their public functions and their parameters.

Example

// ...
/**
 * Manages the view (UI) of the details of an ongoing conference call, this is
 * the presentation of the details of the parties currently participating on it
 * (phone number, contact name, phone number type, etc., if any).
 *
 * The needed HTML code is lazy loaded during initialization in the init()
 * function. It takes care of the initialization automatically.
 *
 * Exposes functions to:
 *  - Add and remove calls to the ongoing conference call details information
 *    overlay.
 *  - Show and hide the ongoing conference call details information overlay.
 *  - Mark the calls participating in an ended conference call as hang up.
 *  - Set the title of the ongoing conference call details information overlay.
 */
var ConferenceGroupUI = (function() {
// ...

Code Style

Code style is largely unstandardized within Gaia. However, we can set our own and adopt it locally.

Long lines

Function calls with callbacks

Function calls that pass in callbacks should wrap to the next line such that the function appears inline with the surroundings, like so:

fooIsAReallyLongFunctionNameThatNeedsWrappingBecauseItIsReallyLong(bar,
function() {
  /* ... */
});

Other sets of parameters

Other large sets of parameters can be wrapped two ways:

fooIsAReallyLongFunctionNameThatNeedsWrapping(someReallyLongBarParam,
                                              'foobarraboof',
                                              moreBar);

or,

fooIsAReallyLongFunctionNameThatNeedsWrapping(
  someReallyLongBarParam, 'foobarraboof', moreBar);

Either is acceptable, but are slightly contextual.

Class Export Pattern

There are two common ways to declare and export classes:

Public 'var' pattern

/* exported Foo */
var Foo = {
};

Exports member pattern

(function(exports) {
  var _foo = {
  };
  exports.Foo = _foo;
})(this);

The former, public 'var' pattern is preferred for the following reasons:

• A /* exported Foo */ declaration is needed to pass jshint, which allows readers to see at a glance what class this file is exporting.
• It is more intuitively obvious what is going on.
• It is less verbose.

Unit Tests

Every new piece of functionality should be accompanied with a unit test. Currently, unit tests are being heavily abused to act as poor man's integration tests. We should instead shift our focus to properly unit testing functions rather than entire end-to-end code paths.

When to Unit Test

Unit tests should be written for any of,

• Public functions of classes.
• Helpers that get called many times throughout a class. In this case, it's ok to call them within tests, even if they are private, of the form _foo().
• Private functions that cannot be adequately tested by testing public functions that call them.

Structuring Code for Unit Tests

Private functions that you are exposing for unit testing should be written like so:

var Foo = {
  _privateBar: function() { },
  publicFoobar: function() { },
};

Another way to write your code is to encapsulate functions without using member variables:

var Foo = {
  publicFoobar: function() {
    function privateBar() { }
    privateBar();
  },
};

This is also perfectly acceptable. You should use this when you want to explicitly hide private functions from being unit tested. Unfortunately, this only works when a helper is only needed by one function.

Reviewer Checklist

• Are the tracking flags set correctly?
  • Is the Target Milestone set to this sprint, or a later one if I think it will take longer?
  • Is the bug's status ASSIGNED?
• Do I know the affected code well?
  • If not, has this been redirected to you by someone who knows this? If so, that's ok, continue and learn the code to review it. Make sure to request feedback from someone who does know the code well. If not, redirect it to someone else.
• Does this change fix the issue?
  • If you're not absolutely certain, you should probably test it yourself.
• Am I certain that this is the best way to fix this issue?
  • If not, or there may be a simpler way. Ask someone who may know better, and/or spend more time investigating it yourself.
• Does every change in functionality come with a new unit test? See #When to Unit Test.
• Is this patch updating any nearby obsolete code to modern standards, such as old-style use of Sinon.JS?
• Does this patch remove files from build/jshint/xfail.list if any of the affected files are in there now?
• Does this patch add any tests that were previously missing for affected code that is nearby the changes?
• Are the changes in this patch commented extensively enough? Did I know fairly quickly what was being intended after reading the comments?
  • This also applies to edge cases that are not immediately obvious without history or context.
  • If comments are inadequate, request more, or for clarifications.
• If UI changes are made, does the code render properly in RTL languages?
  • Things to check: bdi tags around phone numbers to properly render the + in a phone number
• If this is a workaround or is creating tech debt, are followups filed?
  • Are they mentioned as comments in the code?
  • Are they set as dependent on the original bug on Bugzilla?
• Are the summaries accurate and useful?
  • Is the bug summary on Bugzilla accurate and useful?
    • The bug summary should exclude [blah][blah][blah] tags.
    • It should also summarize the issue.
      • If it's a bug in the truest sense of the word, it should state the problem.
      • If it's a feature/improvement, it should state the improvement to be made.
  • Is the patch summary accurate and useful?
    • Does the patch have the Bug XXX - and r=name tags?
      • e.g., Bug 123456 - Remove workaround for \'LazyLoader\' not caching previously loaded files. r=drs is better than fix lazyloader.
    • Is the patch summary a statement of what is being changed or fixed?
      • If this is a bug fix, it should not be a restatement of the bug summary on Bugzilla, but should instead state what changes are being made and what is being fixed.
• Did this patch pass a try run?
  • It's not your job to make certain of this, but having many and/or important tests failing is a canary for bigger problems.
• When I gave review+, did I ask for a demo?
  • Needinfo the patch author and send them a link to the current sprint demo page.
  • Only do this for something user-facing. If it's entirely back-end work, refactors, etc. then this isn't necessary.

Refactor Suggestions

ConferenceGroupHandler

ConferenceGroupHandler should be split into ConferenceGroupHandler and ConferenceGroupUI. Code should be taken from CallScreen' as well as ConferenceGroupHandler to form this new class.

CallLog

CallLog should be split into CallLogHandler, CallLogItem, and CallLogUI.

CallLogDBManager

CallLogDBManager should be split into CallLogDBHandler, and CallLogDBItem.

SuggestionBar

SuggestionBar should be split into SuggestionBarHandler, SuggestionBarItem, and SuggestionBarUI.

CallsHandler

CallsHandler should be split into CallsHandler and CallsUI. CallsUI will support the following functionality:

• CallsUI will have an updateButtonState() function that surveys telephony state and updates all CallScreen buttons appropriately.
• CallsHandler.holdOrResumeSingleCall() should be broken up into functional and UI components. Current callsites will call into the CallsHandler function, which will in turn call into CallsUI.updateButtonState().
• CallsHandler.addCall() should be broken up where CallScreen.render() is called. This should be added to a new CallsUI.renderViewForCall() function.
• CallsHandler.handleCallWaiting() should be broken up into functional and UI components. Since most of this function is UI work in nature, the majority of it should move to a new CallsUI.displayCallWaiting().
• CallsHandler.updatePlaceNewCall(), CallsHandler.updateMergeAndOnHoldStatus(), and its supporting function, CallsHandler::isEstablishingCall() should live on CallsUI.

Use of Platform Features

Discontinue use of `on***` Telephony handlers

See bug 1095366

Gecko work will be done to phase out the `on***` Telephony handlers, such as `onhold`, `onconnected`, etc. To support this, we should only use the `Telephony.oncallschanged` and `TelephonyCall.onstatechange` callbacks.

Syncing UI with Backend

See bug 1082588

UI changes should be synced with the backend whenever possible via web API's. A typical, incorrect, pattern looks like this at a high level:

---------------------
| Handler --> Gecko |
|    |              |
|    v              |
|    UI             |
---------------------

An example of this is as follows:

var call; // assume that this contains a TelephonyCall
call.hold();
CallScreen.onHoldButton.classList.add('active');

This is incorrect because the Dialer is assuming that whatever it's dispatching to Gecko is proceeding without error. A better way to structure these interactions is as follows:

---------------------
|       Gecko       |
|       ^   \       |
|      /     v      |
|   Handler  UI     |
---------------------

Here, the Dialer dispatches an update to Gecko, and the UI has event listeners set to update itself when Gecko confirms that the update proceeded. Here's an example of this in code:

var call; // assume that this contains a TelephonyCall
call.onheld = function() {
  CallScreen.onHoldButton.classList.add('active');
};
call.hold();

Bluetooth Commands Handled by Modem

Bluetooth commands should not actually live in the Dialer. They should be handled by the modem directly, and the Dialer would receive any `Telephony.oncallschanged` and `TelephonyCall.onstatechange` events as a result of this handling. We should move towards this as part of discussions with the Bluetooth and RIL teams.

Gaia-Standardized Policies

These are standards that are Gaia-wide, but are not explicitly written anywhere.

Unit Testing

All changes in functionality should be accompanied with a unit test.

Use of Sinon.JS

There is currently lots of code written like this in our unit tests:

var mySpy = sinon.spy(MockFoo, 'bar');
// do something that calls |MockFoo.bar()|
assert.isTrue(mySpy.called);

There are several problems here:

• Use of a variable for the spy. This is not a universal rule and can be useful in certain situations, but 98% of the time is unneeded.
• Use of |sinon.spy()| instead of sandboxing using |this.sinon.spy()|. Doing this the wrong way prevents Sinon from automatically restoring the function at the end of this test. This can contaminate other tests.
• Use of |assert.is***()| instead of |sinon.assert.called***()|.
• Use of the catch-all |called| instead of |calledOnce()|, |calledWith()|, etc.

With these suggestions in mind, we might write the previous example this way instead:

this.sinon.spy(MockFoo, 'bar');
// do something that calls |MockFoo.bar()|
sinon.assert.calledWith(MockFoo.bar, 'fooBarArg');