Issue 4592 - Adapt messaging code for the new element hiding emulation filters

Issue 4592 - Adapt messaging code for the new element hiding emulation filters

[Felix Dahlke, 2016-11-03 18:25:08]

Some fairly simple changes that accompany
https://codereview.adblockplus.org/29361668/.

[kzar, 2016-11-04 14:37:21]

Otherwise LGTM. (Assuming we do go ahead with parsing emulation filters on the
content script side, but that's a discussion for the other review.)

https://codereview.adblockplus.org/29361722/diff/29361723/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29361722/diff/29361723/messageResponder.js...
messageResponder.js:232: filters =
ElemHideEmulation.getRulesForDomain(sender.frame.url.hostname);
Nit: Mind fixing this long line?

[Felix Dahlke, 2016-11-04 17:38:46]

Thanks, new patch set is up. I will land #4394 before I land this.

https://codereview.adblockplus.org/29361722/diff/29361723/messageResponder.js
File messageResponder.js (right):

https://codereview.adblockplus.org/29361722/diff/29361723/messageResponder.js...
messageResponder.js:232: filters =
ElemHideEmulation.getRulesForDomain(sender.frame.url.hostname);
On 2016/11/04 14:37:21, kzar wrote:
> Nit: Mind fixing this long line?

Done.

[Thomas Greiner, 2016-11-07 13:27:08]

LGTM, just a small suggestion I added.

https://codereview.adblockplus.org/29361722/diff/29361741/background.js
File background.js (right):

https://codereview.adblockplus.org/29361722/diff/29361741/background.js#newco...
background.js:320: getRulesForDomain: function(domain) { }
Detail: It doesn't look like this function is even being used in the test
environment. Therefore I'd suggest removing it while keeping `ElemHideEmulation`
as an empty object.

[Felix Dahlke, 2016-11-21 17:33:06]

Removed the `features` property since we removed it in
https://codereview.adblockplus.org/29361668/.

https://codereview.adblockplus.org/29361722/diff/29361741/background.js
File background.js (right):

https://codereview.adblockplus.org/29361722/diff/29361741/background.js#newco...
background.js:320: getRulesForDomain: function(domain) { }
On 2016/11/07 13:27:08, Thomas Greiner wrote:
> Detail: It doesn't look like this function is even being used in the test
> environment. Therefore I'd suggest removing it while keeping
`ElemHideEmulation`
> as an empty object.

Would you be OK with a separate noissue commit for this? Seems wrong to me to
touch this as part of a dependency update.

[kzar, 2016-11-21 19:14:59]

LGTM

https://codereview.adblockplus.org/29361722/diff/29361741/background.js
File background.js (right):

https://codereview.adblockplus.org/29361722/diff/29361741/background.js#newco...
background.js:320: getRulesForDomain: function(domain) { }
IMO if it's not used we might as well just remove it now, but I don't mind if
it's in a separate commit or not. Up to you guys.

[Thomas Greiner, 2016-11-22 12:04:10]

LGTM

https://codereview.adblockplus.org/29361722/diff/29361741/background.js
File background.js (right):

https://codereview.adblockplus.org/29361722/diff/29361741/background.js#newco...
background.js:320: getRulesForDomain: function(domain) { }
On 2016/11/21 17:33:06, Felix Dahlke wrote:
> On 2016/11/07 13:27:08, Thomas Greiner wrote:
> > Detail: It doesn't look like this function is even being used in the test
> > environment. Therefore I'd suggest removing it while keeping
> `ElemHideEmulation`
> > as an empty object.
> 
> Would you be OK with a separate noissue commit for this? Seems wrong to me to
> touch this as part of a dependency update.

That's fine, yeah.

[Wladimir Palant, 2016-11-22 12:07:20]

LGTM