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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

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.

Comment 1

[:Margaret Leibovic]

Good catch! Sorry I missed that in my review :(

Comment 2

[:Margaret Leibovic]

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.

Comment 3

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea84d90982cb

Comment 4

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bfddc78152a

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: MozReview Request: Bug 1180975 - UI for re-enabling tracking protection no longer shown (Private Browsing); r?liuche

Bug 1180975 - UI for re-enabling tracking protection no longer shown (Private Browsing); r?liuche

Comment 6

[:Margaret Leibovic]

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 7

[Chenxia Liu [:liuche]]

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.

Comment 8

[Martyn Haigh (:mhaigh)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00d255c02008

Comment 9

[:Margaret Leibovic]

What's the status here? I see some tracking protection related failures in that try run. Do you need help debugging?

Comment 10

[Martyn Haigh (: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?

Comment 11

[: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.

Comment 12

[:Margaret Leibovic]

(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.

Comment 13

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3e537106a72

Comment 14

[:Margaret Leibovic]

Attachment: Updated patch w/ test fixes

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.

Comment 15

[:Margaret Leibovic]

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.

Comment 17

[Chenxia Liu [:liuche]]

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!

Comment 18

[:Margaret Leibovic]

url:        https://hg.mozilla.org/integration/fx-team/rev/d8ab8c7fe5ab55a852a6b7631166e7417ddc72d8
changeset:  d8ab8c7fe5ab55a852a6b7631166e7417ddc72d8
user:       Martyn Haigh <[e-mail]>
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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot from 2015-07-22 14:39:10.png

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)