Issue 537 - Mark first few downloads

lib/downloader.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 173 matching lines @@
    * parameters.
    */
   getDownloadUrl: function(/**Downloadable*/ downloadable)  /** String*/
   {
     let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info");
     let url = downloadable.redirectURL || downloadable.url;
     if (url.indexOf("?") >= 0)
       url += "&";
     else
       url += "?";
+    // We limit the download count to 4+ to keep the request anonymized
+    let downloadCount = downloadable.downloadCount;
+    if (downloadCount > 4)
+      downloadCount = "4+";
     url += "addonName=" + encodeURIComponent(addonName) +
         "&addonVersion=" + encodeURIComponent(addonVersion) +
         "&application=" + encodeURIComponent(application) +
         "&applicationVersion=" + encodeURIComponent(applicationVersion) +
         "&platform=" + encodeURIComponent(platform) +
         "&platformVersion=" + encodeURIComponent(platformVersion) +
         "&lastVersion=" + encodeURIComponent(downloadable.lastVersion) +
-        "&downloadCount=" + encodeURIComponent(downloadable.downloadCount);
+        "&downloadCount=" + encodeURIComponent(downloadCount);
     return url;
   },
 
   _download: function(downloadable, redirects)
   {
     if (this.isDownloading(downloadable.url))
       return;
 
     let downloadUrl = this.getDownloadUrl(downloadable);
     let request = null;
@@ skipping 73 matching lines @@
 
       delete this._downloading[downloadable.url];
 
       // Status will be 0 for non-HTTP requests
       if (request.status && request.status != 200)
       {
         errorCallback("synchronize_connection_error");
         return;
       }
 
-      // We limit the download count to 5+ to keep the request anonymized
-      if (!isNaN(downloadable.downloadCount) && ++downloadable.downloadCount > 5)
-        downloadable.downloadCount = "5+";

[Wladimir Palant, 2014/11/10 19:18:15]

Storing numbers and strings in the same variable seems wrong. In particular,
this will turn into a mess once you start incrementing strings. Why not use
(downloadCount > 5 ? "5+" : downloadCount) when setting request parameters?

[Thomas Greiner, 2014/11/11 11:54:46]

What I wanted to avoid is (a) stopping the counter at 6 which could cause
problems in case we need to increase it at some point and (b) have the counter
go beyond 5 even on the client-side.
If you don't consider (b) to be a problem then I don't mind changing it
accordingly.

[Wladimir Palant, 2014/11/11 16:07:00]

Yes, I was also considering whether (b) is a problem - but I don't think it is,
as long as we never send the raw number over the wire. It also makes sense in
case we decide to change the threshold later.

[Thomas Greiner, 2014/11/11 17:17:18]

Done.

+      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);
         }
@@ skipping 73 matching lines @@
   /**
    * Hard expiration interval, this is fixed.
    * @type Integer
    */
   hardExpiration: 0,
   
   /**
    * Number indicating how often the object was downloaded.
    * @type Integer
    */
-  downloadCount: 1,

[Thomas Greiner, 2014/11/10 18:14:55]

This number is always one higher than the number of successful downloads which
is why I described it as an "indicator" (also see subscriptionClasses.js). The
reason for that is that I wanted to avoid having to increment the counter when
making the request due to duplicated logic regarding the "5+" case.

Is this too confusing?

[Wladimir Palant, 2014/11/10 19:18:15]

Incrementing after success seems ok, but why not start with 0 here? Then this
number will actually reflect the number of successful downloads.

[Thomas Greiner, 2014/11/11 11:54:46]

If this is set to 0 then the number we send is 0-based (0-4+) instead of 1-based
(1-5+) as agreed on. I don't have any strong preferences towards either.

[Wladimir Palant, 2014/11/11 16:07:00]

I don't think we actually agreed on the exact meaning of this parameter but
0-based (meaning not counting the current download) makes much more sense IMHO.

[Thomas Greiner, 2014/11/11 17:17:18]

Done.

+  downloadCount: 0,
 };