Issue 2401 - Integrate CSS property rule handling in Firefox

Issue 2401 - Integrate CSS property rule handling in Firefox

Repository: hg.adblockplus.org/adblockplus

[Wladimir Palant, 2016-03-18 15:25:25]

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
File lib/child/cssProperties.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
lib/child/cssProperties.js:82: // here because we know that messaging happens
synchronously.
This is rather ugly. While we made CSSPropertyFilters take window as parameter,
it will still call ext.backgroundPage.sendMessage and expect that the recipient
will automatically know which window this is about. Maybe we should have another
parameter to CSSPropertyFilters constructor called sendMessage? Chrome will
simply pass in ext.backgroundPage.sendMessage there, Firefox will instead pass
some function which is bound to the window in question.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.j...
lib/contentPolicy.js:152: this.isFrameWhitelisted(frames, contentType ==
"ELEMHIDE");
Unfortunately, moving out the check for exception rules wasn't a very clean
change. What happened here:

* Looping over frames moved to isFrameWhitelisted(). Merely the surrounding `if
(Prefs.enabled)` check stayed here. A few variable declarations moved down,
these weren't actually used in this loop.
* Adding a filter hit is still being done here, so isFrameWhitelisted() returns
all the data necessary for that.
* If there was a hit, it could be a DOCUMENT/ELEMHIDE filter ("real"
whitelisting) or GENERICBLOCK/GENERICHIDE (merely disabled generic rules). It's
up to the caller to check matchType.
* Side-effect: this can produce only one hit now which is the correct behavior
IMHO. Original code was slightly buggy because it counted hits immediately when
it found a matching filter. However, in reality a $genericblock filter is
ignored if the frame is whitelisted further up in the frame hierarchy.
* This didn't add any redundancy beyond the getSitekey() call below, this is the
only operation which is duplicated.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.j...
lib/contentPolicy.js:226: isWhitelisted: function(url)
This method was never called with more than one parameter - nor would it produce
correct results if called that way.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/cssProperties.js
File lib/cssProperties.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/cssProperties.j...
lib/cssProperties.js:41: };
That's another issue - Chrome uses a static hierarchical structure for frames,
for Firefox we compile a plain list for each request. So if we want to get rid
of this hack the options are:

1) Make Chrome use flat frame lists.
2) Make Firefox produce a frame hierarchy instead of flat lists, along with URL
objects even though these aren't used internally.
3) Design some common format that can be used without much hassle on all
platforms.

[Wladimir Palant, 2016-03-22 14:01:04]

https://codereview.adblockplus.org/29338638/diff/29338639/lib/cssProperties.js
File lib/cssProperties.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/cssProperties.j...
lib/cssProperties.js:41: };
On 2016/03/18 15:25:24, Wladimir Palant wrote:
> That's another issue - Chrome uses a static hierarchical structure for frames,
> for Firefox we compile a plain list for each request.

Feel free to ignore this comment - I solved this properly in
https://codereview.adblockplus.org/29338928/diff/29338929/ext/common.js#newco....
The list of frames will be "enhanced" with two getters so that it will work
properly with code expecting Chrome's data structures.

[Thomas Greiner, 2016-03-30 18:02:06]

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
File lib/child/cssProperties.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
lib/child/cssProperties.js:44: function loadScript(script)
Detail: This function seems redundant since it's only used once and contains
only a single line which is not less descriptive as the function name.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
lib/child/cssProperties.js:82: // here because we know that messaging happens
synchronously.
On 2016/03/18 15:25:24, Wladimir Palant wrote:
> This is rather ugly. While we made CSSPropertyFilters take window as
parameter,
> it will still call ext.backgroundPage.sendMessage and expect that the
recipient
> will automatically know which window this is about. Maybe we should have
another
> parameter to CSSPropertyFilters constructor called sendMessage? Chrome will
> simply pass in ext.backgroundPage.sendMessage there, Firefox will instead pass
> some function which is bound to the window in question.

Can't think of a better alternative than passing a "sendMessage" parameter
either so I'd be fine with that.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.j...
lib/contentPolicy.js:152: this.isFrameWhitelisted(frames, contentType ==
"ELEMHIDE");
On 2016/03/18 15:25:24, Wladimir Palant wrote:
> Unfortunately, moving out the check for exception rules wasn't a very clean
> change. What happened here:
> 
> * Looping over frames moved to isFrameWhitelisted(). Merely the surrounding
`if
> (Prefs.enabled)` check stayed here. A few variable declarations moved down,
> these weren't actually used in this loop.
> * Adding a filter hit is still being done here, so isFrameWhitelisted()
returns
> all the data necessary for that.
> * If there was a hit, it could be a DOCUMENT/ELEMHIDE filter ("real"
> whitelisting) or GENERICBLOCK/GENERICHIDE (merely disabled generic rules).
It's
> up to the caller to check matchType.
> * Side-effect: this can produce only one hit now which is the correct behavior
> IMHO. Original code was slightly buggy because it counted hits immediately
when
> it found a matching filter. However, in reality a $genericblock filter is
> ignored if the frame is whitelisted further up in the frame hierarchy.
> * This didn't add any redundancy beyond the getSitekey() call below, this is
the
> only operation which is duplicated.

