Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Delta Between Two Patch Sets: lib/filterHits.js

Issue 6337686776315904: Issue 394 - hit statistics tool data collection (Closed)
Left Patch Set: Addressed Wladimir comments (save data in the database instead of memory) Created Feb. 19, 2016, 5:20 p.m.
Right Patch Set: Use Downloader to send the data to server Created April 6, 2016, 3 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
« no previous file with change/comment | « lib/contentPolicy.js ('k') | lib/prefs.json » ('j') | no next file with change/comment »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
LEFTRIGHT
1 /* 1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>, 2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-2016 Eyeo GmbH 3 * Copyright (C) 2006-2016 Eyeo GmbH
4 * 4 *
5 * Adblock Plus is free software: you can redistribute it and/or modify 5 * Adblock Plus is free software: you can redistribute it and/or modify
6 * it under the terms of the GNU General Public License version 3 as 6 * it under the terms of the GNU General Public License version 3 as
7 * published by the Free Software Foundation. 7 * published by the Free Software Foundation.
8 * 8 *
9 * Adblock Plus is distributed in the hope that it will be useful, 9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of 10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12 * GNU General Public License for more details. 12 * GNU General Public License for more details.
13 * 13 *
14 * You should have received a copy of the GNU General Public License 14 * You should have received a copy of the GNU General Public License
15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>. 15 * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
16 */ 16 */
17 17
18 let {Services} = Cu.import("resource://gre/modules/Services.jsm", null); 18 let {Services} = Cu.import("resource://gre/modules/Services.jsm", null);
19 let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", null); 19 let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", null);
20 let {setTimeout} = Cu.import("resource://gre/modules/Timer.jsm", null); 20 let {setTimeout, clearTimeout} = Cu.import("resource://gre/modules/Timer.jsm", n ull);
21 21
22 let {IO} = require("io");
22 let {Prefs} = require("prefs"); 23 let {Prefs} = require("prefs");
23 let {Utils} = require("utils"); 24 let {Downloader, Downloadable, MILLIS_IN_SECOND, MILLIS_IN_MINUTE,
24 let {MILLIS_IN_DAY} = require("downloader"); 25 MILLIS_IN_HOUR, MILLIS_IN_DAY} = require("downloader");
25 let {FilterNotifier} = require("filterNotifier"); 26 let {FilterNotifier} = require("filterNotifier");
26 let {DownloadableSubscription} = require("subscriptionClasses"); 27 let {DownloadableSubscription} = require("subscriptionClasses");
27 28
29 const CHECK_INTERVAL = MILLIS_IN_HOUR;
30 const INITIAL_DELAY = MILLIS_IN_MINUTE * 2;
31 const PUSH_INTERVAL = MILLIS_IN_DAY * 7;
32
28 /** 33 /**
29 * This class collect filter hits statistics in SQLite database 34 * The value of storage statement normal execution constant
30 * and send them to the server when user opt-in for that 35 * @type Number
Wladimir Palant 2016/02/29 14:35:55 Nit: collect => collects, in SQLite => in an SQLit
saroyanm 2016/03/18 18:23:13 Done.
36 */
37 const REASON_FINISHED = Components.interfaces.mozIStorageStatementCallback.REASO N_FINISHED;
38
39 /**
40 * This class collects filter hits statistics in a SQLite database
41 * and sends them to the server when user opt-in for that
31 * @class 42 * @class
32 */ 43 */
33 let FilterHits = exports.FilterHits = 44 let FilterHits = exports.FilterHits =
34 { 45 {
35 /** 46 /**
36 * Indicates the timeframe between pushes 47 * Used to prevent timers running in parallel
37 * @type Number 48 * @type Number
38 */ 49 */
39 _pushInterval: MILLIS_IN_DAY * 7, 50 _prefToggleTimeout: 0,
Wladimir Palant 2016/02/29 14:35:50 That's a constant, it really shouldn't be declared
saroyanm 2016/03/18 18:23:30 Done.
40 51
41 /** 52 get connection()
42 * The value of storage statement normal execution constant 53 {
43 * @type Number 54 if (!this.storageFile)
44 */ 55 return null;
45 _finished: Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED, 56
Wladimir Palant 2016/02/29 14:35:48 Same here, constants shouldn't be properties.
saroyanm 2016/03/18 18:23:23 Done.
46 57 let connection = Services.storage.openDatabase(this.storageFile);
47 /** 58 if (!connection.tableExists("filtersubscriptions"))
48 * Indicates whether the data is being sent to the server 59 {
49 * @type Boolean 60 connection.executeSimpleSQL("CREATE TABLE filtersubscriptions " +
50 */ 61 "(id INTEGER, subscriptions TEXT, PRIMARY KEY" +
51 _sending: false, 62 "(id), UNIQUE(subscriptions))");
63 }
64 if (!connection.tableExists("filterhits"))
65 {
66 connection.executeSimpleSQL("CREATE TABLE filterhits (filter TEXT, " +
67 "host TEXT, thirdParty INTEGER, hitCount INTEGER, lastHit INTEGER, " +
68 "subscriptions INTEGER NOT NULL, PRIMARY KEY(filter, host, " +
69 "thirdParty), FOREIGN KEY(subscriptions) REFERENCES "+
70 "filtersubscriptions(id))");
71 }
72 Object.defineProperty(this, "connection",
73 {
74 value: connection
75 });
76 return connection;
77 },
78
79 /**
80 * @return nsIFile SQLite database file with filter hits data
81 */
82 get storageFile()
83 {
84 let file = IO.resolveFilePath(Prefs.data_directory);
85 if (file)
86 file.append("adblockplus.sqlite");
87
88 Object.defineProperty(this, "storageFile",
89 {
90 value: file
91 });
92 return file;
93 },
52 94
53 /** 95 /**
54 * Called on module startup. 96 * Called on module startup.
55 */ 97 */
56 init: function() 98 init: function()
57 { 99 {
58 Prefs.addListener(function(name) 100 if (!Prefs.sendstats_status.lastPush)
101 {
102 Prefs.sendstats_status.lastPush = Date.now();
103 this._saveFilterHitPrefs();
104 }
105 Prefs.addListener(name =>
59 { 106 {
60 if (name == "sendstats") 107 if (name == "sendstats")
61 { 108 {
62 setTimeout(function() 109 if (this._prefToggleTimeout)
Wladimir Palant 2016/02/29 14:35:57 Nit: Creating a number of timers running in parall
saroyanm 2016/03/18 18:23:12 Done.
110 clearTimeout(this._prefToggleTimeout);
111 this._prefToggleTimeout = setTimeout(() =>
63 { 112 {
64 if (!Prefs[name]) 113 if (!Prefs.sendstats)
Wladimir Palant 2016/02/29 14:35:56 Here as well, just use Prefs.sendstats please - it
saroyanm 2016/03/18 18:23:21 Done.
65 this.resetFilterHits(); 114 this.resetFilterHits();
66 }.bind(this), 1000); 115 }, 1000);
67 } 116 }
68 }.bind(this)); 117 });
Wladimir Palant 2016/02/29 14:35:53 bind(this) shouldn't be necessary in most cases, j
saroyanm 2016/03/18 18:23:31 Done.
69 118
70 let now = new Date().getTime(); 119 let downloader = new Downloader(this._getDownloadables.bind(this),
71 if (now && (now - Prefs.sendstats_last_push > this._pushInterval)) 120 INITIAL_DELAY, CHECK_INTERVAL);
72 this.sendFilterHitsToServer(); 121 downloader.onExpirationChange = this._onExpirationChange.bind(this);
Wladimir Palant 2016/02/29 14:35:55 Please use Date.now(). Also, it's a safe bet that
saroyanm 2016/03/18 18:23:16 I'm not sure if it's a good idea to change the Dow
122 downloader.generateRequestData = this._generateFilterHitsData.bind(this);
123 downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
124 downloader.validResponses.push(204);
125 onShutdown.add(() => downloader.cancel());
126 },
127
128 /**
129 * Yields a Downloadable instance for the filter hits push.
130 */
131 _getDownloadables: function*()
132 {
133 let downloadable = new Downloadable(Prefs.sendstats_url);
134 if (Prefs.sendstats_status.lastError)
135 downloadable.lastError = Prefs.sendstats_status.lastError;
136 if (Prefs.sendstats_status.lastCheck)
137 downloadable.lastCheck = Prefs.sendstats_status.lastCheck;
138 if (Prefs.sendstats_status.lastPush)
139 {
140 downloadable.softExpiration = Prefs.sendstats_status.lastPush + PUSH_INTER VAL;
saroyanm 2016/04/06 15:15:45 I'm not sure regarding this, should we specify sof
141 downloadable.hardExpiration = Prefs.sendstats_status.lastPush + PUSH_INTER VAL;
142 }
143 yield downloadable;
144 },
145
146 _onExpirationChange: function(downloadable)
147 {
148 Prefs.sendstats_status.lastCheck = downloadable.lastCheck;
149 this._saveFilterHitPrefs();
150 },
151
152 _saveFilterHitPrefs: function()
153 {
154 Prefs.sendstats_status = JSON.parse(JSON.stringify(Prefs.sendstats_status));
155 },
156
157 _onDownloadSuccess: function(downloadable, responseText, errorCallback, redire ctCallback)
158 {
159 Prefs.sendstats_status.lastError = 0;
160 Prefs.sendstats_status.lastPush = Date.now();
161 this._saveFilterHitPrefs();
73 }, 162 },
74 163
75 /** 164 /**
76 * Increases the filter hit count 165 * Increases the filter hit count
77 * @param {Filter} filter 166 * @param {Filter} filter
78 * @param {String} host 167 * @param {String} host
79 */ 168 */
80 increaseFilterHits: function(filter, host) 169 increaseFilterHits: function(filter, host, thirdParty)
81 { 170 {
82 let subscriptions = filter.subscriptions; 171 let subscriptions = filter.subscriptions;
83 let downloadableSubscriptions = []; 172 let downloadableSubscriptions = [];
84 for (let i = 0; i < subscriptions.length; i++) 173 for (let i = 0; i < subscriptions.length; i++)
85 { 174 {
86 if (subscriptions[i] instanceof DownloadableSubscription) 175 if (subscriptions[i] instanceof DownloadableSubscription)
87 downloadableSubscriptions.push(subscriptions[i].url); 176 downloadableSubscriptions.push(subscriptions[i].url);
88 } 177 }
89 178
90 if (downloadableSubscriptions.length == 0) 179 if (downloadableSubscriptions.length == 0)
91 return; 180 return;
92 181
93 let storageFile = this.getStorageFile(); 182 if (!this.connection)
94 if (!storageFile)
95 return; 183 return;
96 184
97 let connection = Services.storage.openDatabase(storageFile); 185 let statements = [];
98 this.checkCreateTable(connection); 186 let filterSubscriptions = JSON.stringify(downloadableSubscriptions);
Wladimir Palant 2016/02/29 14:35:51 Should we be doing this on every filter hit? Initi
saroyanm 2016/03/18 18:23:24 Good point, I've cached connection opening, but st
99 187 let subscriptionStatement = this.connection.createStatement("INSERT OR " +
100 let statementText = "INSERT OR REPLACE INTO filterhits " + 188 "IGNORE INTO filtersubscriptions (subscriptions) VALUES " +
101 "(filter, host, firstParty, hitCount, lastHit, subscriptions) VALUES " + 189 "(:subscriptions)");
102 "(:filter, :host, :firstParty, coalesce ((SELECT hitCount " + 190 subscriptionStatement.params.subscriptions = filterSubscriptions;
103 "FROM filterhits WHERE filter=:filter AND host=:host AND " + 191 statements.push(subscriptionStatement);
104 "firstParty=:firstParty), 0) + 1, :lastHit, :subscriptions)"; 192 let filterHitStatement = this.connection.createStatement("INSERT OR " +
105 193 "REPLACE INTO filterhits (filter, host, thirdParty, hitCount, lastHit, " +
106 let statement = connection.createStatement(statementText); 194 "subscriptions) VALUES (:filter, :host, :thirdParty, COALESCE ((SELECT " +
107 statement.params.filter = filter.text; 195 "hitCount FROM filterhits WHERE filter=:filter AND host=:host AND " +
108 statement.params.host = host; 196 "thirdParty=:thirdParty), 0) + 1, :lastHit, (SELECT id " +
109 statement.params.lastHit = filter.lastHit; 197 "FROM filtersubscriptions WHERE subscriptions=:subscriptions))");
110 statement.params.firstParty = filter.thirdParty != "thirdParty"; 198 filterHitStatement.params.filter = filter.text;
Wladimir Palant 2016/02/29 14:35:52 This is a filter flag you are checking here, it on
saroyanm 2016/03/18 18:23:27 Done.
111 statement.params.subscriptions = JSON.stringify(downloadableSubscriptions); 199 filterHitStatement.params.host = host;
112 200 filterHitStatement.params.lastHit = roundToHours(filter.lastHit);
113 statement.executeAsync( 201 filterHitStatement.params.thirdParty = thirdParty;
114 { 202 filterHitStatement.params.subscriptions = filterSubscriptions;
115 handleError: function(aError) 203 statements.push(filterHitStatement);
Wladimir Palant 2016/02/29 14:35:45 Nit: we don't use Hungarian notation for parameter
saroyanm 2016/03/18 18:23:14 Done.
116 { 204
117 Cu.reportError(aError.message); 205 this.connection.executeAsync(statements, statements.length,
206 {
207 handleError: (error) =>
208 {
209 Cu.reportError("Error updating filter hits: " + error.message);
118 }, 210 },
119 211 handleCompletion: (reason) =>
120 handleCompletion: function(aReason) 212 {
121 { 213 if (reason != REASON_FINISHED)
122 if (aReason != this._finished) 214 Cu.reportError("Updating filter hits canceled or aborted: " + reason);
123 Cu.reportError("Writing of filter hits canceled or aborted."); 215 }
Wladimir Palant 2016/02/29 14:35:58 Maybe add aReason to the text, so that we can look
saroyanm 2016/03/18 18:23:28 Done.
124 }.bind(this) 216 });
Wladimir Palant 2016/02/29 14:35:49 Please get rid of bind(this) here as well.
saroyanm 2016/03/18 18:23:22 Done.
125 });
126
127 connection.asyncClose();
128 }, 217 },
129 218
130 /** 219 /**
131 * Remove all local collected filter hits data 220 * Remove all local collected filter hits data
132 */ 221 */
133 resetFilterHits: function() 222 resetFilterHits: function()
134 { 223 {
135 let storageFile = this.getStorageFile(); 224 if (!this.connection)
136 if (!storageFile)
137 return; 225 return;
138 226
139 let connection = Services.storage.openDatabase(storageFile); 227 this.connection.executeSimpleSQL("DELETE FROM filterhits");
140 connection.executeSimpleSQL("DROP TABLE IF EXISTS filterhits"); 228 this.connection.executeSimpleSQL("DELETE FROM filtersubscriptions");
Wladimir Palant 2016/02/29 14:35:56 Probably better to leave the database initialized
saroyanm 2016/03/18 18:23:29 Done.
141 }, 229 },
142 230
143 /** 231 _generateFilterHitsData: function(downloadable)
144 * Send local stored filter hit data to the 232 {
145 * service specified in Prefs.sendstats_url 233 return {
146 */ 234 then: (resolve) =>
147 sendFilterHitsToServer: function() 235 {
148 { 236 if (!this.connection)
149 if (!Prefs.sendstats) 237 return;
150 return; 238
151 239 let statement = this.connection.createStatement("SELECT filterhits.*, " +
152 let storageFile = this.getStorageFile(); 240 "filtersubscriptions.subscriptions FROM filterhits LEFT JOIN " +
153 if (!storageFile) 241 "filtersubscriptions filtersubscriptions ON " +
154 return; 242 "filterhits.subscriptions=filtersubscriptions.id");
155 243 statement.executeAsync(
156 let connection = Services.storage.openDatabase(storageFile);
157 this.checkCreateTable(connection);
158
159 let statement = connection.createStatement("SELECT * FROM filterhits");
160 statement.executeAsync(
161 {
162 handleResult: function(aResultSet)
163 {
164 let filters = Object.create(null);
165 for (let row = aResultSet.getNextRow(); row;
166 row = aResultSet.getNextRow())
167 { 244 {
168 245 handleResult: (result) =>
169 let filterText = row.getResultByName("filter");
170 let host = row.getResultByName("host");
171 let filterType = (row.getResultByName("firstParty") == 1 ?
Wladimir Palant 2016/02/29 14:35:52 a) Not filterType, matchType. b) Don't compare to
saroyanm 2016/03/18 18:23:22 Done.
172 "firstParty" : "thirdParty");
173 let hitCount = row.getResultByName("hitCount");
174 let lastHit = row.getResultByName("lastHit");
175 let subscriptions = row.getResultByName("subscriptions");
176
177 if (!(filterText in filters))
178 filters[filterText] = Object.create(null);
179
180 let filter = filters[filterText];
181
182 if (!filter[subscriptions])
183 filter[subscriptions] = subscriptions;
Wladimir Palant 2016/02/29 14:35:53 You probably meant filter.subscriptions here. In
saroyanm 2016/03/18 18:23:26 Done, but not sure if current implementation is wh
184
185 if (!(filterType in filter))
186 filter[filterType] = Object.create(null);
187
188 if (!(host in filter[filterType]))
Wladimir Palant 2016/02/29 14:35:50 This check is pointless, filter/type/host combinat
saroyanm 2016/03/18 18:23:16 Done.
189 filter[filterType][host] = {"hits": hitCount, latest: lastHit};
190 }
191
192 let request = new XMLHttpRequest();
193 request.open("POST", Prefs.sendstats_url);
194 request.setRequestHeader("Content-Type", "application/json");
195 request.addEventListener("load", function(event)
Wladimir Palant 2016/02/29 14:35:58 You should handle "error" event as well, and be it
saroyanm 2016/03/18 18:23:27 Done.
196 {
197 FilterHits._sending = false;
Wladimir Palant 2016/02/29 14:35:57 Why are you setting this member? It isn't being us
saroyanm 2016/03/18 18:23:15 Done.
198 if (request.status >= 200 && request.status < 300)
199 { 246 {
200 Prefs.sendstats_last_push = new Date().getTime(); 247 let filters = Object.create(null);
201 FilterHits.resetFilterHits(); 248 for (let row = result.getNextRow(); row;
249 row = result.getNextRow())
250 {
251 let filterText = row.getResultByName("filter");
252 let host = row.getResultByName("host");
253 let matchType = (row.getResultByName("thirdParty") ?
254 "thirdParty" : "firstParty");
255 let hitCount = row.getResultByName("hitCount");
256 let lastHit = row.getResultByName("lastHit");
257 let subscriptions = row.getResultByName("subscriptions");
258
259 if (!(filterText in filters))
260 filters[filterText] = Object.create(null);
261
262 let filter = filters[filterText];
263 filter.subscriptions = subscriptions;
264 filter[matchType] = Object.create(null);
265 filter[matchType][host] = {"hits": hitCount, latest: lastHit};
266 }
267 resolve({timeSincePush: roundToHours(Date.now() -
268 Prefs.sendstats_status.lastPush), "filters": filters});
269 },
270 handleError: (error) =>
271 {
272 Prefs.sendstats_status.lastError = Date.now();
273 this._saveFilterHitPrefs();
274 Cu.reportError("Error loading filter hits:" + error.message);
275 },
276 handleCompletion: (reason) =>
277 {
278 if (reason != REASON_FINISHED)
279 {
280 Prefs.sendstats_status.lastError = Date.now();
281 this._saveFilterHitPrefs();
282 Cu.reportError("Loading of filter hits canceled or aborted: " +
283 reason);
284 }
202 } 285 }
203 else 286 });
204 { 287 }
205 Cu.reportError("Couldn't send filter hit statistics to server.");
Wladimir Palant 2016/02/29 14:35:54 I think that this should be more precise - server
saroyanm 2016/03/18 18:23:24 Done.
206 }
207 }, false);
208
209 let {addonName, addonVersion, application,
210 applicationVersion, platform, platformVersion} = require("info");
211 let data = {
212 version: 1,
213 timeSincePush: Prefs.sendstats_last_push,
Wladimir Palant 2016/02/29 14:35:54 That's the time of last push, not the time *since*
saroyanm 2016/03/18 18:23:30 Done, but I can't understand why we need to use th
214 addonName: addonName,
215 addonVersion: addonVersion,
216 application: application,
217 applicationVersion: applicationVersion,
218 platform: platform,
219 platformVersion: platformVersion,
220 filters: filters
221 };
222
223 this._sending = true;
224 request.send(JSON.stringify(data));
225 }.bind(this),
226 handleError: function(aError)
227 {
228 Cu.reportError(aError.message);
229 },
230 handleCompletion: function(aReason)
231 {
232 if (aReason != this._finished)
233 Cu.reportError("Loading of filter hits canceled or aborted.");
234 }.bind(this)
235 });
236 connection.asyncClose();
237 },
238
239 /**
240 * @return nsIFile SQLite database file with filter hits data
241 */
242 getStorageFile: function()
243 {
244 return FileUtils.getFile("ProfD", ["adblockplus.sqlite"]);
Wladimir Palant 2016/02/29 14:35:51 Please don't create files all over the user's prof
saroyanm 2016/03/18 18:23:14 Done.
245 },
246
247 /**
248 * Creates table in local SQLite database for storing
249 * filter hit data if the does not exists yet
250 */
251 checkCreateTable: function(connection)
252 {
253 if (!connection.tableExists("filterhits"))
254 {
255 connection.executeSimpleSQL("CREATE TABLE filterhits (filter TEXT, " +
256 "host TEXT, firstParty INTEGER, hitCount INTEGER, lastHit INTEGER, " +
Wladimir Palant 2016/02/29 14:35:44 We always have thirdParty as a flag, please use it
saroyanm 2016/03/18 18:23:25 Done.
257 "subscriptions TEXT, PRIMARY KEY(filter, host, firstParty))");
258 Prefs.sendstats_last_push = new Date().getTime();
259 } 288 }
260 }, 289 },
261 }; 290 };
262 291
292 function roundToHours(timestamp)
293 {
294 return Math.round(timestamp / MILLIS_IN_HOUR) *
295 (MILLIS_IN_HOUR / MILLIS_IN_SECOND);
296 }
297
263 FilterHits.init(); 298 FilterHits.init();
LEFTRIGHT

Powered by Google App Engine
This is Rietveld