Issue 2397 - Integrate CSS property filters into adblockpluschrome

Issue 2397 - Integrate CSS property filters into adblockpluschrome

[kzar, 2015-11-30 15:51:11]

Patch Set 1

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js#...
include.preload.js:419: new CSSPropertyFilters(window, addElemHideSelectors);
The changes to this file are almost entirely by Sebastian, I can only take
credit for this part as we now need to pass addElemHideSelectors through to the
CSSPropertyFilters constructor.

[Sebastian Noack, 2015-11-30 16:06:29]

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js#...
include.preload.js:361: CSSPropertyFilters.apply();
This method doesn't exist anymore, since we turned it into a class.

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js#...
include.preload.js:370: CSSPropertyFilters.load(function()
Same for that one. Or do I miss something?

[kzar, 2015-11-30 17:39:07]

Patch Set 2 : Adjust code now that CSSPropertyFilters is a class

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js#...
include.preload.js:361: CSSPropertyFilters.apply();
On 2015/11/30 16:06:29, Sebastian Noack wrote:
> This method doesn't exist anymore, since we turned it into a class.

Acknowledged.

https://codereview.adblockplus.org/29331619/diff/29331620/include.preload.js#...
include.preload.js:370: CSSPropertyFilters.load(function()
On 2015/11/30 16:06:29, Sebastian Noack wrote:
> Same for that one. Or do I miss something?

No, you're right. I think it's only a coincidence that the code worked as it
was.

I've had a go at fixing it, not entirely sure about my approach. What do you
think?

[kzar, 2015-11-30 17:57:32]

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#...
include.preload.js:342: var updateStylesheet = function(propertyFilters)
I deliberately added a typo in the propertFilters parameter name here and
rebuilt the extension to ensure it would break. It _didn't_ break and I can't
see why. I'm stopping for this evening now but I just want to say that I'm now
suspicious of this code / my method of testing it.

[kzar, 2015-12-01 10:46:04]

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#...
include.preload.js:342: var updateStylesheet = function(propertyFilters)
On 2015/11/30 17:57:32, kzar wrote:
> I deliberately added a typo in the propertFilters parameter name here and
> rebuilt the extension to ensure it would break. It _didn't_ break and I can't
> see why. I'm stopping for this evening now but I just want to say that I'm now
> suspicious of this code / my method of testing it.

OK I think I understand now, this function is only called when using the "Block
element" feature. I was now able to test that it works as I expect, and that it
didn't work before my last change.

[Sebastian Noack, 2015-12-10 10:30:49]

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#...
include.preload.js:377: ext.backgroundPage.sendMessage({type: "get-selectors"},
addElemHideSelectors);
Any reason we use different logic on initilization than when updating styles? I
guess we can/should keep calling updateStylesheets() in both cases.

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#...
include.preload.js:417: if (window.document == document)
Why don't you just put this code below, outside the functio, similar like it was
done before, if it should only be applied for the top level document?

[kzar, 2015-12-10 15:16:20]

Patch Set 3 : Addressed some of Sebastian's feedback

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#...
include.preload.js:377: ext.backgroundPage.sendMessage({type: "get-selectors"},
addElemHideSelectors);
On 2015/12/10 10:30:49, Sebastian Noack wrote:
> Any reason we use different logic on initilization than when updating styles?
I
> guess we can/should keep calling updateStylesheets() in both cases.

No reason that I know of, Done.

https://codereview.adblockplus.org/29331619/diff/29331639/include.preload.js#...
include.preload.js:417: if (window.document == document)
On 2015/12/10 10:30:49, Sebastian Noack wrote:
> Why don't you just put this code below, outside the functio, similar like it
was
> done before, if it should only be applied for the top level document?

Done.

https://codereview.adblockplus.org/29331619/diff/29332518/dependencies
File dependencies (right):

https://codereview.adblockplus.org/29331619/diff/29332518/dependencies#newcode3
dependencies:3: buildtools = buildtools hg:0b17f4d7a723 git:841d6a2
(Due to rebase, can ignore.)

[Sebastian Noack, 2015-12-10 16:35:37]

https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js#...
include.preload.js:288: var propertyFilters = new CSSPropertyFilters(window,
addElemHideSelectors);
Note that the constructor already calls load() and apply(). So I guess we should
change that in adblockplus.

[kzar, 2015-12-10 19:31:06]

Patch Set 4 : Include cssProperties.js for Safari too

https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29331619/diff/29332518/include.preload.js#...
include.preload.js:288: var propertyFilters = new CSSPropertyFilters(window,
addElemHideSelectors);
On 2015/12/10 16:35:36, Sebastian Noack wrote:
> Note that the constructor already calls load() and apply(). So I guess we
should
> change that in adblockplus.

Acknowledged.

[kzar, 2015-12-11 14:00:00]

Patch Set 5 : Include the CSS property filter unit tests

[Sebastian Noack, 2015-12-12 00:50:02]

LGTM, with the discussed changes to the cssProperttFilters.js content script.

[kzar, 2015-12-12 09:45:14]

On 2015/12/12 00:50:02, Sebastian Noack wrote:
> LGTM, with the discussed changes to the cssProperttFilters.js content script.

I've removed those redundant calls here
https://codereview.adblockplus.org/29332572/ . Once that has landed I can update
the adblockplus dependency in this review.

[kzar, 2015-12-12 12:50:07]

Patch Set 6 : Updated adblockplus dependency again

[Sebastian Noack, 2015-12-12 13:09:10]

LGTM