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

Unified Diff: lib/filterStorage.js

Issue 29886685: Issue 6856 - Remove FilterStorage.moveSubscription (Closed) Base URL: https://hg.adblockplus.org/adblockpluscore
Patch Set: Address PS4 Comment Created Oct. 1, 2018, 2:31 a.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 | « lib/filterListener.js ('k') | lib/synchronizer.js » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -75,37 +75,49 @@
/**
* Map of properties listed in the filter storage file before the sections
* start. Right now this should be only the format version.
*/
fileProperties: Object.create(null),
/**
- * List of filter subscriptions containing all filters
- * @type {Subscription[]}
+ * Yields subscriptions containing all filters
+ * @yields {Subscription}
*/
- subscriptions: [],
+ *subscriptions()
+ {
+ yield* this.knownSubscriptions.values();
+ },
+
+ /**
+ * Number of known subscriptions.
+ * @type {number}
+ */
+ get subscriptionCount()
+ {
+ return this.knownSubscriptions.size;
+ },
/**
* Map of subscriptions already on the list, by their URL/identifier
* @type {Map.<string,Subscription>}
*/
knownSubscriptions: new Map(),
/**
* Finds the filter group that a filter should be added to by default. Will
* return null if this group doesn't exist yet.
* @param {Filter} filter
* @return {?SpecialSubscription}
*/
getGroupForFilter(filter)
{
let generalSubscription = null;
- for (let subscription of FilterStorage.subscriptions)
+ for (let subscription of FilterStorage.subscriptions())
Manish Jethani 2018/10/01 10:40:45 Since this is within the FilterStorage object, may
Jon Sonesen 2018/10/01 15:40:57 Yeah, I think you are right. I had the same though
Manish Jethani 2018/10/01 16:12:52 I'm not sure, but it shouldn't hurt if we just avo
Jon Sonesen 2018/10/02 16:23:48 Done.
{
if (subscription instanceof SpecialSubscription && !subscription.disabled)
{
// Always prefer specialized subscriptions
if (subscription.isDefaultFor(filter))
return subscription;
// If this is a general subscription - store it as fallback
@@ -123,75 +135,40 @@
* Adds a filter subscription to the list
* @param {Subscription} subscription filter subscription to be added
*/
addSubscription(subscription)
{
if (FilterStorage.knownSubscriptions.has(subscription.url))
return;
- FilterStorage.subscriptions.push(subscription);
FilterStorage.knownSubscriptions.set(subscription.url, subscription);
addSubscriptionFilters(subscription);
filterNotifier.emit("subscription.added", subscription);
},
/**
* Removes a filter subscription from the list
* @param {Subscription} subscription filter subscription to be removed
*/
removeSubscription(subscription)
{
- for (let i = 0; i < FilterStorage.subscriptions.length; i++)
- {
- if (FilterStorage.subscriptions[i].url == subscription.url)
- {
- removeSubscriptionFilters(subscription);
-
- FilterStorage.subscriptions.splice(i--, 1);
- FilterStorage.knownSubscriptions.delete(subscription.url);
-
- // This should be the last remaining reference to the Subscription
- // object.
- Subscription.knownSubscriptions.delete(subscription.url);
-
- filterNotifier.emit("subscription.removed", subscription);
- return;
- }
- }
- },
-
- /**
- * Moves a subscription in the list to a new position.
- * @param {Subscription} subscription filter subscription to be moved
- * @param {Subscription} [insertBefore] filter subscription to insert before
- * (if omitted the subscription will be put at the end of the list)
- */
- moveSubscription(subscription, insertBefore)
- {
- let currentPos = FilterStorage.subscriptions.indexOf(subscription);
- if (currentPos < 0)
+ if (!FilterStorage.knownSubscriptions.has(subscription.url))
return;
- let newPos = -1;
- if (insertBefore)
- newPos = FilterStorage.subscriptions.indexOf(insertBefore);
+ removeSubscriptionFilters(subscription);
- if (newPos < 0)
- newPos = FilterStorage.subscriptions.length;
+ FilterStorage.knownSubscriptions.delete(subscription.url);
- if (currentPos < newPos)
- newPos--;
- if (currentPos == newPos)
- return;
+ // This should be the last remaining reference to the Subscription
+ // object.
+ Subscription.knownSubscriptions.delete(subscription.url);
- FilterStorage.subscriptions.splice(currentPos, 1);
- FilterStorage.subscriptions.splice(newPos, 0, subscription);
- filterNotifier.emit("subscription.moved", subscription);
+ filterNotifier.emit("subscription.removed", subscription);
},
/**
* Replaces the list of filters in a subscription by a new list
* @param {Subscription} subscription filter subscription to be updated
* @param {Filter[]} filters new filter list
*/
updateSubscriptionFilters(subscription, filters)
@@ -368,17 +345,16 @@
parser.process(line);
if (line === null)
{
let knownSubscriptions = new Map();
for (let subscription of parser.subscriptions)
knownSubscriptions.set(subscription.url, subscription);
this.fileProperties = parser.fileProperties;
- this.subscriptions = parser.subscriptions;
Manish Jethani 2018/10/01 10:53:13 Note: An implication of this change is that a patt
Jon Sonesen 2018/10/01 15:40:57 Acknowledged.
this.knownSubscriptions = knownSubscriptions;
Filter.knownFilters = parser.knownFilters;
Subscription.knownSubscriptions = parser.knownSubscriptions;
if (!silent)
filterNotifier.emit("load");
}
};
@@ -389,17 +365,17 @@
* @return {Promise} promise resolved or rejected when loading is complete
*/
loadFromDisk()
{
let tryBackup = backupIndex =>
{
return this.restoreBackup(backupIndex, true).then(() =>
{
- if (this.subscriptions.length == 0)
+ if (this.subscriptionCount == 0)
Manish Jethani 2018/10/01 10:40:45 Here (and below) too I think it's probably better
Jon Sonesen 2018/10/01 15:40:57 Acknowledged.
Jon Sonesen 2018/10/02 16:23:48 Done.
return tryBackup(backupIndex + 1);
}).catch(error =>
{
// Give up
});
};
return IO.statFile(this.sourceFile).then(statData =>
@@ -409,17 +385,17 @@
this.firstRun = true;
return;
}
let parser = this.importData(true);
return IO.readFromFile(this.sourceFile, parser).then(() =>
{
parser(null);
- if (this.subscriptions.length == 0)
+ if (this.subscriptionCount == 0)
{
// No filter subscriptions in the file, this isn't right.
throw new Error("No data in the file");
}
});
}).catch(error =>
{
Cu.reportError(error);
@@ -463,17 +439,17 @@
},
/**
* Generator serializing filter data and yielding it line by line.
*/
*exportData()
{
// Do not persist external subscriptions
- let subscriptions = this.subscriptions.filter(
+ let subscriptions = [...this.subscriptions()].filter(
s => !(s instanceof ExternalSubscription)
);
yield "# Adblock Plus preferences";
yield "version=" + formatVersion;
let saved = new Set();
let buf = [];
« no previous file with comments | « lib/filterListener.js ('k') | lib/synchronizer.js » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld