Issue 29728690: Issue 6504, 6458, 6446 - Update selectors based on DOM mutations (Closed)
|
Created:
March 21, 2018, 4:29 a.m. by Manish Jethani Modified:
March 19, 2019, 8:21 a.m. Reviewers:
CC:
kzar, hub Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #
Total comments: 5
Patch Set 2 : Spelling correction #Patch Set 3 : Encapsulate selector management in CSSSelectorManager class #Patch Set 4 : Add special case check for documents with no extended selectors #
Total comments: 3
MessagesTotal messages: 7
Patch Set 1 Patch Set 2: Spelling correction Now that we are using CSS selectors for -abp-has and -abp-contains, we need to keep them up-to-date. We're trying to avoid DOM traversal and filtering as much as we can, which means we don't have the updated selectors in all cases. When an element has moved (because its sibling got deleted), we need to update the selector. This change also helps with #6458. Basically we should maintain the selectors instead of generating them each time. It was OK so far but it's not going to be OK with the switch to user style sheets. https://codereview.adblockplus.org/29728690/diff/29728691/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29728690/diff/29728691/lib/content/elemHid... lib/content/elemHideEmulation.js:477: function updateSelectorsForMutation(mutation, selectorSet, selectors, Hopefully the inline comments are helpful. https://codereview.adblockplus.org/29728690/diff/29728691/lib/content/elemHid... lib/content/elemHideEmulation.js:579: this.selectorSet = new Set(); selectorSet here is mainly for faster lookups and such. selectors and selectorFilters exist because the content script doesn't support append-only and delete-only modes. Ideally addSelectorsFunc would only add new selectors, and there would be a corresponding removeSelectorsFunc that would remove existing selectors. Then we could call them only when there are changes to be made. https://codereview.adblockplus.org/29728690/diff/29728691/lib/content/elemHid... lib/content/elemHideEmulation.js:680: if (patterns.length == 0) Minor optimization, definitely don't need to proceed if there are no patterns to process. https://codereview.adblockplus.org/29728690/diff/29728691/lib/content/elemHid... lib/content/elemHideEmulation.js:768: if (!this.selectorSet.has(selector)) The reason we need this Set object is that we don't want to add the same selector twice, which would happen if we're traversing the DOM in response to a mutation and finding the same selectors again. We cannot prevent that in all cases, but we can avoid duplicating selectors. https://codereview.adblockplus.org/29728690/diff/29728691/lib/content/elemHid... lib/content/elemHideEmulation.js:912: if (selectorsChanged) Call addSelectorsFunc immediately so the UI is updated ASAP, otherwise elements have moved so the existing selectors that exist in the document may end up targeting the wrong elements, at least until the new selectors are found via DOM traversal. That's one reason for this change, the other reason is that we want to keep trying to avoid DOM traversal wherever possible, so we shouldn't have to do it just to update the selectors.
Patch Set 3: Encapsulate selector management in CSSSelectorManager class Patch Set 4: Add special case check for documents with no extended selectors https://codereview.adblockplus.org/29728690/diff/29729629/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29728690/diff/29729629/lib/content/elemHid... lib/content/elemHideEmulation.js:529: if (!this._extended || this.selectors.length == 0) This is useful. In EasyList the number of sites with emulation filters is 45: grep '#?#' easylist.txt | cut -f1 -d'#' | sort -u | tr ',' "\n" | wc -l While the number of sites with extended CSS selector syntax is only 4: grep '#?#.*-abp' easylist.txt | cut -f1 -d'#' | sort -u | tr ',' "\n" | wc -l If a site doesn't use :-abp-* there won't be any path-based selectors (those starting with ":root > ") and there's no need to update anything. Also if there are no selectors, which is true for facebook.com when the page is loading (but not true afterwards), we can skip all the unnecessary code.
https://codereview.adblockplus.org/29728690/diff/29729629/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29728690/diff/29729629/lib/content/elemHid... lib/content/elemHideEmulation.js:953: this.addSelectorsFunc(this.css.selectors, this.css.filters); My concern here is that the intent of queueFiltering was to have addSelectorsFunc() run asynchronously. This here throw it off a bit.
https://codereview.adblockplus.org/29728690/diff/29729629/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29728690/diff/29729629/lib/content/elemHid... lib/content/elemHideEmulation.js:953: this.addSelectorsFunc(this.css.selectors, this.css.filters); On 2018/03/23 15:54:25, hub wrote: > My concern here is that the intent of queueFiltering was to have > addSelectorsFunc() run asynchronously. This here throw it off a bit. The thing is that I'm trying to avoid running full filtering on every DOM mutation, but at the same time we have to keep the selectors up to date (otherwise they hide the wrong elements, i.e. false positives and false negatives). I thought about making this asynchronous, but I wonder if it's necessary. MutationObserver events are already asynchronous (multiple mutations are batched together). But I'll see if I can throw in a bit of asynchronicity here.
This also affects Trac #6446, I have updated the summary.
Since we decided not to use style sheets for element hiding emulation [1], this change no longer makes sense. I'm closing this. [1]: https://issues.adblockplus.org/ticket/6504#Resolution 
|
