Issue 6741 - Use ES2015 classes in lib/downloader.js

lib/downloader.js

Index: lib/downloader.js
===================================================================
--- a/lib/downloader.js
+++ b/lib/downloader.js
@@ -23,108 +23,107 @@
 
 const {Utils} = require("utils");
 
 const MILLIS_IN_SECOND = exports.MILLIS_IN_SECOND = 1000;
 const MILLIS_IN_MINUTE = exports.MILLIS_IN_MINUTE = 60 * MILLIS_IN_SECOND;
 const MILLIS_IN_HOUR = exports.MILLIS_IN_HOUR = 60 * MILLIS_IN_MINUTE;
 const MILLIS_IN_DAY = exports.MILLIS_IN_DAY = 24 * MILLIS_IN_HOUR;
 
-let Downloader =
 /**
  * Creates a new downloader instance.

[Manish Jethani, 2018/08/21 03:56:02]

This JSDoc comment is in fact for the constructor; we should move it to just
above `constructor()`

[Jon Sonesen, 2018/08/21 19:10:01]

Acknowledged. I wasn't sure about that so thanks for clarifying!

  * @param {Function} dataSource
  *   Function that will yield downloadable objects on each check
  * @param {number} initialDelay
  *   Number of milliseconds to wait before the first check
  * @param {number} checkInterval
  *   Interval between the checks
- * @constructor
  */
