Issue 4962 - Fix the tests.- Fix syntax in chrome/content/elemHideEmulation.js to use ES5Regress…

Issue 4962 - Fix the tests.

- Fix syntax in chrome/content/elemHideEmulation.js to use ES5
Regression from 2fc6eeda3478
- Call ElemHideEmulation.apply() since load() is gone
Regression from c7ae7532578a

[Wladimir Palant, 2017-03-08 10:31:50]

https://codereview.adblockplus.org/29378674/diff/29378686/chrome/content/elem...
File chrome/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29378674/diff/29378686/chrome/content/elem...
chrome/content/elemHideEmulation.js:97: this.patterns.forEach(function(pattern)
Last I checked, calling forEach caused suboptimal performance. Also, I don't see
a point risking more regressions here. It should be best to just revert the
unnecessary changes from https://codereview.adblockplus.org/29372676/.

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
test/browser/elemHideEmulation.js:133: callback();
I'm undecided between simplifying the unit test (remove the callback parameter
and just call this function in a synchronous fashion) and leaving things as they
are (could ElemHideEmulation possibly become async again in future?).

[hub, 2017-03-08 13:34:21]

will update the patch

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
test/browser/elemHideEmulation.js:133: callback();
On 2017/03/08 10:31:50, Wladimir Palant wrote:
> I'm undecided between simplifying the unit test (remove the callback parameter
> and just call this function in a synchronous fashion) and leaving things as
they
> are (could ElemHideEmulation possibly become async again in future?).

My take was to avoid logic changes to fix the test because I need passing tests
to move onto the issue I'm working on.

[hub, 2017-03-08 13:36:55]

patch updated

[Felix Dahlke, 2017-03-08 20:35:49]

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
test/browser/elemHideEmulation.js:133: callback();
On 2017/03/08 13:34:21, hub wrote:
> On 2017/03/08 10:31:50, Wladimir Palant wrote:
> > I'm undecided between simplifying the unit test (remove the callback
parameter
> > and just call this function in a synchronous fashion) and leaving things as
> they
> > are (could ElemHideEmulation possibly become async again in future?).
> 
> My take was to avoid logic changes to fix the test because I need passing
tests
> to move onto the issue I'm working on.

No strong opinion, but I think we should simplify this while at it - it's not a
big change and it just doesn't make sense to have the callback parameter now.
Picturing myself reading this in a year from now, I see myself scratching my
head over what the purpose is.

[hub, 2017-03-08 20:39:48]

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
test/browser/elemHideEmulation.js:133: callback();
On 2017/03/08 20:35:49, Felix Dahlke wrote:
> On 2017/03/08 13:34:21, hub wrote:
> > On 2017/03/08 10:31:50, Wladimir Palant wrote:
> > > I'm undecided between simplifying the unit test (remove the callback
> parameter
> > > and just call this function in a synchronous fashion) and leaving things
as
> > they
> > > are (could ElemHideEmulation possibly become async again in future?).
> > 
> > My take was to avoid logic changes to fix the test because I need passing
> tests
> > to move onto the issue I'm working on.
> 
> No strong opinion, but I think we should simplify this while at it - it's not
a
> big change and it just doesn't make sense to have the callback parameter now.
> Picturing myself reading this in a year from now, I see myself scratching my
> head over what the purpose is.

I can make that part of bug #3143 since there will be some rework done anyway.

[Felix Dahlke, 2017-03-08 21:44:56]

(Moved myself back to CC.)

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29378674/diff/29378686/test/browser/elemHi...
test/browser/elemHideEmulation.js:133: callback();
On 2017/03/08 20:39:48, hub wrote:
> On 2017/03/08 20:35:49, Felix Dahlke wrote:
> > On 2017/03/08 13:34:21, hub wrote:
> > > On 2017/03/08 10:31:50, Wladimir Palant wrote:
> > > > I'm undecided between simplifying the unit test (remove the callback
> > parameter
> > > > and just call this function in a synchronous fashion) and leaving things
> as
> > > they
> > > > are (could ElemHideEmulation possibly become async again in future?).
> > > 
> > > My take was to avoid logic changes to fix the test because I need passing
> > tests
> > > to move onto the issue I'm working on.
> > 
> > No strong opinion, but I think we should simplify this while at it - it's
not
> a
> > big change and it just doesn't make sense to have the callback parameter
now.
> > Picturing myself reading this in a year from now, I see myself scratching my
> > head over what the purpose is.
> 
> I can make that part of bug #3143 since there will be some rework done anyway.

Fine by me.

[Wladimir Palant, 2017-03-09 08:52:11]

LGTM