Issue 29494577: Issue 5438 - Observer DOM changes to reapply filters. (Closed)
|
Created:
July 21, 2017, 7:53 p.m. by hub Modified:
Aug. 28, 2017, 12:48 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 5438 - Observer DOM changes to reapply filters.
Patch Set 1 #
Total comments: 9
Patch Set 2 : Improvements #
Total comments: 6
Patch Set 3 : Updated to the new design #
Total comments: 55
Patch Set 4 : Updated patch from review #
Total comments: 20
Patch Set 5 : Updated patch to deal with the review comments. #Patch Set 6 : Now using performance.now() #
Total comments: 14
Patch Set 7 : Updated from review #
Total comments: 4
Patch Set 8 : review feedback #Patch Set 9 : And now the test, with issues #
Total comments: 2
Patch Set 10 : Test harness changes: test pass. #
Total comments: 12
Patch Set 11 : Updated the tests. #
Total comments: 13
Patch Set 12 : Review changes #
Total comments: 10
Patch Set 13 : Addressed comments #
MessagesTotal messages: 30
This is the initial implementation. I need to check the performance impact. It applies on top of the 1.13.3 patches grafted to master.
This isn't quite what I had in mind, see updated issue description. Sorry that it took me so long to write this down. https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... chrome/content/.eslintrc.json:3: "MutationObserver": true, We have been here before. On Firefox this script doesn't run inside a window, so there will be no MutationObserver global. https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... chrome/content/elemHideEmulation.js:320: this.observer = new MutationObserver(this.observe.bind(this)); new window.MutationObserver? https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... chrome/content/elemHideEmulation.js:526: this.queueFiltering(); a) Reapplying the rules on the entire document is quite wasteful when we only need to process a single subtree. b) There are other mutation types having relevant side-effects as well, e.g. attribute changes.
I'll rework the patch based on the changes in the bug description. https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... chrome/content/.eslintrc.json:3: "MutationObserver": true, On 2017/07/21 21:37:28, Wladimir Palant wrote: > We have been here before. On Firefox this script doesn't run inside a window, so > there will be no MutationObserver global. good to know. I guess we'll have to split things out. https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... chrome/content/elemHideEmulation.js:320: this.observer = new MutationObserver(this.observe.bind(this)); On 2017/07/21 21:37:28, Wladimir Palant wrote: > new window.MutationObserver? https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver Also that's what we do albeit in the WebExt code. But since this won't run in the Firefox extension, it'll be moved out. https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... chrome/content/elemHideEmulation.js:526: this.queueFiltering(); On 2017/07/21 21:37:28, Wladimir Palant wrote: > a) Reapplying the rules on the entire document is quite wasteful when we only > need to process a single subtree. > > b) There are other mutation types having relevant side-effects as well, e.g. > attribute changes. Acknowledged.
https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... chrome/content/.eslintrc.json:3: "MutationObserver": true, On 2017/07/22 01:31:24, hub wrote: > I guess we'll have to split things out. Why? Not assuming MutationObserver to be global and rather accessing it via window variable as I suggested should be sufficient.
https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.esl... chrome/content/.eslintrc.json:3: "MutationObserver": true, On 2017/07/22 06:16:20, Wladimir Palant wrote: > On 2017/07/22 01:31:24, hub wrote: > > I guess we'll have to split things out. > > Why? Not assuming MutationObserver to be global and rather accessing it via > window variable as I suggested should be sufficient. I had assumed that you meant it wasn't available at all. My bad. https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/elem... chrome/content/elemHideEmulation.js:320: this.observer = new MutationObserver(this.observe.bind(this)); On 2017/07/22 01:31:24, hub wrote: > On 2017/07/21 21:37:28, Wladimir Palant wrote: > > new window.MutationObserver? > > https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver > > Also that's what we do albeit in the WebExt code. But since this won't run in > the Firefox extension, it'll be moved out. My bad. will actually do that.
This is the second iteration. I'm not totally satisfied by it. https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elem... chrome/content/elemHideEmulation.js:532: this.addSelectors(stylesheets, domUpdate); Here we don't use reason.subtree. Actually I'm not convinced that it is a good idea to restrict only to the nodes that we checked with :-abp-has() (scenario 2 in the issue). In that case reason.subtree is irrelevant and we can simplify lot of the code removing it. https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elem... chrome/content/elemHideEmulation.js:565: stylesheets.push(added.stylesheet); If we have a new style element, then we likely have a new stylesheet. https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elem... chrome/content/elemHideEmulation.js:613: attributeFilter: ["class", "id"] check for the obvious class and id attributes that directly impact filtering. We could eventually when parsing the filters listing other attributes we might be interested in.
https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elem... chrome/content/elemHideEmulation.js:531: let domUpdate = reason.dom; I realise this should have been moved up, out of the lambda to only capture domUpdate. Will be in the next iteration.
While this patch has its issues, maybe we should just try another approach. I outlined the thoughts under https://issues.adblockplus.org/ticket/5438#comment:5 https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/.esl... File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/.esl... chrome/content/.eslintrc.json:4: "Node": true That's once again a global which is only available in the window context, not going to be available in Firefox. Please use window.Node instead of assuming that you will have this. For reference, in Firefox we have only the globals imported at the top of https://hg.adblockplus.org/buildtools/file/tip/templates/bootstrap.js.tmpl in addition to ECMAScript builtins.
This is following the new design. https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/.esl... File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/.esl... chrome/content/.eslintrc.json:4: "Node": true On 2017/08/08 11:01:00, Wladimir Palant wrote: > That's once again a global which is only available in the window context, not > going to be available in Firefox. Please use window.Node instead of assuming > that you will have this. > > For reference, in Firefox we have only the globals imported at the top of > https://hg.adblockplus.org/buildtools/file/tip/templates/bootstrap.js.tmpl in > addition to ECMAScript builtins. Done.
In addition to the points below, this needs unit tests - for the DOM modification handling at the very least (probably not for the rate limiting part, this would get way too complicated I guess). https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:169: if (prefix == null) How would we get into this situation? From what I can tell, the only case where evaluate() is being called with a non-constant prefix is right below - and you already avoid passing in null by checking for `selector == null`. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:179: continue; getSelectors() generator might be performing non-trivial work without producing any non-null values, e.g. if it is a HasSelector instance. So we shouldn't be just dropping any null values it yields but rather pass them through: if (selector == null) yield null; else yield* evaluate(...); https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:184: yield null; I don't think we need this counter, doing this unconditionally should be fine. But maybe add a comment explaining why this is necessary, it's not really obvious. Something like this: // Just in case the getSelectors() generator above had to run some heavy // document.querySelectorAll() call which didn't produce any results, make // sure there is at least one point where execution can pause. yield null; https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:240: yield null; We don't need to protect against this scenario more than once, it is already accounted for in evaluate(). https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:259: yield null; Like above, we don't need the counter - an unconditional `yield null` should do. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:284: yield null; We don't need to protect against this scenario more than once, it is already accounted for in evaluate(). https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:293: } We don't need this counter, just make sure to yield on each loop iteration: if (element.textContent.includes(this._text)) yield element; else yield null; Nit: multi-line for statement should have brackets around its body. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:346: function isSelectorHidingOnlyPattern(pattern) Note that this function is called exactly once, so you could keep this logic inline just as well. I'm fine with either, giving this a name provides some value. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:452: * Indicate this is a DOM update. Why do we need this parameter? Its purpose seems to be summed up as "ignore the stylesheets" parameter. The logic seems wrong right now, if a DOM modification includes a <style> tag we will generally limit the processing of :-abp-properties() clauses. So in case of :-abp-has(:-abp-properties()) we might miss cases where :-abp-properties() matches new elements but due to old CSS rules. From the look of it, we should generally null out the stylesheets parameter for any DOM modifications. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:458: this._lastInvocation = Date.now(); With this method being asynchronous now, I think that we need to set this when it is done, not when it starts. While at it, we should probably switch to Performance.now(). It has an important property: "the values returned by Performance.now() always increase at a constant rate," so no issues with clocks being set by NTP or such. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:530: if (now - lastCycle > 50) No magic numbers please, you should declare a constant like MAX_SYNCHRONOUS_PROCESSING_TIME. The logic is also wrong, it's not the end of the last cycle that's relevant here but the start of the current one. So you need a cycleStart variable being set to Date.now() at the beginning of processPatterns(). https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:537: return; So, why did we go through all the effort yielding null values if we only ever pause at the end of chain processing? :) I think the logic should be something like this: let patterns = this.patterns.slice(); let pattern = null; let generator = null; let processPatterns = () => { if (!pattern) { if (!patterns.length) { ... done(); return; } pattern = patterns.shift(); generator = evaluate(...); } for (let selector of generator) { if (selector != null) { ... } if (Date.now() - cycleStart > MAX_SYNCHRONOUS_PROCESSING_TIME) this.window.setTimeout(processPatterns, 0); } pattern = null; return processPatterns(); }; processPatterns(); Two things to note here: * We are creating a copy of this.patterns here. This is important because its value might change now that our processing is asynchronous. * In the recursive call at the end, we return the resulting value. This isn't really necessary, but allows tail call optimization to kick in. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:545: }.bind(this); Don't call bind(), use an arrow function instead. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:555: /** Filtering reason Nit: We usually have /** on a line of its own and start JSDoc comments on the next line. Same below. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:557: * @property {boolean} dom Indicate the DOM changed (tree or attributes) I get what this sentence means, but I think that it can be phrased better. "Indicates that a DOM change triggered reprocessing" maybe? Note that "tree or attributes" is an implementation detail, one that should already be outdated due to my comments below. We shouldn't be documenting this. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:559: * Indicate the stylesheets that needs refresh Nit: need, not needs However, it's not the stylesheets that need a refresh. Rather, it's new stylesheets that have been added and have to be considered for our filtering. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:568: Date.now() - this._lastInvocation < MIN_INVOCATION_INTERVAL) It seems that we will go into this case if addSelectors() has been running for more than 3 seconds but isn't finished yet. You should be checking the _filteringInProgress flag first. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:570: this._stylesheetQueue = []; This should be `reason.stylesheets || []` I guess? https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:591: if (this._filteringInProgress) This should be `else if` I guess? As it is now, the code might both create a timeout and run processing immediately. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:593: if (!this._nextRun) Why use a new variable here instead of reusing _stylesheetQueue and _domUpdate? This would allow you to merge the `if (this._nextRun)` and `if (this._stylesheetQueue)` scenarios, these are essentially the same. In fact, I'd probably keep _nextRun and merely rename it into _scheduledProcessing for clarity. So the logic would be: if (this._scheduledProcessing) // merge reason with this._scheduledProcessing else if (this._filteringInProgress) this._scheduledProcessing = reason; else if (Date.now() < ...) { this._scheduledProcessing = reason; setTimeout(...); } else this.addSelectors(...); https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:600: this._nextRun.stylesheets.push(...reason.stylesheets); reason.stylesheets is optional, this will fail if it isn't present. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:630: if (mutation.type == "childList") What if the website adds the style element first and sets style.textContent later? https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:635: (added.tagName == "STYLE" || added.tagName == "style") && This should be checking for `added.localName == "style"`. Or you might want to replace both nodeType and localName checks by `added instanceof this.window.HTMLStyleElement`. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:636: added.styesheet) Typo: added.stylesheet I guess that this code path isn't covered by tests? https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:661: this.addSelectors(null, true, () => { If you want queueFiltering() to set important status flags such as _filteringInProgress, I don't think that addSelectors() should ever be called directly any more. However, seeing that you also fail to set _filteringInProgress in queueFiltering() if processing is started immediately, I think that this flag is better maintained by addSelectors(). https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:668: attributeFilter: ["class", "id"] We need to observe characterData changes as well. These are relevant for ContainsSelector and <style> tags. We shouldn't filter attribute changes, you don't know which attributes the rules depend on. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:672: }); The logic here is wrong, we should register the mutation observer and load event handler immediately rather than in the addSelectors() callback. The issue is: with addSelectors() being asynchronous now, DOM modifications can occur while our processing is still in progress. It might be that addSelectors() is early enough in the process that handling these DOM modifications doesn't require a new processing round, but we cannot know that. So we should always schedule new processing just in case.
Two things missing: -Using Performance.now() instead of Date.now() -Writing some test. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:169: if (prefix == null) On 2017/08/10 10:12:20, Wladimir Palant wrote: > How would we get into this situation? From what I can tell, the only case where > evaluate() is being called with a non-constant prefix is right below - and you > already avoid passing in null by checking for `selector == null`. Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:179: continue; On 2017/08/10 10:12:21, Wladimir Palant wrote: > getSelectors() generator might be performing non-trivial work without producing > any non-null values, e.g. if it is a HasSelector instance. So we shouldn't be > just dropping any null values it yields but rather pass them through: > > if (selector == null) > yield null; > else > yield* evaluate(...); Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:184: yield null; On 2017/08/10 10:12:22, Wladimir Palant wrote: > I don't think we need this counter, doing this unconditionally should be fine. > But maybe add a comment explaining why this is necessary, it's not really > obvious. Something like this: > > // Just in case the getSelectors() generator above had to run some heavy > // document.querySelectorAll() call which didn't produce any results, make > // sure there is at least one point where execution can pause. > yield null; Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:240: yield null; On 2017/08/10 10:12:20, Wladimir Palant wrote: > We don't need to protect against this scenario more than once, it is already > accounted for in evaluate(). Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:259: yield null; On 2017/08/10 10:12:22, Wladimir Palant wrote: > Like above, we don't need the counter - an unconditional `yield null` should do. Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:284: yield null; On 2017/08/10 10:12:21, Wladimir Palant wrote: > We don't need to protect against this scenario more than once, it is already > accounted for in evaluate(). Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:293: } On 2017/08/10 10:12:21, Wladimir Palant wrote: > We don't need this counter, just make sure to yield on each loop iteration: > > if (element.textContent.includes(this._text)) > yield element; > else > yield null; > > Nit: multi-line for statement should have brackets around its body. Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:346: function isSelectorHidingOnlyPattern(pattern) On 2017/08/10 10:12:22, Wladimir Palant wrote: > Note that this function is called exactly once, so you could keep this logic > inline just as well. I'm fine with either, giving this a name provides some > value. There was a time it was called twice. but that was unecessary. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:452: * Indicate this is a DOM update. On 2017/08/10 10:12:21, Wladimir Palant wrote: > Why do we need this parameter? Its purpose seems to be summed up as "ignore the > stylesheets" parameter. The logic seems wrong right now, if a DOM modification > includes a <style> tag we will generally limit the processing of > :-abp-properties() clauses. So in case of :-abp-has(:-abp-properties()) we might > miss cases where :-abp-properties() matches new elements but due to old CSS > rules. > > From the look of it, we should generally null out the stylesheets parameter for > any DOM modifications. It doesn't mean "ignore the stylesheets". Both can be together. It means to tell "this is not only a stylesheet only change (line 468 below). But it will become unneeded, so I'm removing it. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:458: this._lastInvocation = Date.now(); On 2017/08/10 10:12:22, Wladimir Palant wrote: > With this method being asynchronous now, I think that we need to set this when > it is done, not when it starts. > > While at it, we should probably switch to Performance.now(). It has an important > property: "the values returned by Performance.now() always increase at a > constant rate," so no issues with clocks being set by NTP or such. Ah right, Date.now() isn't monotonic. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:530: if (now - lastCycle > 50) On 2017/08/10 10:12:21, Wladimir Palant wrote: > No magic numbers please, you should declare a constant like > MAX_SYNCHRONOUS_PROCESSING_TIME. > > The logic is also wrong, it's not the end of the last cycle that's relevant here > but the start of the current one. So you need a cycleStart variable being set to > Date.now() at the beginning of processPatterns(). Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:537: return; On 2017/08/10 10:12:20, Wladimir Palant wrote: > So, why did we go through all the effort yielding null values if we only ever > pause at the end of chain processing? :) > > I think the logic should be something like this: > > let patterns = this.patterns.slice(); > let pattern = null; > let generator = null; > > let processPatterns = () => > { > if (!pattern) > { > if (!patterns.length) > { > ... > done(); > return; > } > > pattern = patterns.shift(); > generator = evaluate(...); > } > > for (let selector of generator) > { > if (selector != null) > { > ... > } > if (Date.now() - cycleStart > MAX_SYNCHRONOUS_PROCESSING_TIME) > this.window.setTimeout(processPatterns, 0); > } > > pattern = null; > return processPatterns(); > }; > processPatterns(); > > Two things to note here: > > * We are creating a copy of this.patterns here. This is important because its > value might change now that our processing is asynchronous. > > * In the recursive call at the end, we return the resulting value. This isn't > really necessary, but allows tail call optimization to kick in. Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:545: }.bind(this); On 2017/08/10 10:12:20, Wladimir Palant wrote: > Don't call bind(), use an arrow function instead. Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:555: /** Filtering reason On 2017/08/10 10:12:21, Wladimir Palant wrote: > Nit: We usually have /** on a line of its own and start JSDoc comments on the > next line. Same below. Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:557: * @property {boolean} dom Indicate the DOM changed (tree or attributes) On 2017/08/10 10:12:21, Wladimir Palant wrote: > I get what this sentence means, but I think that it can be phrased better. > "Indicates that a DOM change triggered reprocessing" maybe? Note that "tree or > attributes" is an implementation detail, one that should already be outdated due > to my comments below. We shouldn't be documenting this. Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:559: * Indicate the stylesheets that needs refresh On 2017/08/10 10:12:22, Wladimir Palant wrote: > Nit: need, not needs > > However, it's not the stylesheets that need a refresh. Rather, it's new > stylesheets that have been added and have to be considered for our filtering. Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:568: Date.now() - this._lastInvocation < MIN_INVOCATION_INTERVAL) On 2017/08/10 10:12:20, Wladimir Palant wrote: > It seems that we will go into this case if addSelectors() has been running for > more than 3 seconds but isn't finished yet. You should be checking the > _filteringInProgress flag first. Oops. Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:570: this._stylesheetQueue = []; On 2017/08/10 10:12:22, Wladimir Palant wrote: > This should be `reason.stylesheets || []` I guess? Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:591: if (this._filteringInProgress) On 2017/08/10 10:12:21, Wladimir Palant wrote: > This should be `else if` I guess? As it is now, the code might both create a > timeout and run processing immediately. Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:593: if (!this._nextRun) On 2017/08/10 10:12:22, Wladimir Palant wrote: > Why use a new variable here instead of reusing _stylesheetQueue and _domUpdate? > This would allow you to merge the `if (this._nextRun)` and `if > (this._stylesheetQueue)` scenarios, these are essentially the same. In fact, I'd > probably keep _nextRun and merely rename it into _scheduledProcessing for > clarity. So the logic would be: > > if (this._scheduledProcessing) > // merge reason with this._scheduledProcessing > else if (this._filteringInProgress) > this._scheduledProcessing = reason; > else if (Date.now() < ...) > { > this._scheduledProcessing = reason; > setTimeout(...); > } > else > this.addSelectors(...); Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:593: if (!this._nextRun) On 2017/08/10 10:12:22, Wladimir Palant wrote: > Why use a new variable here instead of reusing _stylesheetQueue and _domUpdate? > This would allow you to merge the `if (this._nextRun)` and `if > (this._stylesheetQueue)` scenarios, these are essentially the same. In fact, I'd > probably keep _nextRun and merely rename it into _scheduledProcessing for > clarity. So the logic would be: > > if (this._scheduledProcessing) > // merge reason with this._scheduledProcessing > else if (this._filteringInProgress) > this._scheduledProcessing = reason; > else if (Date.now() < ...) > { > this._scheduledProcessing = reason; > setTimeout(...); > } > else > this.addSelectors(...); Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:600: this._nextRun.stylesheets.push(...reason.stylesheets); On 2017/08/10 10:12:23, Wladimir Palant wrote: > reason.stylesheets is optional, this will fail if it isn't present. Acknowledged. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:630: if (mutation.type == "childList") On 2017/08/10 10:12:21, Wladimir Palant wrote: > What if the website adds the style element first and sets style.textContent > later? It might not work. I think we should observe for specific characterData mutations into a HTMLStyleElement. Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:635: (added.tagName == "STYLE" || added.tagName == "style") && On 2017/08/10 10:12:22, Wladimir Palant wrote: > This should be checking for `added.localName == "style"`. Or you might want to > replace both nodeType and localName checks by `added instanceof > this.window.HTMLStyleElement`. Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:636: added.styesheet) On 2017/08/10 10:12:22, Wladimir Palant wrote: > Typo: added.stylesheet oops. > I guess that this code path isn't covered by tests? no. I should add more test. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:661: this.addSelectors(null, true, () => { On 2017/08/10 10:12:22, Wladimir Palant wrote: > If you want queueFiltering() to set important status flags such as > _filteringInProgress, I don't think that addSelectors() should ever be called > directly any more. However, seeing that you also fail to set > _filteringInProgress in queueFiltering() if processing is started immediately, I > think that this flag is better maintained by addSelectors(). I'll call queueFiltering() instead. Totally make sense. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:668: attributeFilter: ["class", "id"] On 2017/08/10 10:12:20, Wladimir Palant wrote: > We need to observe characterData changes as well. These are relevant for > ContainsSelector and <style> tags. > > We shouldn't filter attribute changes, you don't know which attributes the rules > depend on. I kept them deliberately a bit more restricted as to not have too many. Definitely a hole into properly catching all the relevant changes. Done. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elem... chrome/content/elemHideEmulation.js:672: }); On 2017/08/10 10:12:21, Wladimir Palant wrote: > The logic here is wrong, we should register the mutation observer and load event > handler immediately rather than in the addSelectors() callback. The issue is: > with addSelectors() being asynchronous now, DOM modifications can occur while > our processing is still in progress. It might be that addSelectors() is early > enough in the process that handling these DOM modifications doesn't require a > new processing round, but we cannot know that. So we should always schedule new > processing just in case. make sense. Done.
https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:184: yield null; I still don't think we need that counter - we can simply yield unconditionally here. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:443: addSelectors(stylesheets, done) Please rename into _addSelectors() - this is a private function now, only to be called from queueFiltering(). https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:445: this._lastInvocation = Date.now(); This still needs to be set when addSelectors is done, not when it starts. Given that it is being used in queueFiltering(), it makes sense to set this value in the callback there. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:455: if (!stylesheets) Please put the stylesheetOnlyChange logic back, it still makes sense. It should be set to !!stylesheets as before however. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:524: if (Date.now() - cycleStart > MAX_SYNCHRONOUS_PROCESSING_TIME) This check needs to be inside the loop above, so that we can pause while looping through a generator, not merely when done with it. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:549: * @param {FilteringReason} reason why the filtering must be queued. We don't really need this to be a FilteringReason structure any more, an optional stylesheets parameter will do as well. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:572: } This logic is wrong. this._scheduledProcessing.stylesheets should always be null if we've seen DOM modifications, regardless of any stylesheets that loaded. Meaning that we have three scenarios here: 1. this._scheduledProcessing.stylesheets is already null (we've seen DOM modifications) - nothing to do, we need a full reprocessing. 2. this._scheduledProcessing.stylesheets is non-null but reason.stylesheets is null - this is a DOM modification so make sure this._scheduledProcessing.stylesheets is null. 3. Both this._scheduledProcessing.stylesheets and reason.stylesheets are non-null - we've only seen new stylesheets so far, merge them. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:583: let stylesheets = this._scheduledProcessing.stylesheets || []; The `|| []` part here means that we will never do a full reprocessing, only process the additional stylesheets (if any). We should really pass in null if this._scheduledProcessing.stylesheets is null. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:606: let stylesheets = []; stylesheets should always be null for DOM modifications, making sure that we do a full reprocessing. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:614: added.stylesheet) Please drop that special logic. A new <style> element is always a DOM modification, potentially affecting :-abp-has() and :-abp-contains() rules. It means that it warrants a full reprocessing and we don't need to go into the "style changes only" shortcut.
I think have addressed all the comments from that last review. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:184: yield null; On 2017/08/15 11:40:11, Wladimir Palant wrote: > I still don't think we need that counter - we can simply yield unconditionally > here. Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:443: addSelectors(stylesheets, done) On 2017/08/15 11:40:10, Wladimir Palant wrote: > Please rename into _addSelectors() - this is a private function now, only to be > called from queueFiltering(). Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:445: this._lastInvocation = Date.now(); On 2017/08/15 11:40:11, Wladimir Palant wrote: > This still needs to be set when addSelectors is done, not when it starts. Given > that it is being used in queueFiltering(), it makes sense to set this value in > the callback there. Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:455: if (!stylesheets) On 2017/08/15 11:40:11, Wladimir Palant wrote: > Please put the stylesheetOnlyChange logic back, it still makes sense. It should > be set to !!stylesheets as before however. Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:524: if (Date.now() - cycleStart > MAX_SYNCHRONOUS_PROCESSING_TIME) On 2017/08/15 11:40:11, Wladimir Palant wrote: > This check needs to be inside the loop above, so that we can pause while looping > through a generator, not merely when done with it. Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:549: * @param {FilteringReason} reason why the filtering must be queued. On 2017/08/15 11:40:11, Wladimir Palant wrote: > We don't really need this to be a FilteringReason structure any more, an > optional stylesheets parameter will do as well. Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:572: } On 2017/08/15 11:40:11, Wladimir Palant wrote: > This logic is wrong. this._scheduledProcessing.stylesheets should always be null > if we've seen DOM modifications, regardless of any stylesheets that loaded. > Meaning that we have three scenarios here: > > 1. this._scheduledProcessing.stylesheets is already null (we've seen DOM > modifications) - nothing to do, we need a full reprocessing. > 2. this._scheduledProcessing.stylesheets is non-null but reason.stylesheets is > null - this is a DOM modification so make sure > this._scheduledProcessing.stylesheets is null. > 3. Both this._scheduledProcessing.stylesheets and reason.stylesheets are > non-null - we've only seen new stylesheets so far, merge them. Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:583: let stylesheets = this._scheduledProcessing.stylesheets || []; On 2017/08/15 11:40:11, Wladimir Palant wrote: > The `|| []` part here means that we will never do a full reprocessing, only > process the additional stylesheets (if any). We should really pass in null if > this._scheduledProcessing.stylesheets is null. Done. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:606: let stylesheets = []; On 2017/08/15 11:40:11, Wladimir Palant wrote: > stylesheets should always be null for DOM modifications, making sure that we do > a full reprocessing. which is why line 626, we check for the array length before passing it to queueFiltering(). But this code is changing anyway. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elem... chrome/content/elemHideEmulation.js:614: added.stylesheet) On 2017/08/15 11:40:10, Wladimir Palant wrote: > Please drop that special logic. A new <style> element is always a DOM > modification, potentially affecting :-abp-has() and :-abp-contains() rules. It > means that it warrants a full reprocessing and we don't need to go into the > "style changes only" shortcut. Done.
This last review uses Performance.now() instead of Date.now()
https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:546: * the stylesheets triggered the refiltering. This needs to be explained better, particularly the significance of this parameter missing. How about: new stylesheets to be processed. This parameter should be omitted for DOM modification (full reprocessing required). Also, please rename the parameter, it's no longer clear why it should be called `reason`. `stylesheets` will do as a name. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:556: let nextReason = this._scheduledProcessing.stylesheets; Please rename this variable, `stylesheets` will do as a name. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:569: this._scheduledProcessing.stylesheets = reason; Why distinguish between null and undefined? The else case is unnecessary here - if this._scheduledProcessing.stylesheets isn't set we are always in the "nothing to do" case. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:577: this._scheduledProcessing.stylesheets = reason; Please set this in one go (particularly easy if you rename the parameter): this._scheduledProcessing = {stylesheets}; https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:581: // So if _lastInvocation is 0, we assume it never ran. Why so complicated? You can initialize _lastInvocation with -MIN_INVOCATION_INTERVAL, then the interval is guaranteed to be at least MIN_INVOCATION_INTERVAL for the initial run. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:587: this._scheduledProcessing.stylesheets = reason; As above, please set this in one go. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:617: if (mutation.type == "characterData") This special logic should go as well, it's the same as with a new <style> tag because of :-abp-contains() rules.
https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:546: * the stylesheets triggered the refiltering. On 2017/08/16 07:19:55, Wladimir Palant wrote: > This needs to be explained better, particularly the significance of this > parameter missing. How about: > > new stylesheets to be processed. This parameter should be omitted for DOM > modification (full reprocessing required). > > Also, please rename the parameter, it's no longer clear why it should be called > `reason`. `stylesheets` will do as a name. Done. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:556: let nextReason = this._scheduledProcessing.stylesheets; On 2017/08/16 07:19:54, Wladimir Palant wrote: > Please rename this variable, `stylesheets` will do as a name. Done https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:569: this._scheduledProcessing.stylesheets = reason; On 2017/08/16 07:19:54, Wladimir Palant wrote: > Why distinguish between null and undefined? The else case is unnecessary here - > if this._scheduledProcessing.stylesheets isn't set we are always in the "nothing > to do" case. Indeed. Done. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:577: this._scheduledProcessing.stylesheets = reason; On 2017/08/16 07:19:54, Wladimir Palant wrote: > Please set this in one go (particularly easy if you rename the parameter): > > this._scheduledProcessing = {stylesheets}; Done. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:581: // So if _lastInvocation is 0, we assume it never ran. On 2017/08/16 07:19:54, Wladimir Palant wrote: > Why so complicated? You can initialize _lastInvocation with > -MIN_INVOCATION_INTERVAL, then the interval is guaranteed to be at least > MIN_INVOCATION_INTERVAL for the initial run. Done. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:587: this._scheduledProcessing.stylesheets = reason; On 2017/08/16 07:19:55, Wladimir Palant wrote: > As above, please set this in one go. Done. https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elem... chrome/content/elemHideEmulation.js:617: if (mutation.type == "characterData") On 2017/08/16 07:19:54, Wladimir Palant wrote: > This special logic should go as well, it's the same as with a new <style> tag > because of :-abp-contains() rules. if it is a characterData change on something else, the whole filtering it triggered. if it is on a style element I was limiting the update to stylesheet only, which could be overridden by the next change. Removing it.
Only minor issues remaining now - and test coverage. https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elem... chrome/content/elemHideEmulation.js:428: _lastInvocation: -MIN_INVOCATION_INTERVAL, Nit: please move this declaration down. This member is only used by queueFiltering() now, it should be declared next to _filteringInProgress. https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elem... chrome/content/elemHideEmulation.js:571: this._scheduledProcessing.stylesheets = null; No need for multiple indentation levels here: if (!stylesheets) ... else if (this._scheduledProcessing.stylesheets) ...
test coverage will follow. https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elem... File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elem... chrome/content/elemHideEmulation.js:428: _lastInvocation: -MIN_INVOCATION_INTERVAL, On 2017/08/17 08:58:59, Wladimir Palant wrote: > Nit: please move this declaration down. This member is only used by > queueFiltering() now, it should be declared next to _filteringInProgress. Done. https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elem... chrome/content/elemHideEmulation.js:571: this._scheduledProcessing.stylesheets = null; On 2017/08/17 08:58:59, Wladimir Palant wrote: > No need for multiple indentation levels here: > > if (!stylesheets) > ... > else if (this._scheduledProcessing.stylesheets) > ... Done.
Looks good, now this only needs tests.
I need to address these failures that I can reproduce with master first. https://codereview.adblockplus.org/29494577/diff/29523751/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29523751/test/browser/elemHi... test/browser/elemHideEmulation.js:507: //expectVisible(test, child); This is where test are running into problems. This expectation fails. There other commented below as well. This kind of issue are also happening with master when I wait 4000ms and check again which leads me to believe it is not cause by this patch. I'm looking at what the problem actually is. https://codereview.adblockplus.org/29494577/diff/29523751/test/browser/elemHi... test/browser/elemHideEmulation.js:533: expectVisible(test, parent); This one not commented out fail.
The problem boils down to the fact that we register the event listener for "load" but don't unregister it. But for the test, the document object is reused, so we end up with several listener and several ElemHideEmulation instance.
This allow test to work. I create an iframe to run the tests and clean it up. This clear up the "load" eventListener.
https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:26: testDocument = null; This assignment seems pointless. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:37: let iframe = document.getElementById("test-iframe"); Use `testDocument.defaultView.frameElement` here? Then you won't need the frame id either. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:38: iframe.parentNode.removeChild(iframe); Please null out testDocument as well, so that trying to use it after this point throws. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:519: }); Please use a helper to promisify timeouts: function timeout(delay) { return new Promise((resolve, reject) => { window.setTimeout(resolve, delay); }); } Then you can write this and similar passages in a much clearer fashion: .then(() => { ... return timeout(0); }).then(() => { ... return timeout(4000); }).then(() => { ... }) https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:534: child.innerText = "hide me"; Please use child.textContent, innerText is non-standard. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:586: }, 4000); The huge delays here are an issue. We need to change MIN_INVOCATION_INTERVAL for unit tests to a much lower value, like 100. Not sure what the best approach for this would be, maybe string-replace-webpack-plugin?
Updated patch. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:26: testDocument = null; On 2017/08/23 13:23:31, Wladimir Palant wrote: > This assignment seems pointless. Done. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:37: let iframe = document.getElementById("test-iframe"); On 2017/08/23 13:23:32, Wladimir Palant wrote: > Use `testDocument.defaultView.frameElement` here? Then you won't need the frame > id either. Done. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:38: iframe.parentNode.removeChild(iframe); On 2017/08/23 13:23:31, Wladimir Palant wrote: > Please null out testDocument as well, so that trying to use it after this point > throws. Done. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:519: }); On 2017/08/23 13:23:32, Wladimir Palant wrote: > Please use a helper to promisify timeouts: > > function timeout(delay) > { > return new Promise((resolve, reject) => > { > window.setTimeout(resolve, delay); > }); > } > > Then you can write this and similar passages in a much clearer fashion: > > .then(() => > { > ... > return timeout(0); > }).then(() => > { > ... > return timeout(4000); > }).then(() => > { > ... > }) Done. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:534: child.innerText = "hide me"; On 2017/08/23 13:23:32, Wladimir Palant wrote: > Please use child.textContent, innerText is non-standard. Done. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHi... test/browser/elemHideEmulation.js:586: }, 4000); On 2017/08/23 13:23:32, Wladimir Palant wrote: > The huge delays here are an issue. We need to change MIN_INVOCATION_INTERVAL for > unit tests to a much lower value, like 100. Not sure what the best approach for > this would be, maybe string-replace-webpack-plugin? IMHO the best approach would be to have it configurable in elemHideEmulation. This would be clean, and we could also have the possibility to make that a preference for a reason or the other. Done.
https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHid... lib/content/elemHideEmulation.js:511: }, You can have a setter without a getter but that's not nice to say the least. Given that this is really for tests only, maybe it's best to keep the MIN_INVOCATION_INTERVAL global around and merely make it non-constant. Then you could add a getter/setter combination here and note in a comment that unit tests need to change this constant: get MIN_INVOCATION_INTERVAL() { return MIN_INVOCATION_INTERVAL; }, set MIN_INVOCATION_INTERVAL(interval) { MIN_INVOCATION_INTERVAL = interval; } https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHid... lib/content/elemHideEmulation.js:514: _lastInvocation: -this._MIN_INVOCATION_INTERVAL, This assignment won't work, `this` isn't pointing to the (not yet existent) object. https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:136: elemHideEmulation._invocationInterval = 100; REFRESH_INTERVAL / 2? https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:261: // Re-evaluation will only happen after a few seconds "after hundred milliseconds" or just "after a delay"? https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:262: expectVisible(test, toHide); Testing for synchronous changes here and below is a bit pointless, mutation observers aren't synchronous. Maybe `return timeout(0)` before reevaluating the state? https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:564: I guess the two lines above should be removed? You are testing the state of sibling below.
https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHid... lib/content/elemHideEmulation.js:511: }, On 2017/08/25 07:44:58, Wladimir Palant wrote: > You can have a setter without a getter but that's not nice to say the least. > Given that this is really for tests only, maybe it's best to keep the > MIN_INVOCATION_INTERVAL global around and merely make it non-constant. Then you > could add a getter/setter combination here and note in a comment that unit tests > need to change this constant: > > get MIN_INVOCATION_INTERVAL() > { > return MIN_INVOCATION_INTERVAL; > }, > > set MIN_INVOCATION_INTERVAL(interval) > { > MIN_INVOCATION_INTERVAL = interval; > } Done. https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHid... lib/content/elemHideEmulation.js:514: _lastInvocation: -this._MIN_INVOCATION_INTERVAL, On 2017/08/25 07:44:58, Wladimir Palant wrote: > This assignment won't work, `this` isn't pointing to the (not yet existent) > object. oops. https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:136: elemHideEmulation._invocationInterval = 100; On 2017/08/25 07:44:59, Wladimir Palant wrote: > REFRESH_INTERVAL / 2? Done. https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:261: // Re-evaluation will only happen after a few seconds On 2017/08/25 07:44:59, Wladimir Palant wrote: > "after hundred milliseconds" or just "after a delay"? Done. https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:262: expectVisible(test, toHide); On 2017/08/25 07:44:59, Wladimir Palant wrote: > Testing for synchronous changes here and below is a bit pointless, mutation > observers aren't synchronous. Maybe `return timeout(0)` before reevaluating the > state? Done. https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:564: On 2017/08/25 07:44:59, Wladimir Palant wrote: > I guess the two lines above should be removed? You are testing the state of > sibling below. Done.
https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHi... test/browser/elemHideEmulation.js:262: expectVisible(test, toHide); On 2017/08/25 13:48:14, hub wrote: > On 2017/08/25 07:44:59, Wladimir Palant wrote: > > Testing for synchronous changes here and below is a bit pointless, mutation > > observers aren't synchronous. Maybe `return timeout(0)` before reevaluating > the > > state? > > Done. You only fixed one instance of this issue. I noted the other ones. https://codereview.adblockplus.org/29494577/diff/29527741/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29527741/lib/content/elemHid... lib/content/elemHideEmulation.js:505: // this pair ot setter/getter is only used in the tests to Nit: "This property is only used" (simpler and capitalized properly) https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:517: insertStyleRule("body #" + parent.id + " > div { background-color: #000}"); `return timeout(0)` before retesting state? https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:540: child.textContent = "hide me"; `return timeout(0)` before retesting state? https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:565: sibling = createElementWithStyle("{}"); `return timeout(0)` before retesting state? https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:578: sibling); `return timeout(0)` before retesting state?
https://codereview.adblockplus.org/29494577/diff/29527741/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29527741/lib/content/elemHid... lib/content/elemHideEmulation.js:505: // this pair ot setter/getter is only used in the tests to On 2017/08/25 20:57:41, Wladimir Palant wrote: > Nit: "This property is only used" (simpler and capitalized properly) Done. https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:517: insertStyleRule("body #" + parent.id + " > div { background-color: #000}"); On 2017/08/25 20:57:41, Wladimir Palant wrote: > `return timeout(0)` before retesting state? Done. https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:540: child.textContent = "hide me"; On 2017/08/25 20:57:41, Wladimir Palant wrote: > `return timeout(0)` before retesting state? Done. https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:565: sibling = createElementWithStyle("{}"); On 2017/08/25 20:57:41, Wladimir Palant wrote: > `return timeout(0)` before retesting state? Done. https://codereview.adblockplus.org/29494577/diff/29527741/test/browser/elemHi... test/browser/elemHideEmulation.js:578: sibling); On 2017/08/25 20:57:41, Wladimir Palant wrote: > `return timeout(0)` before retesting state? Done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
