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

Side by Side Diff: lib/filterHits.js

Issue 6337686776315904: Issue 394 - hit statistics tool data collection (Closed)
Patch Set: Addressed Wladimir comments (save data in the database instead of memory) Created Feb. 19, 2016, 5:20 p.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View unified diff | Download patch
« defaults/prefs.json ('K') | « lib/contentPolicy.js ('k') | no next file » | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
(Empty)
1 /*
2 * This file is part of Adblock Plus <https://adblockplus.org/>,
3 * Copyright (C) 2006-2016 Eyeo GmbH
4 *
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
7 * published by the Free Software Foundation.
8 *
9 * Adblock Plus is distributed in the hope that it will be useful,
10 * but WITHOUT ANY WARRANTY; without even the implied warranty of
11 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12 * GNU General Public License for more details.
13 *
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/>.
16 */
17
18 let {Services} = Cu.import("resource://gre/modules/Services.jsm", null);
19 let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", null);
20 let {setTimeout} = Cu.import("resource://gre/modules/Timer.jsm", null);
21
22 let {Prefs} = require("prefs");
23 let {Utils} = require("utils");
24 let {MILLIS_IN_DAY} = require("downloader");
25 let {FilterNotifier} = require("filterNotifier");
26 let {DownloadableSubscription} = require("subscriptionClasses");
27
28 /**
29 * This class collect filter hits statistics in SQLite database
30 * and send them to the server when user opt-in for that
Wladimir Palant 2016/02/29 14:35:55 Nit: collect => collects, in SQLite => in an SQLit
saroyanm 2016/03/18 18:23:13 Done.
31 * @class
32 */
33 let FilterHits = exports.FilterHits =
34 {
35 /**
36 * Indicates the timeframe between pushes
37 * @type Number
38 */
39 _pushInterval: MILLIS_IN_DAY * 7,
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
41 /**
42 * The value of storage statement normal execution constant
43 * @type Number
44 */
45 _finished: Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED,
Wladimir Palant 2016/02/29 14:35:48 Same here, constants shouldn't be properties.
saroyanm 2016/03/18 18:23:23 Done.
46
47 /**
48 * Indicates whether the data is being sent to the server
49 * @type Boolean
50 */
51 _sending: false,
52
53 /**
54 * Called on module startup.
55 */
56 init: function()
57 {
58 Prefs.addListener(function(name)
59 {
60 if (name == "sendstats")
61 {
62 setTimeout(function()
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.
63 {
64 if (!Prefs[name])
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();
66 }.bind(this), 1000);
67 }
68 }.bind(this));
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
70 let now = new Date().getTime();
71 if (now && (now - Prefs.sendstats_last_push > this._pushInterval))
72 this.sendFilterHitsToServer();
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
73 },
74
75 /**
76 * Increases the filter hit count
77 * @param {Filter} filter
78 * @param {String} host
79 */
80 increaseFilterHits: function(filter, host)
81 {
82 let subscriptions = filter.subscriptions;
83 let downloadableSubscriptions = [];
84 for (let i = 0; i < subscriptions.length; i++)
85 {
86 if (subscriptions[i] instanceof DownloadableSubscription)
87 downloadableSubscriptions.push(subscriptions[i].url);
88 }
89
90 if (downloadableSubscriptions.length == 0)
91 return;
92
93 let storageFile = this.getStorageFile();
94 if (!storageFile)
95 return;
96
97 let connection = Services.storage.openDatabase(storageFile);
98 this.checkCreateTable(connection);
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
100 let statementText = "INSERT OR REPLACE INTO filterhits " +
101 "(filter, host, firstParty, hitCount, lastHit, subscriptions) VALUES " +
102 "(:filter, :host, :firstParty, coalesce ((SELECT hitCount " +
103 "FROM filterhits WHERE filter=:filter AND host=:host AND " +
104 "firstParty=:firstParty), 0) + 1, :lastHit, :subscriptions)";
105
106 let statement = connection.createStatement(statementText);
107 statement.params.filter = filter.text;
108 statement.params.host = host;
109 statement.params.lastHit = filter.lastHit;
110 statement.params.firstParty = filter.thirdParty != "thirdParty";
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);
112
113 statement.executeAsync(
114 {
115 handleError: function(aError)
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 {
117 Cu.reportError(aError.message);
118 },
119
120 handleCompletion: function(aReason)
121 {
122 if (aReason != this._finished)
123 Cu.reportError("Writing of filter hits canceled or aborted.");
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)
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 },
129
130 /**
131 * Remove all local collected filter hits data
132 */
133 resetFilterHits: function()
134 {
135 let storageFile = this.getStorageFile();
136 if (!storageFile)
137 return;
138
139 let connection = Services.storage.openDatabase(storageFile);
140 connection.executeSimpleSQL("DROP TABLE IF EXISTS filterhits");
Wladimir Palant 2016/02/29 14:35:56 Probably better to leave the database initialized
saroyanm 2016/03/18 18:23:29 Done.
141 },
142
143 /**
144 * Send local stored filter hit data to the
145 * service specified in Prefs.sendstats_url
146 */
147 sendFilterHitsToServer: function()
148 {
149 if (!Prefs.sendstats)
150 return;
151
152 let storageFile = this.getStorageFile();
153 if (!storageFile)
154 return;
155
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 {
168
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 {
200 Prefs.sendstats_last_push = new Date().getTime();
201 FilterHits.resetFilterHits();
202 }
203 else
204 {
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 }
260 },
261 };
262
263 FilterHits.init();
OLDNEW
« defaults/prefs.json ('K') | « lib/contentPolicy.js ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld