Closed Bug 1180975 Opened 6 years ago Closed 6 years ago

UI for re-enabling tracking protection no longer shown (Private Browsing)

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox42 verified, fennec42+)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- verified
fennec 42+ ---

People

(Reporter: liuche, Assigned: mhaigh)

References

Details

Attachments

(3 files)

After disabling tracking protection from the doorhanger, the UI for re-enabling it is no longer shown.

Bug 1175969 switched tracking protection to private browsing only, but we didn't include handling of the new pref in browser.js, so the Gecko code that handles those changes don't trigger because they're still checking the old preference (privacy.trackingprotection.enabled) for state instead of the new preference (privacy.trackingprotection.pbmode.enabled).

http://mxr.mozilla.org/mozilla-central/search?string=trackingprotection.enabled&find=mobile/android&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Apparently, there is also a testTrackingProtection robocop test, oops! That should be updated to reference the PB pref.
Good catch! Sorry I missed that in my review :(
tracking-fennec: --- → ?
Well, the original pref *should* still work, because users could flip that from about:config, so we may want to change the logic to support either pref case.

In the case where it's only enabled in private browsing, we'd also need to check to make sure we're in a private tab.

As for the test, it could be good to make sure it's enabled by default in a private tab, but I think the test will still work fine if we continue to support that global pref.
tracking-fennec: ? → 42+
Bug 1180975 - UI for re-enabling tracking protection no longer shown (Private Browsing); r?liuche
Attachment #8632007 - Flags: review?(liuche)
https://reviewboard.mozilla.org/r/12985/#review11625

::: mobile/android/chrome/content/browser.js:600
(Diff revision 1)
> -            Services.prefs.getBoolPref("privacy.trackingprotection.enabled")));
> +            Services.prefs.getBoolPref("privacy.trackingprotection.pbmode.enabled")));

Drive-by: I don't think we should update this pref, since the semantics will change (this new pref is now enabled by default). I would hold off on any telemetry changes until we figure out what to do in the telemetry bugs.

Alternately, we could just remove this probe altogether for the time being

::: mobile/android/tests/browser/robocop/testTrackingProtection.js:50
(Diff revision 1)
> -var PREF = "privacy.trackingprotection.enabled";
> +var PREF = "privacy.trackingprotection.pbmode.enabled";

We should be able to get rid of this variable, if we don't need to toggle the pref anymore.

We should also file a follow-up to add a testcase to make sure the TP UI doesn't show up in a normal tab.
Comment on attachment 8632007 [details]
MozReview Request: Bug 1180975 - UI for re-enabling tracking protection no longer shown (Private Browsing); r?liuche

https://reviewboard.mozilla.org/r/12985/#review11773

Like Margaret observed, normal tracking protection is now a little broken. (If you enable the privacy.trackingprotection.enabled pref, the TP icon is displayed on every site, including about:home, for normal and private tabs.) Do we care about that for this bug? If you don't think it's in the scope of this bug, definitely file another one for fixing our handling of privacy.trackingprotection.enabled and have it depend on this bug.

Also +1 to margaret's comments.

::: mobile/android/chrome/content/browser.js:600
(Diff revision 1)
> -            Services.prefs.getBoolPref("privacy.trackingprotection.enabled")));
> +            Services.prefs.getBoolPref("privacy.trackingprotection.pbmode.enabled")));

Re: margaret's comment - with these changes, I think tracking protection in normal browsing no longer works properly, so we can probably just remove this pref.
Attachment #8632007 - Flags: review?(liuche) → review+
Depends on: 1183643
Blocks: 1183646
What's the status here? I see some tracking protection related failures in that try run. Do you need help debugging?
Flags: needinfo?(mhaigh)
margaret: Yes!  Juggling a lot of work atm and trying to make the most of the time with antlam - would really appreciate a hand on this one.  Perhaps we can discuss on Monday?
Flags: needinfo?(mhaigh) → needinfo?(margaret.leibovic)
(In reply to Martyn Haigh (:mhaigh) from comment #10)
> margaret: Yes!  Juggling a lot of work atm and trying to make the most of
> the time with antlam - would really appreciate a hand on this one.  Perhaps
> we can discuss on Monday?

Sure thing. I bet antlam does not want to be debugging robocop tests :)

I may have some time this afternoon to dig into this, so I'll update you if I make any progress.
Flags: needinfo?(margaret.leibovic)
Status: NEW → ASSIGNED
(In reply to :Margaret Leibovic from comment #11)
> (In reply to Martyn Haigh (:mhaigh) from comment #10)
> > margaret: Yes!  Juggling a lot of work atm and trying to make the most of
> > the time with antlam - would really appreciate a hand on this one.  Perhaps
> > we can discuss on Monday?
> 
> Sure thing. I bet antlam does not want to be debugging robocop tests :)
> 
> I may have some time this afternoon to dig into this, so I'll update you if
> I make any progress.

Turns out these tests are failing because we removed the line of code to disable tracking protection, but we still had testcases after that to make sure the tracking protection UI isn't shown when the pref is disabled:
https://hg.mozilla.org/try/rev/00d255c02008#l2.70

I'll post a new patch and try run.
This passes for me locally, but I also pushed a try run.

I took this opportunity to clean up the test file a bit to make it more explicit that this task is about testing the private browsing UI. I'll follow this up with a patch in another bug for more test cases.
Attachment #8635570 - Flags: review?(liuche)
I also decided to keep the telemetry probe for whether or not "privacy.trackingprotection.enabled" is enabled, since we do want to continue supporting that pref. We can use bug to to make additional probes for this new private browsing pref.
Duplicate of this bug: 1183646
No longer blocks: 1183646
Comment on attachment 8635570 [details] [diff] [review]
Updated patch w/ test fixes

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

Also looks green on try :)

::: mobile/android/chrome/content/browser.js
@@ +7111,5 @@
>  
>      // Only show an indicator for loaded tracking content if the pref to block it is enabled
> +    let tpEnabled = Services.prefs.getBoolPref("privacy.trackingprotection.enabled") ||
> +                    (Services.prefs.getBoolPref("privacy.trackingprotection.pbmode.enabled") &&
> +                     PrivateBrowsingUtils.isBrowserPrivate(aBrowser));

Nice catch!
Attachment #8635570 - Flags: review?(liuche) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/d8ab8c7fe5ab55a852a6b7631166e7417ddc72d8
changeset:  d8ab8c7fe5ab55a852a6b7631166e7417ddc72d8
user:       Martyn Haigh <mhaigh@mozilla.org>
date:       Tue Jul 14 12:04:50 2015 +0100
description:
Bug 1180975 - UI for re-enabling tracking protection no longer shown (Private Browsing); r=liuche
https://hg.mozilla.org/mozilla-central/rev/d8ab8c7fe5ab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
1. Go to https://mozweekend.de
2. Tap the shield icon from the URL Bar 
3. Choose "Disable protection" from the doorhanger 
4. Now tap the shield with a red strike-through
5. Choose "Enable protection" from the doorhanger 

=>  tracking protection is re-enabled
Verified fixed using:
Device: Nexus 4 (Android 5.1)
Build: Firefox for Android 42.0a1 (2015-07-22)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.