Issue 29349187: Issue 4167 - getSelectorsForDomain criteria + keys (Closed)
|
Created:
Aug. 8, 2016, 3:24 p.m. by kzar Modified:
Sept. 27, 2016, 3:28 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionIssue 4167 - getSelectorsForDomain criteria + keys
Patch Set 1 #
Total comments: 10
Patch Set 2 : Only store one filter key per unconditional selector #Patch Set 3 : Remove unrelated changes #
Total comments: 10
Patch Set 4 : Improved tests, fixed bugs spotted in the process #
Total comments: 1
Patch Set 5 : Further improve tests #
Total comments: 12
Patch Set 6 : Addressed feedback #
Total comments: 9
Patch Set 7 : Addressed further feedback #
Total comments: 10
Patch Set 8 : Improved comments #MessagesTotal messages: 14
Patch Set 1 Since the changes for #4167 have been sat gathering dust for some time I thought I'd try to move things forward. I've put the changes for the provideFitlerKeys and noUnconditional parameters on top of the current code in a way that hopefully shouldn't break anything. I figure we can then switch the Firefox extension to using getSelectorsForDomain at our own leisure and finally remove redundant stuff like ElemHide.getSelectors(). (I've checked and these changes don't seem to slow down getSelectorsForDomain on Chrome much or at all. I have also included more unit tests which pass and checked that element hiding still works for Chrome. I have not tested element hiding on Firefox however.)
https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:45: var usingGetSelectorsForDomain = !("nsIStyleSheetService" in Ci); This flag needs to go - we'll be using this functionality in Firefox as well now. https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:178: filterKeys.push(filter); Shouldn't you push `key` here? Given that you have unit tests, I guess that you need to make sure all edge cases are really tested? https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:338: unconditionalFilterKeys.push(key); On Firefox we actually need a mapping from selectors to filter keys (one key per selector). So it should be best if we push only the first key for each selector, then the selector and key at the same position in the array will belong together. https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:346: * (Must not be used in Firefox - when usingGetSelectorsForDomain is false). Actually, it will be used in Firefox now, that's the only platform using filter keys ;) https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:357: if (!(specificOnly || noUnconditional)) Personally, I find this easier to understand if you get rid of parentheses: if (!specificOnly && !noUnconditional)
Patch Set 2 : Only store one filter key per unconditional selector https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:45: var usingGetSelectorsForDomain = !("nsIStyleSheetService" in Ci); On 2016/09/19 08:53:59, Wladimir Palant wrote: > This flag needs to go - we'll be using this functionality in Firefox as well > now. Done. https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:178: filterKeys.push(filter); On 2016/09/19 08:53:59, Wladimir Palant wrote: > Shouldn't you push `key` here? > > Given that you have unit tests, I guess that you need to make sure all edge > cases are really tested? As discussed in IRC it turns out this branch was rarely used and mostly irrelevant even when it was. I have simplified this logic and related data structure. https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:338: unconditionalFilterKeys.push(key); On 2016/09/19 08:53:59, Wladimir Palant wrote: > On Firefox we actually need a mapping from selectors to filter keys (one key per > selector). So it should be best if we push only the first key for each selector, > then the selector and key at the same position in the array will belong > together. Since we only store one key per unconditional selector now anyway this has been taken care of :) https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:346: * (Must not be used in Firefox - when usingGetSelectorsForDomain is false). On 2016/09/19 08:53:59, Wladimir Palant wrote: > Actually, it will be used in Firefox now, that's the only platform using filter > keys ;) Done. https://codereview.adblockplus.org/29349187/diff/29349188/lib/elemHide.js#new... lib/elemHide.js:357: if (!(specificOnly || noUnconditional)) On 2016/09/19 08:53:59, Wladimir Palant wrote: > Personally, I find this easier to understand if you get rid of parentheses: > > if (!specificOnly && !noUnconditional) Done.
Patch Set 3 : Remove unrelated changes
https://codereview.adblockplus.org/29349187/diff/29353425/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29353425/lib/elemHide.js#new... lib/elemHide.js:215: filterKeys.splice(index, 1); We need to null out unconditionalFilterKeys here if index is 0. https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:168: addFilter("~example.com##foo"); Please add at least one unconditional filter, otherwise you aren't really testing noUnconditional. Ideally, you should also test that two unconditional filters with the same selector don't result in the selector being duplicated, and that removing one of the two selectors still leaves the other one intact. https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:170: selectorsEqual("foo.com", ["foo"], false, true); You are not testing the scenario where both specificOnly and noUnconditional are true, this implies that it's an invalid combination. I don't think we want implicit assumptions here, so it's probably better to replace these two parameters by a single numerical parameter: ALL_MATCHING = 0; NO_UNCONDITIONAL = 1; SPECIFIC_ONLY = 2; Note that the set supposed to be returned is getting smaller with higher values, so you can use comparison operators: if (criteria < NO_UNCONDITIONAL) selectors = unconditionalSelectors; https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:183: let selectorsWithKeys = ElemHide.getSelectorsForDomain("foo.com", false, How about: let [selectors, filterKeys] = ... https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:190: normalizeSelectors(["bar", "foo", "hello", "world"])); One-time correctness test is definitely not sufficient here, we need to verify that result stays correct on modification as well. For example: add bar.com##foo remove bar.com##foo add ,##hello remove ##hello remove ,##hello add ~bar.com##nope remove ~bar.com##nope I didn't verify that these operations will really trigger all the code branches but it's a start.
Patch Set 4 : Improved tests, fixed bugs spotted in the process https://codereview.adblockplus.org/29349187/diff/29353425/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29353425/lib/elemHide.js#new... lib/elemHide.js:215: filterKeys.splice(index, 1); On 2016/09/20 10:36:07, Wladimir Palant wrote: > We need to null out unconditionalFilterKeys here if index is 0. Good point, Done. https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:168: addFilter("~example.com##foo"); On 2016/09/20 10:36:07, Wladimir Palant wrote: > Please add at least one unconditional filter, otherwise you aren't really > testing noUnconditional. Ideally, you should also test that two unconditional > filters with the same selector don't result in the selector being duplicated, > and that removing one of the two selectors still leaves the other one intact. Done. https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:170: selectorsEqual("foo.com", ["foo"], false, true); On 2016/09/20 10:36:08, Wladimir Palant wrote: > You are not testing the scenario where both specificOnly and noUnconditional are > true, this implies that it's an invalid combination. I don't think we want > implicit assumptions here, so it's probably better to replace these two > parameters by a single numerical parameter: > > ALL_MATCHING = 0; > NO_UNCONDITIONAL = 1; > SPECIFIC_ONLY = 2; > > Note that the set supposed to be returned is getting smaller with higher values, > so you can use comparison operators: > > if (criteria < NO_UNCONDITIONAL) > selectors = unconditionalSelectors; Done. (I use slightly different logic for the checks since the parameter is optional.) https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:183: let selectorsWithKeys = ElemHide.getSelectorsForDomain("foo.com", false, On 2016/09/20 10:36:08, Wladimir Palant wrote: > How about: > > let [selectors, filterKeys] = ... Done. https://codereview.adblockplus.org/29349187/diff/29353425/test/elemHide.js#ne... test/elemHide.js:190: normalizeSelectors(["bar", "foo", "hello", "world"])); On 2016/09/20 10:36:08, Wladimir Palant wrote: > One-time correctness test is definitely not sufficient here, we need to verify > that result stays correct on modification as well. For example: > > add bar.com##foo > remove bar.com##foo > add ,##hello > remove ##hello > remove ,##hello > add ~bar.com##nope > remove ~bar.com##nope > > I didn't verify that these operations will really trigger all the code branches > but it's a start. I've made it so the filter keys are tested for all the other tests too. Also added some more advanced tests below. https://codereview.adblockplus.org/29349187/diff/29354331/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354331/lib/elemHide.js#new... lib/elemHide.js:389: return [selectors, filterKeys.map(key => parseInt(key, 10))]; (Since filtersByDomain[domain] is an Object we end up with strings instead of integers for filter keys of selectors which aren't unconditionally matched.)
Patch Set 5 : Further improve tests
I haven't looked into the unit tests again, will do that tomorrow. https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:218: filterKeys.splice(index, 1); index should be positive but if we ever have a bug and it is -1 bad things will happen. Should we check maybe, just in case? In fact, maybe this entire code is better off like this: function _removeFilterKey(key, filter) { let filterKeys = filterKeysBySelector[filter.selector]; if (filterKeys) { let index = filterKeys.indexOf(key); if (index >= 0) { if (filterKeys.length > 1) { filterKeys.splice(index, 1); ... } else { delete filterKeysBySelector[filter.selector]; ... } return; } } // We haven't found this filter in unconditional filters, look // in filtersByDomain. let domains = filter.domains || defaultDomains; ... } https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:232: // filtersByDomain instead. This comment makes little sense, should really be something like "Even unconditional filters will end up in filtersByDomain after an exception applying to them is removed" https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:339: * 2 - Only specifically matching selectors allowed No magic numbers please, these should be exposed as constants. https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:347: if (!criteria) Should be something like: if (typeof criteria == "undefined") criteria = Elemhide.ALL_MATCHING; if (criteria < ElemHide.NO_UNCONDITIONAL) https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:354: let specificOnly = criteria == 2; Should be something like: let specificOnly = (criteria >= ElemHide.SPECIFIC_ONLY); https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:389: return [selectors, filterKeys.map(key => parseInt(key, 10))]; Keys are opaque to the caller, we used to have strings here and the caller doesn't need to know that they are numeric now. Converting them to numbers is pointless.
Patch Set 6 : Addressed feedback https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:218: filterKeys.splice(index, 1); On 2016/09/20 16:31:14, Wladimir Palant wrote: > index should be positive but if we ever have a bug and it is -1 bad things will > happen. Should we check maybe, just in case? > > In fact, maybe this entire code is better off like this: > > function _removeFilterKey(key, filter) > { > let filterKeys = filterKeysBySelector[filter.selector]; > if (filterKeys) > { > let index = filterKeys.indexOf(key); > if (index >= 0) > { > if (filterKeys.length > 1) > { > filterKeys.splice(index, 1); > ... > } > else > { > delete filterKeysBySelector[filter.selector]; > ... > } > return; > } > } > > // We haven't found this filter in unconditional filters, look > // in filtersByDomain. > let domains = filter.domains || defaultDomains; > ... > } Done. https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:232: // filtersByDomain instead. On 2016/09/20 16:31:14, Wladimir Palant wrote: > This comment makes little sense, should really be something like "Even > unconditional filters will end up in filtersByDomain after an exception applying > to them is removed" Done. https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:339: * 2 - Only specifically matching selectors allowed On 2016/09/20 16:31:14, Wladimir Palant wrote: > No magic numbers please, these should be exposed as constants. Done. https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:347: if (!criteria) On 2016/09/20 16:31:14, Wladimir Palant wrote: > Should be something like: > > if (typeof criteria == "undefined") > criteria = Elemhide.ALL_MATCHING; > if (criteria < ElemHide.NO_UNCONDITIONAL) Done. https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:354: let specificOnly = criteria == 2; On 2016/09/20 16:31:14, Wladimir Palant wrote: > Should be something like: > > let specificOnly = (criteria >= ElemHide.SPECIFIC_ONLY); Done. https://codereview.adblockplus.org/29349187/diff/29354334/lib/elemHide.js#new... lib/elemHide.js:389: return [selectors, filterKeys.map(key => parseInt(key, 10))]; On 2016/09/20 16:31:14, Wladimir Palant wrote: > Keys are opaque to the caller, we used to have strings here and the caller > doesn't need to know that they are numeric now. Converting them to numbers is > pointless. Done. https://codereview.adblockplus.org/29349187/diff/29354337/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354337/lib/elemHide.js#new... lib/elemHide.js:189: if (index >= 0) It kind of bugs me how we check index twice, also how we potentially check that filterKeys.length > 1 even if the index is bigger than 0. I can't see a cleaner way however :(
https://codereview.adblockplus.org/29349187/diff/29354337/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354337/lib/elemHide.js#new... lib/elemHide.js:351: * value of ALL_MATCHING, NO_UNCONDITIONAL or SPECIFIC_ONLY if provided. Nit: You are documenting return value and parameters here, not the function. How about documenting everything properly rather than inline: Determines from the current filter list which selectors should be applied on a particular host name. @param {String} domain @param {Number} [criteria] One of ALL_MATCHING, NO_UNCONDITIONAL or SPECIFIC_ONLY. @param {Boolean} [provideFilterKeys] If true, will return a list of corresponding filter keys in addition to selectors. @returns {string[]|Array.<string[]>} List of selectors or an array with two elements (list of selectors and list of corresponding keys) if provideFilterKeys is true. https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js#ne... test/elemHide.js:193: removeFilter("##hello"); How about: testResult("foo.com", []); This is just begging for you to check the final state - that this selectors goes away after you remove all filters. https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js#ne... test/elemHide.js:200: removeFilter("foo.com##hello"); Same here, how about testing the final state? https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js#ne... test/elemHide.js:201: How about testing the combination of ##hello and foo.com#@#hello? Also what you get after removing foo.com#@#hello. I don't see any other test forcing an unconditional filter to by-domain maps right now.
Patch Set 7 : Addressed further feedback https://codereview.adblockplus.org/29349187/diff/29354337/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354337/lib/elemHide.js#new... lib/elemHide.js:351: * value of ALL_MATCHING, NO_UNCONDITIONAL or SPECIFIC_ONLY if provided. On 2016/09/26 14:56:54, Wladimir Palant wrote: > Nit: You are documenting return value and parameters here, not the function. How > about documenting everything properly rather than inline: > > Determines from the current filter list which selectors should be applied on a > particular host name. > > @param {String} domain > @param {Number} [criteria] > One of ALL_MATCHING, NO_UNCONDITIONAL or SPECIFIC_ONLY. > @param {Boolean} [provideFilterKeys] > If true, will return a list of corresponding filter keys in addition to > selectors. > @returns {string[]|Array.<string[]>} List of selectors or an array with two > elements (list of selectors and list of corresponding keys) if provideFilterKeys > is true. Done. https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js#ne... test/elemHide.js:193: removeFilter("##hello"); On 2016/09/26 14:56:54, Wladimir Palant wrote: > How about: > > testResult("foo.com", []); > > This is just begging for you to check the final state - that this selectors goes > away after you remove all filters. Done. https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js#ne... test/elemHide.js:200: removeFilter("foo.com##hello"); On 2016/09/26 14:56:54, Wladimir Palant wrote: > Same here, how about testing the final state? Done. https://codereview.adblockplus.org/29349187/diff/29354337/test/elemHide.js#ne... test/elemHide.js:201: On 2016/09/26 14:56:54, Wladimir Palant wrote: > How about testing the combination of ##hello and foo.com#@#hello? Also what you > get after removing foo.com#@#hello. I don't see any other test forcing an > unconditional filter to by-domain maps right now. Done.
A bunch of documentation nits but otherwise we are finally done. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:331: * Constant used by getSelectorsForDomain's when matching all selectors. Nit: why "'s"? How about: Constant used by getSelectorsForDomain to return all selectors applying on a particular hostname. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:337: * selectors. How about: Constant used by getSelectorsForDomain to exclude filters matching on all websites without exception. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:343: * selectors. Nit: Just realized, "specific selectors" makes no sense. How about: Constant used by getSelectorsForDomain to return only selectors for filters which specifically match the given host name. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:353: * One of the following: Nit: Please indent parameter and return value descriptions - the blocks here should be clearly visible. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:358: * specifically match the given host name. Please don't document these constants twice, just reference them here.
Patch Set 8 : Improved comments https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:331: * Constant used by getSelectorsForDomain's when matching all selectors. On 2016/09/27 13:08:38, Wladimir Palant wrote: > Nit: why "'s"? No idea, I guess it's a typo that I missed and then copy-pasted... Done. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:337: * selectors. On 2016/09/27 13:08:38, Wladimir Palant wrote: > How about: Constant used by getSelectorsForDomain to exclude filters matching on > all websites without exception. Done. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:343: * selectors. On 2016/09/27 13:08:38, Wladimir Palant wrote: > Nit: Just realized, "specific selectors" makes no sense. > > How about: Constant used by getSelectorsForDomain to return only selectors for > filters which specifically match the given host name. Done. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:353: * One of the following: On 2016/09/27 13:08:38, Wladimir Palant wrote: > Nit: Please indent parameter and return value descriptions - the blocks here > should be clearly visible. Done. https://codereview.adblockplus.org/29349187/diff/29355121/lib/elemHide.js#new... lib/elemHide.js:358: * specifically match the given host name. On 2016/09/27 13:08:38, Wladimir Palant wrote: > Please don't document these constants twice, just reference them here. Done.
LGTM 
|