-exports.Downloader = function(dataSource, initialDelay, checkInterval)
-{
-  this.dataSource = dataSource;
-  this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
-  this._timer.initWithCallback(() =>
-  {
-    this._timer.delay = checkInterval;
-    this._doCheck();
-  }, initialDelay, Ci.nsITimer.TYPE_REPEATING_SLACK);
-  this._downloading = new Set();
-};
-Downloader.prototype =
+class Downloader
 {
-  /**
-   * Timer triggering the downloads.
-   * @type {nsITimer}
-   */
-  _timer: null,
+  constructor(dataSource, initialDelay, checkInterval)
+  {
+    /**
+     * Timer triggering the downloads.
+     * @type {nsITimer}

[Manish Jethani, 2018/08/21 03:56:02]

So you see there was always an inconsistency between the JSDoc and the line
below it: in this case, it says this property cannot be null (no `?` next to the
type), but we're setting it to null anyway. That's because the real
initialization happens in the constructor.

But now we can do this properly so that the initialization and the JSDoc comment
are in the same place. I think we should set this property once below and move
the JSDoc comment there.

I've been thinking of a convention: (1) first, properties with default values
like null, true, etc.; (2) next properties with values from the constructor
arguments; (3) last, properties with values assigned based on some internal
logic. It's rough, but we can use this for now.

[Jon Sonesen, 2018/08/21 19:10:01]

Yeah, I also wasn't sure about that either and I like the convention you
outlined, I considered that same convention but figured it could be clarified
here.

+     */
+    this._timer = null;
 
-  /**
-   * Set containing the URLs of objects currently being downloaded.
-   * @type {Set.<string>}
-   */
-  _downloading: null,
+    /**
+     * Set containing the URLs of objects currently being downloaded.
+     * @type {Set.<string>}
+     */
+    this._downloading = null;
 
-  /**
-   * Function that will yield downloadable objects on each check.
-   * @type {Function}
-   */
-  dataSource: null,
+    /**
+     * Function that will yield downloadable objects on each check.
+     * @type {Function}
+     */
+    this.dataSource = null;
 
-  /**
-   * Maximal time interval that the checks can be left out until the soft
-   * expiration interval increases.
-   * @type {number}
-   */
-  maxAbsenceInterval: 1 * MILLIS_IN_DAY,
+    /**
+     * Maximal time interval that the checks can be left out until the soft
+     * expiration interval increases.
+     * @type {number}
+     */
+    this.maxAbsenceInterval = 1 * MILLIS_IN_DAY;
+
+    /**
+     * Minimal time interval before retrying a download after an error.
+     * @type {number}
+     */
+    this.minRetryInterval = 1 * MILLIS_IN_DAY;
 
-  /**
-   * Minimal time interval before retrying a download after an error.
-   * @type {number}
-   */
-  minRetryInterval: 1 * MILLIS_IN_DAY,
+    /**
+     * Maximal allowed expiration interval; larger expiration intervals will be
+     * corrected.
+     * @type {number}
+     */
+    this.maxExpirationInterval = 14 * MILLIS_IN_DAY;
 
-  /**
-   * Maximal allowed expiration interval, larger expiration intervals will be
-   * corrected.
-   * @type {number}
-   */
-  maxExpirationInterval: 14 * MILLIS_IN_DAY,
+    /**
+     * Maximal number of redirects before the download is considered as failed.
+     * @type {number}
+     */
+    this.maxRedirects = 5;
 
-  /**
-   * Maximal number of redirects before the download is considered as failed.
-   * @type {number}
-   */
-  maxRedirects: 5,
+    /**
+     * Called whenever expiration intervals for an object need to be adapted.
+     * @type {Function}
+     */
+    this.onExpirationChange = null;
 
-  /**
-   * Called whenever expiration intervals for an object need to be adapted.
-   * @type {Function}
-   */
-  onExpirationChange: null,
+    /**
+     * Callback to be triggered whenever a download starts.
+     * @type {Function}
+     */
+    this.onDownloadStarted = null;
 
-  /**
-   * Callback to be triggered whenever a download starts.
-   * @type {Function}
-   */
-  onDownloadStarted: null,
+    /**
+     * Callback to be triggered whenever a download finishes successfully. The
+     * callback can return an error code to indicate that the data is wrong.
+     * @type {Function}
+     */
+    this.onDownloadSuccess = null;
 
-  /**
-   * Callback to be triggered whenever a download finishes successfully. The
-   * callback can return an error code to indicate that the data is wrong.
-   * @type {Function}
-   */
-  onDownloadSuccess: null,
+    /**
+     * Callback to be triggered whenever a download fails.
+     * @type {Function}
+     */
+    this.onDownloadError = null;
 
-  /**
-   * Callback to be triggered whenever a download fails.
-   * @type {Function}
-   */
-  onDownloadError: null,
+    this.dataSource = dataSource;
+    this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+    this._timer.initWithCallback(() =>
+    {
+      this._timer.delay = checkInterval;
+      this._doCheck();
+    }, initialDelay, Ci.nsITimer.TYPE_REPEATING_SLACK);
+    this._downloading = new Set();
+  }
 
   /**
    * Checks whether anything needs downloading.
    */
   _doCheck()
   {
     let now = Date.now();
     for (let downloadable of this.dataSource())
@@ -161,45 +160,45 @@
       if (downloadable.lastError &&
           now - downloadable.lastError < this.minRetryInterval)
       {
         continue;
       }
 
       this._download(downloadable, 0);
     }
-  },
+  }
 
   /**
    * Stops the periodic checks.
    */
   cancel()
   {
     this._timer.cancel();
-  },
+  }
 
   /**
    * Checks whether an address is currently being downloaded.
    * @param {string} url
    * @return {boolean}
    */
   isDownloading(url)
   {
     return this._downloading.has(url);
-  },
+  }
 
   /**
    * Starts downloading for an object.
    * @param {Downloadable} downloadable
    */
   download(downloadable)
   {
     // Make sure to detach download from the current execution context
     Utils.runAsync(this._download.bind(this, downloadable, 0));
-  },
+  }
 
   /**
    * Generates the real download URL for an object by appending various
    * parameters.
    * @param {Downloadable} downloadable
    * @return {string}
    */
   getDownloadUrl(downloadable)
@@ -219,17 +218,17 @@
         "&addonVersion=" + encodeURIComponent(addonVersion) +
         "&application=" + encodeURIComponent(application) +
         "&applicationVersion=" + encodeURIComponent(applicationVersion) +
         "&platform=" + encodeURIComponent(platform) +
         "&platformVersion=" + encodeURIComponent(platformVersion) +
         "&lastVersion=" + encodeURIComponent(downloadable.lastVersion) +
         "&downloadCount=" + encodeURIComponent(downloadCount);
     return url;
