Issue 537 - Mark first few downloads

Issue 537 - Mark subsequent downloads

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

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/downloader.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:391: downloadCount: 1,
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]

Note that unit tests need to be updated as well.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/downloader.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:297: downloadable.downloadCount = "5+";
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?

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:391: downloadCount: 1,
Incrementing after success seems ok, but why not start with 0 here? Then this
number will actually reflect the number of successful downloads.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/notification.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/notification.js:110: if ("downloadCount" in Prefs.notificationdata)
Should be a number with the comments above addressed.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/subscriptionClasses.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/subscriptionClasses.js:222: result.downloadCount = obj.downloadCount;
Should be a number with the comments above addressed.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/subscriptionClasses.js:570: downloadCount: 1,
Again, I think that the default should be 0.

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

I'm going to work on the unit tests right after that review is done.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/downloader.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:297: downloadable.downloadCount = "5+";
On 2014/11/10 19:18:15, Wladimir Palant wrote:
> 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?

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.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:391: downloadCount: 1,
On 2014/11/10 19:18:15, Wladimir Palant wrote:
> Incrementing after success seems ok, but why not start with 0 here? Then this
> number will actually reflect the number of successful downloads.

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]

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/downloader.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:297: downloadable.downloadCount = "5+";
On 2014/11/11 11:54:46, Thomas Greiner wrote:
> If you don't consider (b) to be a problem then I don't mind changing it
> accordingly.

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.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:391: downloadCount: 1,
On 2014/11/11 11:54:46, Thomas Greiner wrote:
> 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.

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:17]

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/downloader.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:297: downloadable.downloadCount = "5+";
On 2014/11/11 16:07:00, Wladimir Palant wrote:
> On 2014/11/11 11:54:46, Thomas Greiner wrote:
> > If you don't consider (b) to be a problem then I don't mind changing it
> > accordingly.
> 
> 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.

Done.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/downloader.js:391: downloadCount: 1,
On 2014/11/11 16:07:00, Wladimir Palant wrote:
> On 2014/11/11 11:54:46, Thomas Greiner wrote:
> > 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.
> 
> 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.

Done.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/notification.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/notification.js:110: if ("downloadCount" in Prefs.notificationdata)
On 2014/11/10 19:18:15, Wladimir Palant wrote:
> Should be a number with the comments above addressed.

Done.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
File lib/subscriptionClasses.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/subscriptionClasses.js:222: result.downloadCount = obj.downloadCount;
On 2014/11/10 19:18:15, Wladimir Palant wrote:
> Should be a number with the comments above addressed.

Done.

http://codereview.adblockplus.org/6013081427640320/diff/5629499534213120/lib/...
lib/subscriptionClasses.js:570: downloadCount: 1,
On 2014/11/10 19:18:15, Wladimir Palant wrote:
> Again, I think that the default should be 0.

Done.

[Wladimir Palant, 2014-11-11 17:28:15]

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
File lib/downloader.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
lib/downloader.js:196: if (downloadable.downloadCount > 4)
No need to reference downloadable again here, just use the downloadCount
variable.

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
File lib/subscriptionClasses.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
lib/subscriptionClasses.js:222: result.downloadCount =
parseInt(obj.downloadCount) || 0;
Nit: while at it, could you change that into parseInt(..., 10) here and above?
parseInt() should always be used with an explicit radix...

[Thomas Greiner, 2014-11-11 17:53:18]

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
File lib/downloader.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
lib/downloader.js:196: if (downloadable.downloadCount > 4)
On 2014/11/11 17:28:16, Wladimir Palant wrote:
> No need to reference downloadable again here, just use the downloadCount
> variable.

Sorry, I missed that. Done.

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
File lib/subscriptionClasses.js (right):

http://codereview.adblockplus.org/6013081427640320/diff/5717271485874176/lib/...
lib/subscriptionClasses.js:222: result.downloadCount =
parseInt(obj.downloadCount) || 0;
On 2014/11/11 17:28:16, Wladimir Palant wrote:
> Nit: while at it, could you change that into parseInt(..., 10) here and above?
> parseInt() should always be used with an explicit radix...

No problem. Done.

[Wladimir Palant, 2014-11-11 18:47:30]

LGTM, thank you.