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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months ago by hub
Modified:
1 month, 3 weeks 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 #

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+317 lines, -84 lines) Patch
M lib/content/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 10 11 12 9 chunks +166 lines, -45 lines 0 comments Download
M test/browser/elemHideEmulation.js View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +151 lines, -39 lines 0 comments Download

Messages

Total messages: 30
hub
3 months 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 ...
3 months 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 ...
3 months 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 ...
3 months 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 ...
3 months 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: > ...
2 months, 4 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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): ...
2 months, 1 week 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 ...
2 months 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): ...
2 months ago (2017-08-15 16:39:10 UTC) #14
hub
This last review uses Performance.now() instead of Date.now()
2 months 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 ...
2 months 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, ...
2 months 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: ...
2 months 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, ...
2 months ago (2017-08-18 13:03:35 UTC) #19
Wladimir Palant
Looks good, now this only needs tests.
2 months ago (2017-08-18 20:56:15 UTC) #20
hub
I need to address these failures that I can reproduce with master first. https://codereview.adblockplus.org/29494577/diff/29523751/test/browser/elemHideEmulation.js File ...
1 month, 4 weeks ago (2017-08-23 01:11:31 UTC) #21
hub
The problem boils down to the fact that we register the event listener for "load" ...
1 month, 4 weeks ago (2017-08-23 02:21:06 UTC) #22
hub
This allow test to work. I create an iframe to run the tests and clean ...
1 month, 4 weeks ago (2017-08-23 02:44:27 UTC) #23
Wladimir Palant
https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHideEmulation.js File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHideEmulation.js#newcode26 test/browser/elemHideEmulation.js:26: testDocument = null; This assignment seems pointless. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHideEmulation.js#newcode37 test/browser/elemHideEmulation.js:37: ...
1 month, 4 weeks ago (2017-08-23 13:23:32 UTC) #24
hub
Updated patch. https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHideEmulation.js File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29523754/test/browser/elemHideEmulation.js#newcode26 test/browser/elemHideEmulation.js:26: testDocument = null; On 2017/08/23 13:23:31, Wladimir ...
1 month, 4 weeks ago (2017-08-23 16:38:01 UTC) #25
Wladimir Palant
https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHideEmulation.js#newcode511 lib/content/elemHideEmulation.js:511: }, You can have a setter without a getter ...
1 month, 3 weeks ago (2017-08-25 07:45:00 UTC) #26
hub
https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/lib/content/elemHideEmulation.js#newcode511 lib/content/elemHideEmulation.js:511: }, On 2017/08/25 07:44:58, Wladimir Palant wrote: > You ...
1 month, 3 weeks ago (2017-08-25 13:48:14 UTC) #27
Wladimir Palant
https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHideEmulation.js File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29524913/test/browser/elemHideEmulation.js#newcode262 test/browser/elemHideEmulation.js:262: expectVisible(test, toHide); On 2017/08/25 13:48:14, hub wrote: > On ...
1 month, 3 weeks ago (2017-08-25 20:57:43 UTC) #28
hub
https://codereview.adblockplus.org/29494577/diff/29527741/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29494577/diff/29527741/lib/content/elemHideEmulation.js#newcode505 lib/content/elemHideEmulation.js:505: // this pair ot setter/getter is only used in ...
1 month, 3 weeks ago (2017-08-26 01:45:07 UTC) #29
Wladimir Palant
1 month, 3 weeks ago (2017-08-28 08:01:07 UTC) #30
LGTM
Sign in to reply to this message.

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