-  },
+  }
 
   _download(downloadable, redirects)
   {
     if (this.isDownloading(downloadable.url))
       return;
 
     let downloadUrl = this.getDownloadUrl(downloadable);
     let request = null;
@@ -337,86 +336,87 @@
       );
     });
 
     request.send(null);
 
     this._downloading.add(downloadable.url);
     if (this.onDownloadStarted)
       this.onDownloadStarted(downloadable);
-  },
+  }
 
   /**
    * Produces a soft and a hard expiration interval for a given supplied
    * expiration interval.
    * @param {number} interval
    * @return {Array} soft and hard expiration interval
    */
   processExpirationInterval(interval)
   {
     interval = Math.min(Math.max(interval, 0), this.maxExpirationInterval);
     let soft = Math.round(interval * (Math.random() * 0.4 + 0.8));
     let hard = interval * 2;
     let now = Date.now();
     return [now + soft, now + hard];
   }
-};
+}

[Manish Jethani, 2018/08/21 03:56:02]

Blank line after `}`

[Jon Sonesen, 2018/08/21 19:10:01]

Do you mean insert a blank line here?

[Manish Jethani, 2018/08/22 06:13:34]

Yes, I meant we should have a blank line after `class { ... }`

[Jon Sonesen, 2018/08/22 18:18:50]

Done.

+exports.Downloader = Downloader;
 
 /**
  * An object that can be downloaded by the downloadable

[Manish Jethani, 2018/08/21 03:56:02]

Same as above, this is JSDoc for the constructor.

[Jon Sonesen, 2018/08/21 19:10:00]

Acknowledged.

  * @param {string} url  URL that has to be requested for the object
- * @constructor
  */
-let Downloadable = exports.Downloadable = function Downloadable(url)
-{
-  this.url = url;
-};
-Downloadable.prototype =
+class Downloadable
 {
-  /**
-   * URL that has to be requested for the object.
-   * @type {string}
-   */
-  url: null,
+  constructor(url)
+  {
+    /**
+     * URL that has to be requested for the object.
+     * @type {string}
+     */
+    this.url = null;
 
-  /**
-   * URL that the download was redirected to if any.
-   * @type {string}
-   */
-  redirectURL: null,
+    /**
+     * URL that the download was redirected to if any.
+     * @type {string}
+     */
+    this.redirectURL = null;
 
-  /**
-   * Time of last download error or 0 if the last download was successful.
-   * @type {number}
-   */
-  lastError: 0,
+    /**
+     * Time of last download error or 0 if the last download was successful.
+     * @type {number}
+     */
+    this.lastError = 0;
+
+    /**
+     * Time of last check whether the object needs downloading.
+     * @type {number}
+     */
+    this.lastCheck = 0;
 
-  /**
-   * Time of last check whether the object needs downloading.
-   * @type {number}
-   */
-  lastCheck: 0,
+    /**
+     * Object version corresponding to the last successful download.
+     * @type {number}
+     */
+    this.lastVersion = 0;
 
-  /**
-   * Object version corresponding to the last successful download.
-   * @type {number}
-   */
-  lastVersion: 0,
+    /**
+     * Soft expiration interval; will increase if no checks are performed for a
+     * while.
+     * @type {number}
+     */
+    this.softExpiration = 0;
 
-  /**
-   * Soft expiration interval, will increase if no checks are performed for a
-   * while.
-   * @type {number}
-   */
-  softExpiration: 0,
+    /**
+     * Hard expiration interval; this is fixed.
+     * @type {number}
+     */
+    this.hardExpiration = 0;
 
-  /**
-   * Hard expiration interval, this is fixed.
-   * @type {number}
-   */
-  hardExpiration: 0,
-
-  /**
-   * Number indicating how often the object was downloaded.
-   * @type {number}
-   */
-  downloadCount: 0
-};
+    /**
+     * Number indicating how often the object was downloaded.
+     * @type {number}
+     */
+    this.downloadCount = 0;
+    this.url = url;
+  }
+}
+exports.Downloadable = Downloadable;