All in all sounds reasonable.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.j...
lib/contentPolicy.js:159: else
Detail: This `else` is unnecessary due to the preceding `return`.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/whitelisting.js...
lib/whitelisting.js:35: if (matchType == "DOCUMENT" || matchType == "ELEMHIDE")
I noticed that this check doesn't exist in the original `checkWhitelisted()`
function. Depending on the intended purpose of this function (the description
and the implementation in adblockpluschrome are a bit divergent) we need to
either adapt the code here or in adblockpluschrome.

[Sebastian Noack, 2016-03-30 18:26:40]

I wonder whether it wouldn't make more sense to have a separate message
responder here and in advblockpluschrome rather than mimicking APIs from
adblockpluschrome here just for the matter of reusing the same message responder
which arguably doesn't belong into adblockplusui, and is fairly trivial, anyway.

[Wladimir Palant, 2016-03-30 18:42:17]

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
File lib/child/cssProperties.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
lib/child/cssProperties.js:44: function loadScript(script)
On 2016/03/30 18:02:05, Thomas Greiner wrote:
> Detail: This function seems redundant since it's only used once and contains
> only a single line which is not less descriptive as the function name.

The idea was originally that this would be a generic content script loading
module but that didn't really work out. Removed.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/child/cssProper...
lib/child/cssProperties.js:82: // here because we know that messaging happens
synchronously.
On 2016/03/30 18:02:05, Thomas Greiner wrote:
> Can't think of a better alternative than passing a "sendMessage" parameter
> either so I'd be fine with that.

Yes, we'll need to fix this later.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.js
File lib/contentPolicy.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/contentPolicy.j...
lib/contentPolicy.js:159: else
On 2016/03/30 18:02:05, Thomas Greiner wrote:
> Detail: This `else` is unnecessary due to the preceding `return`.

Sure, yet I'd prefer to keep it. Compilers don't need it, for humans however
it's more obvious that setting nogeneric is the action for match types other
than document and elemhide.

https://codereview.adblockplus.org/29338638/diff/29338639/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29338638/diff/29338639/lib/whitelisting.js...
lib/whitelisting.js:35: if (matchType == "DOCUMENT" || matchType == "ELEMHIDE")
On 2016/03/30 18:02:06, Thomas Greiner wrote:
> I noticed that this check doesn't exist in the original `checkWhitelisted()`
> function. Depending on the intended purpose of this function (the description
> and the implementation in adblockpluschrome are a bit divergent) we need to
> either adapt the code here or in adblockpluschrome.

This function doesn't really offer the same functionality as the one in
adblockpluschrome. This is fine however as there are really only two meaningful
values for typeMask parameter. I've added a comment on this.

IMHO the long-term solution would be moving part of content policy logic
(looping through frames in particular) into adblockpluscore so that the message
responder wouldn't rely on platform-specific stuff.

[Thomas Greiner, 2016-03-31 10:52:51]

LGTM

I don't have a strong preference regarding whether Sebastian's suggestion should
be tackled as part of this review or separately.

[Wladimir Palant, 2016-03-31 11:03:36]

On 2016/03/31 10:52:51, Thomas Greiner wrote:
> I don't have a strong preference regarding whether Sebastian's suggestion
should
> be tackled as part of this review or separately.

As I said under
https://codereview.adblockplus.org/29338638/diff/29338639/lib/whitelisting.js...,
I think that the proper solution would be generalizing the platform-dependent
code that message responder depends on and moving it to adblockpluscore.
Definitely not something to tackle in this review however.

[Sebastian Noack, 2016-03-31 11:48:34]

On 2016/03/31 11:03:36, Wladimir Palant wrote:
> As I said under
>
https://codereview.adblockplus.org/29338638/diff/29338639/lib/whitelisting.js...,
> I think that the proper solution would be generalizing the platform-dependent
> code that message responder depends on and moving it to adblockpluscore.
> Definitely not something to tackle in this review however.

Well, that would require even more abstraction among our Firefox and Platform
code base. I'm not sure if that is a good idea. Given how simple the message
responder logic for CSS property filters is, it seems more reasonable to me, to
just have a different implementation of it for Firefox and Chrome. Besides,
simplifying the problem here, that way we can also move that code out of
adblockplusui were it never belonged in the first place. And in the long term,
thanks to WebExtensions, the current Firefox code will be obsolete, anyway.