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

lib/downloader.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 10 matching lines @@
  * @fileOverview Downloads a set of URLs in regular time intervals.
  */
 
 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;
 
-/**
- * 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
- */
 class 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(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 allowed expiration interval; larger expiration intervals will be
+     * corrected.
+     * @type {number}
+     */
+    this.maxExpirationInterval = 14 * 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;
+
+    /**
+     * Callback to be triggered whenever a download starts.
+     * @type {function?}
+     */
+    this.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 fails.
+     * @type {function?}
+     */
+    this.onDownloadError = null;
+
+    /**
+     * Function that will yield downloadable objects on each check.
+     * @type {function}
+     */
+    this.dataSource = dataSource;
+
     /**
      * 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>}
-     */
-    this._downloading = 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}
-     */
-    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 allowed expiration interval; larger expiration intervals will be
-     * corrected.
-     * @type {number}
-     */
-    this.maxExpirationInterval = 14 * 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;
-
-    /**
-     * Callback to be triggered whenever a download starts.
-     * @type {Function}
-     */
-    this.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 fails.
-     * @type {Function}
-     */
-    this.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);
+
+    /**
+     * 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())
@@ skipping 41 matching lines @@
    * Stops the periodic checks.
    */
   cancel()
   {
     this._timer.cancel();
   }
 
   /**
    * Checks whether an address is currently being downloaded.
    * @param {string} url
-   * @return {boolean}
+   * @returns {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}
+   * @returns {string}
    */
   getDownloadUrl(downloadable)
   {
     const {addonName, addonVersion, application, applicationVersion,
            platform, platformVersion} = require("info");
     let url = downloadable.redirectURL || downloadable.url;
     if (url.includes("?"))
       url += "&";
     else
       url += "?";
@@ skipping 127 matching lines @@
 
     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
+   * @returns {Array.<number>} 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
- */
 class Downloadable
 {
+  /**
+   * Creates an object that can be downloaded by the downloader.
+   * @param {string} url  URL that has to be requested for the object
+   */
   constructor(url)
   {
+    /**
+     * 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}
+     */
+    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}
+     */
+    this.lastVersion = 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}
+     */
+    this.hardExpiration = 0;
+
+    /**
+     * Number indicating how often the object was downloaded.
+     * @type {number}
+     */
+    this.downloadCount = 0;
+
     /**
      * 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}
-     */
-    this.redirectURL = null;
-
-    /**
-     * 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}
-     */
-    this.lastVersion = 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}
-     */
-    this.hardExpiration = 0;
-
-    /**
-     * Number indicating how often the object was downloaded.
-     * @type {number}
-     */
-    this.downloadCount = 0;
     this.url = url;
   }
 }
+
 exports.Downloadable = Downloadable;