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

Delta Between Two Patch Sets: lib/subscriptionClasses.js

Issue 29934588: Issue 7094 - Encapsulate management of subscription filters (Closed) Base URL: https://hg.adblockplus.org/adblockpluscore/
Left Patch Set: Simplify patch Created Nov. 17, 2018, 9:25 p.m.
Right Patch Set: Remove hasFilter and related code Created Nov. 18, 2018, 10:21 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/iniParser.js ('k') | lib/synchronizer.js » ('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-present eyeo GmbH 3 * Copyright (C) 2006-present 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
(...skipping 22 matching lines...) Expand all
33 * @param {string} [title] title of the filter subscription 33 * @param {string} [title] title of the filter subscription
34 * @constructor 34 * @constructor
35 */ 35 */
36 function Subscription(url, title) 36 function Subscription(url, title)
37 { 37 {
38 this.url = url; 38 this.url = url;
39 39
40 this._filterText = []; 40 this._filterText = [];
41 this._filters = []; 41 this._filters = [];
42 42
43 this._filterTextSet = new Set();
44
45 if (title) 43 if (title)
46 this._title = title; 44 this._title = title;
47 45
48 Subscription.knownSubscriptions.set(url, this); 46 Subscription.knownSubscriptions.set(url, this);
49 } 47 }
50 exports.Subscription = Subscription; 48 exports.Subscription = Subscription;
51 49
52 Subscription.prototype = 50 Subscription.prototype =
53 { 51 {
54 /** 52 /**
55 * Download location of the subscription 53 * Download location of the subscription
56 * @type {string} 54 * @type {string}
57 */ 55 */
58 url: null, 56 url: null,
59 57
60 /** 58 /**
61 * Type of the subscription 59 * Type of the subscription
62 * @type {?string} 60 * @type {?string}
63 */ 61 */
64 type: null, 62 type: null,
65 63
66 /** 64 /**
67 * Filter text contained in the filter subscription. 65 * Filter text contained in the filter subscription.
68 * @type {Array.<string>} 66 * @type {Array.<string>}
69 * @private 67 * @private
70 */ 68 */
71 _filterText: null, 69 _filterText: null,
hub 2018/11/19 17:47:49 shouldn't this be named `_filtersText` since it is
Manish Jethani 2018/11/20 00:19:39 I thought about calling it _filtersText but that s
hub 2018/11/20 19:30:19 my bad. let's leave it at that.
72 70
73 /** 71 /**
74 * {@link Filter} objects corresponding to the subscription's filter text. 72 * {@link Filter} objects corresponding to the subscription's filter text.
75 * @type {Array.<Filter>} 73 * @type {Array.<Filter>}
76 * @private 74 * @private
77 */ 75 */
78 _filters: null, 76 _filters: null,
79
80 /**
81 * Set of filter text contained in the filter subscription, used for faster
82 * lookup.
83 * @type {Set.<string>}
84 * @private
85 */
86 _filterTextSet: null,
87 77
88 _title: null, 78 _title: null,
89 _fixedTitle: false, 79 _fixedTitle: false,
90 _disabled: false, 80 _disabled: false,
91 81
92 /** 82 /**
93 * Title of the filter subscription 83 * Title of the filter subscription
94 * @type {string} 84 * @type {string}
95 */ 85 */
96 get title() 86 get title()
(...skipping 87 matching lines...) Expand 10 before | Expand all | Expand 10 after
184 }, 174 },
185 175
186 /** 176 /**
187 * Returns the 0-based index of the given filter. 177 * Returns the 0-based index of the given filter.
188 * @param {Filter} filter 178 * @param {Filter} filter
189 * @param {number} [fromIndex] The index from which to start the search. 179 * @param {number} [fromIndex] The index from which to start the search.
190 * @return {number} 180 * @return {number}
191 */ 181 */
192 searchFilter(filter, fromIndex = 0) 182 searchFilter(filter, fromIndex = 0)
193 { 183 {
194 return this._filterText.indexOf(filter.text, fromIndex); 184 return this._filterText.indexOf(filter.text, fromIndex);
Manish Jethani 2018/11/17 21:41:09 Note: We look up a filter always by the text, not
195 }, 185 },
196 186
197 /** 187 /**
198 * Checks whether the subscription contains the given filter.
199 * @param {Filter} filter
200 * @return {boolean}
201 */
202 hasFilter(filter)
Manish Jethani 2018/11/17 21:41:09 We will use this in this patch: https://codereview
203 {
204 return this._filterTextSet.has(filter.text);
205 },
206
207 /**
208 * Removes all filters from the subscription. 188 * Removes all filters from the subscription.
209 */ 189 */
210 clearFilters() 190 clearFilters()
211 { 191 {
212 this._filterText = []; 192 this._filterText = [];
213 this._filters = []; 193 this._filters = [];
214
215 this._filterTextSet.clear();
216 }, 194 },
217 195
218 /** 196 /**
219 * Adds a filter to the subscription. 197 * Adds a filter to the subscription.
220 * @param {Filter} filter 198 * @param {Filter} filter
221 */ 199 */
222 addFilter(filter) 200 addFilter(filter)
223 { 201 {
224 this._filterText.push(filter.text); 202 this._filterText.push(filter.text);
225 this._filters.push(filter); 203 this._filters.push(filter);
226
227 this._filterTextSet.add(filter.text);
228 }, 204 },
229 205
230 /** 206 /**
231 * Inserts a filter into the subscription. 207 * Inserts a filter into the subscription.
232 * @param {Filter} filter 208 * @param {Filter} filter
233 * @param {number} index The index at which to insert the filter. 209 * @param {number} index The index at which to insert the filter.
234 */ 210 */
235 insertFilterAt(filter, index) 211 insertFilterAt(filter, index)
236 { 212 {
237 this._filterText.splice(index, 0, filter.text); 213 this._filterText.splice(index, 0, filter.text);
238 this._filters.splice(index, 0, filter); 214 this._filters.splice(index, 0, filter);
239
240 this._filterTextSet.add(filter.text);
241 }, 215 },
242 216
243 /** 217 /**
244 * Deletes a filter from the subscription. 218 * Deletes a filter from the subscription.
245 * @param {number} index The index at which to delete the filter. 219 * @param {number} index The index at which to delete the filter.
246 */ 220 */
247 deleteFilterAt(index) 221 deleteFilterAt(index)
248 { 222 {
249 let items = this._filterText.splice(index, 1); 223 // Ignore index if out of bounds on the negative side, for consistency.
250 224 if (index < 0)
225 return;
226
227 this._filterText.splice(index, 1);
251 this._filters.splice(index, 1); 228 this._filters.splice(index, 1);
252
253 // Performance note: A subscription can contain the same filter multiple
254 // times. We can only delete the text from the set if all occurrences of
255 // the filter have been removed. This makes deletion an expensive
256 // operation. It is only needed when a user-defined filter is removed,
257 // which is relatively rare. In comparison, filters must be looked up in
258 // subscriptions a lot more often; therefore, it makes sense to maintain
259 // the set at this cost. An alternative here would be to use a map instead
260 // of a set, with the value of each entry being the reference count, but
261 // this would make addition more expensive thus slowing down the loading of
262 // the initial subscriptions.
263 if (items.length > 0 && this._filterText.indexOf(items[0]) == -1)
264 this._filterTextSet.delete(items[0]);
265 }, 229 },
266 230
267 /** 231 /**
268 * Serializes the subscription for writing out on disk. 232 * Serializes the subscription for writing out on disk.
269 * @yields {string} 233 * @yields {string}
270 */ 234 */
271 *serialize() 235 *serialize()
272 { 236 {
273 let {url, type, _title, _fixedTitle, _disabled} = this; 237 let {url, type, _title, _fixedTitle, _disabled} = this;
274 238
(...skipping 447 matching lines...) Expand 10 before | Expand all | Expand 10 after
722 if (errors) 686 if (errors)
723 yield "errors=" + errors; 687 yield "errors=" + errors;
724 if (version) 688 if (version)
725 yield "version=" + version; 689 yield "version=" + version;
726 if (requiredVersion) 690 if (requiredVersion)
727 yield "requiredVersion=" + requiredVersion; 691 yield "requiredVersion=" + requiredVersion;
728 if (downloadCount) 692 if (downloadCount)
729 yield "downloadCount=" + downloadCount; 693 yield "downloadCount=" + downloadCount;
730 } 694 }
731 }); 695 });
LEFTRIGHT

Powered by Google App Engine
This is Rietveld