1196017 - about:privatebrowsing should check if Tracking Protection is turned on globally

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Tanvi Vyas[:tanvi]]

1) Go to about:config and set privacy.trackingprotection.enabled to true.  Optionally also set privacy.trackingprotection.ui.enabled to true.
2) Turn off tracking protection in private browsing mode by 
a) setting privacy.trackingprotection.pbmode.enabled to false
b) unchecking the "Use Tracking Protection Mode in Private Windows" in about:preferences under Privacy

3) Open a private window.  Observe that the about:privatebrowsing page says Tracking Protection is OFF.
4) Go to a page that has trackers in private browsing mode - example: https://blog.mozilla.org/futurereleases/2015/08/14/new-experimental-private-browsing-and-add-ons-features-ready-for-pre-beta-testing-in-firefox/
5) Observe the shield - tracking protection is in fact enabled (globally) and is blocking trackers.

Hence the UI in step 3 should indicate that Tracking Protection is ON not OFF.

This is probably an easy fix.  Add a check for the privacy.trackingprotection.enabled pref in the code that decides whether tracking protection is on or off.

Comment 1

[François Marier [:francois]]

Attachment: MozReview Request: Bug 1196017 - Deal with TP turned on globally in about:privatebrowsing. r?paolo

Bug 1196017 - Deal with TP turned on globally in about:privatebrowsing. r?paolo

Comment 2

[François Marier [:francois]]

Paolo, I've changed the check for TP to make it aware of the global pref (which takes precedence over the pbmode one).

My assumption is that if a user wants to turn TP off, we should do it regardless of how they turned it on in the first place.

If they want to turn it on again, then we can flip the one pref that is exposed in the UI (pbmode.enabled). The global one is for advanced users who know about it.

Comment 3

[:Paolo Amadini]

This solution doesn't look bad but, for users who enabled the global Tracking Protection preference manually, it might allow them to disable it unknowingly by clicking the link in the new about:privatebrowsing interface twice.

We should show Tracking Protection as enabled if the global preference is enabled, regardless of the state of the Private Browsing only preference, but in that case it might be better to just hide the link to disable Tracking Protection in Private Browsing - if you went to about:config to enable it globally you can still go to about:config to disable.

Comment 4

[:Paolo Amadini]

Note that you can just set the "hidden" attribute on the link element at load time - no need to the track changes to the global preference in the observer. That would allow the logic for this edge case to be very simple, which is a win.

Comment 5

[François Marier [:francois]]

Comment on attachment 8649622 [details]
MozReview Request: Bug 1196017 - Deal with TP turned on globally in about:privatebrowsing. r?paolo

Bug 1196017 - Deal with TP turned on globally in about:privatebrowsing. r?paolo

Comment 6

[François Marier [:francois]]

(In reply to :Paolo Amadini from comment #3)
> This solution doesn't look bad but, for users who enabled the global
> Tracking Protection preference manually, it might allow them to disable it
> unknowingly by clicking the link in the new about:privatebrowsing interface
> twice.
> 
> We should show Tracking Protection as enabled if the global preference is
> enabled, regardless of the state of the Private Browsing only preference,
> but in that case it might be better to just hide the link to disable
> Tracking Protection in Private Browsing - if you went to about:config to
> enable it globally you can still go to about:config to disable.

You're right that's a much better solution. I've updated the patch.

Comment 7

[:Paolo Amadini]

Comment on attachment 8649622 [details]
MozReview Request: Bug 1196017 - Deal with TP turned on globally in about:privatebrowsing. r?paolo

https://reviewboard.mozilla.org/r/16449/#review15093

Ship It!

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:23
(Diff revision 2)
> +      document.body.setAttribute("globalTpEnabled", "true");
> +    } else {
> +      document.body.removeAttribute("globalTpEnabled");
> +    } 

Oh no, whitespace at the end of the line!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f84e6e88b82

Comment 9

[François Marier [:francois]]

(In reply to :Paolo Amadini from comment #7)
> ::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.js:23
> (Diff revision 2)
> > +      document.body.setAttribute("globalTpEnabled", "true");
> > +    } else {
> > +      document.body.removeAttribute("globalTpEnabled");
> > +    } 
> 
> Oh no, whitespace at the end of the line!

Fixed in my push to inbound :)

Comment 10

[François Marier [:francois]]

Comment on attachment 8649622 [details]
MozReview Request: Bug 1196017 - Deal with TP turned on globally in about:privatebrowsing. r?paolo

Approval Request Comment
[Feature/regressing bug #]: 1177156
[User impact if declined]: users that have tracking protection enabled globally will see the wrong status in about:privatebrowsing
[Describe test coverage new/current, TreeHerder]: manual testing of the 4 pref combinations
[Risks and why]: it could break the indicator and the toggle button state in about:privatebrowsing (no impact to the pref in about:preferences)
[String/UUID change made/needed]: none

Comment 11

[François Marier [:francois]]

By "manual testing of the 4 pref combinations", I mean:

- privacy.trackingprotection.enabled = true; privacy.trackingprotection.pbmode.enabled = true
- privacy.trackingprotection.enabled = true; privacy.trackingprotection.pbmode.enabled = false
- privacy.trackingprotection.enabled = false; privacy.trackingprotection.pbmode.enabled = true
- privacy.trackingprotection.enabled = false; privacy.trackingprotection.pbmode.enabled = false

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f84e6e88b82

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8649622 [details]
MozReview Request: Bug 1196017 - Deal with TP turned on globally in about:privatebrowsing. r?paolo

Seems ok in Nightly and we want private browsing to be fabulous for 42. Approved for uplift to aurora.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/119f61ff79af

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed in Fx42/Fx43, 2015-09-01.