Issue 394 - hit statistics tool data collection (core)

This is dependency review for core changes of filter hit statistics initial implementation:
https://codereview.adblockplus.org/6337686776315904/

[saroyanm, 2016-02-19 18:31:35]

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterStorage.j...
lib/filterStorage.js:336: if (Prefs.sendstats)
Hmm, I think it's make sense to check if the module exists, or move filter hit
statistics to the core as well ?

[Wladimir Palant, 2016-02-29 14:40:31]

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterListener....
lib/filterListener.js:33: let {FilterHits} = require("filterHits");
This needs to be included conditionally - this file will be used on all kind of
platforms, the filterHits module is currently Firefox-only however. I guess
something like that:

let FilterHits = null;
try
{
  ({FilterHits} = require("filterHits"));
}
catch (e) {}

You can then use it conditionally below.

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterStorage.j...
lib/filterStorage.js:32: let {FilterHits} = require("filterHits");
Same here, you cannot expect this module to be present.

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterStorage.j...
lib/filterStorage.js:336: if (Prefs.sendstats)
On 2016/02/19 18:31:35, saroyanm wrote:
> Hmm, I think it's make sense to check if the module exists, or move filter hit
> statistics to the core as well ? 

Currently that module has too much Firefox-specific stuff in it to be moved into
core.

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

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterListener.js
File lib/filterListener.js (right):

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterListener....
lib/filterListener.js:33: let {FilterHits} = require("filterHits");
On 2016/02/29 14:40:30, Wladimir Palant wrote:
> This needs to be included conditionally - this file will be used on all kind
of
> platforms, the filterHits module is currently Firefox-only however. I guess
> something like that:
> 
> let FilterHits = null;
> try
> {
>   ({FilterHits} = require("filterHits"));
> }
> catch (e) {}
> 
> You can then use it conditionally below.

Done.

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29336735/diff/29336736/lib/filterStorage.j...
lib/filterStorage.js:32: let {FilterHits} = require("filterHits");
On 2016/02/29 14:40:30, Wladimir Palant wrote:
> Same here, you cannot expect this module to be present.

Done.

https://codereview.adblockplus.org/29336735/diff/29338570/lib/downloader.js
File lib/downloader.js (right):

https://codereview.adblockplus.org/29336735/diff/29338570/lib/downloader.js#n...
lib/downloader.js:37: let Downloader = exports.Downloader = function
Downloader(dataSource, initialDelay, checkInterval, downloadCallback)
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:02]

https://codereview.adblockplus.org/29336735/diff/29338570/lib/downloader.js
File lib/downloader.js (right):

https://codereview.adblockplus.org/29336735/diff/29338570/lib/downloader.js#n...
lib/downloader.js:37: let Downloader = exports.Downloader = function
Downloader(dataSource, initialDelay, checkInterval, downloadCallback)
On 2016/03/18 18:24:48, saroyanm wrote:
> Maybe this changes will be reverted if you agree with my comment in another
> related review, about updating the Downloader.

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]

Wladimir can you please have another look.

https://codereview.adblockplus.org/29336735/diff/29338570/lib/downloader.js
File lib/downloader.js (right):

https://codereview.adblockplus.org/29336735/diff/29338570/lib/downloader.js#n...
lib/downloader.js:37: let Downloader = exports.Downloader = function
Downloader(dataSource, initialDelay, checkInterval, downloadCallback)
On 2016/03/23 11:06:01, Wladimir Palant wrote:
> On 2016/03/18 18:24:48, saroyanm wrote:
> > Maybe this changes will be reverted if you agree with my comment in another
> > related review, about updating the Downloader.
> 
> 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);

Thanks for detailed explanation, Done.

[Wladimir Palant, 2017-12-21 10:47:43]

I'm pretty sure that this is no longer relevant, closing.