Issue 29805597: Issue 6699 - Support the "circumvention" filter list as default subscription (Closed)
|
Created:
June 13, 2018, 3:33 a.m. by hub Modified:
June 19, 2018, 9:20 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 6699 - Support the "circumvention" filter list as default subscription
Patch Set 1 #
Total comments: 34
Patch Set 2 : Updated comments/doc #
Total comments: 5
Patch Set 3 : Use "circumvention" instead of "cv" #
Total comments: 6
Patch Set 4 : More review fixes. #
Total comments: 2
Patch Set 5 : Use "==" instead of "in" #
MessagesTotal messages: 16
This implement the logic changes to select a default "cv" subscription. The UI changes are part of https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/34 https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:329: exports.chooseFilterSubscriptions = chooseFilterSubscriptions; The only reason I'm exporting this is for the qunit tests. Maybe it should be exported as "_chooseFilterSubscriptions" instead?
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:90: * Finds the element for the default ad blocking filter subscription based "element" and "subscription" should be plural now? https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:94: * @return {Object} We should probably return a Map here, since it is dynamically constructed. If the keys are always going to be the same ("ads", "cv"), then it's better as an object, but then we should document the keys like so: /** * @typedef {object} SubscriptionMap * @property {?Element} ads * @property {?Element} cv */ /** * ... * @returns {SubscriptionMap} */ https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:110: if (subscriptionType in ["ads", "cv"] && !selectedItem[subscriptionType]) (This is a change in behavior, just noting. Previously the first subscription would always be returned even if there was no "ads" type of subscription. Now the default return value will be an empty object.) https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:115: // The "ads" subscription is the one driving the selection. Why only "ads", shouldn't this logic apply to all types of subscriptions? https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:128: // If multiple items have a matching prefix of the same length: (Note: This comment is a bit misleading, because the chances are not even. If there are two subscriptions with the same prefix, the chances are 75:25 rather than 50:50. This is because the first one gets two shots. The subscriptions that appear after have a significantly lower likelihood of getting selected than the ones that come before, progressively. This would be a serious error in cryptography, probably not so much here; nevertheless the comment should be updated. Let's do it as a separate patch.) https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:139: else if (subscriptionType == "cv") (I notice that the XML file uses full names like "ads", "social", "privacy", and so on. Can we call this "circumvention" or "anticircumvention" rather than "cv", for consistency?) https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:217: let subs = chooseFilterSubscriptions(nodes); OK, so the return value of chooseFilterSubscriptions should be a Map now. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:233: subscription.type = node.getAttribute("type"); Don't we have to add a type property to the Subscription class in core now? You can file an issue for that, if you haven't. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:326: Let's use "//" for the comment here to clearly distinguish it from JSDoc comments. // Export for tests only.
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:128: // If multiple items have a matching prefix of the same length: On 2018/06/13 08:15:16, Manish Jethani wrote: > (Note: This comment is a bit misleading, because the chances are not even. If > there are two subscriptions with the same prefix, the chances are 75:25 rather > than 50:50. This is because the first one gets two shots. The subscriptions that > appear after have a significantly lower likelihood of getting selected than the > ones that come before, progressively. This would be a serious error in > cryptography, probably not so much here; nevertheless the comment should be > updated. Let's do it as a separate patch.) Duh! Sorry, please ignore the above, it's rubbish. I fell for a common fallacy, of course the probability is independent on each selection.
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:115: // The "ads" subscription is the one driving the selection. On 2018/06/13 08:15:16, Manish Jethani wrote: > Why only "ads", shouldn't this logic apply to all types of subscriptions? OK, I guess since the "cv" subscriptions are our own we just have one anyway? In that case it's probably OK, let's leave it as it is.
https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... qunit/tests/subscriptionInit.js:41: .then(response => response.text()) This appears not to be mentioned in the style guide, but I think the convention is to align the `.then` with the `fetch` above it.
updated patch https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:90: * Finds the element for the default ad blocking filter subscription based On 2018/06/13 08:15:16, Manish Jethani wrote: > "element" and "subscription" should be plural now? Done. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:94: * @return {Object} On 2018/06/13 08:15:16, Manish Jethani wrote: > We should probably return a Map here, since it is dynamically constructed. If > the keys are always going to be the same ("ads", "cv"), then it's better as an > object, but then we should document the keys like so: > > /** > * @typedef {object} SubscriptionMap > * @property {?Element} ads > * @property {?Element} cv > */ > > /** > * ... > * @returns {SubscriptionMap} > */ Renamed it "DefaultSubscriptions" https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:110: if (subscriptionType in ["ads", "cv"] && !selectedItem[subscriptionType]) On 2018/06/13 08:15:16, Manish Jethani wrote: > (This is a change in behavior, just noting. Previously the first subscription > would always be returned even if there was no "ads" type of subscription. Now > the default return value will be an empty object.) There is a guarantee to have an "ads" subscription since the subscriptions.xml ship with the add-on. This doesn't change. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:115: // The "ads" subscription is the one driving the selection. On 2018/06/13 13:34:02, Manish Jethani wrote: > On 2018/06/13 08:15:16, Manish Jethani wrote: > > Why only "ads", shouldn't this logic apply to all types of subscriptions? > > OK, I guess since the "cv" subscriptions are our own we just have one anyway? In > that case it's probably OK, let's leave it as it is. Only "ads" and "cv" are relevant here. Also "cv" will have language variants. At least that's the plan. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:128: // If multiple items have a matching prefix of the same length: On 2018/06/13 09:42:29, Manish Jethani wrote: > On 2018/06/13 08:15:16, Manish Jethani wrote: > > (Note: This comment is a bit misleading, because the chances are not even. If > > there are two subscriptions with the same prefix, the chances are 75:25 rather > > than 50:50. This is because the first one gets two shots. The subscriptions > that > > appear after have a significantly lower likelihood of getting selected than > the > > ones that come before, progressively. This would be a serious error in > > cryptography, probably not so much here; nevertheless the comment should be > > updated. Let's do it as a separate patch.) > > Duh! Sorry, please ignore the above, it's rubbish. I fell for a common fallacy, > of course the probability is independent on each selection. Acknowledged. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:139: else if (subscriptionType == "cv") On 2018/06/13 08:15:16, Manish Jethani wrote: > (I notice that the XML file uses full names like "ads", "social", "privacy", and > so on. Can we call this "circumvention" or "anticircumvention" rather than "cv", > for consistency?) It is not visible to the user. We have been using the term "cv" and it is much shorter. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:217: let subs = chooseFilterSubscriptions(nodes); On 2018/06/13 08:15:16, Manish Jethani wrote: > OK, so the return value of chooseFilterSubscriptions should be a Map now. An Object, not a Map. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:233: subscription.type = node.getAttribute("type"); On 2018/06/13 08:15:16, Manish Jethani wrote: > Don't we have to add a type property to the Subscription class in core now? You > can file an issue for that, if you haven't. Added that to issue #6689 (patch not yet in review). https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:326: On 2018/06/13 08:15:16, Manish Jethani wrote: > Let's use "//" for the comment here to clearly distinguish it from JSDoc > comments. > > // Export for tests only. Done. https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... qunit/tests/subscriptionInit.js:41: .then(response => response.text()) On 2018/06/13 13:45:39, Manish Jethani wrote: > This appears not to be mentioned in the style guide, but I think the convention > is to align the `.then` with the `fetch` above it. That's how it is done in `subscriptionInit.js`. Given that it is a continuation it is indented. The linter say so.
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:110: if (subscriptionType in ["ads", "cv"] && !selectedItem[subscriptionType]) On 2018/06/13 16:33:36, hub wrote: > On 2018/06/13 08:15:16, Manish Jethani wrote: > > (This is a change in behavior, just noting. Previously the first subscription > > would always be returned even if there was no "ads" type of subscription. Now > > the default return value will be an empty object.) > > There is a guarantee to have an "ads" subscription since the subscriptions.xml > ship with the add-on. This doesn't change. Acknowledged. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:115: // The "ads" subscription is the one driving the selection. On 2018/06/13 16:33:36, hub wrote: > On 2018/06/13 13:34:02, Manish Jethani wrote: > > On 2018/06/13 08:15:16, Manish Jethani wrote: > > > Why only "ads", shouldn't this logic apply to all types of subscriptions? > > > > OK, I guess since the "cv" subscriptions are our own we just have one anyway? > In > > that case it's probably OK, let's leave it as it is. > > Only "ads" and "cv" are relevant here. Also "cv" will have language variants. At > least that's the plan. Acknowledged. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:139: else if (subscriptionType == "cv") On 2018/06/13 16:33:36, hub wrote: > On 2018/06/13 08:15:16, Manish Jethani wrote: > > (I notice that the XML file uses full names like "ads", "social", "privacy", > and > > so on. Can we call this "circumvention" or "anticircumvention" rather than > "cv", > > for consistency?) > > It is not visible to the user. > We have been using the term "cv" and it is much shorter. We had this discussion about the label to use on Trac, for example. "cv" was suggested, but we went with "circumvention". Even our IRC channel is called #circumvention. "cv" is not a popular abbreviation of circumvention (e.g. you won't find it in any book or dictionary), it's something we invented, so somebody reading the XML would have to wonder what "cv" means exactly. Anyway, my two cents. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:217: let subs = chooseFilterSubscriptions(nodes); On 2018/06/13 16:33:37, hub wrote: > On 2018/06/13 08:15:16, Manish Jethani wrote: > > OK, so the return value of chooseFilterSubscriptions should be a Map now. > > An Object, not a Map. Yeah, I mean let's make it a Map. We have to iterate over the entries, it's almost certainly more efficient as a Map object. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:233: subscription.type = node.getAttribute("type"); On 2018/06/13 16:33:36, hub wrote: > On 2018/06/13 08:15:16, Manish Jethani wrote: > > Don't we have to add a type property to the Subscription class in core now? > You > > can file an issue for that, if you haven't. > > Added that to issue #6689 (patch not yet in review). Acknowledged. https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... qunit/tests/subscriptionInit.js:41: .then(response => response.text()) On 2018/06/13 16:33:37, hub wrote: > On 2018/06/13 13:45:39, Manish Jethani wrote: > > This appears not to be mentioned in the style guide, but I think the > convention > > is to align the `.then` with the `fetch` above it. > > That's how it is done in `subscriptionInit.js`. Given that it is a continuation > it is indented. The linter say so. What I see in most places is this: func( param ).then(() => {}) .then(() => {}); Since here func opens and closes on the same line, the `.then` would ideally be aligned with the first character, like so: func(param) .then(() => {}) .then(() => {}); When we break lines we continue the "expression" in the same column where it started: let foo = 1 + 2 + 3 4 + 5 + 6; let bar = one.two.three .four.five.six(); The linter doesn't care about this because we actually disabled some of these rules for ESLint 4, if I remember correctly. But is the convention we are following in general (so I hope). Anyway, I can see some inconsistencies in this file, I don't care if it is all fixed as a separate patch.
https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... lib/subscriptionInit.js:225: for (let name in subs) By the way, if you want to keep the return type a plain object rather than a Map object, I think Object.keys would be faster and more appropriate because it only returns the _own_ enumerable properties.
Updated patch https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:139: else if (subscriptionType == "cv") On 2018/06/13 16:53:09, Manish Jethani wrote: > On 2018/06/13 16:33:36, hub wrote: > > On 2018/06/13 08:15:16, Manish Jethani wrote: > > > (I notice that the XML file uses full names like "ads", "social", "privacy", > > and > > > so on. Can we call this "circumvention" or "anticircumvention" rather than > > "cv", > > > for consistency?) > > > > It is not visible to the user. > > We have been using the term "cv" and it is much shorter. > > We had this discussion about the label to use on Trac, for example. "cv" was > suggested, but we went with "circumvention". Even our IRC channel is called > #circumvention. > > "cv" is not a popular abbreviation of circumvention (e.g. you won't find it in > any book or dictionary), it's something we invented, so somebody reading the XML > would have to wonder what "cv" means exactly. > > Anyway, my two cents. Done. https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:217: let subs = chooseFilterSubscriptions(nodes); On 2018/06/13 16:53:09, Manish Jethani wrote: > On 2018/06/13 16:33:37, hub wrote: > > On 2018/06/13 08:15:16, Manish Jethani wrote: > > > OK, so the return value of chooseFilterSubscriptions should be a Map now. > > > > An Object, not a Map. > > Yeah, I mean let's make it a Map. We have to iterate over the entries, it's > almost certainly more efficient as a Map object. To iterate over two entries? In a function that might be called at most once? https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... qunit/tests/subscriptionInit.js:41: .then(response => response.text()) On 2018/06/13 16:53:09, Manish Jethani wrote: > On 2018/06/13 16:33:37, hub wrote: > > On 2018/06/13 13:45:39, Manish Jethani wrote: > > > This appears not to be mentioned in the style guide, but I think the > > convention > > > is to align the `.then` with the `fetch` above it. > > > > That's how it is done in `subscriptionInit.js`. Given that it is a > continuation > > it is indented. The linter say so. > > What I see in most places is this: > > func( > param > ).then(() => {}) > .then(() => {}); > > Since here func opens and closes on the same line, the `.then` would ideally be > aligned with the first character, like so: > > func(param) > .then(() => {}) > .then(() => {}); > > When we break lines we continue the "expression" in the same column where it > started: > > let foo = 1 + 2 + 3 > 4 + 5 + 6; > let bar = one.two.three > .four.five.six(); > > The linter doesn't care about this because we actually disabled some of these > rules for ESLint 4, if I remember correctly. But is the convention we are > following in general (so I hope). > > Anyway, I can see some inconsistencies in this file, I don't care if it is all > fixed as a separate patch. I just followed `lib/subscriptionInit.js:l202` Other occurence `lib/io.js:109` https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... lib/subscriptionInit.js:225: for (let name in subs) On 2018/06/13 16:58:12, Manish Jethani wrote: > By the way, if you want to keep the return type a plain object rather than a Map > object, I think Object.keys would be faster and more appropriate because it only > returns the _own_ enumerable properties. There are at most two properties.
https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/lib/subscriptionIni... lib/subscriptionInit.js:217: let subs = chooseFilterSubscriptions(nodes); On 2018/06/13 21:58:18, hub wrote: > On 2018/06/13 16:53:09, Manish Jethani wrote: > > On 2018/06/13 16:33:37, hub wrote: > > > On 2018/06/13 08:15:16, Manish Jethani wrote: > > > > OK, so the return value of chooseFilterSubscriptions should be a Map now. > > > > > > An Object, not a Map. > > > > Yeah, I mean let's make it a Map. We have to iterate over the entries, it's > > almost certainly more efficient as a Map object. > > To iterate over two entries? In a function that might be called at most once? OK, sure if you prefer it this way. https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29805598/qunit/tests/subscri... qunit/tests/subscriptionInit.js:41: .then(response => response.text()) On 2018/06/13 21:58:18, hub wrote: > On 2018/06/13 16:53:09, Manish Jethani wrote: > > On 2018/06/13 16:33:37, hub wrote: > > > On 2018/06/13 13:45:39, Manish Jethani wrote: > > > > This appears not to be mentioned in the style guide, but I think the > > > convention > > > > is to align the `.then` with the `fetch` above it. > > > > > > That's how it is done in `subscriptionInit.js`. Given that it is a > > continuation > > > it is indented. The linter say so. > > > > What I see in most places is this: > > > > func( > > param > > ).then(() => {}) > > .then(() => {}); > > > > Since here func opens and closes on the same line, the `.then` would ideally > be > > aligned with the first character, like so: > > > > func(param) > > .then(() => {}) > > .then(() => {}); > > > > When we break lines we continue the "expression" in the same column where it > > started: > > > > let foo = 1 + 2 + 3 > > 4 + 5 + 6; > > let bar = one.two.three > > .four.five.six(); > > > > The linter doesn't care about this because we actually disabled some of these > > rules for ESLint 4, if I remember correctly. But is the convention we are > > following in general (so I hope). > > > > Anyway, I can see some inconsistencies in this file, I don't care if it is all > > fixed as a separate patch. > > I just followed `lib/subscriptionInit.js:l202` > > Other occurence `lib/io.js:109` Acknowledged. https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... lib/subscriptionInit.js:225: for (let name in subs) On 2018/06/13 21:58:19, hub wrote: > On 2018/06/13 16:58:12, Manish Jethani wrote: > > By the way, if you want to keep the return type a plain object rather than a > Map > > object, I think Object.keys would be faster and more appropriate because it > only > > returns the _own_ enumerable properties. > > There are at most two properties. Acknowledged. Since the calling code is now assuming the number of properties, maybe it could assume the names as well? So `for (let name of ["ads", "circumvention"])`. https://codereview.adblockplus.org/29805597/diff/29806576/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806576/lib/subscriptionIni... lib/subscriptionInit.js:223: let subs = chooseFilterSubscriptions(nodes); Just a thought: Maybe a good idea to rename `subs` to `defaultSubscriptions`? Not sure. https://codereview.adblockplus.org/29805597/diff/29806576/qunit/subscriptions... File qunit/subscriptions.xml (right): https://codereview.adblockplus.org/29805597/diff/29806576/qunit/subscriptions... qunit/subscriptions.xml:151: prefixes="en" What do you think about making the prefixes here "fr,en,en-US"? It would test a couple of other parts of the logic. https://codereview.adblockplus.org/29805597/diff/29806576/qunit/tests/subscri... File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806576/qunit/tests/subscri... qunit/tests/subscriptionInit.js:33: // TODO restore appLocale We don't normally leave "todo" comments in the code, from what I can tell. Is this still a work in progress?
https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... lib/subscriptionInit.js:225: for (let name in subs) On 2018/06/14 05:07:34, Manish Jethani wrote: > On 2018/06/13 21:58:19, hub wrote: > > On 2018/06/13 16:58:12, Manish Jethani wrote: > > > By the way, if you want to keep the return type a plain object rather than a > > Map > > > object, I think Object.keys would be faster and more appropriate because it > > only > > > returns the _own_ enumerable properties. > > > > There are at most two properties. > > Acknowledged. > > Since the calling code is now assuming the number of properties, maybe it could > assume the names as well? So `for (let name of ["ads", "circumvention"])`. OK, forget this, it makes it worse. Like you said it doesn't matter in this case, but when the names of the keys are unknown (coming from an XML file for example), a Map object performs significantly better (regardless of the number of keys). Here's a test: (function() { function choose() { //return new Map([ // ["" + Math.random(), (Math.random() * 2) | 0], // ["" + Math.random(), (Math.random() * 3) | 0] //]); return { ["" + Math.random()]: (Math.random() * 2) | 0, ["" + Math.random()]: (Math.random() * 3) | 0 }; } let array = new Array(100000); for (let i = 0; i < array.length; i++) array[i] = choose(); let s = Date.now(); let n = 0; for (let i = 0; i < array.length; i++) { let item = array[i]; //for (let [, value] of item) // n += value; for (let prop in item) n += item[prop]; } console.log(Date.now() - s, n); })();
Updated patch https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806566/lib/subscriptionIni... lib/subscriptionInit.js:225: for (let name in subs) On 2018/06/14 05:42:56, Manish Jethani wrote: > On 2018/06/14 05:07:34, Manish Jethani wrote: > > On 2018/06/13 21:58:19, hub wrote: > > > On 2018/06/13 16:58:12, Manish Jethani wrote: > > > > By the way, if you want to keep the return type a plain object rather than > a > > > Map > > > > object, I think Object.keys would be faster and more appropriate because > it > > > only > > > > returns the _own_ enumerable properties. > > > > > > There are at most two properties. > > > > Acknowledged. > > > > Since the calling code is now assuming the number of properties, maybe it > could > > assume the names as well? So `for (let name of ["ads", "circumvention"])`. > > OK, forget this, it makes it worse. > > Like you said it doesn't matter in this case, but when the names of the keys are > unknown (coming from an XML file for example), a Map object performs > significantly better (regardless of the number of keys). > > Here's a test: > > (function() > { > function choose() > { > //return new Map([ > // ["" + Math.random(), (Math.random() * 2) | 0], > // ["" + Math.random(), (Math.random() * 3) | 0] > //]); > return { > ["" + Math.random()]: (Math.random() * 2) | 0, > ["" + Math.random()]: (Math.random() * 3) | 0 > }; > } > > let array = new Array(100000); > for (let i = 0; i < array.length; i++) > array[i] = choose(); > > let s = Date.now(); > let n = 0; > for (let i = 0; i < array.length; i++) > { > let item = array[i]; > //for (let [, value] of item) > // n += value; > for (let prop in item) > n += item[prop]; > } > console.log(Date.now() - s, n); > })(); Acknowledged. https://codereview.adblockplus.org/29805597/diff/29806576/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806576/lib/subscriptionIni... lib/subscriptionInit.js:223: let subs = chooseFilterSubscriptions(nodes); On 2018/06/14 05:07:34, Manish Jethani wrote: > Just a thought: Maybe a good idea to rename `subs` to `defaultSubscriptions`? > Not sure. Done. https://codereview.adblockplus.org/29805597/diff/29806576/qunit/subscriptions... File qunit/subscriptions.xml (right): https://codereview.adblockplus.org/29805597/diff/29806576/qunit/subscriptions... qunit/subscriptions.xml:151: prefixes="en" On 2018/06/14 05:07:34, Manish Jethani wrote: > What do you think about making the prefixes here "fr,en,en-US"? It would test a > couple of other parts of the logic. Good idea. Done. https://codereview.adblockplus.org/29805597/diff/29806576/qunit/tests/subscri... File qunit/tests/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29806576/qunit/tests/subscri... qunit/tests/subscriptionInit.js:33: // TODO restore appLocale On 2018/06/14 05:07:34, Manish Jethani wrote: > We don't normally leave "todo" comments in the code, from what I can tell. Is > this still a work in progress? I had forgotten to remove it, done.
Sorry about the delay. LGTM, except for the one comment below. https://codereview.adblockplus.org/29805597/diff/29807582/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29807582/lib/subscriptionIni... lib/subscriptionInit.js:115: if (subscriptionType in ["ads", "circumvention"] && We should probably just use == here instead of the in operator.
Updated patch. https://codereview.adblockplus.org/29805597/diff/29807582/lib/subscriptionIni... File lib/subscriptionInit.js (right): https://codereview.adblockplus.org/29805597/diff/29807582/lib/subscriptionIni... lib/subscriptionInit.js:115: if (subscriptionType in ["ads", "circumvention"] && On 2018/06/19 09:23:30, Manish Jethani wrote: > We should probably just use == here instead of the in operator. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
