Issue 29712655: Issue 6437 - Skip styles-only patterns on DOM mutations (Closed)
|
Created:
March 1, 2018, 5:11 p.m. by Manish Jethani Modified:
March 20, 2018, 8:58 a.m. CC:
sergei Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
Patch Set 1 #
Total comments: 3
Patch Set 2 : Minor changes #Patch Set 3 : Minor changes #Patch Set 4 : Rebase #Patch Set 5 : Fix ESLint errors #Patch Set 6 : Merge patch #29716641 #
Total comments: 4
Patch Set 7 : Use typeof #MessagesTotal messages: 15
Patch Set 1 See #6437 description. https://codereview.adblockplus.org/29712655/diff/29712656/lib/content/elemHid... File lib/content/elemHideEmulation.js (left): https://codereview.adblockplus.org/29712655/diff/29712656/lib/content/elemHid... lib/content/elemHideEmulation.js:563: if (stylesheetOnlyChange && This is taken care of in shouldProcessPattern now. It was nice that this was happening asynchronously, but I don't think it matter so much in practice. On the other hand it's nice to filter out the unnecessary patterns up front. https://codereview.adblockplus.org/29712655/diff/29712656/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29712655/diff/29712656/lib/content/elemHid... lib/content/elemHideEmulation.js:247: dependsOnDOM: true, -abp-has and -abp-contains definitely, one hundred percent, depend on the DOM (i.e. they should be considered for processing in response to DOM mutations). https://codereview.adblockplus.org/29712655/diff/29712656/lib/content/elemHid... lib/content/elemHideEmulation.js:679: queueFiltering(stylesheets, mutations) The addition of the mutations parameter here is mainly just for passing it on to _addSelectors. It is treated exactly like the stylesheets parameter.
Patch Set 2: Minor changes
Patch Set 3: Minor changes
Patch Set 4: Rebase
Patch Set 5: Fix ESLint errors
Patch Set 6: Merge patch #29716641 https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... lib/content/elemHideEmulation.js:637: if (selectors.length > 0) We need to do this because of an existing bug, which happens more often with these changes. Anyway it's a good idea not to call the functions if we don't have to.
By the way I'm also trying to address the issue on the WebExt side, see https://issues.adblockplus.org/ticket/6446
https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... lib/content/elemHideEmulation.js:637: if (selectors.length > 0) On 2018/03/08 21:08:15, Manish Jethani wrote: > We need to do this because of an existing bug, which happens more often with > these changes. Anyway it's a good idea not to call the functions if we don't > have to. Sorry, I just checked, there is no bug. In any case, it would be a bug if we didn't check for selectors.length here, because the length is 0 precisely when there's a DOM mutation but no style sheet changes (what we're optimizing for), but since the list of selectors is empty it would wipe out all existing selectors in the document.
On 2018/03/08 21:28:34, Manish Jethani wrote: > https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... > File lib/content/elemHideEmulation.js (right): > > https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... > lib/content/elemHideEmulation.js:637: if (selectors.length > 0) > On 2018/03/08 21:08:15, Manish Jethani wrote: > > We need to do this because of an existing bug, which happens more often with > > these changes. Anyway it's a good idea not to call the functions if we don't > > have to. > > Sorry, I just checked, there is no bug. I have to correct myself again, there is a bug of course: https://issues.adblockplus.org/ticket/6458
https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... lib/content/elemHideEmulation.js:29: if (value === undefined) there is the argument of value === undefined vs typeof value == "undefined" to be settled here. It is cosmetic though.
Patch Set 7: Use typeof https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29712655/diff/29717623/lib/content/elemHid... lib/content/elemHideEmulation.js:29: if (value === undefined) On 2018/03/14 13:04:03, hub wrote: > there is the argument of value === undefined vs typeof value == "undefined" to > be settled here. It is cosmetic though. Oh yeah, for the sake of consistency though I've made it use typeof now.
LGTM
On 2018/03/15 00:20:52, hub wrote: > LGTM Thanks! Dave, would you like to take a look?
On 2018/03/15 05:32:56, Manish Jethani wrote: > On 2018/03/15 00:20:52, hub wrote: > > LGTM > > Thanks! > > Dave, would you like to take a look? I think Hubert's approval is enough for this element hiding emulation code. 
|
