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,103 @@
 
 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.
- * @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,
-
-  /**
-   * Set containing the URLs of objects currently being downloaded.
-   * @type {Set.<string>}
-   */
-  _downloading: null,
-
-  /**
-   * Function that will yield downloadable objects on each check.
-   * @type {Function}
+   * Creates a new downloader instance.
+   * @param {Function} dataSource

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

Another JSDoc convention that I think that we should follow is to use the
lower-case type name for primitive types [1]. In this case, let's make it
`function` instead of `Function` (throughout the file).

[1]:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Data_...

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

Done.

+   *   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
    */
-  dataSource: null,
+  constructor(dataSource, initialDelay, checkInterval)
+  {
+    /**
+     * 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;
 
-  /**
-   * Maximal time interval that the checks can be left out until the soft
-   * expiration interval increases.
-   * @type {number}
-   */
-  maxAbsenceInterval: 1 * MILLIS_IN_DAY,
+    /**
+     * Maximal allowed expiration interval; larger expiration intervals will be
+     * corrected.
+     * @type {number}
+     */
+    this.maxExpirationInterval = 14 * MILLIS_IN_DAY;
 
-  /**
-   * Minimal time interval before retrying a download after an error.
-   * @type {number}
-   */
-  minRetryInterval: 1 * MILLIS_IN_DAY,
+    /**
+     * Maximal number of redirects before the download is considered as failed.
+     * @type {number}
+     */
+    this.maxRedirects = 5;
+
+    /**
+     * Called whenever expiration intervals for an object need to be adapted.
+     * @type {Function}
+     */
+    this.onExpirationChange = null;
 
-  /**
-   * Maximal allowed expiration interval, larger expiration intervals will be
-   * corrected.
-   * @type {number}
-   */
-  maxExpirationInterval: 14 * MILLIS_IN_DAY,
+    /**
+     * Callback to be triggered whenever a download starts.
+     * @type {Function}
+     */
+    this.onDownloadStarted = null;
 
-  /**
-   * Maximal number of redirects before the download is considered as failed.
-   * @type {number}
-   */
-  maxRedirects: 5,
+    /**
+     * 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;
 
-  /**
-   * Called whenever expiration intervals for an object need to be adapted.
-   * @type {Function}
-   */
-  onExpirationChange: null,
+    /**
+     * Callback to be triggered whenever a download fails.
+     * @type {Function}
+     */
+    this.onDownloadError = null;
 
-  /**
-   * Callback to be triggered whenever a download starts.
-   * @type {Function}
-   */
-  onDownloadStarted: null,
+    /**
+     * Function that will yield downloadable objects on each check.
+     * @type {Function}
+     */
+    this.dataSource = dataSource;
 
-  /**
-   * 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,
+    /**
+     * Timer triggering the downloads.
+     * @type {nsITimer}
+     */
+    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);
 
-  /**
-   * Callback to be triggered whenever a download fails.
-   * @type {Function}
-   */
-  onDownloadError: null,
+    /**
+     * Set containing the URLs of objects currently being downloaded.
+     * @type {Set.<string>}
+     */
+    this._downloading = new Set();
+  }
 
   /**
    * Checks whether anything needs downloading.
    */
   _doCheck()
   {
     let now = Date.now();
     for (let downloadable of this.dataSource())
@@ -161,45 +156,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 +214,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 +332,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];
   }
-};
+}
+exports.Downloader = Downloader;
 
-/**
- * An object that can be downloaded by the downloadable
- * @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}
+   * An object that can be downloaded by the downloadable

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

I think this should be "Creates an object that can be downloaded by the
downloader." (Note: downloader, not downloadable (that's a mistake), and a
period at the end.)

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

Done.

+   * @param {string} url  URL that has to be requested for the object
    */
-  url: null,
-
-  /**
-   * URL that the download was redirected to if any.
-   * @type {string}
-   */
-  redirectURL: null,
+  constructor(url)
+  {
+    /**
+     * URL that has to be requested for the object.
+     * @type {string}
+     */
+    this.url = null;

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

Let's also just assign `this.url` directly at the end of the constructor and
move the JSDoc comment there.

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

Done.

 
-  /**
-   * Time of last download error or 0 if the last download was successful.
-   * @type {number}
-   */
-  lastError: 0,
+    /**
+     * URL that the download was redirected to if any.
+     * @type {string}
+     */
+    this.redirectURL = null;
 
-  /**
-   * Time of last check whether the object needs downloading.
-   * @type {number}
-   */
-  lastCheck: 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;
 
-  /**
-   * Object version corresponding to the last successful download.
-   * @type {number}
-   */
-  lastVersion: 0,
+    /**
+     * Object version corresponding to the last successful download.
+     * @type {number}
+     */
+    this.lastVersion = 0;
 
-  /**
-   * Soft expiration interval, will increase if no checks are performed for a
-   * while.
-   * @type {number}
-   */
-  softExpiration: 0,
+    /**
+     * Soft expiration interval; will increase if no checks are performed for a
+     * while.
+     * @type {number}
+     */
+    this.softExpiration = 0;
 
-  /**
-   * Hard expiration interval, this is fixed.
-   * @type {number}
-   */
-  hardExpiration: 0,
+    /**
+     * Hard expiration interval; this is fixed.
+     * @type {number}
+     */
+    this.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;