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

Unified Diff: lib/filterHits.js

Issue 6337686776315904: Issue 394 - hit statistics tool data collection (Closed)
Patch Set: Addressed Wladimir comments (bunch of changes) Created March 18, 2016, 5:32 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: lib/filterHits.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/lib/filterHits.js
@@ -0,0 +1,323 @@
+/*
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-2016 Eyeo GmbH
+ *
+ * Adblock Plus is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Adblock Plus is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+let {Services} = Cu.import("resource://gre/modules/Services.jsm", null);
+let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", null);
+let {setTimeout, clearTimeout} = Cu.import("resource://gre/modules/Timer.jsm", null);
+
+let {IO} = require("io");
+let {Prefs} = require("prefs");
+let {Downloader, Downloadable, MILLIS_IN_SECOND, MILLIS_IN_DAY, MILLIS_IN_HOUR} = require("downloader");
+let {FilterNotifier} = require("filterNotifier");
+let {DownloadableSubscription} = require("subscriptionClasses");
+
+const CHECK_INTERVAL = MILLIS_IN_DAY;
Wladimir Palant 2016/03/22 21:32:32 IMHO this should be one hour, like for subscriptio
saroyanm 2016/04/06 15:15:32 Done.
+const INITIAL_DELAY = MILLIS_IN_SECOND * 5;
Wladimir Palant 2016/03/22 21:32:34 IMHO this is much too short. Given that this isn't
saroyanm 2016/04/06 15:15:38 Done.
+const PUSH_INTERVAL = MILLIS_IN_DAY * 7;
+
+/**
+ * The value of storage statement normal execution constant
+ * @type Number
+ */
+const REASON_FINISHED = Components.interfaces.mozIStorageStatementCallback.REASON_FINISHED;
+
+/**
+ * This class collects filter hits statistics in a SQLite database
+ * and sends them to the server when user opt-in for that
+ * @class
+ */
+let FilterHits = exports.FilterHits =
+{
+ /**
+ * Used to prevent timers running in parallel
+ * @type Number
+ */
+ _prefToggleTimeoutId: 0,
+
+ /**
+ * Indicates whether the data is being sent to the server
+ * @type Boolean
+ */
+ _sending: false,
+
+ get connection()
+ {
+ let connection = Services.storage.openDatabase(this.getStorageFile);
+ if (!connection.tableExists("filtersubscriptions"))
+ {
+ connection.executeSimpleSQL("CREATE TABLE filtersubscriptions " +
+ "(id INTEGER, subscriptions TEXT, PRIMARY KEY" +
+ "(id), UNIQUE(subscriptions))");
+ }
+ if (!connection.tableExists("filterhits"))
+ {
+ connection.executeSimpleSQL("CREATE TABLE filterhits (filter TEXT, " +
+ "host TEXT, thirdParty INTEGER, hitCount INTEGER, lastHit INTEGER, " +
+ "subscriptions INTEGER NOT NULL, PRIMARY KEY(filter, host, " +
+ "thirdParty), FOREIGN KEY(subscriptions) REFERENCES "+
+ "filtersubscriptions(id))");
+ Prefs.sendstats_last_push = Date.now();
+ }
+ Object.defineProperty(this, "connection",
+ {
+ value: connection
+ });
+ return connection;
+ },
+
+ /**
+ * @return nsIFile SQLite database file with filter hits data
+ */
+ get getStorageFile()
Wladimir Palant 2016/03/22 21:32:27 This is a property, its name shouldn't start with
saroyanm 2016/04/06 15:15:26 Done.
+ {
+ let file = IO.resolveFilePath(Prefs.data_directory);
+ if (file)
+ file.append("adblockplus.sqlite");
+
+ Object.defineProperty(this, "getStorageFile",
+ {
+ value: file
+ });
+ return file;
+ },
+
+ /**
+ * Called on module startup.
+ */
+ init: function()
+ {
+ Prefs.addListener(name =>
+ {
+ if (name == "sendstats")
+ {
+ if (this._prefToggleTimeout)
+ clearTimeout(this._prefToggleTimeout);
+ this._prefToggleTimeout = setTimeout(() =>
+ {
+ if (!Prefs.sendstats)
+ this.resetFilterHits();
+ }, 1000);
+ }
+ });
+
+ let downloader = new Downloader(this._getDownloadables.bind(this),
+ INITIAL_DELAY, CHECK_INTERVAL, this.sendFilterHitsToServer);
Wladimir Palant 2016/03/22 21:32:28 Why isn't sendFilterHitsToServer bound? How does i
saroyanm 2016/04/06 15:15:31 My bad, changed the implementation already.
+ downloader.onExpirationChange = this._onExpirationChange.bind(this);
+ onShutdown.add(() => downloader.cancel());
+ },
+
+ /**
+ * Yields a Downloadable instances for the notifications download.
Wladimir Palant 2016/03/22 21:32:25 instances => instance notifications download => fi
saroyanm 2016/04/06 15:15:23 Done.
+ */
+ _getDownloadables: function*()
+ {
+ let downloadable = new Downloadable();
+ if (typeof Prefs.sendstats_status.lastError === "number")
+ downloadable.lastError = Prefs.sendstats_status.lastError;
+ if (typeof Prefs.sendstats_status.lastCheck === "number")
+ downloadable.lastCheck = Prefs.sendstats_status.lastCheck;
+ if (typeof Prefs.sendstats_status.softExpiration === "number")
+ downloadable.softExpiration = Prefs.sendstats_status.softExpiration;
+ if (typeof Prefs.sendstats_status.hardExpiration === "number")
+ downloadable.hardExpiration = Prefs.sendstats_status.hardExpiration;
+ yield downloadable;
+ },
+
+ _onExpirationChange: function(downloadable)
+ {
+ Prefs.sendstats_status.lastCheck = downloadable.lastCheck;
+ Prefs.sendstats_status.softExpiration = downloadable.softExpiration;
+ Prefs.sendstats_status.hardExpiration = downloadable.hardExpiration;
Wladimir Palant 2016/03/22 21:32:26 There is a limitation of Prefs module when it come
saroyanm 2016/04/06 15:15:42 Done.
+ },
+
+ /**
+ * Increases the filter hit count
+ * @param {Filter} filter
+ * @param {String} host
+ */
+ increaseFilterHits: function(filter, host, thirdParty)
+ {
+ let subscriptions = filter.subscriptions;
+ let downloadableSubscriptions = [];
+ for (let i = 0; i < subscriptions.length; i++)
+ {
+ if (subscriptions[i] instanceof DownloadableSubscription)
+ downloadableSubscriptions.push(subscriptions[i].url);
+ }
+
+ if (downloadableSubscriptions.length == 0)
+ return;
+
+ if (!this.getStorageFile)
Wladimir Palant 2016/03/22 21:32:35 How is the storage file related to the code here?
saroyanm 2016/04/06 15:15:41 Done.
+ return;
+
+ let statementsArray = [];
Wladimir Palant 2016/03/22 21:32:20 I don't think there is much point into adding data
saroyanm 2016/04/06 15:15:30 Done.
+ let filtersubscriptions = JSON.stringify(downloadableSubscriptions);
Wladimir Palant 2016/03/22 21:32:31 Nit: this should be camel case, filterSubscription
saroyanm 2016/04/06 15:15:24 Done.
+ let subscriptionStatement = this.connection.createStatement("INSERT OR " +
+ "IGNORE INTO filtersubscriptions (subscriptions) VALUES " +
+ "(:subscriptions)");
+ subscriptionStatement.params.subscriptions = filtersubscriptions;
+ statementsArray.push(subscriptionStatement);
+ let filterHitstatement = this.connection.createStatement("INSERT OR " +
Wladimir Palant 2016/03/22 21:32:30 Nit: proper camel case would be filterHitStatement
saroyanm 2016/04/06 15:15:39 Done.
+ "REPLACE INTO filterhits (filter, host, thirdParty, hitCount, lastHit, " +
+ "subscriptions) VALUES (:filter, :host, :thirdParty, coalesce ((SELECT " +
Wladimir Palant 2016/03/22 21:32:18 Nit: COALESCE should be capitalized.
saroyanm 2016/04/06 15:15:40 Done.
+ "hitCount FROM filterhits WHERE filter=:filter AND host=:host AND " +
+ "thirdParty=:thirdParty), 0) + 1, :lastHit, (SELECT id " +
+ "FROM filtersubscriptions WHERE subscriptions=:subscriptions))");
+ filterHitstatement.params.filter = filter.text;
+ filterHitstatement.params.host = host;
+ filterHitstatement.params.lastHit = roundToHours(filter.lastHit);
+ filterHitstatement.params.thirdParty = thirdParty;
+ filterHitstatement.params.subscriptions = filtersubscriptions;
+ statementsArray.push(filterHitstatement);
+
+ this.connection.executeAsync(statementsArray, statementsArray.length,
+ {
+ handleError: function(error)
+ {
+ Cu.reportError("Error during writing of filter hits: " + reason);
Wladimir Palant 2016/03/22 21:32:28 Variable reason is not defined. Also a nit: "Erro
saroyanm 2016/04/06 15:15:28 Done.
+ },
+ handleCompletion: function(reason)
+ {
+ if (reason != REASON_FINISHED)
+ Cu.reportError("Writing of filter hits canceled or aborted: " + reason);
Wladimir Palant 2016/03/22 21:32:32 Nit: "Updating filter hits canceled or aborted"?
saroyanm 2016/04/06 15:15:27 Done.
+ }
Wladimir Palant 2016/03/22 21:32:27 I actually meant getting rid of bind(this) by mean
saroyanm 2016/04/06 15:15:33 Done.
+ });
+ },
+
+ /**
+ * Remove all local collected filter hits data
+ */
+ resetFilterHits: function()
+ {
+ if (!this.getStorageFile)
Wladimir Palant 2016/03/22 21:32:34 As above, this should be checking this.connection
saroyanm 2016/04/06 15:15:22 Done.
+ return;
+
+ this.connection.executeSimpleSQL("DELETE FROM filterhits");
+ this.connection.executeSimpleSQL("DELETE FROM filtersubscriptions");
+ },
+
+ /**
+ * Send local stored filter hit data to the
+ * service specified in Prefs.sendstats_url
+ */
+ sendFilterHitsToServer: function()
+ {
+ let now = Date.now();
+ if (Prefs.sendstats_last_push > now)
+ Prefs.sendstats_last_push = now;
Wladimir Palant 2016/03/22 21:32:25 I don't think you want to set sendstats_last_push
saroyanm 2016/04/06 15:15:30 I tried to fix the local time change issue here, b
+
+ if ((now - Prefs.sendstats_last_push < PUSH_INTERVAL) || !Prefs.sendstats
Wladimir Palant 2016/03/22 21:32:24 Why are you double-checking the push interval? Sup
saroyanm 2016/04/06 15:15:28 My bad, re-implemented this part.
+ || !this.getStorageFile)
+ return;
+
+ let statement = this.connection.createStatement("SELECT filterhits.*, " +
Wladimir Palant 2016/03/22 21:32:33 I guess you want to check this.connection here as
saroyanm 2016/04/06 15:15:25 Done.
+ "filtersubscriptions.subscriptions FROM filterhits LEFT JOIN " +
+ "filtersubscriptions filtersubscriptions ON " +
+ "filterhits.subscriptions=filtersubscriptions.id");
+ statement.executeAsync(
+ {
+ handleResult: (result) =>
+ {
+ let filters = Object.create(null);
+ for (let row = result.getNextRow(); row;
+ row = result.getNextRow())
+ {
+ let filterText = row.getResultByName("filter");
+ let host = row.getResultByName("host");
+ let matchType = (row.getResultByName("thirdParty") ?
+ "thirdParty" : "firstParty");
+ let hitCount = row.getResultByName("hitCount");
+ let lastHit = row.getResultByName("lastHit");
+ let subscriptions = row.getResultByName("subscriptions");
+
+ if (!(filterText in filters))
+ filters[filterText] = Object.create(null);
+
+ let filter = filters[filterText];
+ filter.subscriptions = subscriptions;
+ filter[matchType] = Object.create(null);
+ filter[matchType][host] = {"hits": hitCount, latest: lastHit};
+ }
+
+ let request = new XMLHttpRequest();
+ request.open("POST", Prefs.sendstats_url);
+ request.setRequestHeader("Content-Type", "application/json");
+ request.addEventListener("load", () =>
+ {
+ this._sending = false;
+ if (request.status >= 200 && request.status < 300)
+ {
+ Prefs.sendstats_status.lastError = 0;
+ Prefs.sendstats_last_push = Date.now();
+ FilterHits.resetFilterHits();
+ }
+ else
+ {
+ Prefs.sendstats_status.lastError = Date.now();
+ this._sending = false;
+ Cu.reportError("Couldn't send filter hit statistics to server. Status code: " + request.status);
+ }
+ }, false);
+ request.addEventListener("error", () =>
+ {
+ this._sending = false;
Wladimir Palant 2016/03/22 21:32:33 Set lastError here as well?
saroyanm 2016/04/06 15:15:41 Changed implementation, this should be done by Dow
+ Cu.reportError("Error occured while sending filter hit statistics to server.");
+ }, false);
+
+ let {addonName, addonVersion, application,
+ applicationVersion, platform, platformVersion} = require("info");
+ let data = {
+ version: 1,
+ timeSincePush: roundToHours(Date.now() - Prefs.sendstats_last_push),
+ addonName: addonName,
+ addonVersion: addonVersion,
+ application: application,
+ applicationVersion: applicationVersion,
+ platform: platform,
+ platformVersion: platformVersion,
+ filters: filters
+ };
+ if (!this._sending)
Wladimir Palant 2016/03/22 21:32:29 Shouldn't you check this *before* going through al
saroyanm 2016/04/06 15:15:21 Changed implementation, this should be done by Dow
+ {
+ this._sending = true;
+ request.send(JSON.stringify(data));
+ }
+ },
+ handleError: function(error)
+ {
+ Prefs.sendstats_status.lastError = Date.now();
+ Cu.reportError(error.message);
Wladimir Palant 2016/03/22 21:32:19 More informative message here as well? "Error load
saroyanm 2016/04/06 15:15:33 Done.
+ },
+ handleCompletion: function(reason)
+ {
+ if (reason != REASON_FINISHED)
+ {
+ Prefs.sendstats_status.lastError = Date.now();
+ Cu.reportError("Loading of filter hits canceled or aborted: " + reason);
+ }
+ }
+ });
+ },
+};
+
+function roundToHours(timestamp)
+{
+ return Math.round(timestamp / MILLIS_IN_HOUR) * 3600;
Wladimir Palant 2016/03/22 21:32:30 3600 is actually (MILLIS_IN_HOUR / MILLIS_IN_SECON
saroyanm 2016/04/06 15:15:24 Done.
+}
+
+FilterHits.init();

Powered by Google App Engine
This is Rietveld