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

Issue 29372676: [adblockpluscore] Issue 4825 - Let ElemHideEmulation associate selector and filter when calling add… (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 12 months ago by wspee
Modified:
2 years, 10 months ago
Visibility:
Public.

Description

[adblockpluscore] Issue 4825 - Let ElemHideEmulation associate selector and filter when calling add…

Patch Set 1 #

Total comments: 4

Patch Set 2 : Use map instead of object #

Total comments: 2

Patch Set 3 : No longer create temp array and use for of #

Total comments: 4

Patch Set 4 : Removed unnecessary variable and use let instead of var #

Total comments: 6

Patch Set 5 : Changed map to two arrays #

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

Messages

Total messages: 12
Sebastian Noack
https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elemHideEmulation.js#newcode100 chrome/content/elemHideEmulation.js:100: var selectors = [] Nit: Missing semicolon https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elemHideEmulation.js#newcode106 chrome/content/elemHideEmulation.js:106: ...
2 years, 11 months ago (2017-02-06 14:59:21 UTC) #1
wspee
https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29372677/chrome/content/elemHideEmulation.js#newcode100 chrome/content/elemHideEmulation.js:100: var selectors = [] On 2017/02/06 14:59:21, Sebastian Noack ...
2 years, 11 months ago (2017-02-07 11:59:41 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elemHideEmulation.js#newcode107 chrome/content/elemHideEmulation.js:107: Array.prototype.push.apply(filters.get(pattern.text), selectors); Creating a temporary array, just to merge ...
2 years, 11 months ago (2017-02-07 12:35:59 UTC) #3
wspee
https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374665/chrome/content/elemHideEmulation.js#newcode107 chrome/content/elemHideEmulation.js:107: Array.prototype.push.apply(filters.get(pattern.text), selectors); On 2017/02/07 12:35:59, Sebastian Noack wrote: > ...
2 years, 11 months ago (2017-02-07 14:15:08 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elemHideEmulation.js#newcode105 chrome/content/elemHideEmulation.js:105: let subSelectors = splitSelector(rule.selectorText); The temporary variable is no ...
2 years, 11 months ago (2017-02-07 14:55:53 UTC) #5
wspee
https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374670/chrome/content/elemHideEmulation.js#newcode105 chrome/content/elemHideEmulation.js:105: let subSelectors = splitSelector(rule.selectorText); On 2017/02/07 14:55:53, Sebastian Noack ...
2 years, 11 months ago (2017-02-07 15:01:06 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js#newcode93 chrome/content/elemHideEmulation.js:93: Nit: The blank line added here is unrelated at ...
2 years, 11 months ago (2017-02-07 15:37:57 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js#newcode117 chrome/content/elemHideEmulation.js:117: this.addSelectorsFunc(filters); On 2017/02/07 15:37:57, Sebastian Noack wrote: > I ...
2 years, 11 months ago (2017-02-07 15:59:56 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js#newcode117 chrome/content/elemHideEmulation.js:117: this.addSelectorsFunc(filters); On 2017/02/07 15:59:56, Sebastian Noack wrote: > On ...
2 years, 11 months ago (2017-02-08 07:12:30 UTC) #9
wspee
https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js File chrome/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29372676/diff/29374672/chrome/content/elemHideEmulation.js#newcode93 chrome/content/elemHideEmulation.js:93: On 2017/02/07 15:37:57, Sebastian Noack wrote: > Nit: The ...
2 years, 11 months ago (2017-02-09 08:40:34 UTC) #10
Sebastian Noack
LGTM. But since Wladimir is module owner for core, he should be aware of this ...
2 years, 11 months ago (2017-02-09 10:33:16 UTC) #11
Wladimir Palant
2 years, 11 months ago (2017-02-21 10:25:04 UTC) #12
LGTM
Sign in to reply to this message.

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