Coding style

Random coding style-related issues that bothered me while reading the
source code touched by "Fix local video disappearing from local
thumbnail" (https://github.com/jitsi/lib-jitsi-meet/pull/571). These
include naming, formatting, comments, typos, duplication.

modules/RTC/JitsiLocalTrack.js

@@ -4,8 +4,12 @@ import CameraFacingMode from '../../service/RTC/CameraFacingMode';
 import { getLogger } from 'jitsi-meet-logger';
 import JitsiTrack from './JitsiTrack';
 import JitsiTrackError from '../../JitsiTrackError';
-import * as JitsiTrackErrors from '../../JitsiTrackErrors';
-import * as JitsiTrackEvents from '../../JitsiTrackEvents';
+import { TRACK_NO_STREAM_FOUND } from '../../JitsiTrackErrors';
+import {
+    LOCAL_TRACK_STOPPED,
+    NO_DATA_FROM_SOURCE,
+    TRACK_MUTE_CHANGED
+} from '../../JitsiTrackEvents';
 import * as MediaType from '../../service/RTC/MediaType';
 import RTCBrowserType from './RTCBrowserType';
 import RTCEvents from '../../service/RTC/RTCEvents';
@@ -37,27 +41,23 @@ export default class JitsiLocalTrack extends JitsiTrack {
      * source. NOTE: defined for desktop sharing tracks only.
      * @constructor
      */
-    constructor(trackInfo) {
-        const {
-            rtcId,
-            stream,
-            track,
-            mediaType,
-            videoType,
-            resolution,
-            deviceId,
-            facingMode,
-            sourceId,
-            sourceType
-        } = trackInfo;
-
+    constructor({
+        deviceId,
+        facingMode,
+        mediaType,
+        resolution,
+        rtcId,
+        sourceId,
+        sourceType,
+        stream,
+        track,
+        videoType
+    }) {
         super(
-            null /* RTC */,
+            /* conference */ null,
             stream,
             track,
-            () => {
-                this.emit(JitsiTrackEvents.LOCAL_TRACK_STOPPED);
-            } /* inactiveHandler */,
+            /* streamInactiveHandler */ () => this.emit(LOCAL_TRACK_STOPPED),
             mediaType,
             videoType);
 
@@ -66,29 +66,25 @@ export default class JitsiLocalTrack extends JitsiTrack {
          * @type {number}
          */
         this.rtcId = rtcId;
-        this.resolution = resolution;
         this.sourceId = sourceId;
         this.sourceType = sourceType;
 
-        // FIXME: currently firefox is ignoring our constraints about
+        // FIXME Currently, Firefox is ignoring our constraints about
         // resolutions so we do not store it, to avoid wrong reporting of local
-        // track resolution
-        if (RTCBrowserType.isFirefox()) {
-            this.resolution = null;
-        }
+        // track resolution.
+        this.resolution = RTCBrowserType.isFirefox() ? null : resolution;
 
         this.deviceId = deviceId;
-        this.storedMSID = this.getMSID();
 
         /**
          * The <tt>Promise</tt> which represents the progress of a previously
-         * queued/scheduled {@link _setMute} (from the point of view of
-         * {@link _queueSetMute}).
+         * queued/scheduled {@link _setMuted} (from the point of view of
+         * {@link _queueSetMuted}).
          *
          * @private
          * @type {Promise}
          */
-        this._prevSetMute = Promise.resolve();
+        this._prevSetMuted = Promise.resolve();
 
         /**
          * The facing mode of the camera from which this JitsiLocalTrack
@@ -123,19 +119,6 @@ export default class JitsiLocalTrack extends JitsiTrack {
         // soon as it's called.
         this._realDeviceId = this.deviceId === '' ? undefined : this.deviceId;
 
-        /**
-         * Set to <tt>true</tt> when there's ongoing "mute/unmute" operation in
-         * progress. Used by {@link LocalSdpMunger}.
-         * @type {boolean}
-         */
-        this.inMuteOrUnmuteProgress = true;
-
-        /**
-         * Indicates that we have called RTCUtils.stopMediaStream for the
-         * MediaStream related to this JitsiTrack object.
-         */
-        this.stopStreamInProgress = false;
-
         /**
          * On mute event we are waiting for 3s to check if the stream is going
          * to be still muted before firing the event for camera issue detected
@@ -234,11 +217,10 @@ export default class JitsiLocalTrack extends JitsiTrack {
     }
 
     /**
-     * Fires JitsiTrackEvents.NO_DATA_FROM_SOURCE and logs it to analytics and
-     * callstats.
+     * Fires NO_DATA_FROM_SOURCE event and logs it to analytics and callstats.
      */
     _fireNoDataFromSourceEvent() {
-        this.emit(JitsiTrackEvents.NO_DATA_FROM_SOURCE);
+        this.emit(NO_DATA_FROM_SOURCE);
         const eventName = `${this.getType()}.no_data_from_source`;
 
         Statistics.analytics.sendEvent(eventName);
@@ -273,13 +255,15 @@ export default class JitsiLocalTrack extends JitsiTrack {
     /**
      * Sets the stream property of JitsiLocalTrack object and sets all stored
      * handlers to it.
+     *
      * @param {MediaStream} stream the new stream.
+     * @protected
      */
     _setStream(stream) {
         super._setStream(stream);
 
-        // Store the MSID for video mute/unmute purposes
         if (stream) {
+            // Store the MSID for video mute/unmute purposes.
             this.storedMSID = this.getMSID();
             logger.debug(`Setting new MSID: ${this.storedMSID} on ${this}`);
         } else {
@@ -288,81 +272,88 @@ export default class JitsiLocalTrack extends JitsiTrack {
     }
 
     /**
-     * Mutes the track. Will reject the Promise if there is mute/unmute
-     * operation in progress.
+     * Asynchronously mutes this track.
+     *
      * @returns {Promise}
      */
     mute() {
-        return this._queueSetMute(true);
+        return this._queueSetMuted(true);
     }
 
     /**
-     * Unmutes the track. Will reject the Promise if there is mute/unmute
-     * operation in progress.
+     * Asynchronously unmutes this track.
+     *
      * @returns {Promise}
      */
     unmute() {
-        return this._queueSetMute(false);
+        return this._queueSetMuted(false);
     }
 
     /**
-     * Initializes a new Promise to execute {@link _setMute}. May be called
-     * multiple times in a row and the invocations of {@link _setMute} and,
-     * consequently, {@link mute} and/or {@link unmute} will be resolved in a
+     * Initializes a new Promise to execute {@link #_setMuted}. May be called
+     * multiple times in a row and the invocations of {@link #_setMuted} and,
+     * consequently, {@link #mute} and/or {@link #unmute} will be resolved in a
      * serialized fashion.
      *
-     * @param {boolean} mute - Whether to mute or unmute this track.
+     * @param {boolean} muted - The value to invoke <tt>_setMuted</tt> with.
      * @returns {Promise}
      */
-    _queueSetMute(mute) {
-        const setMute = this._setMute.bind(this, mute);
+    _queueSetMuted(muted) {
+        const setMuted = this._setMuted.bind(this, muted);
 
-        this._prevSetMute = this._prevSetMute.then(setMute, setMute);
+        this._prevSetMuted = this._prevSetMuted.then(setMuted, setMuted);
 
-        return this._prevSetMute;
+        return this._prevSetMuted;
     }
 
     /**
-     * Mutes / unmutes the track.
+     * Mutes / unmutes this track.
      *
-     * @param {boolean} mute - If true the track will be muted. Otherwise the
-     * track will be unmuted.
+     * @param {boolean} muted - If <tt>true</tt>, this track will be muted;
+     * otherwise, this track will be unmuted.
      * @private
      * @returns {Promise}
      */
-    _setMute(mute) {
-        if (this.isMuted() === mute) {
+    _setMuted(muted) {
+        if (this.isMuted() === muted) {
             return Promise.resolve();
         }
 
         let promise = Promise.resolve();
 
-        this.inMuteOrUnmuteProgress = true;
+        /**
+         * Set to <tt>true</tt> when there's ongoing "mute/unmute" operation in
+         * progress. Used by {@link LocalSdpMunger}.
+         *
+         * @public
+         * @type {boolean}
+         */
+        this.setMutedInProgress = true;
 
         // A function that will print info about muted status transition
-        const logMuteInfo = () => logger.info(`Mute ${this}: ${mute}`);
+        const logMuteInfo = () => logger.info(`Mute ${this}: ${muted}`);
 
         if (this.isAudioTrack()
-            || this.videoType === VideoType.DESKTOP
-            || !RTCBrowserType.doesVideoMuteByStreamRemove()) {
+                || this.videoType === VideoType.DESKTOP
+                || !RTCBrowserType.doesVideoMuteByStreamRemove()) {
             logMuteInfo();
             if (this.track) {
-                this.track.enabled = !mute;
+                this.track.enabled = !muted;
             }
-        } else if (mute) {
+        } else if (muted) {
             promise = new Promise((resolve, reject) => {
                 logMuteInfo();
-                this._removeStreamFromConferenceAsMute(() => {
-                    // FIXME: Maybe here we should set the SRC for the
-                    // containers to something
-                    // We don't want any events to be fired on this stream
-                    this._unregisterHandlers();
-                    this._stopMediaStream();
-                    this._setStream(null);
-                    resolve();
-                }, err => {
-                    reject(err);
-                });
+                this._removeStreamFromConferenceAsMute(
+                    () => {
+                        // FIXME: Maybe here we should set the SRC for the
+                        // containers to something
+                        // We don't want any events to be fired on this stream
+                        this._unregisterHandlers();
+                        this._stopStream();
+                        this._setStream(null);
+                        resolve();
+                    },
+                    reject);
             });
         } else {
             logMuteInfo();
@@ -397,8 +388,7 @@ export default class JitsiLocalTrack extends JitsiTrack {
                             this.videoType = streamInfo.videoType;
                         }
                     } else {
-                        throw new JitsiTrackError(
-                            JitsiTrackErrors.TRACK_NO_STREAM_FOUND);
+                        throw new JitsiTrackError(TRACK_NO_STREAM_FOUND);
                     }
 
                     this.containers = this.containers.map(
@@ -409,17 +399,17 @@ export default class JitsiLocalTrack extends JitsiTrack {
         }
 
         return promise
-            .then(() => this._sendMuteStatus(mute))
-            .then(() => {
-                this.inMuteOrUnmuteProgress = false;
-            }, error => {
-                this.inMuteOrUnmuteProgress = false;
-
-                throw error;
-            })
-            .then(() => {
-                this.emit(JitsiTrackEvents.TRACK_MUTE_CHANGED, this);
-            });
+            .then(() => this._sendMuteStatus(muted))
+            .then(
+                /* onFulfilled */ () => {
+                    this.setMutedInProgress = false;
+                },
+                /* onRejected */ error => {
+                    this.setMutedInProgress = false;
+
+                    throw error;
+                })
+            .then(() => this.emit(TRACK_MUTE_CHANGED, this));
     }
 
     /**
@@ -503,7 +493,7 @@ export default class JitsiLocalTrack extends JitsiTrack {
         }
 
         if (this.stream) {
-            this._stopMediaStream();
+            this._stopStream();
             this.detach();
         }
 
@@ -648,10 +638,23 @@ export default class JitsiLocalTrack extends JitsiTrack {
     /**
      * Stops the associated MediaStream.
      */
-    _stopMediaStream() {
-        this.stopStreamInProgress = true;
-        RTCUtils.stopMediaStream(this.stream);
-        this.stopStreamInProgress = false;
+    _stopStream() {
+
+        /**
+         * Indicates that we are executing {@link #_stopStream} i.e.
+         * {@link RTCUtils#stopMediaStream} for the <tt>MediaStream</tt>
+         * associated with this <tt>JitsiTrack</tt> instance.
+         *
+         * @private
+         * @type {boolean}
+         */
+        this._stopStreamInProgress = true;
+
+        try {
+            RTCUtils.stopMediaStream(this.stream);
+        } finally {
+            this._stopStreamInProgress = false;
+        }
     }
 
     /**
@@ -685,8 +688,9 @@ export default class JitsiLocalTrack extends JitsiTrack {
      * @returns {boolean} true if an issue is detected and false otherwise
      */
     _checkForCameraIssues() {
-        if (!this.isVideoTrack() || this.stopStreamInProgress
-            || this.videoType === VideoType.DESKTOP) {
+        if (!this.isVideoTrack()
+                || this._stopStreamInProgress
+                || this.videoType === VideoType.DESKTOP) {
             return false;
         }
 

modules/RTC/JitsiTrack.js

@@ -62,7 +62,7 @@ export default class JitsiTrack extends EventEmitter {
     /**
      * Represents a single media track (either audio or video).
      * @constructor
-     * @param rtc the rtc instance
+     * @param conference the rtc instance
      * @param stream the WebRTC MediaStream instance
      * @param track the WebRTC MediaStreamTrack instance, must be part of
      * the given <tt>stream</tt>.
@@ -72,12 +72,12 @@ export default class JitsiTrack extends EventEmitter {
      * @param videoType the VideoType for this track if any
      */
     constructor(
-        conference,
-        stream,
-        track,
-        streamInactiveHandler,
-        trackMediaType,
-        videoType) {
+            conference,
+            stream,
+            track,
+            streamInactiveHandler,
+            trackMediaType,
+            videoType) {
         super();
 
         // aliases for addListener/removeListener
@@ -90,7 +90,6 @@ export default class JitsiTrack extends EventEmitter {
          */
         this.containers = [];
         this.conference = conference;
-        this.stream = stream;
         this.audioLevel = -1;
         this.type = trackMediaType;
         this.track = track;
@@ -108,11 +107,14 @@ export default class JitsiTrack extends EventEmitter {
 
         /**
          * The inactive handler which will be triggered when the underlying
-         * media stream ends.
+         * <tt>MediaStream</tt> ends.
+         *
+         * @private
          * @type {Function}
          */
         this._streamInactiveHandler = streamInactiveHandler;
-        this._bindInactiveHandler(streamInactiveHandler);
+
+        this._setStream(stream);
     }
 
     /* eslint-enable max-params */
@@ -148,7 +150,7 @@ export default class JitsiTrack extends EventEmitter {
         }
 
         if (this.stream) {
-            // FIXME why only video tacks ?
+            // FIXME Why only video tracks?
             for (const track of this.stream.getVideoTracks()) {
                 track[trackHandler2Prop[type]] = handler;
             }
@@ -168,7 +170,7 @@ export default class JitsiTrack extends EventEmitter {
         }
 
         for (const type of this.handlers.keys()) {
-            // FIXME why only video tracks ?
+            // FIXME Why only video tracks?
             for (const videoTrack of this.stream.getVideoTracks()) {
                 videoTrack[trackHandler2Prop[type]] = undefined;
             }
@@ -181,21 +183,28 @@ export default class JitsiTrack extends EventEmitter {
     /**
      * Sets the stream property of JitsiTrack object and sets all stored
      * handlers to it.
+     *
      * @param {MediaStream} stream the new stream.
+     * @protected
      */
     _setStream(stream) {
         if (this.stream === stream) {
-            logger.warn(`Attempt to set the same stream twice on ${this}`);
-
             return;
         }
 
         this.stream = stream;
-        for (const type of this.handlers.keys()) {
-            this._setHandler(type, this.handlers.get(type));
-        }
-        if (this._streamInactiveHandler && this.stream) {
-            this._bindInactiveHandler(this._streamInactiveHandler);
+
+        // TODO Practically, that's like the opposite of _unregisterHandlers
+        // i.e. may be abstracted into a function/method called
+        // _registerHandlers for clarity and easing the maintenance of the two
+        // pieces of source code.
+        if (this.stream) {
+            for (const type of this.handlers.keys()) {
+                this._setHandler(type, this.handlers.get(type));
+            }
+            if (this._streamInactiveHandler) {
+                this._bindInactiveHandler(this._streamInactiveHandler);
+            }
         }
     }
 
@@ -437,7 +446,6 @@ export default class JitsiTrack extends EventEmitter {
         const streamId = this.getStreamId();
         const trackId = this.getTrackId();
 
-
         return streamId && trackId ? `${streamId} ${trackId}` : null;
     }
 

modules/RTC/LocalSdpMunger.js

@@ -65,19 +65,17 @@ export default class LocalSdpMunger {
         let modified = false;
 
         for (const videoTrack of localVideos) {
-            const isMuted = videoTrack.isMuted();
-            const muteInProgress = videoTrack.inMuteOrUnmuteProgress;
-            const shouldFakeSdp = isMuted || muteInProgress;
+            const muted = videoTrack.isMuted();
+            const { setMutedInProgress } = videoTrack;
+            const shouldFakeSdp = muted || setMutedInProgress;
 
             logger.debug(
-                `${this.tpc} ${videoTrack
-                 } isMuted: ${isMuted
-                 }, is mute in progress: ${muteInProgress
-                 } => should fake sdp ? : ${shouldFakeSdp}`);
+                `${this.tpc} ${videoTrack} muted: ${muted
+                    }, is mute/unmute in progress: ${setMutedInProgress
+                    } => should fake sdp ? : ${shouldFakeSdp}`);
 
             if (!shouldFakeSdp) {
-                // eslint-disable-next-line no-continue
-                continue;
+                continue; // eslint-disable-line no-continue
             }
 
             // Inject removed SSRCs
@@ -90,8 +88,7 @@ export default class LocalSdpMunger {
                 logger.error(
                     `No SSRCs stored for: ${videoTrack} in ${this.tpc}`);
 
-                // eslint-disable-next-line no-continue
-                continue;
+                continue; // eslint-disable-line no-continue
             }
 
             modified = true;
@@ -99,18 +96,16 @@ export default class LocalSdpMunger {
             // We need to fake sendrecv.
             // NOTE the SDP produced here goes only to Jicofo and is never set
             // as localDescription. That's why
-            // {@link TraceablePeerConnection.mediaTransferActive} is ignored
-            // here.
+            // TraceablePeerConnection.mediaTransferActive is ignored here.
             videoMLine.direction = 'sendrecv';
 
             // Check if the recvonly has MSID
             const primarySSRC = requiredSSRCs[0];
 
-            // FIXME the cname could come from the stream, but may
-            // turn out to be too complex. It is fine to come up
-            // with any value, as long as we only care about
-            // the actual SSRC values when deciding whether or not
-            // an update should be sent
+            // FIXME The cname could come from the stream, but may turn out to
+            // be too complex. It is fine to come up with any value, as long as
+            // we only care about the actual SSRC values when deciding whether
+            // or not an update should be sent.
             const primaryCname = `injected-${primarySSRC}`;
 
             for (const ssrcNum of requiredSSRCs) {
@@ -119,8 +114,8 @@ export default class LocalSdpMunger {
 
                 // Inject
                 logger.debug(
-                    `${this.tpc} injecting video SSRC: `
-                        + `${ssrcNum} for ${videoTrack}`);
+                    `${this.tpc} injecting video SSRC: ${ssrcNum} for ${
+                        videoTrack}`);
                 videoMLine.addSSRCAttribute({
                     id: ssrcNum,
                     attribute: 'cname',