1078740 - Add tracking controls to security notification bar

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[[e-mail]]

As yet we only have a notification bar for security notifications, but we should add the tracking controls to it.

Comment 1

[[e-mail]]

Attachment: WIP

Comment 2

[rsx11m]

Comment on attachment 8500673 [details] [diff] [review]
WIP

This looks good and seems to work fine. I've checked the patch with the unofficial test page provided at http://people.mozilla.org/~mchew/test_tp.html and just found out afterwards that https://www.mozilla.org/ is triggering the tracking warning already - kind of ironic, isn't it?  :-)

So, "Keep Blocking" essentially just dismisses the dialog while "Unblock" adds the domain as "Allow" to the permissions. This works and the entry shows up nicely in the Data Manager. After an "Unblock" the 3-bottom info disappears and directly after that the 1-bottom warning of tracked content loaded appears. Along with the slide-out/slide-in animation that's a bit annoying (but may be the same for the SSL warnings too from which this was inherited, don't know).

There are no doorhanger versions for the SSL warnings either, I assume?

Also, once "Unblock" has been selected, there is no obvious way to block again in the warning bar (but then, that's after the fact anyway). Thus, a possible thought might be to add a "Block Again" button (which probably would/should be inherited by the SSL warnings too for consistency, if that's the way to go).

Comment 3

[[e-mail]]

(In reply to rsx11m from comment #2)
> that https://www.mozilla.org/ is triggering the tracking warning already -
Bugzilla itself was triggering the tracking warning at one point ;-)

> loaded appears. Along with the slide-out/slide-in animation that's a bit
> annoying (but may be the same for the SSL warnings too from which this was
> inherited, don't know).
Yes, the SSL warnings slide out and in too.

> There are no doorhanger versions for the SSL warnings either, I assume?
No, I only write the notification bars (they work in sidebar and RSS too).

> Also, once "Unblock" has been selected, there is no obvious way to block
> again in the warning bar (but then, that's after the fact anyway). Thus, a
> possible thought might be to add a "Block Again" button (which probably
> would/should be inherited by the SSL warnings too for consistency, if that's
> the way to go).
Hmm, I would have to find out what whether the SSL warning can do this (it's actually slightly different from the tracking warning in that it reloads the page with a special "please load mixed active content" flag). (And isn't it too late anyway?)

Comment 4

[[e-mail]]

(In reply to from comment #3)
> (In reply to rsx11m from comment #2)
> > Also, once "Unblock" has been selected, there is no obvious way to block
> > again in the warning bar (but then, that's after the fact anyway). Thus, a
> > possible thought might be to add a "Block Again" button (which probably
> > would/should be inherited by the SSL warnings too for consistency, if that's
> > the way to go).
> Hmm, I would have to find out what whether the SSL warning can do this
OK, so if the SSL mixed active content gets unblocked then activeBrowser.docShell.mixedContentChannel will become non-null and if we want to block it again we need to clear it and reload the page.

Comment 5

[rsx11m]

(In reply to [e-mail] from comment #3)
> (And isn't it too late anyway?)

That's certainly the main point and made me thinking twice about this suggestion as soon as I came up with it. Thus, if it appears to be additional work that's not worth much in actually providing any protection for the user, let's stick with simply opening the global preferences only. We can add some additional information to the Help content there and point the user to the Data Manager.

Comment 6

[[e-mail]]

Attachment: Proposed patch

* Added notification for tracking content
* Added pref to control notifications
* Added tracking content allow action button
* Added allow permission entries in Data Manager
* Not added: undo unblock notification button
Probably want to leave that for a followup, sorry.

Comment 7

[rsx11m]

Comment on attachment 8523213 [details] [diff] [review]
Proposed patch

(In reply to [e-mail] from comment #6)
> * Not added: undo unblock notification button
> Probably want to leave that for a followup, sorry.

That's fine, it'll likely involve other notifications as well (such as the mixed-content ones), thus would be better off to be handled everything in a separate bug and keeping things consistent.

There are no Help updates, which may be a separate bug as well. No problem, I can write something up once the main patch has been approved.

>-              aState = lastState;
>+              aState = this.lastState;

Hmm, I'm wondering which "lastState" is taken /now/ or is the "this." implicit in this case?

Comment 8

[[e-mail]]

(In reply to rsx11m from comment #7)
> There are no Help updates, which may be a separate bug as well. No problem,
> I can write something up once the main patch has been approved.

Thanks, that's muchly appreciated.

> >-              aState = lastState;
> >+              aState = this.lastState;
> Hmm, I'm wondering which "lastState" is taken /now/ or is the "this."
> implicit in this case?

It used to be window.lastState which was a coding error on my part. (And I've just noticed that I've overlooked on occurrence. Oops.)

Comment 9

[rsx11m]

Comment on attachment 8523213 [details] [diff] [review]
Proposed patch

(In reply to [e-mail] from comment #8)
> It used to be window.lastState which was a coding error on my part. (And
> I've just noticed that I've overlooked on occurrence. Oops.)

That's probably what you are referring to:

>+              if (!this.usePrivateBrowsing) {
>+                lastState = aState & ~nsIWebProgressListener.STATE_BLOCKED_TRACKING_CONTENT;

works fine for me with that fixed to "this.lastState = ..."

Comment 10

[[e-mail]]

(In reply to rsx11m from comment #9)
> That's probably what you are referring to:
> 
> >+              if (!this.usePrivateBrowsing) {
> >+                lastState = aState & ~nsIWebProgressListener.STATE_BLOCKED_TRACKING_CONTENT;

Indeed, the very same.

Comment 11

[rsx11m]

Sorry for the noise, but I figured that I spin off the bugs while I still remember to do so. ;-)

Comment 12

[Ian Neal]

Comment on attachment 8523213 [details] [diff] [review]
Proposed patch

r=me with the typo fixed (this.lastState)

Comment 13

[[e-mail]]

Pushed comm-central changeset adae3689794e.