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 15 matching lines @@
 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;
 
 class Downloader
 {
   /**
    * Creates a new downloader instance.
    * @param {function} dataSource
-   *   function that will yield downloadable objects on each check

[Manish Jethani, 2018/08/23 15:03:39]

This should still be capital F? It's not the type name here, just prose (similar
to "Number of milliseconds ..." below).

[Jon Sonesen, 2018/08/23 16:41:46]

Done. I used sed replacement carelessly to do the change, my bad.

+   *   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.
@@ skipping 15 matching lines @@
     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}

[Manish Jethani, 2018/08/23 15:03:39]

Let's change the type here to `function?` (also below wherever it's set to null
by default).

[Jon Sonesen, 2018/08/23 16:41:46]

Done.

+     * @type {function?}
      */
     this.onExpirationChange = null;
 
     /**
      * Callback to be triggered whenever a download starts.
-     * @type {function}
+     * @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}
+     * @type {function?}
      */
     this.onDownloadSuccess = null;
 
     /**
      * Callback to be triggered whenever a download fails.
-     * @type {function}
+     * @type {function?}
      */
     this.onDownloadError = null;
 
     /**
-     * function that will yield downloadable objects on each check.

[Manish Jethani, 2018/08/23 15:03:39]

This too.

[Jon Sonesen, 2018/08/23 16:41:46]

Done.

+     * Function that will yield downloadable objects on each check.
      * @type {function}
      */
     this.dataSource = dataSource;
 
     /**
      * Timer triggering the downloads.
      * @type {nsITimer}
      */
     this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
     this._timer.initWithCallback(() =>
@@ skipping 60 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];
   }
 }
 
+exports.Downloader = Downloader;
+
 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}

[Manish Jethani, 2018/08/23 15:03:39]

Since redirectURL can be null (starts out as null by default, in fact), let's
change the type here to `string?`.

[Jon Sonesen, 2018/08/23 16:41:46]

Done.

+     * @type {string?}
      */
     this.redirectURL = null;
 
     /**
      * Time of last download error or 0 if the last download was successful.
      * @type {number}
      */
     this.lastError = 0;
 
     /**
@@ skipping 28 matching lines @@
     this.downloadCount = 0;
 
     /**
      * URL that has to be requested for the object.
      * @type {string}
      */
     this.url = url;
   }
 }
 
-exports.Downloader = Downloader;
 exports.Downloadable = Downloadable;

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

I thought having a blank line, the addition to exports and another blank line
looked a bit weird, and I kinda like having definitions complete, then followed
by assignments as far as readability goes.

[Manish Jethani, 2018/08/23 15:03:39]

Sure, this is how I do it too for my personal projects, but here we should try
to be consistent with the rest of the codebase.

[Jon Sonesen, 2018/08/23 16:41:46]

Done.