305010 - Make ad blocking pref apply without a restart

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Simon Fraser [no longer active]]

We should really fix the ad blocking pref to apply without the user having to
restart camino. It can also be fixed to not clobber userContent.css.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Patch

This patch allows us to load/unload the ad_blocking.css file on launch, and
when the user toggles the checkbox.

Note that the user will have to reload pages to see the effects. We could do
this dynamically (maybe by touching one of the prefs that pres contexts listen
to).

Comment 2

[Torben]

AFAICT, this will always use the ad_blocking.css within the Camino bundle.
Wouldn't it be better to copy this (if it doesn't already exist) to the profile
and read from there? That way it would be easier for the user to make changes
and these would persist through updates of Camino?

The drawback is that updates made to the Camino adblock-file wouldn't be
catched, but if a user wants these he/she could disable adblocking, remove the
file, and reenable adblocking.

Maybe the best solution would be to first check whether ad_blocking.css exist in
the profile folder, and if not then use the one in the Camino bundle?

Comment 3

[Samuel Sidler (old account; do not CC)]

(In reply to comment #2)
> AFAICT, this will always use the ad_blocking.css within the Camino bundle.
> Wouldn't it be better to copy this (if it doesn't already exist) to the profile
> and read from there? That way it would be easier for the user to make changes
> and these would persist through updates of Camino?

Users can still use userContent.css for their own purposes. I say it's better to
keep it simple and easily updated by Camino.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Which takes precedence, userContent.css or ad_blocking.css?

I would argue that if a user wants to add a rule to block more ads or override a
rule that's blocking something he/she doesn't want blocked, the user should edit
userContent.css.  That assumes it would take priority over ad_blocking.css, but
I don't know if that is the case.

(Even if the above proposal were to work, there'd still not be a quick-and-easy
way to turn off "ad-blocking" completely because a user-provided/edited
userContent.css could still be in a user's profile--bearing in mind, of course,
I can't read the patch to understand what it actually does.)

We've already had at least one bug filed that was caused by a rule Simon had
already removed; right now ad_blocking.css is copied to userContent.css and
persists as it was until the user deletes the file manually or disables
ad-blocking (in which case userContent.css is deleted by Camino), even if the
Camino devs push an update to the blocking rules.  People who turned on
ad-blocking when it first appeared in Camino are now two revisions old.

For anyone wanting to get up to speed on this, see discussion in bug 302547 and
the various ad-blocking problems/dupes in bug 296783.

Comment 5

[Simon Fraser [no longer active]]

(In reply to comment #4)
> Which takes precedence, userContent.css or ad_blocking.css?
> 
> I would argue that if a user wants to add a rule to block more ads or override a
> rule that's blocking something he/she doesn't want blocked, the user should edit
> userContent.css.  That assumes it would take priority over ad_blocking.css, but
> I don't know if that is the case.

That was my thought too, but it might be that whoever loads last wins.

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 192987 [details] [diff] [review]
Patch

remove the naked NSLog and r=pink.

The only remaining Q is what to do about the user style sheet that was there
from 09a1/2.

Comment 7

[Simon Fraser [no longer active]]

If the "camino.enable_ad_blocking" pref does not exist (i.e. first run with this
patch), then I'm going to look for userContent.css. If it exists, then I'm going
to move it to userContent_unused.css, then set "camino.enable_ad_blocking" to
TRUE (and load ad_blocking.css), otherwise I'll set "camino.enable_ad_blocking"
to FALSE.

If "camino.enable_ad_blocking" does exist, then I'll just follow it to load
ad_blocking.css, and if there's a userContent.css, we'll just load that too.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

> If "camino.enable_ad_blocking" does exist, then I'll just follow it to load
> ad_blocking.css, and if there's a userContent.css, we'll just load that too.

At this point, when one thens changes "camino.enable.ad_blocking" to be false,
will it "unload" only ad_blocking.css or both ad_blocking.css and userContent.css?

Comment 9

[Simon Fraser [no longer active]]

(In reply to comment #8)
> > If "camino.enable_ad_blocking" does exist, then I'll just follow it to load
> > ad_blocking.css, and if there's a userContent.css, we'll just load that too.
> 
> At this point, when one thens changes "camino.enable.ad_blocking" to be false,
> will it "unload" only ad_blocking.css or both ad_blocking.css and userContent.css?

If you uncheck the box, it will just unload ad_blocking.css (and not load it in
subsequent sessions). userContent.css will still get loaded if it exists, but
that should only happen if you put it back manually (or I guess run an older
build and click the checkbox again).

Comment 10

[Simon Fraser [no longer active]]

Fixed on trunk and branch.

Comment 11

[Torben]

> > Which takes precedence, userContent.css or ad_blocking.css?
> > 
> > I would argue that if a user wants to add a rule to block more ads or
> > override a rule that's blocking something he/she doesn't want blocked,
> > the user should edit userContent.css.  That assumes it would take priority
> > over ad_blocking.css, but I don't know if that is the case.
> 
> That was my thought too, but it might be that whoever loads last wins.

Just tested, userContent.css does _not_ override the built in adblock list :-(

Comment 12

[Simon Fraser [no longer active]]

(In reply to comment #11)
> Just tested, userContent.css does _not_ override the built in adblock list :-(

Yeah. See bug 305026.