Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(236)

Issue 29494577: Issue 5438 - Observer DOM changes to reapply filters.

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month ago by hub
Modified:
3 days, 21 hours ago
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+153 lines, -44 lines) Patch
M chrome/content/elemHideEmulation.js View 1 2 3 4 5 6 7 9 chunks +153 lines, -44 lines 0 comments Download

Messages

Total messages: 20
hub
1 month ago (2017-07-21 19:53:38 UTC) #1
hub
This is the initial implementation. I need to check the performance impact. It applies on ...
1 month ago (2017-07-21 19:55:28 UTC) #2
Wladimir Palant
This isn't quite what I had in mind, see updated issue description. Sorry that it ...
1 month ago (2017-07-21 21:37:29 UTC) #3
hub
I'll rework the patch based on the changes in the bug description. https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.eslintrc.json File chrome/content/.eslintrc.json ...
1 month ago (2017-07-22 01:31:24 UTC) #4
Wladimir Palant
https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.eslintrc.json File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.eslintrc.json#newcode3 chrome/content/.eslintrc.json:3: "MutationObserver": true, On 2017/07/22 01:31:24, hub wrote: > I ...
1 month ago (2017-07-22 06:16:21 UTC) #5
hub
https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.eslintrc.json File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29494578/chrome/content/.eslintrc.json#newcode3 chrome/content/.eslintrc.json:3: "MutationObserver": true, On 2017/07/22 06:16:20, Wladimir Palant wrote: > ...
1 month ago (2017-07-23 15:36:34 UTC) #6
hub
This is the second iteration. I'm not totally satisfied by it. https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): ...
2 weeks, 6 days ago (2017-08-02 04:10:55 UTC) #7
hub
https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/elemHideEmulation.js#newcode531 chrome/content/elemHideEmulation.js:531: let domUpdate = reason.dom; I realise this should have ...
2 weeks, 5 days ago (2017-08-03 16:28:18 UTC) #8
Wladimir Palant
While this patch has its issues, maybe we should just try another approach. I outlined ...
2 weeks ago (2017-08-08 11:01:00 UTC) #9
hub
This is following the new design. https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/.eslintrc.json File chrome/content/.eslintrc.json (right): https://codereview.adblockplus.org/29494577/diff/29502655/chrome/content/.eslintrc.json#newcode4 chrome/content/.eslintrc.json:4: "Node": true On ...
1 week, 5 days ago (2017-08-09 20:16:57 UTC) #10
Wladimir Palant
In addition to the points below, this needs unit tests - for the DOM modification ...
1 week, 5 days ago (2017-08-10 10:12:23 UTC) #11
hub
Two things missing: -Using Performance.now() instead of Date.now() -Writing some test. https://codereview.adblockplus.org/29494577/diff/29510638/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): ...
1 week, 4 days ago (2017-08-11 16:26:51 UTC) #12
Wladimir Palant
https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elemHideEmulation.js#newcode184 chrome/content/elemHideEmulation.js:184: yield null; I still don't think we need that ...
1 week ago (2017-08-15 11:40:12 UTC) #13
hub
I think have addressed all the comments from that last review. https://codereview.adblockplus.org/29494577/diff/29512716/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): ...
1 week ago (2017-08-15 16:39:10 UTC) #14
hub
This last review uses Performance.now() instead of Date.now()
6 days, 19 hours ago (2017-08-15 23:31:37 UTC) #15
Wladimir Palant
https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elemHideEmulation.js#newcode546 chrome/content/elemHideEmulation.js:546: * the stylesheets triggered the refiltering. This needs to ...
6 days, 11 hours ago (2017-08-16 07:19:55 UTC) #16
hub
https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29516677/chrome/content/elemHideEmulation.js#newcode546 chrome/content/elemHideEmulation.js:546: * the stylesheets triggered the refiltering. On 2017/08/16 07:19:55, ...
6 days, 4 hours ago (2017-08-16 13:57:29 UTC) #17
Wladimir Palant
Only minor issues remaining now - and test coverage. https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elemHideEmulation.js#newcode428 chrome/content/elemHideEmulation.js:428: ...
5 days, 9 hours ago (2017-08-17 08:58:59 UTC) #18
hub
test coverage will follow. https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29517631/chrome/content/elemHideEmulation.js#newcode428 chrome/content/elemHideEmulation.js:428: _lastInvocation: -MIN_INVOCATION_INTERVAL, On 2017/08/17 08:58:59, ...
4 days, 5 hours ago (2017-08-18 13:03:35 UTC) #19
Wladimir Palant
3 days, 21 hours ago (2017-08-18 20:56:15 UTC) #20
Looks good, now this only needs tests.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5