1178985 - Show tracking protection section in Control Center even when it's only enabled in PB mode

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1175327#c12, we need to make sure that the TP section shows up when privacy.trackingprotection.pbmode.enabled is set to true.

We also need to think about how telemetry data is gathered with respect to this change.

Comment 1

[Brian Grinstead [:bgrins]]

Copying in Tim's original comment:
----

We need to also support "privacy.trackingprotection.pbmode.enabled". It would be easy to check whether the parent browser window is private and then just consider that for TP.enabled, however we'd probably change the meaning of a few telemetry measurements here...

For TRACKING_PROTECTION_SHIELD we'd probably want to have .enabled report both prefs as we're interested in knowing how many pages track and how many don't

For TRACKING_PROTECTION_ENABLED we shouldn't use both prefs because that would skew the number of people that enabled it manually.

Comment 2

[Brian Grinstead [:bgrins]]

As discussed, we are not going to do any Telemetry related changes in this bug and instead file a new one for that.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: tp-in-pb.patch

Shows the TP section in the control center when pbmode is enabled and it's a private window.  Also updates both existing tests to cover both cases (normal mode with "privacy.trackingprotection.enabled" and pb mode with "privacy.trackingprotection.pbmode.enabled".  And adds a third test that just confirms that TP is enabled in all permutations of the window type and pref values.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8631070 [details] [diff] [review]
tp-in-pb.patch

Review of attachment 8631070 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-trackingprotection.js
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  let TrackingProtection = {
> +  PREF_ALWAYS_ENABLED: "privacy.trackingprotection.enabled",

Nit: Maybe rather "globally" enabled than "always"? PREF_ENABLED_GLOBALLY?

@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  let TrackingProtection = {
> +  PREF_ALWAYS_ENABLED: "privacy.trackingprotection.enabled",
> +  PREF_PB_ENABLED: "privacy.trackingprotection.pbmode.enabled",

Nit: PREF_ENABLED_IN_PRIVATE_WINDOWS? (Not too happy about the pref name itself but whatever. We don't have a private browsing mode any longer :)

@@ +8,2 @@
>  
>    init() {

Above here, we should add:

alwaysEnabled: false,
pbEnabled: false,

init() { ...

Otherwise these properties seem to show up out of nowhere and one has to find out they're set when updateEnabled() is called the first time.

@@ +35,2 @@
>    updateEnabled() {
> +    this.alwaysEnabled = Services.prefs.getBoolPref(this.PREF_ALWAYS_ENABLED);

Nit: this.enabledGlobally?

@@ +35,3 @@
>    updateEnabled() {
> +    this.alwaysEnabled = Services.prefs.getBoolPref(this.PREF_ALWAYS_ENABLED);
> +    this.pbEnabled = Services.prefs.getBoolPref(this.PREF_PB_ENABLED);

Nit: this.enabledInPrivateWindows?

It's a little long but also more telling.

::: browser/base/content/test/general/browser_trackingUI_3.js
@@ +10,5 @@
> +
> +let PREF = "privacy.trackingprotection.enabled";
> +let PB_PREF = "privacy.trackingprotection.pbmode.enabled";
> +let BENIGN_PAGE = "http://tracking.example.org/browser/browser/base/content/test/general/benignPage.html";
> +let TRACKING_PAGE = "http://tracking.example.org/browser/browser/base/content/test/general/trackingPage.html";

Nit: the four above can be "const".

@@ +11,5 @@
> +let PREF = "privacy.trackingprotection.enabled";
> +let PB_PREF = "privacy.trackingprotection.pbmode.enabled";
> +let BENIGN_PAGE = "http://tracking.example.org/browser/browser/base/content/test/general/benignPage.html";
> +let TRACKING_PAGE = "http://tracking.example.org/browser/browser/base/content/test/general/trackingPage.html";
> +let TrackingProtection = null;

Nit: don't need this global variable here, can just be test-local.

@@ +12,5 @@
> +let PB_PREF = "privacy.trackingprotection.pbmode.enabled";
> +let BENIGN_PAGE = "http://tracking.example.org/browser/browser/base/content/test/general/benignPage.html";
> +let TRACKING_PAGE = "http://tracking.example.org/browser/browser/base/content/test/general/trackingPage.html";
> +let TrackingProtection = null;
> +let browser = null;

Nit: don't need this global variable either.

@@ +20,5 @@
> +  Services.prefs.clearUserPref(PREF);
> +  Services.prefs.clearUserPref(PB_PREF);
> +  while (gBrowser.tabs.length > 1) {
> +    gBrowser.removeCurrentTab();
> +  }

We don't open any new tabs, do we? Think we can remove this.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: tp-in-pb-r=ttaubert.patch

Thanks for the review - I've updated as suggested

Comment 6

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/fe871d82b7a5

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe871d82b7a5

Comment 8

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

Verified Nightly Fx42.0a1, 2015-07-20.

Comment 9

[Catalin Varga [QA][:VarCat]]

Do we want to show Tracking Protection section in Control Center on normal pages when only PB mode is enabled?
I find that the information that is displayed under that section on normal pages in this situation is misleading(it always displays "No tracking elements detected on this page"). Isn't a better behavior to display it only on private pages if only PB mode is enabled?

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Catalin Varga [QA][:VarCat] from comment #9)
> Do we want to show Tracking Protection section in Control Center on normal
> pages when only PB mode is enabled?
> I find that the information that is displayed under that section on normal
> pages in this situation is misleading(it always displays "No tracking
> elements detected on this page"). Isn't a better behavior to display it only
> on private pages if only PB mode is enabled?

Hi Catalin, that's how it was designed to work.  I guess so that it's obvious the feature is working and just didn't find anything as opposed to being disabled / broken?  But I can't speak too much towards the designs, I've CC'ed Aislinn in case you want to discuss further.

Comment 11

[Catalin Varga [QA][:VarCat]]

The problem is that it displays "No tracking elements detected on this page" when there are tracking elements.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Catalin Varga [QA][:VarCat] from comment #11)
> The problem is that it displays "No tracking elements detected on this page"
> when there are tracking elements.

Oh, I see.  Aislinn, should we remove the Tracking Protection section from the control center if it's been disabled?  I think this used to be the behavior

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Catalin Varga [QA][:VarCat] from comment #11)
> The problem is that it displays "No tracking elements detected on this page"
> when there are tracking elements.

This issue has been fixed (and landed on aurora) in Bug 1194258