Issue 394 - hit statistics tool data collection (core)

lib/downloader.js

Index: lib/downloader.js
===================================================================
--- a/lib/downloader.js
+++ b/lib/downloader.js
@@ -31,11 +31,13 @@
  * @param {Function} dataSource  Function that will yield downloadable objects on each check
  * @param {Integer} initialDelay  Number of milliseconds to wait before the first check
  * @param {Integer} checkInterval  Interval between the checks
+ * @param {function} downloadCallback optional function that's being called when something needs to be downloaded
  * @constructor
  */
-let Downloader = exports.Downloader = function Downloader(dataSource, initialDelay, checkInterval)
+let Downloader = exports.Downloader = function Downloader(dataSource, initialDelay, checkInterval, downloadCallback)

[saroyanm, 2016/03/18 18:24:48]

Maybe this changes will be reverted if you agree with my comment in another
related review, about updating the Downloader.

[Wladimir Palant, 2016/03/23 11:06:01]

You are still duplicating quite a bit of the logic in filterHits.js - while not
providing all the functionality (e.g. mozBackgroundRequest isn't being set,
reporting of the error cause isn't as detailed). I think rather than replacing
_download you should provide an optional callback that will generate the POST
data. This callback shouldn't be set in the constructor either (there is only so
many positional parameters that can be provided without the code becoming
unreadable), you can configure it after creating the object:

  downloader.generateRequestData = ...;

This would work similarly to onDownloadError callback. If the callback is
present a POST request should be created, async behavior can be implemented via
promises:

  if (this.generateRequestData)
  {
    Promise.resolve(this.generateRequestData(downloadable))
           .then(data => request.send(data));  
  }
  else
    request.send(null);

Similarly you could configure valid response codes, the constructor would then
initialize it this field with 200:

  this.validResponses = [200];

And your code would add to it:

  downloader.validResponses.push(204);

[saroyanm, 2016/04/06 15:12:49]

Thanks for detailed explanation, Done.

 {
   this.dataSource = dataSource;
+  this.downloadCallback = downloadCallback || this._download;
   this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
   this._timer.initWithCallback(function()
   {
@@ -64,6 +66,12 @@
   dataSource: null,
 
   /**
+   * Function that's being called when something needs to be downloaded.
+   * @type Function
+   */
+  downloadCallback: null,
+
+  /**
    * Maximal time interval that the checks can be left out until the soft
    * expiration interval increases.
    * @type Integer
@@ -150,7 +158,7 @@
       if (downloadable.lastError && now - downloadable.lastError < this.minRetryInterval)
         continue;
 
-      this._download(downloadable, 0);
+      this.downloadCallback(downloadable, 0);
     }
   },