Issue 394 - hit statistics tool data collection (core)

lib/downloader.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 13 matching lines @@
 let MILLIS_IN_SECOND = exports.MILLIS_IN_SECOND = 1000;
 let MILLIS_IN_MINUTE = exports.MILLIS_IN_MINUTE = 60 * MILLIS_IN_SECOND;
 let MILLIS_IN_HOUR = exports.MILLIS_IN_HOUR = 60 * MILLIS_IN_MINUTE;
 let MILLIS_IN_DAY = exports.MILLIS_IN_DAY = 24 * MILLIS_IN_HOUR;
 
 /**
  * Creates a new downloader instance.
  * @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, 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.

+let Downloader = exports.Downloader = function Downloader(dataSource, initialDelay, checkInterval)
 {
   this.dataSource = dataSource;
-  this.downloadCallback = downloadCallback || this._download;
   this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
   this._timer.initWithCallback(function()
   {
     this._timer.delay = checkInterval;
     this._doCheck();
   }.bind(this), initialDelay, Ci.nsITimer.TYPE_REPEATING_SLACK);
   this._downloading = Object.create(null);
 }
 Downloader.prototype =
 {
   /**
    * Timer triggering the downloads.
    * @type nsITimer
    */
   _timer: null,
 
   /**
    * Map containing the URLs of objects currently being downloaded as its keys.
    */
   _downloading: null,
 
   /**
    * Function that will yield downloadable objects on each check.
    * @type Function
    */
   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
    */
   maxAbsenceInterval: 1 * MILLIS_IN_DAY,
 
   /**
    * Minimal time interval before retrying a download after an error.
    * @type Integer
    */
@@ skipping 29 matching lines @@
    * 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
    */
   onDownloadError: null,
+
+  /**
+   * Callback to be triggered for POST request data generation.
+   * @type Function
+   */
+  generateRequestData: null,
+
+  /**
+   * List of valid responses.
+   * @type Array
+   */
+  validResponses: [200],
 
   /**
    * Checks whether anything needs downloading.
    */
   _doCheck: function()
   {
     let now = Date.now();
     for (let downloadable of this.dataSource())
     {
       if (downloadable.lastCheck && now - downloadable.lastCheck > this.maxAbsenceInterval)
@@ skipping 17 matching lines @@
         this.onExpirationChange(downloadable);
 
       // Does that object need downloading?
       if (downloadable.softExpiration > now && downloadable.hardExpiration > now)
         continue;
 
       // Do not retry downloads too often
       if (downloadable.lastError && now - downloadable.lastError < this.minRetryInterval)
         continue;
 
-      this.downloadCallback(downloadable, 0);
+      this._download(downloadable, 0);
     }
   },
 
   /**
    * Stops the periodic checks.
    */
   cancel: function()
   {
     this._timer.cancel();
   },
@@ skipping 79 matching lines @@
         }
 
         this.onDownloadError(downloadable, downloadUrl, error, channelStatus, responseStatus, redirectCallback);
       }
     }.bind(this);
 
     try
     {
       request = new XMLHttpRequest();
       request.mozBackgroundRequest = true;
-      request.open("GET", downloadUrl);
+      request.open(this.generateRequestData ? "POST" : "GET", downloadUrl);
     }
     catch (e)
     {
       errorCallback("synchronize_invalid_url");
       return;
     }
 
     try {
       request.overrideMimeType("text/plain");
       request.channel.loadFlags = request.channel.loadFlags |
@@ skipping 19 matching lines @@
     }.bind(this), false);
 
     request.addEventListener("load", function(event)
     {
       if (onShutdown.done)
         return;
 
       delete this._downloading[downloadable.url];
 
       // Status will be 0 for non-HTTP requests
-      if (request.status && request.status != 200)
+      if (request.status && this.validResponses.indexOf(request.status) == -1)
       {
         errorCallback("synchronize_connection_error");
         return;
       }
 
       downloadable.downloadCount++;
 
       this.onDownloadSuccess(downloadable, request.responseText, errorCallback, function redirectCallback(url)
       {
         if (redirects >= this.maxRedirects)
           errorCallback("synchronize_connection_error");
         else
         {
           downloadable.redirectURL = url;
           this._download(downloadable, redirects + 1);
         }
       }.bind(this));
     }.bind(this), false);
 
-    request.send(null);
+    if (this.generateRequestData)
+    {
+      Promise.resolve(this.generateRequestData(downloadable))
+        .then(data => {
+          request.setRequestHeader("Content-Type", "application/json");
+          request.send(JSON.stringify(data));
+        }); 
+    }
+    else
+      request.send(null);
 
     this._downloading[downloadable.url] = true;
     if (this.onDownloadStarted)
       this.onDownloadStarted(downloadable);
   },
 
   /**
    * Produces a soft and a hard expiration interval for a given supplied
    * expiration interval.
    * @return {Array} soft and hard expiration interval
@@ skipping 61 matching lines @@
    * @type Integer
    */
   hardExpiration: 0,
   
   /**
    * Number indicating how often the object was downloaded.
    * @type Integer
    */
   downloadCount: 0,
 };