Issue 6337686776315904: Issue 394 - hit statistics tool data collection (Closed)
|
Created:
April 6, 2014, 3:16 p.m. by saroyanm Modified:
Dec. 21, 2017, 10:47 a.m. CC:
Sebastian Noack Visibility:
Public. |
DescriptionThis code review is related to current ticket:
https://issues.adblockplus.org/ticket/394
Patch Set 1 #Patch Set 2 : subscriptions and thirdparty filter hit added to statistics #Patch Set 3 : reset filterhits #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : #
Total comments: 41
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 33
Patch Set 9 : #
Total comments: 31
Patch Set 10 : Addressed Dave Comments and updated increaseFilterHits parameter #Patch Set 11 : Use braces for consistency #Patch Set 12 : Rebase to changeset: 4056 #Patch Set 13 : Changed the submission URL #Patch Set 14 : Rebase to changeset: 4095 #
Total comments: 14
Patch Set 15 : Addressed Wladimir comments (save data in the database instead of memory) #
Total comments: 50
Patch Set 16 : Addressed Wladimir comments (bunch of changes) #
Total comments: 48
Patch Set 17 : Rebase to changeset 4157 #Patch Set 18 : Use Downloader to send the data to server #
Total comments: 1
MessagesTotal messages: 30
Hi Wladimir, Actually I couldn't describe what exactly I don't understand, so I've decided to implement something so we can discuss the way we can continue from here, After your comments I'm pretty sure that the things will come into their places. I've just noticed that the filter contain also the subscriptions, where the filter was hit, I guess that also should be added into collected data, I'll add that feature with next patch. While I was having Easy-list and Ru+Easy-list I was thinking the filter contains all my subscription, just noticed that basically it was just being hit in both subscriptions :) Anyway the implementation I'm pretty sure will be changed after your comments. Thanks in advance.
I've updated the patch to also collect filter hit subscriptions and third party info.
Wladimir can you please have a look when you'll get time. I think we will need to push changes, only after backend script is ready to process the data. http://codereview.adblockplus.org/6337686776315904/diff/5692201761767424/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5692201761767424/lib/... lib/filterHits.js:33: _serviceURL: "", Here should go the URL for backend service.
Whilst working on the back end it occurred to us that we should consider timezones. The server expects the `latest` timestamp field to be in UTC and I wanted to ensure the client will always transmit it as such. I couldn't immediately see from this codereview if that is already the case or not, could you check this for me saroyanm? (I've added a note in the issue description as well.)
On 2015/02/13 14:16:37, kzar wrote: > Whilst working on the back end it occurred to us that we should consider > timezones. The server expects the `latest` timestamp field to be in UTC and I > wanted to ensure the client will always transmit it as such. > > I couldn't immediately see from this codereview if that is already the case or > not, could you check this for me saroyanm? > > (I've added a note in the issue description as well.) Yes we are not considering 'latest' and 'timeSincePush' fields to be in UTC timestamp, I'll have a look on that later, I guess I should also update review patch to current revision while 4 months passed after last update.
@Dave: I guess I was wrong regarding UTC, seems like we already using UTC, please see the comment in the code. I've also updated the patch to latest(#3896) revision and made some small fixes. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/filterStorage.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterStorage.js:339: filter.lastHit = Date.now(); @Dave here is where we are setting the lastHit for the filterObject and we use same value for filters hit calculation. According to -> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-date.now Date.now(); should return UTC time. So I guess we are Okey here.
Looks nice overall. The comments are only about minor things. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/chro... chrome/locale/en-US/overlay.dtd:33: <!ENTITY sendstats.label "Help us improve ABP by sending anonymous statistics"> We're not calling it "ABP" anywhere in the extension (except for Firefox Mobile where we have limited space) so I'd stick with "Adblock Plus". http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:33: _serviceURL: "", Where is the value for this supposed to be coming from? http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:38: _sending: false, Detail: Since you're documenting the rest of the code please also add descriptions for each of those variables (see lib/contentPolicy.js for example). http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:47: if (!filter.text || (filter.subscriptions[0] && filter.subscriptions[0].url.indexOf("~user~") == 0)) This means that if the first subscription a filter is in, is a custom subscription, then it shouldn't increase the filter hit count. That doesn't match the behavior described in the issue "the data should include only filters from filter subscriptions, not the user's own filters" Instead you need to check whether there's a downloadable subscription in that list. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:51: this.filters[filter.text] = {}; We use `Object.create(null)` instead of `{}` to create an object without `Object` prototype. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:53: var statFilter = this.filters[filter.text]; Any reason why you're using `var` here? http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:91: let prepareData = function() The code inside that function works just as fine without the function wrapper around it. So what's the point of that? http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:112: let request = event.target; Detail: Here you can directly access `request` from the outer scope so no need to declare this variable. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:120: Cu.reportError("could not send filter hit statistics to AdBlock Plus server"); Detail: It's "Adblock Plus". http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:120: Cu.reportError("could not send filter hit statistics to AdBlock Plus server"); Detail: In the other error messages you start with a capital letter and end with a "." http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:155: handleResult: function(aResultSet) Detail: I assume you copy/pasted this variable name from MDN. Naming it "results" should be sufficient since we don't use the "a" variable prefix. (see also "aError" and "aReason" below) http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:165: FilterHits._loading = false; This seems redundant since `_loading` should already be set to `false` in the `handleCompletion` callback. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:170: Cu.reportError(aError.message); Unless `handleCompletion` is also called when there's an error, the `_loading` variable will not be set to `false` in case of an error. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:175: if (aReason != Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED) Detail: You can abbreviate `Components.interfaces.` to `Ci.` just like you do with `Cu.` FYI: Here's the excerpt from buildtools/bootstrap.js.tmpl that initializes those constants: const Cc = Components.classes; const Ci = Components.interfaces; const Cr = Components.results; const Cu = Components.utils; http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:193: this._lastPush = new Date().getTime(); You're using `new Date().getTime()` twice so best to create a new variable for that (e.g. "now") to also ensure that it returns the same value. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:218: Cu.reportError(aError.message); Unless `handleCompletion` is also called when there's an error, the `_saving` variable will not be set to `false` in case of an error. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/filterStorage.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterStorage.js:341: FilterHits.increaseFilterHits(filter, wnd); `FilterNotifier` allows you to listen to the "filter.hitCount" event which is fired whenever `filter.lastHit` changes. That means that the filter hits can be increased from within the `FilterHits` module itself. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterStorage.js:451: FilterHits.loadFilterHitsFromDatabase(); `FilterNotifier` allows you to listen to the "load" event which is fired whenever filters were loaded into memory. That means that the loading of filter hits can be triggered from within the `FilterHits` module itself. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/ui.js:1203: return; The function returns here anyway so this `return` statement is redundant.
New patch uploaded. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/chro... chrome/locale/en-US/overlay.dtd:33: <!ENTITY sendstats.label "Help us improve ABP by sending anonymous statistics"> On 2015/02/23 18:43:05, Thomas Greiner wrote: > We're not calling it "ABP" anywhere in the extension (except for Firefox Mobile > where we have limited space) so I'd stick with "Adblock Plus". Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:33: _serviceURL: "", On 2015/02/23 18:43:05, Thomas Greiner wrote: > Where is the value for this supposed to be coming from? Initially I was thinking that should be hardcoded in the code, but now I think it's better to get rid of this variable and get it from prefs ? http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:38: _sending: false, On 2015/02/23 18:43:05, Thomas Greiner wrote: > Detail: Since you're documenting the rest of the code please also add > descriptions for each of those variables (see lib/contentPolicy.js for example). Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:47: if (!filter.text || (filter.subscriptions[0] && filter.subscriptions[0].url.indexOf("~user~") == 0)) On 2015/02/23 18:43:05, Thomas Greiner wrote: > This means that if the first subscription a filter is in, is a custom > subscription, then it shouldn't increase the filter hit count. That doesn't > match the behavior described in the issue "the data should include only filters > from filter subscriptions, not the user's own filters" > > Instead you need to check whether there's a downloadable subscription in that > list. Agree, implementation is stupid :) Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:51: this.filters[filter.text] = {}; On 2015/02/23 18:43:05, Thomas Greiner wrote: > We use `Object.create(null)` instead of `{}` to create an object without > `Object` prototype. Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:53: var statFilter = this.filters[filter.text]; On 2015/02/23 18:43:05, Thomas Greiner wrote: > Any reason why you're using `var` here? No reason to use var here, changed. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:91: let prepareData = function() On 2015/02/23 18:43:05, Thomas Greiner wrote: > The code inside that function works just as fine without the function wrapper > around it. So what's the point of that? Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:112: let request = event.target; On 2015/02/23 18:43:05, Thomas Greiner wrote: > Detail: Here you can directly access `request` from the outer scope so no need > to declare this variable. Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:120: Cu.reportError("could not send filter hit statistics to AdBlock Plus server"); On 2015/02/23 18:43:05, Thomas Greiner wrote: > Detail: It's "Adblock Plus". Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:120: Cu.reportError("could not send filter hit statistics to AdBlock Plus server"); On 2015/02/23 18:43:05, Thomas Greiner wrote: > Detail: In the other error messages you start with a capital letter and end with > a "." Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:155: handleResult: function(aResultSet) On 2015/02/23 18:43:05, Thomas Greiner wrote: > Detail: I assume you copy/pasted this variable name from MDN. Naming it > "results" should be sufficient since we don't use the "a" variable prefix. (see > also "aError" and "aReason" below) Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:165: FilterHits._loading = false; On 2015/02/23 18:43:05, Thomas Greiner wrote: > This seems redundant since `_loading` should already be set to `false` in the > `handleCompletion` callback. Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:170: Cu.reportError(aError.message); On 2015/02/23 18:43:05, Thomas Greiner wrote: > Unless `handleCompletion` is also called when there's an error, the `_loading` > variable will not be set to `false` in case of an error. Right, updated to always set `_loading` variable to false on handleCompletion. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:175: if (aReason != Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED) On 2015/02/23 18:43:05, Thomas Greiner wrote: > Detail: You can abbreviate `Components.interfaces.` to `Ci.` just like you do > with `Cu.` > > FYI: Here's the excerpt from buildtools/bootstrap.js.tmpl that initializes those > constants: > > const Cc = Components.classes; > const Ci = Components.interfaces; > const Cr = Components.results; > const Cu = Components.utils; Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:193: this._lastPush = new Date().getTime(); On 2015/02/23 18:43:05, Thomas Greiner wrote: > You're using `new Date().getTime()` twice so best to create a new variable for > that (e.g. "now") to also ensure that it returns the same value. Done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:218: Cu.reportError(aError.message); On 2015/02/23 18:43:05, Thomas Greiner wrote: > Unless `handleCompletion` is also called when there's an error, the `_saving` > variable will not be set to `false` in case of an error. Handle completion should also be triggered during Error: https://developer.mozilla.org/en-US/docs/MozIStorageStatementCallback#Constants So the `_saving` variable should be set to false even on error occurrence. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/filterStorage.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterStorage.js:341: FilterHits.increaseFilterHits(filter, wnd); On 2015/02/23 18:43:05, Thomas Greiner wrote: > `FilterNotifier` allows you to listen to the "filter.hitCount" event which is > fired whenever `filter.lastHit` changes. That means that the filter hits can be > increased from within the `FilterHits` module itself. I also need the window object where the match has been occurred so I can get the Domain and we are not passing the window of the occurrence to trigger function as one of the param with current filterClasses implementation. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterStorage.js:451: FilterHits.loadFilterHitsFromDatabase(); On 2015/02/23 18:43:05, Thomas Greiner wrote: > `FilterNotifier` allows you to listen to the "load" event which is fired > whenever filters were loaded into memory. That means that the loading of filter > hits can be triggered from within the `FilterHits` module itself. Good point, done. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/ui.js:1203: return; On 2015/02/23 18:43:05, Thomas Greiner wrote: > The function returns here anyway so this `return` statement is redundant. Done.
http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterHits.js:33: _serviceURL: "", On 2015/02/28 15:24:30, saroyanm wrote: > On 2015/02/23 18:43:05, Thomas Greiner wrote: > > Where is the value for this supposed to be coming from? > > Initially I was thinking that should be hardcoded in the code, but now I think > it's better to get rid of this variable and get it from prefs ? Yes, any of our URLs are specified in the preferences. http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... File lib/filterStorage.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... lib/filterStorage.js:341: FilterHits.increaseFilterHits(filter, wnd); On 2015/02/28 15:24:30, saroyanm wrote: > On 2015/02/23 18:43:05, Thomas Greiner wrote: > > `FilterNotifier` allows you to listen to the "filter.hitCount" event which is > > fired whenever `filter.lastHit` changes. That means that the filter hits can > be > > increased from within the `FilterHits` module itself. > > I also need the window object where the match has been occurred so I can get the > Domain and we are not passing the window of the occurrence to trigger function > as one of the param with current filterClasses implementation. Ah, right. We also cannot add it to the `FilterNotifier.triggerListeners` call because that one also doesn't have access to the Window. Unfortunately, seems like we need to do it like this then. http://codereview.adblockplus.org/6337686776315904/diff/5675267779461120/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5675267779461120/lib/... lib/filterHits.js:87: subscriptionTitles.push(filter.subscriptions[i]._title) See https://issues.adblockplus.org/ticket/394#comment:40 http://codereview.adblockplus.org/6337686776315904/diff/5675267779461120/lib/... lib/filterHits.js:91: return; Rather than constructing an array on each function call I'd suggest having a simpler loop which would also allow you to break early in some cases. let subscriptions = filter.subscriptions; let inDownloadableSubscription = false; for (let i = 0; i < subscriptions.length; i++) { if (subscriptions[i] instanceof DownloadableSubscription) { inDownloadableSubscription = true; break; } } if (!inDownloadableSubscription) return; The theory behind it is that array modifications are more expensive than looping through an array and that it would therefore turn out to be more performant. I was able to confirm that by measuring the performance of both methods within my local Adblock Plus instance.
Patch, updated. http://codereview.adblockplus.org/6337686776315904/diff/5675267779461120/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5675267779461120/lib/... lib/filterHits.js:87: subscriptionTitles.push(filter.subscriptions[i]._title) On 2015/03/06 11:29:19, Thomas Greiner wrote: > See https://issues.adblockplus.org/ticket/394#comment:40 Ohh, so stupid from my side :/ Done. http://codereview.adblockplus.org/6337686776315904/diff/5675267779461120/lib/... lib/filterHits.js:91: return; On 2015/03/06 11:29:19, Thomas Greiner wrote: > Rather than constructing an array on each function call I'd suggest having a > simpler loop which would also allow you to break early in some cases. > > let subscriptions = filter.subscriptions; > let inDownloadableSubscription = false; > for (let i = 0; i < subscriptions.length; i++) > { > if (subscriptions[i] instanceof DownloadableSubscription) > { > inDownloadableSubscription = true; > break; > } > } > > if (!inDownloadableSubscription) > return; > > The theory behind it is that array modifications are more expensive than looping > through an array and that it would therefore turn out to be more performant. I > was able to confirm that by measuring the performance of both methods within my > local Adblock Plus instance. Done.
LGTM
I just tested this out with a locally running filter hits server. Good news is data seems to be submitted correctly to the server (including the date/times) and I can query it back with no errors. Bad news is the data was not submitted as frequently as I expected. (See my comment by _pushInterval in filterHits.js.) I have attached the data that was generated to ticket #395, https://issues.adblockplus.org/attachment/ticket/395/2015-03-23.tar.gz in case anyone wants to have a look. (I have not looked through the code changes you've made in detail, just wanted to test it worked in practice.) http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... chrome/locale/en-US/overlay.dtd:33: <!ENTITY sendstats.label "Help us improve Adblock Plus by sending anonymous statistics"> I don't think this description is very clear, also it seems too long for the options menu interface. http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/defa... File defaults/prefs.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/defa... defaults/prefs.js:30: pref("extensions.adblockplus.sendstats_url", ""); This URL should be "https://hitstats.adblockplus.org/submit" I believe. (See Issue #396) http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:39: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:50: _pushInterval: MILLIS_IN_DAY * 7, I set this to 60000 (number of milliseconds in one minute) and I found that the data was not submitted once a minute. I kept it running for a couple of hours and I got POST requests at these times: [23/Mar/2015 11:09:56] [23/Mar/2015 11:57:25] [23/Mar/2015 12:09:56] Any idea the post requests were not made once a minute as I expected? http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:53: * Indicates whether the data are being loaded from storage Nit: These three comments should start with "Indicates whether the data is..." instead of "Indicates whether the data are..." http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:112: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:121: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:127: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:164: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:169: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:175: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:187: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:205: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:210: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:219: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:222: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:231: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:244: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:257: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:258: handleCompletion: function(aReason) Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:266: Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/filterStorage.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterStorage.js:341: FilterHits.increaseFilterHits(filter, wnd); Nit: Trailing whitespace http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/ui.js:1206: Nit: Trailing whitespace
http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... chrome/locale/en-US/overlay.dtd:33: <!ENTITY sendstats.label "Help us improve Adblock Plus by sending anonymous statistics"> On 2015/03/23 13:11:05, kzar wrote: > I don't think this description is very clear, also it seems too long for the > options menu interface. FYI: We have a similar situation in #2193 which Sven proposed to tackle like this: https://issues.adblockplus.org/attachment/ticket/2193/notification-menu_firef...
http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... chrome/locale/en-US/overlay.dtd:33: <!ENTITY sendstats.label "Help us improve Adblock Plus by sending anonymous statistics"> On 2015/03/23 13:11:05, kzar wrote: > I don't think this description is very clear, also it seems too long for the > options menu interface. I think this needs to be discussed in the ticket while it's in sync with description. http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:50: _pushInterval: MILLIS_IN_DAY * 7, On 2015/03/23 13:11:05, kzar wrote: > I set this to 60000 (number of milliseconds in one minute) and I found that the > data was not submitted once a minute. I kept it running for a couple of hours > and I got POST requests at these times: > > [23/Mar/2015 11:09:56] > [23/Mar/2015 11:57:25] > [23/Mar/2015 12:09:56] > > Any idea the post requests were not made once a minute as I expected? Yes because we are saving the data to database and same time check whether we need to send the data to server only after some amount of items are being blocked, as you can see from filterListener.js current weight of filterHit is set to 0.002, so we will call saveFilterHitsToDatabase method each time after 500 ads being blocked.
http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:50: _pushInterval: MILLIS_IN_DAY * 7, On 2015/03/24 10:42:40, saroyanm wrote: > On 2015/03/23 13:11:05, kzar wrote: > > I set this to 60000 (number of milliseconds in one minute) and I found that > the > > data was not submitted once a minute. I kept it running for a couple of hours > > and I got POST requests at these times: > > > > [23/Mar/2015 11:09:56] > > [23/Mar/2015 11:57:25] > > [23/Mar/2015 12:09:56] > > > > Any idea the post requests were not made once a minute as I expected? > > Yes because we are saving the data to database and same time check whether we > need to send the data to server only after some amount of items are being > blocked, as you can see from filterListener.js current weight of filterHit is > set to 0.002, so we will call saveFilterHitsToDatabase method each time after > 500 ads being blocked. If the data submission interval is too variable it will effect the geometrical mean calculations on the server. I suppose with larger values for _pushInterval it wont be so much of a problem, what do you think Wladimir?
The patch is updated: The new patch contain updates to @Dave comments, as well the option item for opting in the filter hits functionality moved to the "Filter preferences" dialog. The patch is also updated to latest revision (3929) http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/chro... chrome/locale/en-US/overlay.dtd:33: <!ENTITY sendstats.label "Help us improve Adblock Plus by sending anonymous statistics"> On 2015/03/24 10:42:40, saroyanm wrote: > On 2015/03/23 13:11:05, kzar wrote: > > I don't think this description is very clear, also it seems too long for the > > options menu interface. > > I think this needs to be discussed in the ticket while it's in sync with > description. Updated, now this option is located in filter preferences. http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/defa... File defaults/prefs.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/defa... defaults/prefs.js:30: pref("extensions.adblockplus.sendstats_url", ""); On 2015/03/23 13:11:05, kzar wrote: > This URL should be "https://hitstats.adblockplus.org/submit" I believe. (See > Issue #396) Done. http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:39: On 2015/03/23 13:11:05, kzar wrote: > Nit: Trailing whitespace Done. http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterHits.js:53: * Indicates whether the data are being loaded from storage On 2015/03/23 13:11:05, kzar wrote: > Nit: These three comments should start with "Indicates whether the data is..." > instead of "Indicates whether the data are..." Done. http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/filterStorage.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/filterStorage.js:341: FilterHits.increaseFilterHits(filter, wnd); On 2015/03/23 13:11:05, kzar wrote: > Nit: Trailing whitespace Done. http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5704147139559424/lib/... lib/ui.js:1206: On 2015/03/23 13:11:05, kzar wrote: > Nit: Trailing whitespace Done.
As mentioned in IRC if rebasing please submit a separate patch set for that. Also it would help to name the patch sets, so we know which ones are rebases and which ones are your changes. Lastly a tip Felix mentioned to me, when sending the email for a new patch set paste the patch set name + number at the top of the message, it makes it easier for the reviewer to check which patch set they previously were looking at. Anyway no harm done, I have gone through the changes. Not familiar with a lot of the code but tried my best. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:43: */ I guess we don't need to document these private variables, they're not part of an API that we need to generate documentation for and they're all fairly self-explanatory. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:71: * Increases the filter hit count Perhaps "Increases the hit count for the given filter"? http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:75: increaseFilterHits: function(filter, wnd) Maybe we should just make host the parameter here, as that's all we need, instead of passing the whole Window object through? http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:114: statFilter[filterType][host] = {hits: 1, latest: filter.lastHit}; I think we're supposed to use braces consistently within if...else statements. So as you need the braces for the else clause, add them for the if clause too. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:144: else Again here with the if...else brace consistency. Probably elsewhere too, I'll stop adding comments for these. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:145: Cu.reportError("Could not send filter hit statistics to Adblock Plus server."); If sending fails and it's the server's fault / a connection problem will we retry later? http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:148: let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info"); Possible to add a newline in this list? It seems like quite a long line. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:201: FilterHits.filters = JSON.parse(filters); We are serialising some of the data into JSON and then storing that in the database? Shouldn't we instead serialise everything and store a JSON file or avoid serialising anything and properly structure the database? http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:232: if (!this._sending && now - this._lastPush > this._pushInterval) I think that perhaps the logic here for sending the data belongs elsewhere. It seems counter intuitive that a function to save data to the database would occasionally trigger posting data. (Also it means we do not honour the push interval very accurately.) http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:234: this.sendFilterHitsToServer(); If we are going to perform the web request here shouldn't it be at the end of the function at least? It seems even weirder that a function to save data to the database _doesn't_ save data to the database when it decides it should post some data.
Patch Set 10 : Addressed Dave Comments and updated increaseFilterHits parameter http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:43: */ On 2015/04/14 16:33:30, kzar wrote: > I guess we don't need to document these private variables, they're not part of > an API that we need to generate documentation for and they're all fairly > self-explanatory. Please have a look on current discussion: http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... I think documenting shouldn't harm anyway and from consistency point of view I think it make sense to leave them here. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:71: * Increases the filter hit count On 2015/04/14 16:33:30, kzar wrote: > Perhaps "Increases the hit count for the given filter"? No actually the filter in the parameter is the filter object that has been hit, so I'm using that object to update the "filters" object in the memory. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:75: increaseFilterHits: function(filter, wnd) On 2015/04/14 16:33:30, kzar wrote: > Maybe we should just make host the parameter here, as that's all we need, > instead of passing the whole Window object through? Done. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:114: statFilter[filterType][host] = {hits: 1, latest: filter.lastHit}; On 2015/04/14 16:33:30, kzar wrote: > I think we're supposed to use braces consistently within if...else statements. > So as you need the braces for the else clause, add them for the if clause too. I'm not sure if be honest, I can see your point, but we don't have any guideline that explicitly says that we should go one or other way, please have a look in the link below: https://hg.adblockplus.org/adblockplus/file/1669f6fb7b7e/lib/contentPolicy.js... Both for this and for documenting would be really nice to have a guideline, if be honest not sure, I'll be okey in both ways. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:144: else On 2015/04/14 16:33:30, kzar wrote: > Again here with the if...else brace consistency. Probably elsewhere too, I'll > stop adding comments for these. Same as my reply above. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:145: Cu.reportError("Could not send filter hit statistics to Adblock Plus server."); On 2015/04/14 16:33:30, kzar wrote: > If sending fails and it's the server's fault / a connection problem will we > retry later? Yes, but it depends on filterStorage action. Here is how the saving works now: We have setDirty indicator which decide when we should save the data to the patterns.ini and when the data should be sent to the server: http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... this indicator reset after successfuly save to patterns.ini. So in general it should dependent whether data was saved to patterns.ini. So in case the data is successfully saved to patterns.ini we will have next request to send the data to server after 500 hits, in case it's not been saved we will send the data after each hit in case we are not waiting for the response from the server, please note also we will try to save the data to patterns.ini until it's been saved successfully. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:148: let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info"); On 2015/04/14 16:33:30, kzar wrote: > Possible to add a newline in this list? It seems like quite a long line. Couldn't find any documentation that explicitly says so but looks like it possible to have multiple lines. http://people.mozilla.org/~jorendorff/es6-draft.html#sec-destructuring-assign... Done. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:201: FilterHits.filters = JSON.parse(filters); On 2015/04/14 16:33:30, kzar wrote: > We are serialising some of the data into JSON and then storing that in the > database? Shouldn't we instead serialise everything and store a JSON file or > avoid serialising anything and properly structure the database? The idea was to use Storage API which is actually SQLlite database. Maybe using json file will be more clear, but make sense to discuss using filestorage with @Wladimir, as it was his idea to use Storage API. But meanwhile I don't understand why we need proper structure if we are not going to make search in that ? Can you please let me know why you will prefare some other structure instead of serializing and deserializing JSON object in this particular case ? http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:232: if (!this._sending && now - this._lastPush > this._pushInterval) On 2015/04/14 16:33:30, kzar wrote: > I think that perhaps the logic here for sending the data belongs elsewhere. It > seems counter intuitive that a function to save data to the database would > occasionally trigger posting data. (Also it means we do not honour the push > interval very accurately.) Yes you are right this should go elsewhere, but before moving it somewhere I would like to consider two scenarios: 1. This can go to filterListener.js right before saveFilterHitsToDatabase metod call. The interval will work same way as it described in my reply above. 2. This can go to increaseFilterHits method: We will check for the date on every hit, but it will have small impact on performance, I think maybe we should consider also small performance difference in this case, even if we will not honor the push interval very careful anyway the data will be sent, please note if the user will not turn on his computer for several days the data will again not be sent, so I think it doesn't make really sense to be so cautious regarding the interval. So this is two scenarios I'm considering. Please let me know what you think, before performing this changes, because could be I'm not really understand why we are trying to be so insistent regarding the 1 week interval. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:234: this.sendFilterHitsToServer(); On 2015/04/14 16:33:30, kzar wrote: > If we are going to perform the web request here shouldn't it be at the end of > the function at least? It seems even weirder that a function to save data to the > database _doesn't_ save data to the database when it decides it should post some > data. Yes it doesn't because after the successful posting the data should be reset, I can't see point why we should save them back it's additional action that can cost some performance, not a big deal, but I would rather maybe vote for this solution.
http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:43: */ On 2015/04/16 10:42:59, saroyanm wrote: > On 2015/04/14 16:33:30, kzar wrote: > > I guess we don't need to document these private variables, they're not part of > > an API that we need to generate documentation for and they're all fairly > > self-explanatory. > > Please have a look on current discussion: > http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... > I think documenting shouldn't harm anyway and from consistency point of view I > think it make sense to leave them here. I disagree on this one, I've been told not to over-comment before. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:71: * Increases the filter hit count On 2015/04/16 10:42:59, saroyanm wrote: > On 2015/04/14 16:33:30, kzar wrote: > > Perhaps "Increases the hit count for the given filter"? > > No actually the filter in the parameter is the filter object that has been hit, > so I'm using that object to update the "filters" object in the memory. I think you've slightly missed my point here. We are increasing the hit count for the specified filter, as opposed to all filters for example. (Sure the hit count is not stored in the filter itself. Feel free to clarify where the hit count we're increasing is stored too if you like, but that's something else.) Anyway it's not hugely important either way I suppose. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:114: statFilter[filterType][host] = {hits: 1, latest: filter.lastHit}; On 2015/04/16 10:42:59, saroyanm wrote: > On 2015/04/14 16:33:30, kzar wrote: > > I think we're supposed to use braces consistently within if...else statements. > > So as you need the braces for the else clause, add them for the if clause too. > > I'm not sure if be honest, I can see your point, but we don't have any guideline > that explicitly says that we should go one or other way, please have a look in > the link below: > https://hg.adblockplus.org/adblockplus/file/1669f6fb7b7e/lib/contentPolicy.js... > > Both for this and for documenting would be really nice to have a guideline, if > be honest not sure, I'll be okey in both ways. I've definitely been told that the braces should be consistent within if...else clauses on other code reviews. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:148: let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info"); On 2015/04/16 10:42:59, saroyanm wrote: > On 2015/04/14 16:33:30, kzar wrote: > > Possible to add a newline in this list? It seems like quite a long line. > > > Couldn't find any documentation that explicitly says so but looks like it > possible to have multiple lines. > > http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-destructuring-assi... > > Done. It is possible, I was just being polite. (British thing I guess.) Thanks :) http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:201: FilterHits.filters = JSON.parse(filters); On 2015/04/16 10:42:59, saroyanm wrote: > On 2015/04/14 16:33:30, kzar wrote: > > We are serialising some of the data into JSON and then storing that in the > > database? Shouldn't we instead serialise everything and store a JSON file or > > avoid serialising anything and properly structure the database? > > The idea was to use Storage API which is actually SQLlite database. > Maybe using json file will be more clear, but make sense to discuss using > filestorage with @Wladimir, as it was his idea to use Storage API. > But meanwhile I don't understand why we need proper structure if we are not > going to make search in that ? Can you please let me know why you will prefare > some other structure instead of serializing and deserializing JSON object in > this particular case ? Well it just seems like a hack to mix them both to me. Either serialize everything as JSON and store that or use a database and structure it properly. I've seen too much hacked together PHP+MySQL code in the past where data is just serialized and shoved into some string DB field to think this is a good idea. Maybe Wladimir will disagree, probably best to see what he thinks here as it's a potentially big change. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:232: if (!this._sending && now - this._lastPush > this._pushInterval) On 2015/04/16 10:42:59, saroyanm wrote: > On 2015/04/14 16:33:30, kzar wrote: > > I think that perhaps the logic here for sending the data belongs elsewhere. It > > seems counter intuitive that a function to save data to the database would > > occasionally trigger posting data. (Also it means we do not honour the push > > interval very accurately.) > > Yes you are right this should go elsewhere, > but before moving it somewhere I would like to consider two scenarios: > 1. This can go to filterListener.js right before saveFilterHitsToDatabase metod > call. > The interval will work same way as it described in my reply above. > 2. This can go to increaseFilterHits method: > We will check for the date on every hit, but it will have small impact on > performance, I think maybe we should consider also small performance difference > in this case, even if we will not honor the push interval very careful anyway > the data will be sent, please note if the user will not turn on his computer for > several days the data will again not be sent, so I think it doesn't make really > sense to be so cautious regarding the interval. > > So this is two scenarios I'm considering. Please let me know what you think, > before performing this changes, because could be I'm not really understand why > we are trying to be so insistent regarding the 1 week interval. The geometrical mean calculation on the server takes into account the submission interval. If the submission interval varies a lot the results will be less accurate. I'm not sure how much this matters, or what the best approach is here though however. I think it's best to see what Wladimir thinks. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:234: this.sendFilterHitsToServer(); On 2015/04/16 10:42:59, saroyanm wrote: > On 2015/04/14 16:33:30, kzar wrote: > > If we are going to perform the web request here shouldn't it be at the end of > > the function at least? It seems even weirder that a function to save data to > the > > database _doesn't_ save data to the database when it decides it should post > some > > data. > > Yes it doesn't because after the successful posting the data should be reset, I > can't see point why we should save them back it's additional action that can > cost some performance, not a big deal, but I would rather maybe vote for this > solution. I guess we can ignore this one for now as it will no longer be relevant once data posting is triggered from elsewhere.
Patch Set 11 : Use braces for consistency http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... File lib/filterHits.js (right): http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:43: */ On 2015/04/16 12:46:26, kzar wrote: > On 2015/04/16 10:42:59, saroyanm wrote: > > On 2015/04/14 16:33:30, kzar wrote: > > > I guess we don't need to document these private variables, they're not part > of > > > an API that we need to generate documentation for and they're all fairly > > > self-explanatory. > > > > Please have a look on current discussion: > > > http://codereview.adblockplus.org/6337686776315904/diff/5842605074022400/lib/... > > I think documenting shouldn't harm anyway and from consistency point of view I > > think it make sense to leave them here. > > I disagree on this one, I've been told not to over-comment before. I guess sometimes would be nice to have link to that discussion, to understand if that is general and cover this case, or special case. So we have two thoughts, let's keep it like this for now and align with Wladimir, as well. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:114: statFilter[filterType][host] = {hits: 1, latest: filter.lastHit}; On 2015/04/16 12:46:26, kzar wrote: > On 2015/04/16 10:42:59, saroyanm wrote: > > On 2015/04/14 16:33:30, kzar wrote: > > > I think we're supposed to use braces consistently within if...else > statements. > > > So as you need the braces for the else clause, add them for the if clause > too. > > > > I'm not sure if be honest, I can see your point, but we don't have any > guideline > > that explicitly says that we should go one or other way, please have a look in > > the link below: > > > https://hg.adblockplus.org/adblockplus/file/1669f6fb7b7e/lib/contentPolicy.js... > > > > Both for this and for documenting would be really nice to have a guideline, if > > be honest not sure, I'll be okey in both ways. > > I've definitely been told that the braces should be consistent within if...else > clauses on other code reviews. Would be nice to have some reference for future, feel free to send link with relevant discussion. Anyway I've updated for consistency as you suggested. http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:148: let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info"); On 2015/04/16 12:46:26, kzar wrote: > On 2015/04/16 10:42:59, saroyanm wrote: > > On 2015/04/14 16:33:30, kzar wrote: > > > Possible to add a newline in this list? It seems like quite a long line. > > > > > > Couldn't find any documentation that explicitly says so but looks like it > > possible to have multiple lines. > > > > > http://people.mozilla.org/%257Ejorendorff/es6-draft.html#sec-destructuring-as... > > > > Done. > > It is possible, I was just being polite. (British thing I guess.) Thanks :) Ha ha :) That's funny :D http://codereview.adblockplus.org/6337686776315904/diff/5747350215589888/lib/... lib/filterHits.js:234: this.sendFilterHitsToServer(); On 2015/04/16 12:46:26, kzar wrote: > On 2015/04/16 10:42:59, saroyanm wrote: > > On 2015/04/14 16:33:30, kzar wrote: > > > If we are going to perform the web request here shouldn't it be at the end > of > > > the function at least? It seems even weirder that a function to save data to > > the > > > database _doesn't_ save data to the database when it decides it should post > > some > > > data. > > > > Yes it doesn't because after the successful posting the data should be reset, > I > > can't see point why we should save them back it's additional action that can > > cost some performance, not a big deal, but I would rather maybe vote for this > > solution. > > I guess we can ignore this one for now as it will no longer be relevant once > data posting is triggered from elsewhere. Yes agree.
Rebased.
Submission URL updated.
Rebased to latest changeset
https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... File chrome/content/ui/filters-subscriptionview.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... chrome/content/ui/filters-subscriptionview.js:335: FilterHits.resetFilterHits(); Two notes: 1) I think this code is better placed in FilterHits itself, not the UI - it should listen to Prefs.sendstats changes, regardless of how these changes were performed. 2) I wonder whether we should clear the data immediately - what if the user disables the pref only to enable it again immediately? We might be better off waiting a second after a change. https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... chrome/content/ui/filters-subscriptionview.js:338: } This code doesn't really belong here - all the logic here is grouped under ListManager and deals with filter subscriptions. I'd recommend putting it into filters.js - unless you want to create a new file with a new container for it. https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... chrome/content/ui/filters-subscriptionview.js:343: E("sendStats").checked = Prefs.sendstats; What if Prefs.savestats isn't set? I think we don't want to show this checkbox at all then. https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/content... lib/contentPolicy.js:113: FilterStorage.increaseHitCount(filter, getHostname(topWindowLocation)); getHostname(topWindowLocation) should be identical to docDomain. https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterH... File lib/filterHits.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterH... lib/filterHits.js:190: let statement = connection.createStatement("SELECT * FROM filterhits"); That was not really the idea. We use the database to *avoid* loading all the data into memory, because we expect lots of data here. Of course, for that we would need to store the data in a more meaningful format than a JSON blob. So our table should be something like: CREATE TABLE filterhits (filter TEXT, host TEXT, hitCount INTEGER, lastHit INTEGER, PRIMARY KEY(filter, host)) Note: I'm not sure whether SQLite allows text fields to be keys, if not we might also have to save the MD5 or SHA1 hash of the filter and host as well and use those for the key. And to increase a hit you would run an SQL statement like the following: INSERT OR REPLACE INTO filterhits (filter, host, hitCount, lastHit) VALUES (?, ?, (SELECT hitCount FROM filterhits WHERE filter = ? AND host = ?) + 1, ?) There should be no explicit "save to disk," SQLite should rather take care of that by itself. https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterS... File lib/filterStorage.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterS... lib/filterStorage.js:329: increaseHitCount: function(filter, hostanme) Typo: hostname https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/ui.js File lib/ui.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/ui.js#n... lib/ui.js:1190: Prefs.sendstats = false; To reinforce my point from before: currently this won't reset hits, this should just happen automatically in FilterHits whenever Prefs.sendstats is set to false.
@Wladimir can you please have a look into the changes ? I also created separate review for the changes related to Core repository: https://codereview.adblockplus.org/29336735/ https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... File chrome/content/ui/filters-subscriptionview.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... chrome/content/ui/filters-subscriptionview.js:335: FilterHits.resetFilterHits(); On 2016/02/15 12:25:39, Wladimir Palant wrote: > Two notes: > > 1) I think this code is better placed in FilterHits itself, not the UI - it > should listen to Prefs.sendstats changes, regardless of how these changes were > performed. Good point. Done. > 2) I wonder whether we should clear the data immediately - what if the user > disables the pref only to enable it again immediately? We might be better off > waiting a second after a change. Done. https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... chrome/content/ui/filters-subscriptionview.js:338: } On 2016/02/15 12:25:39, Wladimir Palant wrote: > This code doesn't really belong here - all the logic here is grouped under > ListManager and deals with filter subscriptions. I'd recommend putting it into > filters.js - unless you want to create a new file with a new container for it. Done. https://codereview.adblockplus.org/6337686776315904/diff/29332765/chrome/cont... chrome/content/ui/filters-subscriptionview.js:343: E("sendStats").checked = Prefs.sendstats; On 2016/02/15 12:25:39, Wladimir Palant wrote: > What if Prefs.savestats isn't set? I think we don't want to show this checkbox > at all then. Good point, Done. https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/content... File lib/contentPolicy.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/content... lib/contentPolicy.js:113: FilterStorage.increaseHitCount(filter, getHostname(topWindowLocation)); On 2016/02/15 12:25:40, Wladimir Palant wrote: > getHostname(topWindowLocation) should be identical to docDomain. Done. https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterH... File lib/filterHits.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterH... lib/filterHits.js:190: let statement = connection.createStatement("SELECT * FROM filterhits"); On 2016/02/15 12:25:40, Wladimir Palant wrote: > That was not really the idea. We use the database to *avoid* loading all the > data into memory, because we expect lots of data here. > > Of course, for that we would need to store the data in a more meaningful format > than a JSON blob. So our table should be something like: > > CREATE TABLE filterhits (filter TEXT, host TEXT, hitCount INTEGER, lastHit > INTEGER, PRIMARY KEY(filter, host)) > > Note: I'm not sure whether SQLite allows text fields to be keys, if not we might > also have to save the MD5 or SHA1 hash of the filter and host as well and use > those for the key. > > And to increase a hit you would run an SQL statement like the following: > > INSERT OR REPLACE INTO filterhits (filter, host, hitCount, lastHit) > VALUES (?, ?, > (SELECT hitCount FROM filterhits WHERE filter = ? AND host = ?) + 1, > ?) > > There should be no explicit "save to disk," SQLite should rather take care of > that by itself. Fare enough, makes more sense, not sure why initially was implemented like that. Also It's possible to have TEXT as primary key: http://www.sqlite.org/faq.html#q4 https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterS... File lib/filterStorage.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/filterS... lib/filterStorage.js:329: increaseHitCount: function(filter, hostanme) On 2016/02/15 12:25:41, Wladimir Palant wrote: > Typo: hostname Done. https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/ui.js File lib/ui.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29332765/lib/ui.js#n... lib/ui.js:1190: Prefs.sendstats = false; On 2016/02/15 12:25:41, Wladimir Palant wrote: > To reinforce my point from before: currently this won't reset hits, this should > just happen automatically in FilterHits whenever Prefs.sendstats is set to > false. Done.
https://codereview.adblockplus.org/6337686776315904/diff/29336726/chrome/cont... File chrome/content/ui/filters.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29336726/chrome/cont... chrome/content/ui/filters.js:33: E("sendStats").getElementsByTagName("checkbox")[0].checked = Prefs.sendstats; Please don't access elements like that, they should have an ID unless there is a very good reason why they cannot. If you need an ID attribute on the container then you can add id="sendStats-container" on it but you should still access the checkbox by its own ID rather than relying on the structure of the document. https://codereview.adblockplus.org/6337686776315904/diff/29336726/chrome/cont... chrome/content/ui/filters.js:37: E("sendStats").hidden = !Prefs[name]; Nit: No point obfuscating this - just use Prefs.savestats instead of Prefs[name] here. Also, I think that you want to listen to the sendstats preference as well here and update the checkbox accordingly (just in case the pref is changed from elsewhere). https://codereview.adblockplus.org/6337686776315904/diff/29336726/defaults/pr... File defaults/prefs.json (right): https://codereview.adblockplus.org/6337686776315904/diff/29336726/defaults/pr... defaults/prefs.json:31: "sendstats_url": "http://localhost/test.php", Probably a good reason to add a TODO comment here, so that you don't forget to change this back ;) https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... File lib/filterHits.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:30: * and send them to the server when user opt-in for that Nit: collect => collects, in SQLite => in an SQLite, send => sends, when user opt-in => if the user opts into (also, period at the end of sentence please). https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:39: _pushInterval: MILLIS_IN_DAY * 7, That's a constant, it really shouldn't be declared as a property. How about: const PUSH_INTERVAL = MILLIS_IN_DAY * 7; (at the top of the file of course). https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:45: _finished: Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED, Same here, constants shouldn't be properties. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:62: setTimeout(function() Nit: Creating a number of timers running in parallel (and potentially clearing the filter hits multiple times) isn't nice. Usual pattern is: if (this._resetTimeout) clearTimeout(this._resetTimeout); this._resetTimeout = setTimeout(() => { ... }); https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:64: if (!Prefs[name]) Here as well, just use Prefs.sendstats please - it's not some generic pref you are looking at. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:68: }.bind(this)); bind(this) shouldn't be necessary in most cases, just use arrow functions: Prefs.addListener(name => { ... }); https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:72: this.sendFilterHitsToServer(); Please use Date.now(). Also, it's a safe bet that it will return something meaningful (meaning: not zero). We shouldn't be sending the data immediately on startup, the browser is busy already. We should rather wait a few minutes (5?). Also, if the user keeps the browser open forever we will never send right now. I guess that the logic should go like this: if (now - last_push >= PUSH_INTERVAL) send(); else setTimeout(send, PUSH_INTERVAL - now + last_push); Finally, what will happen if the user sets his clock to next year, sends the data and then resets the clock to the correct time? Prefs.sendstats_last_push will point to next year and we will wait a year before sending again. If you see Prefs.sendstats_last_push being higher than now you need to correct it. Ideally, we would just use Downloader here, it already has all the necessary checks. This would require adding ability to send data there, also some way to specify that 204 is ok but otherwise it should work just fine. Various parameters like addonName would move from the payload into the URL but that should be ok. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:98: this.checkCreateTable(connection); Should we be doing this on every filter hit? Initializing the connection should be a one-time thing, after that we should merely be using it. Probably a good idea to use memoization here, something like this: get connection() { let connection = ...; Object.defineProperty(this, "connection", { enumerable: true, value: connection }); return connection; }, This will make sure we don't initialize the database unless we actually need it. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:110: statement.params.firstParty = filter.thirdParty != "thirdParty"; This is a filter flag you are checking here, it only tells you whether the filter is supposed to match first-party, third-party or any website. We already know that from the filter text. It tells you nothing about this particular match however. You'll need an additional parameter to increaseFilterHit I guess, simply pass thirdParty from the content policy. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:115: handleError: function(aError) Nit: we don't use Hungarian notation for parameters, it should be error here. Similarly, reason rather than aReason below. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:123: Cu.reportError("Writing of filter hits canceled or aborted."); Maybe add aReason to the text, so that we can look up which one it was? https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:124: }.bind(this) Please get rid of bind(this) here as well. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:140: connection.executeSimpleSQL("DROP TABLE IF EXISTS filterhits"); Probably better to leave the database initialized and merely remove all entries, otherwise you'll always have to check whether the table exists. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:171: let filterType = (row.getResultByName("firstParty") == 1 ? a) Not filterType, matchType. b) Don't compare to 1, true can be represented as different integers. Just (row.getResultByName("thirdParty") ? "thirdParty" : "firstParty") will do. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:183: filter[subscriptions] = subscriptions; You probably meant filter.subscriptions here. In general, I wonder whether it's a good idea to store subscriptions with each single entry. It should really be an additional table, it can be called filtersubscriptions. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:188: if (!(host in filter[filterType])) This check is pointless, filter/type/host combination is guaranteed to be unique - so just store the data without checking. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:195: request.addEventListener("load", function(event) You should handle "error" event as well, and be it in order to log an error message. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:197: FilterHits._sending = false; Why are you setting this member? It isn't being used for anything. If you need it you should reset it in case of error as well however. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:205: Cu.reportError("Couldn't send filter hit statistics to server."); I think that this should be more precise - server responded with a status code that we didn't expect. The message should actually include that status code in order to simplify debugging. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:213: timeSincePush: Prefs.sendstats_last_push, That's the time of last push, not the time *since* the last push ;) Also, it seems that the results of https://intraforum.adblockplus.org/t/q4-kick-off-telemetry-filter-hit-statist... didn't make it into the issue description. This should be rounded to hours, along these lines: function roundToHours(timestamp) { return Math.round(timestamp / MILLIS_IN_HOUR) * MILLIS_IN_HOUR; } The same function should be applied to filter.lastHit, *before* storing it in the database. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:244: return FileUtils.getFile("ProfD", ["adblockplus.sqlite"]); Please don't create files all over the user's profile, this should be in our directory: let file = IO.resolveFilePath(Prefs.data_directory); file.append("adblockplus.sqlite"); FilterStorage invests some more effort into this, shouldn't be necessary here however - this is a non-essential feature. Just make sure we don't spam the Error Console if IO.resolveFilePath() returns null or we cannot create the database because of file permission issues. One error message is ok, just not on every filter hit. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:256: "host TEXT, firstParty INTEGER, hitCount INTEGER, lastHit INTEGER, " + We always have thirdParty as a flag, please use it here as well. Having to write |statement.firstParty = !thirdParty| doesn't make much sense.
Sorry Wladimir for the delay, hope this changes will make more sense, but still I have a doubt regarding the changes in Downloader I've made, I think after your reply, things will be more clear. https://codereview.adblockplus.org/6337686776315904/diff/29336726/chrome/cont... File chrome/content/ui/filters.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29336726/chrome/cont... chrome/content/ui/filters.js:33: E("sendStats").getElementsByTagName("checkbox")[0].checked = Prefs.sendstats; On 2016/02/29 14:35:43, Wladimir Palant wrote: > Please don't access elements like that, they should have an ID unless there is a > very good reason why they cannot. If you need an ID attribute on the container > then you can add id="sendStats-container" on it but you should still access the > checkbox by its own ID rather than relying on the structure of the document. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/chrome/cont... chrome/content/ui/filters.js:37: E("sendStats").hidden = !Prefs[name]; On 2016/02/29 14:35:43, Wladimir Palant wrote: > Nit: No point obfuscating this - just use Prefs.savestats instead of Prefs[name] > here. > > Also, I think that you want to listen to the sendstats preference as well here > and update the checkbox accordingly (just in case the pref is changed from > elsewhere). Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/defaults/pr... File defaults/prefs.json (right): https://codereview.adblockplus.org/6337686776315904/diff/29336726/defaults/pr... defaults/prefs.json:31: "sendstats_url": "http://localhost/test.php", On 2016/02/29 14:35:44, Wladimir Palant wrote: > Probably a good reason to add a TODO comment here, so that you don't forget to > change this back ;) Acknowledged. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... File lib/filterHits.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:30: * and send them to the server when user opt-in for that On 2016/02/29 14:35:55, Wladimir Palant wrote: > Nit: collect => collects, in SQLite => in an SQLite, send => sends, when user > opt-in => if the user opts into (also, period at the end of sentence please). Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:39: _pushInterval: MILLIS_IN_DAY * 7, On 2016/02/29 14:35:50, Wladimir Palant wrote: > That's a constant, it really shouldn't be declared as a property. How about: > > const PUSH_INTERVAL = MILLIS_IN_DAY * 7; > > (at the top of the file of course). Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:45: _finished: Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED, On 2016/02/29 14:35:48, Wladimir Palant wrote: > Same here, constants shouldn't be properties. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:62: setTimeout(function() On 2016/02/29 14:35:57, Wladimir Palant wrote: > Nit: Creating a number of timers running in parallel (and potentially clearing > the filter hits multiple times) isn't nice. Usual pattern is: > > if (this._resetTimeout) > clearTimeout(this._resetTimeout); > this._resetTimeout = setTimeout(() => { > ... > }); Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:64: if (!Prefs[name]) On 2016/02/29 14:35:56, Wladimir Palant wrote: > Here as well, just use Prefs.sendstats please - it's not some generic pref you > are looking at. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:68: }.bind(this)); On 2016/02/29 14:35:53, Wladimir Palant wrote: > bind(this) shouldn't be necessary in most cases, just use arrow functions: > > Prefs.addListener(name => { > ... > }); Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:72: this.sendFilterHitsToServer(); On 2016/02/29 14:35:55, Wladimir Palant wrote: > Please use Date.now(). Also, it's a safe bet that it will return something > meaningful (meaning: not zero). > > We shouldn't be sending the data immediately on startup, the browser is busy > already. We should rather wait a few minutes (5?). Also, if the user keeps the > browser open forever we will never send right now. I guess that the logic should > go like this: > > if (now - last_push >= PUSH_INTERVAL) > send(); > else > setTimeout(send, PUSH_INTERVAL - now + last_push); > > Finally, what will happen if the user sets his clock to next year, sends the > data and then resets the clock to the correct time? Prefs.sendstats_last_push > will point to next year and we will wait a year before sending again. If you see > Prefs.sendstats_last_push being higher than now you need to correct it. > > Ideally, we would just use Downloader here, it already has all the necessary > checks. This would require adding ability to send data there, also some way to > specify that 204 is ok but otherwise it should work just fine. Various > parameters like addonName would move from the payload into the URL but that > should be ok. I'm not sure if it's a good idea to change the Downloader in the way that it also will send data, but I added ability to set a callback function that will be triggered instead of _download function, so we can separate the logic of sending the data from the logic of the time calculation, so we can send the data in the format we want. Also Downloader requires initialDelay which will fire every time user will launch the browser and checkInterval will be retested, so I assume it's not possible to make all the integration of sending the data in the Downloader. I hope that my current implementation make sense, I've made it similar to how notifications initialize the Downloader, but not really sure if my implementation is correct. Also the features that I use from Downloader I assume it's easy to implement in Filter hits statistics, so please let me know if it's only about the initial check interval and recurrent check I think we shouldn't do anything in Downloader, so yes I'll be happy to revert all the changes in Downloader and implement that both in the Filter hits. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:98: this.checkCreateTable(connection); On 2016/02/29 14:35:51, Wladimir Palant wrote: > Should we be doing this on every filter hit? Initializing the connection should > be a one-time thing, after that we should merely be using it. > > Probably a good idea to use memoization here, something like this: > > get connection() > { > let connection = ...; > Object.defineProperty(this, "connection", { > enumerable: true, > value: connection > }); > return connection; > }, > > This will make sure we don't initialize the database unless we actually need it. Good point, I've cached connection opening, but still checking for the table, I assume your comment were related to the upper row(97). https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:110: statement.params.firstParty = filter.thirdParty != "thirdParty"; On 2016/02/29 14:35:52, Wladimir Palant wrote: > This is a filter flag you are checking here, it only tells you whether the > filter is supposed to match first-party, third-party or any website. We already > know that from the filter text. It tells you nothing about this particular match > however. > > You'll need an additional parameter to increaseFilterHit I guess, simply pass > thirdParty from the content policy. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:115: handleError: function(aError) On 2016/02/29 14:35:45, Wladimir Palant wrote: > Nit: we don't use Hungarian notation for parameters, it should be error here. > Similarly, reason rather than aReason below. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:123: Cu.reportError("Writing of filter hits canceled or aborted."); On 2016/02/29 14:35:58, Wladimir Palant wrote: > Maybe add aReason to the text, so that we can look up which one it was? Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:124: }.bind(this) On 2016/02/29 14:35:49, Wladimir Palant wrote: > Please get rid of bind(this) here as well. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:140: connection.executeSimpleSQL("DROP TABLE IF EXISTS filterhits"); On 2016/02/29 14:35:56, Wladimir Palant wrote: > Probably better to leave the database initialized and merely remove all entries, > otherwise you'll always have to check whether the table exists. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:171: let filterType = (row.getResultByName("firstParty") == 1 ? On 2016/02/29 14:35:52, Wladimir Palant wrote: > a) Not filterType, matchType. > b) Don't compare to 1, true can be represented as different integers. Just > (row.getResultByName("thirdParty") ? "thirdParty" : "firstParty") will do. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:183: filter[subscriptions] = subscriptions; On 2016/02/29 14:35:53, Wladimir Palant wrote: > You probably meant filter.subscriptions here. > > In general, I wonder whether it's a good idea to store subscriptions with each > single entry. It should really be an additional table, it can be called > filtersubscriptions. Done, but not sure if current implementation is what you meant, because it's not proper relation, I could create a separate table that will grow with each filter, but I decided to stick with current solution, not to make the table grow while we expect user not have more than 5 filter lists usually, but let me know if you consider the 3 table solution better, so I can update the implementation. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:188: if (!(host in filter[filterType])) On 2016/02/29 14:35:50, Wladimir Palant wrote: > This check is pointless, filter/type/host combination is guaranteed to be unique > - so just store the data without checking. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:195: request.addEventListener("load", function(event) On 2016/02/29 14:35:58, Wladimir Palant wrote: > You should handle "error" event as well, and be it in order to log an error > message. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:197: FilterHits._sending = false; On 2016/02/29 14:35:57, Wladimir Palant wrote: > Why are you setting this member? It isn't being used for anything. If you need > it you should reset it in case of error as well however. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:205: Cu.reportError("Couldn't send filter hit statistics to server."); On 2016/02/29 14:35:54, Wladimir Palant wrote: > I think that this should be more precise - server responded with a status code > that we didn't expect. The message should actually include that status code in > order to simplify debugging. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:213: timeSincePush: Prefs.sendstats_last_push, On 2016/02/29 14:35:54, Wladimir Palant wrote: > That's the time of last push, not the time *since* the last push ;) > > Also, it seems that the results of > https://intraforum.adblockplus.org/t/q4-kick-off-telemetry-filter-hit-statist... > didn't make it into the issue description. This should be rounded to hours, > along these lines: > > function roundToHours(timestamp) > { > return Math.round(timestamp / MILLIS_IN_HOUR) * MILLIS_IN_HOUR; > } > > The same function should be applied to filter.lastHit, *before* storing it in > the database. Done, but I can't understand why we need to use this function also on filter.lastHit ? Also I assume you meant to multiply the rounded hour by the amount of seconds in hour. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:244: return FileUtils.getFile("ProfD", ["adblockplus.sqlite"]); On 2016/02/29 14:35:51, Wladimir Palant wrote: > Please don't create files all over the user's profile, this should be in our > directory: > > let file = IO.resolveFilePath(Prefs.data_directory); > file.append("adblockplus.sqlite"); > > FilterStorage invests some more effort into this, shouldn't be necessary here > however - this is a non-essential feature. Just make sure we don't spam the > Error Console if IO.resolveFilePath() returns null or we cannot create the > database because of file permission issues. One error message is ok, just not on > every filter hit. Done. https://codereview.adblockplus.org/6337686776315904/diff/29336726/lib/filterH... lib/filterHits.js:256: "host TEXT, firstParty INTEGER, hitCount INTEGER, lastHit INTEGER, " + On 2016/02/29 14:35:44, Wladimir Palant wrote: > We always have thirdParty as a flag, please use it here as well. Having to write > |statement.firstParty = !thirdParty| doesn't make much sense. Done.
https://codereview.adblockplus.org/6337686776315904/diff/29338658/chrome/cont... File chrome/content/ui/filters.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29338658/chrome/cont... chrome/content/ui/filters.js:36: else if (name == "sendStats") The pref is called sendstats, not sendStats. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... File lib/filterHits.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:28: const CHECK_INTERVAL = MILLIS_IN_DAY; IMHO this should be one hour, like for subscription downloads. Otherwise many people will only push when opening their browser (meaning: some time in the morning), simply because their sessions never last a whole day. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:29: const INITIAL_DELAY = MILLIS_IN_SECOND * 5; IMHO this is much too short. Given that this isn't something we need to send out at any cost, 2 minutes should do as well (not 1 minute because we run subscription updates there). This will make sure that all the startup tasks are already done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:85: get getStorageFile() This is a property, its name shouldn't start with "get" - storageFile? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:118: INITIAL_DELAY, CHECK_INTERVAL, this.sendFilterHitsToServer); Why isn't sendFilterHitsToServer bound? How does it even work with a bogus `this` value? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:124: * Yields a Downloadable instances for the notifications download. instances => instance notifications download => filter hits push https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:144: Prefs.sendstats_status.hardExpiration = downloadable.hardExpiration; There is a limitation of Prefs module when it comes to objects - changes will only be saved if you assign a different value. So you need to add the following: Prefs.sendstats_status = JSON.parse(JSON.stringify(Prefs.sendstats_status)); https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:165: if (!this.getStorageFile) How is the storage file related to the code here? I assume that you really want to check this.connection - and make sure that this.connection is null if storage file cannot be resolved. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:168: let statementsArray = []; I don't think there is much point into adding data type to the variable name, at least not in this case. Why not just `statements`? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:169: let filtersubscriptions = JSON.stringify(downloadableSubscriptions); Nit: this should be camel case, filterSubscriptions? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:175: let filterHitstatement = this.connection.createStatement("INSERT OR " + Nit: proper camel case would be filterHitStatement. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:177: "subscriptions) VALUES (:filter, :host, :thirdParty, coalesce ((SELECT " + Nit: COALESCE should be capitalized. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:192: Cu.reportError("Error during writing of filter hits: " + reason); Variable reason is not defined. Also a nit: "Error updating filter hits"? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:197: Cu.reportError("Writing of filter hits canceled or aborted: " + reason); Nit: "Updating filter hits canceled or aborted"? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:198: } I actually meant getting rid of bind(this) by means of arrow functions - even though we don't use `this` in these two callbacks, with arrow functions we wouldn't need to think about that. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:207: if (!this.getStorageFile) As above, this should be checking this.connection rather than some unrelated property. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:222: Prefs.sendstats_last_push = now; I don't think you want to set sendstats_last_push before actually sending the data. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:224: if ((now - Prefs.sendstats_last_push < PUSH_INTERVAL) || !Prefs.sendstats Why are you double-checking the push interval? Supposedly, Downloader has done all this already. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:228: let statement = this.connection.createStatement("SELECT filterhits.*, " + I guess you want to check this.connection here as well? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:278: this._sending = false; Set lastError here as well? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:295: if (!this._sending) Shouldn't you check this *before* going through all the effort compiling the data? Like at the beginning of the function? https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:304: Cu.reportError(error.message); More informative message here as well? "Error loading filter hits: " + error.message https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:320: return Math.round(timestamp / MILLIS_IN_HOUR) * 3600; 3600 is actually (MILLIS_IN_HOUR / MILLIS_IN_SECOND) https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/prefs.json File lib/prefs.json (right): https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/prefs.j... lib/prefs.json:31: "sendstats_last_push": 0, How about making last_push part of sendstats_status as well?
Wladimir can you please have nother look, I think this time I understand what you meant by using Downloader, but still have a question regarding the softExpiration in the comment. https://codereview.adblockplus.org/6337686776315904/diff/29338658/chrome/cont... File chrome/content/ui/filters.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29338658/chrome/cont... chrome/content/ui/filters.js:36: else if (name == "sendStats") On 2016/03/22 21:32:18, Wladimir Palant wrote: > The pref is called sendstats, not sendStats. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... File lib/filterHits.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:28: const CHECK_INTERVAL = MILLIS_IN_DAY; On 2016/03/22 21:32:32, Wladimir Palant wrote: > IMHO this should be one hour, like for subscription downloads. Otherwise many > people will only push when opening their browser (meaning: some time in the > morning), simply because their sessions never last a whole day. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:29: const INITIAL_DELAY = MILLIS_IN_SECOND * 5; On 2016/03/22 21:32:34, Wladimir Palant wrote: > IMHO this is much too short. Given that this isn't something we need to send out > at any cost, 2 minutes should do as well (not 1 minute because we run > subscription updates there). This will make sure that all the startup tasks are > already done. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:85: get getStorageFile() On 2016/03/22 21:32:27, Wladimir Palant wrote: > This is a property, its name shouldn't start with "get" - storageFile? Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:118: INITIAL_DELAY, CHECK_INTERVAL, this.sendFilterHitsToServer); On 2016/03/22 21:32:28, Wladimir Palant wrote: > Why isn't sendFilterHitsToServer bound? How does it even work with a bogus > `this` value? My bad, changed the implementation already. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:124: * Yields a Downloadable instances for the notifications download. On 2016/03/22 21:32:25, Wladimir Palant wrote: > instances => instance > notifications download => filter hits push Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:144: Prefs.sendstats_status.hardExpiration = downloadable.hardExpiration; On 2016/03/22 21:32:26, Wladimir Palant wrote: > There is a limitation of Prefs module when it comes to objects - changes will > only be saved if you assign a different value. So you need to add the following: > > Prefs.sendstats_status = JSON.parse(JSON.stringify(Prefs.sendstats_status)); Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:165: if (!this.getStorageFile) On 2016/03/22 21:32:35, Wladimir Palant wrote: > How is the storage file related to the code here? I assume that you really want > to check this.connection - and make sure that this.connection is null if storage > file cannot be resolved. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:168: let statementsArray = []; On 2016/03/22 21:32:20, Wladimir Palant wrote: > I don't think there is much point into adding data type to the variable name, at > least not in this case. Why not just `statements`? Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:169: let filtersubscriptions = JSON.stringify(downloadableSubscriptions); On 2016/03/22 21:32:31, Wladimir Palant wrote: > Nit: this should be camel case, filterSubscriptions? Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:175: let filterHitstatement = this.connection.createStatement("INSERT OR " + On 2016/03/22 21:32:30, Wladimir Palant wrote: > Nit: proper camel case would be filterHitStatement. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:177: "subscriptions) VALUES (:filter, :host, :thirdParty, coalesce ((SELECT " + On 2016/03/22 21:32:18, Wladimir Palant wrote: > Nit: COALESCE should be capitalized. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:192: Cu.reportError("Error during writing of filter hits: " + reason); On 2016/03/22 21:32:28, Wladimir Palant wrote: > Variable reason is not defined. > > Also a nit: "Error updating filter hits"? Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:197: Cu.reportError("Writing of filter hits canceled or aborted: " + reason); On 2016/03/22 21:32:32, Wladimir Palant wrote: > Nit: "Updating filter hits canceled or aborted"? Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:198: } On 2016/03/22 21:32:27, Wladimir Palant wrote: > I actually meant getting rid of bind(this) by means of arrow functions - even > though we don't use `this` in these two callbacks, with arrow functions we > wouldn't need to think about that. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:207: if (!this.getStorageFile) On 2016/03/22 21:32:34, Wladimir Palant wrote: > As above, this should be checking this.connection rather than some unrelated > property. Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:222: Prefs.sendstats_last_push = now; On 2016/03/22 21:32:25, Wladimir Palant wrote: > I don't think you want to set sendstats_last_push before actually sending the > data. I tried to fix the local time change issue here, but doesn't matter anymore while this is being handled by donwloader with new implemantation. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:224: if ((now - Prefs.sendstats_last_push < PUSH_INTERVAL) || !Prefs.sendstats On 2016/03/22 21:32:24, Wladimir Palant wrote: > Why are you double-checking the push interval? Supposedly, Downloader has done > all this already. My bad, re-implemented this part. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:228: let statement = this.connection.createStatement("SELECT filterhits.*, " + On 2016/03/22 21:32:33, Wladimir Palant wrote: > I guess you want to check this.connection here as well? Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:278: this._sending = false; On 2016/03/22 21:32:33, Wladimir Palant wrote: > Set lastError here as well? Changed implementation, this should be done by Downloader already. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:295: if (!this._sending) On 2016/03/22 21:32:29, Wladimir Palant wrote: > Shouldn't you check this *before* going through all the effort compiling the > data? Like at the beginning of the function? Changed implementation, this should be done by Downloader already. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:304: Cu.reportError(error.message); On 2016/03/22 21:32:19, Wladimir Palant wrote: > More informative message here as well? "Error loading filter hits: " + > error.message Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/filterH... lib/filterHits.js:320: return Math.round(timestamp / MILLIS_IN_HOUR) * 3600; On 2016/03/22 21:32:30, Wladimir Palant wrote: > 3600 is actually (MILLIS_IN_HOUR / MILLIS_IN_SECOND) Done. https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/prefs.json File lib/prefs.json (right): https://codereview.adblockplus.org/6337686776315904/diff/29338658/lib/prefs.j... lib/prefs.json:31: "sendstats_last_push": 0, On 2016/03/22 21:32:36, Wladimir Palant wrote: > How about making last_push part of sendstats_status as well? Done, you think I can also make "sendstats" part of "sendstats_status" and rename it to "sendstats" ? https://codereview.adblockplus.org/6337686776315904/diff/29339506/lib/filterH... File lib/filterHits.js (right): https://codereview.adblockplus.org/6337686776315904/diff/29339506/lib/filterH... lib/filterHits.js:140: downloadable.softExpiration = Prefs.sendstats_status.lastPush + PUSH_INTERVAL; I'm not sure regarding this, should we specify softExpiration ? BTW why do we have softExpiration ? What it suppose to do ?
This review is no longer relevant, closing it. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
