Closed Bug 1178985 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: General, defect, P1)

defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.1 - Jul 13
Tracking Status
firefox42 --- verified

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(1 file, 1 obsolete file)

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.
Flags: firefox-backlog?
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.
Points: --- → 3
Rank: 1
Flags: qe-verify+
Flags: firefox-backlog?
Flags: firefox-backlog+
Priority: -- → P1
As discussed, we are not going to do any Telemetry related changes in this bug and instead file a new one for that.
Blocks: 1179441
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 42.1 - Jul 13
QA Contact: mwobensmith
Attached patch tp-in-pb.patch (obsolete) — Splinter Review
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.
Attachment #8631070 - Flags: review?(ttaubert)
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.
Attachment #8631070 - Flags: review?(ttaubert) → review+
Thanks for the review - I've updated as suggested
Attachment #8631070 - Attachment is obsolete: true
Attachment #8631841 - Flags: review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/fe871d82b7a5
Whiteboard: [fxprivacy] → [fixed-in-fx-team][fxprivacy]
https://hg.mozilla.org/mozilla-central/rev/fe871d82b7a5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][fxprivacy] → [fxprivacy]
Target Milestone: --- → Firefox 42
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Verified Nightly Fx42.0a1, 2015-07-20.
Status: RESOLVED → VERIFIED
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?
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
The problem is that it displays "No tracking elements detected on this page" when there are tracking elements.
(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
Flags: needinfo?(agrigas)
(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
Flags: needinfo?(agrigas)
You need to log in before you can comment on or make changes to this bug.