Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29859558: Issue 6741 - Use ES2015 classes in lib/content/elemHideEmulation.js (Closed)

Created:
Aug. 20, 2018, 5:55 p.m. by Manish Jethani
Modified:
Aug. 21, 2018, 3:42 p.m.
Reviewers:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 #

Total comments: 6

Patch Set 2 : Initialize properties with default values first #

Unified diffs Side-by-side diffs Delta from patch set Stats (+85 lines, -79 lines) Patch
M lib/content/elemHideEmulation.js View 1 11 chunks +85 lines, -79 lines 0 comments Download

Messages

Total messages: 7
Manish Jethani
Aug. 20, 2018, 5:55 p.m. (2018-08-20 17:55:19 UTC) #1
Manish Jethani
Patch Set 1
Aug. 20, 2018, 5:59 p.m. (2018-08-20 17:59:14 UTC) #2
hub
https://codereview.adblockplus.org/29859558/diff/29859559/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29859558/diff/29859559/lib/content/elemHideEmulation.js#newcode273 lib/content/elemHideEmulation.js:273: this.dependsOnDOM = true; I'd feel better if the properties ...
Aug. 20, 2018, 7:51 p.m. (2018-08-20 19:51:21 UTC) #3
Manish Jethani
Patch Set 2: Initialize properties with default values first https://codereview.adblockplus.org/29859558/diff/29859559/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29859558/diff/29859559/lib/content/elemHideEmulation.js#newcode273 lib/content/elemHideEmulation.js:273: ...
Aug. 21, 2018, 3:45 a.m. (2018-08-21 03:45:30 UTC) #4
hub
LGTM Have we validated this is not a problem on Edge? I see on MDN ...
Aug. 21, 2018, 2:32 p.m. (2018-08-21 14:32:47 UTC) #5
Manish Jethani
On 2018/08/21 14:32:47, hub wrote: > LGTM Thanks! > Have we validated this is not ...
Aug. 21, 2018, 2:47 p.m. (2018-08-21 14:47:58 UTC) #6
hub
Aug. 21, 2018, 3:42 p.m. (2018-08-21 15:42:30 UTC) #7
Message was sent while issue was closed.
> > Have we validated this is not a problem on Edge? I see on MDN "class" is
> > supported.
> 
> According to this comment we have Edge covered:
> 
> https://issues.adblockplus.org/ticket/6482#comment:18

Excellent!

Powered by Google App Engine
This is Rietveld