Issue 521- Inject our stylesheet on per-site basis rather than globally

Issue 521- Inject our stylesheet on per-site basis rather than globally

[Wladimir Palant, 2014-05-20 19:36:12]

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
File lib/elemHide.js (right):

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
lib/elemHide.js:86: });
This code is no longer necessary.

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
lib/elemHide.js:140: }
Please use Policy.isBlockableScheme() here (from contentPolicy.js).

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
lib/elemHide.js:144: return;
Wow, performance will go down the drain here. We can simply assume that there
are always *some* filters applying - as long as element hiding isn't generally
disabled.

Currently this check is being performed in Policy.processNode() (called from
elemHideHitRegistration.js). We need to move this logic into a separate
function, it will now be called both from Policy.processNode() and from here.
For the window and each of its parents, check whether the scheme is blockable at
all, whether any exception rule for type DOCUMENT or type ELEMHIDE exists for
that URL, plus sitekey-based exceptions.

The logic in elemHideHitRegistration.js should be simplified then. Rather than
going through the full Policy.processNode() processing, it should merely check
whether any element hiding exceptions apply and register a hit.

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
lib/elemHide.js:247: apply: function()
saveStylesheet()?

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
lib/elemHide.js:283: FilterNotifier.triggerListeners("elemhideupdate");
Apparently, this is dead code. This notification has been introduced in
https://hg.adblockplus.org/adblockplus/rev/1c10d9bd251d and is no longer used
anywhere - the e10s support is no more. Feel free to remove it while at it.

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
lib/elemHide.js:353: },
This function should be removed.

http://codereview.adblockplus.org/6201308310667264/diff/5629499534213120/lib/...
lib/elemHide.js:359: get styleURL() styleURL.spec,
This getter was also introduced in
https://hg.adblockplus.org/adblockplus/rev/1c10d9bd251d and is no longer being
used either, feel free to remove it.

[tschuster, 2014-06-04 17:55:08]

This is not totally ready yet, but I thought it would be a good idea to upload a
wip to make sure I am not getting totally off the track

[tschuster, 2014-06-04 18:07:40]

http://codereview.adblockplus.org/6201308310667264/diff/5649050225344512/lib/...
File lib/contentPolicy.js (right):

http://codereview.adblockplus.org/6201308310667264/diff/5649050225344512/lib/...
lib/contentPolicy.js:167: if (!(match instanceof WhitelistFilter))
I think we should make this match === null, because isWhitelisted can only
return an instance of WhitelistFilter  or null

http://codereview.adblockplus.org/6201308310667264/diff/5649050225344512/lib/...
lib/contentPolicy.js:184: match = keyMatch; // XXX what is this?
Okay I am not sure about this case, is that |keyMatch| a whitelistfilter?
Otherwise we probably have to return it.

[tschuster, 2014-06-05 19:00:34]

Hey Wladimir, I would appreciate if you could answer some question, sadly we
seem to be totally out of sync on IRC at the moment.

http://codereview.adblockplus.org/6201308310667264/diff/5649050225344512/lib/...
File lib/contentPolicy.js (right):

http://codereview.adblockplus.org/6201308310667264/diff/5649050225344512/lib/...
lib/contentPolicy.js:185: }
I have an other concern: This is never going to work when we are in
"content-document-global-created" because the DOM is not ready yet. Is this code
relevant to elemhide filters?

[tschuster, 2014-06-28 10:38:35]

New patch set that is a lot simpler and retains compatibility with old Firefox
versions where the Firefox API that I am proposing isn't implemented

[tschuster, 2014-07-18 15:47:43]

Wladimir what do you think? I think I have seen the case happen once where we
observe 'content-document-global-created', but the styleSheet isn't loaded yet.
I am however unable to reproduce it right now.

[tschuster, 2014-08-06 16:30:18]

Wladimir please review.

[tschuster, 2014-10-19 21:51:14]

On 2014/08/06 16:30:18, tschuster wrote:
> Wladimir please review.

This could reduce memory quite a bit, I would appreciate if you could take a
look.

[tschuster, 2015-07-21 10:37:16]

On 2014/10/19 21:51:14, tschuster wrote:
> On 2014/08/06 16:30:18, tschuster wrote:
> > Wladimir please review.
> 
> This could reduce memory quite a bit, I would appreciate if you could take a
> look.

I don't think so. -> Closing