Improve Synchronizer functionality

lib/downloader.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 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 50 matching lines @@
    * Function that will yield downloadable objects on each check.
    * @type Function
    */
   dataSource: null,
 
   /**
    * Maximal time interval that the checks can be left out until the soft
    * expiration interval increases.
    * @type Integer
    */
-  maxAbsenseInterval: 1 * MILLIS_IN_DAY,

[Thomas Greiner, 2013/07/25 15:21:37]

typo: maxAbsenceInterval

+  maxAbsenceInterval: 1 * MILLIS_IN_DAY,
 
   /**
    * Minimal time interval before retrying a download after an error.
    * @type Integer
    */
   minRetryInterval: 1 * MILLIS_IN_DAY,
 
   /**
    * Maximal allowed expiration interval, larger expiration intervals will be
    * corrected.
@@ skipping 33 matching lines @@
   onDownloadError: null,
 
   /**
    * Checks whether anything needs downloading.
    */
   _doCheck: function()
   {
     let now = Date.now();
     for each (let downloadable in this.dataSource())
     {
-      if (downloadable.lastCheck && now - downloadable.lastCheck > this.maxAbsenseInterval)
+      if (downloadable.lastCheck && now - downloadable.lastCheck > this.maxAbsenceInterval)
       {
         // No checks for a long time interval - user must have been offline, e.g.
         // during a weekend. Increase soft expiration to prevent load peaks on the
         // server.
         downloadable.softExpiration += now - downloadable.lastCheck;
       }
       downloadable.lastCheck = now;
 
       // Sanity check: do expiration times make sense? Make sure people changing
       // system clock don't get stuck with outdated subscriptions.
@@ skipping 28 matching lines @@
 
   /**
    * Checks whether an address is currently being downloaded.
    */
   isDownloading: function(/**String*/ url) /**Boolean*/
   {
     return url in this._downloading;
   },
 
   /**
-   * Starts a download.
-   * @param {Downloadable} url  the object to be downloaded

[Felix Dahlke, 2013/07/25 13:16:02]

The doc strings are a bit inconsistent. The one above specifies the type inline,
this one uses @param.

[Wladimir Palant, 2013/11/05 06:40:42]

I generally use @param only when there is a need to explain the parameter.
However, inline specification seems fine here if the overall description is
slightly adjusted - and the @param tag gets the name of the parameter wrong
anyway.

-   */
-  download: function(downloadable)
+   * Starts downloading for an object.
+   */
+  download: function(/**Downloadable*/ 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.
    */
   getDownloadUrl: function(/**Downloadable*/ downloadable)  /** String*/
   {
-    let {addonName, addonVersion, application} = require("info");
+    let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info");
     let url = downloadable.redirectURL || downloadable.url;
     if (url.indexOf("?") >= 0)
       url += "&";
     else
       url += "?";
     url += "addonName=" + encodeURIComponent(addonName) +
         "&addonVersion=" + encodeURIComponent(addonVersion) +
         "&application=" + encodeURIComponent(application) +
+        "&applicationVersion=" + encodeURIComponent(applicationVersion) +
+        "&platform=" + encodeURIComponent(platform) +
+        "&platformVersion=" + encodeURIComponent(platformVersion) +
         "&lastVersion=" + encodeURIComponent(downloadable.lastVersion);
     return url;
   },
 
   _download: function(downloadable, redirects)
   {
-    if (downloadable.url in this._downloading)

[Thomas Greiner, 2013/07/25 15:21:37]

You could use this.isDownloading(downloadable.url) instead.

[Wladimir Palant, 2013/11/05 06:40:42]

It was hard to convince myself that microoptimizing wasn't worth it here - fixed
:)

+    if (this.isDownloading(downloadable.url))
       return;
 
-    let downloadURL = this.getDownloadUrl(downloadable);

[Felix Dahlke, 2013/07/25 13:16:02]

Inconsistent camel casing :P

+    let downloadUrl = this.getDownloadUrl(downloadable);
     let request = null;
 
     let errorCallback = function errorCallback(error)
     {
       let channelStatus = -1;
       try
       {
         channelStatus = request.channel.status;
       } catch (e) {}
 
-      let responseStatus = "";
-      try
-      {
-        responseStatus = request.channel.QueryInterface(Ci.nsIHttpChannel).responseStatus;
-      } catch (e) {}
+      let responseStatus = request.status;
 
       Cu.reportError("Adblock Plus: Downloading URL " + downloadable.url + " failed (" + error + ")\n" +
-                     "Download address: " + downloadURL + "\n" +
+                     "Download address: " + downloadUrl + "\n" +
                      "Channel status: " + channelStatus + "\n" +
                      "Server response: " + responseStatus);
 
       if (this.onDownloadError)
       {
         // Allow one extra redirect if the error handler gives us a redirect URL
         let redirectCallback = null;
         if (redirects <= this.maxRedirects)
         {
           redirectCallback = function redirectCallback(url)
           {
             downloadable.redirectURL = url;
             this._download(downloadable, redirects + 1);
           }.bind(this);
         }
 
-        this.onDownloadError(downloadable, downloadURL, error, channelStatus, responseStatus, redirectCallback);
+        this.onDownloadError(downloadable, downloadUrl, error, channelStatus, responseStatus, redirectCallback);
       }
     }.bind(this);
 
     try
     {
-      request = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
+      request = new XMLHttpRequest();
       request.mozBackgroundRequest = true;
-      request.open("GET", downloadURL);
+      request.open("GET", downloadUrl);
     }
     catch (e)
     {
       errorCallback("synchronize_invalid_url");
       return;
     }
 
     try {
       request.overrideMimeType("text/plain");
       request.channel.loadFlags = request.channel.loadFlags |
@@ skipping 37 matching lines @@
         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);
+
     this._downloading[downloadable.url] = true;
     if (this.onDownloadStarted)
       this.onDownloadStarted(downloadable);

[Thomas Greiner, 2013/07/25 15:21:37]

Seems weird to me that onDownloadStarted is fired before download actually
starts.

[Wladimir Palant, 2013/11/05 06:40:42]

Well, nothing really wrong with it (assuming that request.send() won't throw)
but reordering this won't hurt either...

-    request.send(null);
   },
 
   /**
    * Produces a soft and a hard expiration interval for a given supplied
    * expiration interval.
    * @return {Array} soft and hard expiration interval
    */
   processExpirationInterval: function(/**Integer*/ interval)
   {
     interval = Math.min(Math.max(interval, 0), this.maxExpirationInterval);
@@ skipping 51 matching lines @@
    * @type Integer
    */
   softExpiration: 0,
 
   /**
    * Hard expiration interval, this is fixed.
    * @type Integer
    */
   hardExpiration: 0,
 };