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

Unified Diff: lib/filterStorage.js

Issue 29408742: Issue 5059 - Simplify I/O API and FilterStorage implementation (Closed) Base URL: https://hg.adblockplus.org/adblockpluscore
Patch Set: Created April 10, 2017, 2:44 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
« no previous file with comments | « no previous file | test/filterStorage_readwrite.js » ('j') | test/filterStorage_readwrite.js » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -17,26 +17,22 @@
"use strict";
/**
* @fileOverview FilterStorage class responsible for managing user's
* subscriptions and filters.
*/
-const {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", {});
-const {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
-
const {IO} = require("io");
const {Prefs} = require("prefs");
const {Filter, ActiveFilter} = require("filterClasses");
const {Subscription, SpecialSubscription,
ExternalSubscription} = require("subscriptionClasses");
const {FilterNotifier} = require("filterNotifier");
-const {Utils} = require("utils");
/**
* Version number of the filter storage file format.
* @type {number}
*/
let formatVersion = 4;
/**
@@ -57,59 +53,22 @@ let FilterStorage = exports.FilterStorag
* @type {number}
*/
get formatVersion()
{
return formatVersion;
},
/**
- * File that the filter list has been loaded from and should be saved to
- * @type {nsIFile}
+ * File containing the filter list
+ * @type {string}
*/
get sourceFile()
{
- let file = null;
- if (Prefs.patternsfile)
- {
- // Override in place, use it instead of placing the file in the
- // regular data dir
- file = IO.resolveFilePath(Prefs.patternsfile);
- }
- if (!file)
- {
- // Place the file in the data dir
- file = IO.resolveFilePath(Prefs.data_directory);
- if (file)
- file.append("patterns.ini");
- }
- if (!file)
- {
- // Data directory pref misconfigured? Try the default value
- try
- {
- let dir = Services.prefs.getDefaultBranch("extensions.adblockplus.")
- .getCharPref("data_directory");
- file = IO.resolveFilePath(dir);
- if (file)
- file.append("patterns.ini");
- }
- catch (e) {}
- }
-
- if (!file)
- {
- Cu.reportError("Adblock Plus: Failed to resolve filter file location " +
- "from extensions.adblockplus.patternsfile preference");
- }
-
- // Property is configurable because of the test suite.
- Object.defineProperty(this, "sourceFile",
- {value: file, configurable: true});
- return file;
+ return "patterns.ini";
Wladimir Palant 2017/04/10 14:59:15 We are removing configurability here, there is no
},
/**
* Will be set to true if no patterns.ini file exists.
* @type {boolean}
*/
firstRun: false,
@@ -404,23 +363,25 @@ let FilterStorage = exports.FilterStorag
/**
* @callback TextSink
* @param {string?} line
*/
/**
* Allows importing previously serialized filter data.
+ * @param {boolean} silent
+ * If true, no "load" notification will be sent out.
* @return {TextSink}
* Function to be called for each line of data. Calling it with null as
* parameter finalizes the import and replaces existing data. No changes
* will be applied before finalization, so import can be "aborted" by
* forgetting this callback.
*/
- importData()
+ importData(silent)
{
let parser = new INIParser();
return line =>
{
parser.process(line);
if (line === null)
{
// Old special groups might have been converted, remove them if
@@ -447,105 +408,97 @@ let FilterStorage = exports.FilterStorag
Subscription.knownSubscriptions = parser.knownSubscriptions;
if (parser.userFilters)
{
for (let filter of parser.userFilters)
this.addFilter(Filter.fromText(filter), null, undefined, true);
}
- FilterNotifier.triggerListeners("load");
+ if (!silent)
+ FilterNotifier.triggerListeners("load");
}
};
},
/**
- * Loads all subscriptions from the disk
+ * Loads all subscriptions from the disk.
+ * @return {Promise} promise resolved or rejected when loading is complete
*/
loadFromDisk()
{
- let readFile = () =>
- {
- let parser = {
- process: this.importData()
- };
- IO.readFromFile(this.sourceFile, parser, readFromFileException =>
- {
- this.initialized = true;
-
- if (!readFromFileException && this.subscriptions.length == 0)
- {
- // No filter subscriptions in the file, this isn't right.
- readFromFileException = new Error("No data in the file");
- }
-
- if (readFromFileException)
- Cu.reportError(readFromFileException);
-
- if (readFromFileException)
- tryBackup(1);
- });
- };
-
let tryBackup = backupIndex =>
{
- this.restoreBackup(backupIndex).then(() =>
+ return this.restoreBackup(backupIndex, true).then(() =>
{
if (this.subscriptions.length == 0)
- tryBackup(backupIndex + 1);
+ return tryBackup(backupIndex + 1);
}).catch(error =>
{
// Give up
});
};
- IO.statFile(this.sourceFile, (statError, statData) =>
+ return IO.statFile(this.sourceFile).then(statData =>
{
- if (statError || !statData.exists)
+ if (!statData.exists)
{
this.firstRun = true;
- this.initialized = true;
- FilterNotifier.triggerListeners("load");
+ return;
}
- else
- readFile();
+
+ let parser = this.importData(true);
+ return IO.readFromFile(this.sourceFile, parser).then(() =>
+ {
+ parser(null);
+ if (this.subscriptions.length == 0)
+ {
+ // No filter subscriptions in the file, this isn't right.
+ throw new Error("No data in the file");
+ }
+ });
+ }).catch(error =>
+ {
+ Cu.reportError(error);
+ return tryBackup(1);
+ }).then(() =>
+ {
+ this.initialized = true;
+ FilterNotifier.triggerListeners("load");
Wladimir Palant 2017/04/10 14:59:15 There is a change here: load notification is fired
});
},
/**
+ * Constructs the file name for a patterns.ini backup.
+ * @param {number} backupIndex
+ * number of the backup file (1 being the most recent)
+ * @return {string} backup file name
+ */
+ getBackupName(backupIndex)
+ {
+ let [name, extension] = this.sourceFile.split(".", 2);
+ return (name + "-backup" + backupIndex + "." + extension);
+ },
+
+ /**
* Restores an automatically created backup.
* @param {number} backupIndex
* number of the backup to restore (1 being the most recent)
+ * @param {boolean} silent
+ * If true, no "load" notification will be sent out.
* @return {Promise} promise resolved or rejected when restoring is complete
*/
- restoreBackup(backupIndex)
+ restoreBackup(backupIndex, silent)
{
- return new Promise((resolve, reject) =>
+ let backupFile = this.getBackupName(backupIndex);
+ let parser = this.importData(silent);
+ return IO.readFromFile(backupFile, parser).then(() =>
{
- // Attempt to load a backup
- let [, part1, part2] = /^(.*)(\.\w+)$/.exec(
- this.sourceFile.leafName
- ) || [null, this.sourceFile.leafName, ""];
-
- let backupFile = this.sourceFile.clone();
- backupFile.leafName = (part1 + "-backup" + backupIndex + part2);
-
- let parser = {
- process: this.importData()
- };
- IO.readFromFile(backupFile, parser, error =>
- {
- if (error)
- reject(error);
- else
- {
- this.saveToDisk();
- resolve();
- }
- });
+ parser(null);
+ return this.saveToDisk();
});
},
/**
* Generator serializing filter data and yielding it line by line.
*/
*exportData()
{
@@ -603,121 +556,100 @@ let FilterStorage = exports.FilterStorag
* Will be set to true if a saveToDisk() call arrives while saveToDisk() is
* already running (delayed execution).
* @type {boolean}
*/
_needsSave: false,
/**
* Saves all subscriptions back to disk
+ * @return {Promise} promise resolved or rejected when saving is complete
*/
saveToDisk()
{
if (this._saving)
{
this._needsSave = true;
return;
}
- // Make sure the file's parent directory exists
- let targetFile = this.sourceFile;
- try
- {
- targetFile.parent.create(Ci.nsIFile.DIRECTORY_TYPE,
- FileUtils.PERMS_DIRECTORY);
- }
- catch (e) {}
Wladimir Palant 2017/04/10 14:59:15 This code block is removed, now the IO module is r
-
- let writeFilters = () =>
- {
- IO.writeToFile(targetFile, this.exportData(), e =>
- {
- this._saving = false;
-
- if (e)
- Cu.reportError(e);
-
- if (this._needsSave)
- {
- this._needsSave = false;
- this.saveToDisk();
- }
- else
- FilterNotifier.triggerListeners("save");
- });
- };
-
- let checkBackupRequired = (callbackNotRequired, callbackRequired) =>
- {
- if (Prefs.patternsbackups <= 0)
- callbackNotRequired();
- else
- {
- IO.statFile(targetFile, (statFileException, statData) =>
- {
- if (statFileException || !statData.exists)
- callbackNotRequired();
- else
- {
- let [, part1, part2] = /^(.*)(\.\w+)$/.exec(targetFile.leafName) ||
- [null, targetFile.leafName, ""];
- let newestBackup = targetFile.clone();
- newestBackup.leafName = part1 + "-backup1" + part2;
- IO.statFile(
- newestBackup,
- (statBackupFileException, statBackupData) =>
- {
- if (!statBackupFileException && (!statBackupData.exists ||
- (Date.now() - statBackupData.lastModified) /
- 3600000 >= Prefs.patternsbackupinterval))
- {
- callbackRequired(part1, part2);
- }
- else
- callbackNotRequired();
- }
- );
- }
- });
- }
- };
-
- let removeLastBackup = (part1, part2) =>
- {
- let file = targetFile.clone();
- file.leafName = part1 + "-backup" + Prefs.patternsbackups + part2;
- IO.removeFile(
- file, e => renameBackup(part1, part2, Prefs.patternsbackups - 1)
- );
- };
-
- let renameBackup = (part1, part2, index) =>
- {
- if (index > 0)
- {
- let fromFile = targetFile.clone();
- fromFile.leafName = part1 + "-backup" + index + part2;
-
- let toName = part1 + "-backup" + (index + 1) + part2;
-
- IO.renameFile(fromFile, toName, e => renameBackup(part1, part2,
- index - 1));
- }
- else
- {
- let toFile = targetFile.clone();
- toFile.leafName = part1 + "-backup" + (index + 1) + part2;
-
- IO.copyFile(targetFile, toFile, writeFilters);
- }
- };
-
this._saving = true;
- checkBackupRequired(writeFilters, removeLastBackup);
+ return Promise.resolve().then(() =>
+ {
+ // First check whether we need to create a backup
+ if (Prefs.patternsbackups <= 0)
+ return false;
+
+ return IO.statFile(this.sourceFile).then(statData =>
+ {
+ if (!statData.exists)
+ return false;
+
+ return IO.statFile(this.getBackupName(1)).then(backupStatData =>
+ {
+ if (backupStatData.exists &&
+ (Date.now() - backupStatData.lastModified) / 3600000 <
+ Prefs.patternsbackupinterval)
+ {
+ return false;
+ }
+ return true;
+ });
+ });
+ }).then(backupRequired =>
+ {
+ if (!backupRequired)
+ return;
+
+ let ignoreErrors = error =>
+ {
+ // Expected error, backup file doesn't exist.
+ };
+
+ let renameBackup = index =>
+ {
+ if (index > 0)
+ {
+ return IO.renameFile(this.getBackupName(index),
+ this.getBackupName(index + 1))
+ .catch(ignoreErrors)
+ .then(() => renameBackup(index - 1));
+ }
+
+ return IO.renameFile(this.sourceFile, this.getBackupName(1))
+ .catch(ignoreErrors);
Wladimir Palant 2017/04/10 14:59:15 The logic is slightly simplified by not removing t
+ };
+
+ // Rename existing files
+ return renameBackup(Prefs.patternsbackups - 1);
+ }).catch(error =>
+ {
+ // Errors during backup creation shouldn't prevent writing filters.
+ Cu.reportError(error);
+ }).then(() =>
+ {
+ return IO.writeToFile(this.sourceFile, this.exportData());
+ }).then(() =>
+ {
+ FilterNotifier.triggerListeners("save");
+ }).catch(error =>
+ {
+ // If saving failed, report error but continue - we still have to process
+ // flags.
+ Cu.reportError(error);
+ }).then(() =>
+ {
+ this._saving = false;
+ if (this._needsSave)
+ {
+ this._needsSave = false;
+ this.saveToDisk();
+ }
+ });
},
/**
* @typedef FileInfo
* @type {object}
* @property {nsIFile} file
* @property {number} lastModified
*/
@@ -725,40 +657,33 @@ let FilterStorage = exports.FilterStorag
/**
* Returns a promise resolving in a list of existing backup files.
* @return {Promise.<FileInfo[]>}
*/
getBackupFiles()
{
let backups = [];
- let [, part1, part2] = /^(.*)(\.\w+)$/.exec(
- FilterStorage.sourceFile.leafName
- ) || [null, FilterStorage.sourceFile.leafName, ""];
-
function checkBackupFile(index)
{
- return new Promise((resolve, reject) =>
+ return IO.statFile(this.getBackupName(index)).then(statData =>
{
- let file = FilterStorage.sourceFile.clone();
- file.leafName = part1 + "-backup" + index + part2;
+ if (!statData.exists)
+ return backups;
- IO.statFile(file, (error, result) =>
- {
- if (!error && result.exists)
- {
- backups.push({
- index,
- lastModified: result.lastModified
- });
- resolve(checkBackupFile(index + 1));
- }
- else
- resolve(backups);
+ backups.push({
+ index,
+ lastModified: statData.lastModified
});
+ return checkBackupFile(index + 1);
+ }).catch(error =>
+ {
+ // Something went wrong, return whatever data we got so far.
+ Cu.reportError(error);
+ return backups;
});
}
return checkBackupFile(1);
}
};
/**
@@ -789,17 +714,17 @@ function removeSubscriptionFilters(subsc
{
let i = filter.subscriptions.indexOf(subscription);
if (i >= 0)
filter.subscriptions.splice(i, 1);
}
}
/**
- * IO.readFromFile() listener to parse filter data.
+ * Listener returned by FilterStorage.importData(), parses filter data.
* @constructor
*/
function INIParser()
{
this.fileProperties = this.curObj = {};
this.subscriptions = [];
this.knownFilters = Object.create(null);
this.knownSubscriptions = Object.create(null);
@@ -892,16 +817,10 @@ INIParser.prototype =
else if (this.wantObj === false && val)
this.curObj.push(val.replace(/\\\[/g, "["));
}
finally
{
Filter.knownFilters = origKnownFilters;
Subscription.knownSubscriptions = origKnownSubscriptions;
}
-
- // Allow events to be processed every now and then.
- // Note: IO.readFromFile() will deal with the potential reentrance here.
- this.linesProcessed++;
- if (this.linesProcessed % 1000 == 0)
- return Utils.yield();
}
};
« no previous file with comments | « no previous file | test/filterStorage_readwrite.js » ('j') | test/filterStorage_readwrite.js » ('J')

Powered by Google App Engine
This is Rietveld