Allow 'enable tracking protection in normal browsing' for release and beta channel.

VERIFIED FIXED in Firefox 57

Status

()

Product:
Firefox for Android
Component:
Settings and Preferences
Status:
VERIFIED FIXED
Reported:
a year ago
Modified:
a year ago

People

(Reporter: cnevinchen, Assigned: cnevinchen)

Tracking

Version:
unspecified
Target:
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [FNC][SPT57.3][INT])

Attachments

(1 attachment, 1 obsolete attachment)

Bug 1399014 - Allow 'enable tracking protection in normal browsing' for release and beta channel.
59 bytes, text/x-review-board-request
maliu
: review+
Details
(Assignee)

Description

•
a year ago 
We want to experiment on tracking protection.
Currently there are two options
1. A/B testing on options:
   For Nightly and in-experiment Beta/Release users, we show three options:
   A. Enable
   B. Enable in Private Browsing
   C. Disabled
   For non-experiment Beta/Release users, we show two options:
   A. Enable in Private Browsing
   B. Disabled
2. A/B testing on default selection:
   All users have three options: 
   A. Enable
   B. Enable in Private Browsing
   C. Disabled
   Except   
   For Nightly and in-experiment Beta/Release users, default is A
   For non-experiment Beta/Release users, default is B
(Assignee)

Comment 1

•
a year ago 
Hi Joe
Please help decide the experiment we want.
Flags: needinfo?(jcheng)
(Assignee)

Comment 2

•
a year ago 
To add one thing, we also need to handle the behavior during experiment transition.

A user should expect below behavior:
TP(tracking protection) 
PB(private browsing)

    Before                     After in experiment
-------------------------------------------------------------
Enable in PB                     Enable in PB         
    Disable                       Disable


Before leaving Experiment      After leaving experiment
-------------------------------------------------------------
    Enable                       Enable in PB 
Enable in PB                     Enable in PB         
    Disable                       Disable
(Assignee)

Comment 3

•
a year ago 
tldr; if we want comment 2, it'll take more time. at least (full) 2 days to code, 3 day to test, and 2 days to fix bugs on  
1) Leanplum 
2) The new switchboard migration logic

Looking at our code, I found that we didn't save experiment state( i.e. we don't know if we are 'entering' or 'leaving' an experiment). So transition in comment 2 without changing our experiment code is hard)


what I can do is :
1) Add two callbacks to Switchboard, one is to keep previous experiment/kinto configuration before we replace the old one[1] and the second is to do the migration logic in postExecution here[2]

2) Since Leanplum(another component) is also doing something after this loadConfig() call, I may need to move its' logic and add stuff there. ( using composite/listeners or inherence )

3) If we use composite/listeners, we need to add some subscribe/unsubscribe  code. If we use inherence, these two post loadConfig() logic will be coupled together. Not a very pretty design.

[1] https://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#901
[2] https://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/mobile/android/base/java/org/mozilla/gecko/switchboard/AsyncConfigLoader.java#44
Flags: needinfo?(max)
(Assignee)

Comment 4

•
a year ago 
(In reply to Nevin Chen [:nechen] from comment #0)
> We want to experiment on tracking protection.
> Currently there are two options

Looking at email thread, we are going for this approach

> 1. A/B testing on options:

pref name :  privacy.trackingprotection.state
>    For Nightly and in-experiment Beta/Release users, we show three options:
>    A. Enable
>    B. Enable in Private Browsing
>    C. Disabled

pref name : privacy.trackingprotection.pbmode.enabled 
>    For non-experiment Beta/Release users, we show two options:
>    A. Enable in Private Browsing
>    B. Disabled


The setting transition in comment 2 need to check both pref before/after above change occurs.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

•
a year ago
Assignee: nobody → cnevinchen

Comment 9

•
a year ago
mozreview-review 
Comment on attachment 8907162 [details]
Bug 1399014 - Remove old tracking protection pref UI

https://reviewboard.mozilla.org/r/178844/#review184792
Attachment #8907162 - Flags: review?(max) → review+

Comment 10

•
a year ago
mozreview-review 
Comment on attachment 8907161 [details]
Bug 1399014 - Allow 'enable tracking protection in normal browsing' for release and beta channel.

https://reviewboard.mozilla.org/r/178842/#review184790

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSharedPrefs.java:80
(Diff revision 2)
>          "sendReport",
>          "includeUrl",
>          "allowContact",
>          "contactEmail"
>      };
> +    private static final String MIGRATE_PREFS_TRACKING_PROTECTION = "privacy.trackingprotection.state";

How about make this reference to GeckoPreferences.PREFS_TRACKING_PROTECTION instead of dup the value here?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSharedPrefs.java:81
(Diff revision 2)
>          "includeUrl",
>          "allowContact",
>          "contactEmail"
>      };
> +    private static final String MIGRATE_PREFS_TRACKING_PROTECTION = "privacy.trackingprotection.state";
> +    private static final String MIGRATE_PREFS_TRACKING_PROTECTION_PB = "privacy.trackingprotection.pbmode.enabled";

Add a comment to indicate the key is deprecated, put it here is only for removal.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSharedPrefs.java:259
(Diff revision 2)
>  
>          Log.d(LOGTAG, "All keys have been migrated");
>      }
>  
> +    private static void migrateTrackingProtection(SharedPreferences appPrefs) {
> +        // Nightly and Local builds already usedq privacy.trackingprotection.state so don't need migration.

typo: usedq

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSharedPrefs.java:261
(Diff revision 2)
>      }
>  
> +    private static void migrateTrackingProtection(SharedPreferences appPrefs) {
> +        // Nightly and Local builds already usedq privacy.trackingprotection.state so don't need migration.
> +        if (AppConstants.RELEASE_OR_BETA) {
> +            final boolean isTrackingProtectionEnable = appPrefs.getBoolean(MIGRATE_PREFS_TRACKING_PROTECTION_PB, false);

Should the default value be false?
Attachment #8907161 - Flags: review?(max) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

•
a year ago
Attachment #8907162 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 13

•
a year ago 
hg error in cmd: hg pull gecko -r 0b79b57ec3038e13cec4e88c20479ef2049af405: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 504: Gateway Timeout

Comment 14

•
a year ago 
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bc5e32dd5f6
Alow 'enable tracking protection in normal browsing' for release and beta channel. r=maliu
Comment hidden (mozreview-request)

Comment 17

•
a year ago 
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d59c91f1ad4
Allow 'enable tracking protection in normal browsing' for release and beta channel. r=maliu

Comment 18

•
a year ago
bugherder 
https://hg.mozilla.org/mozilla-central/rev/0d59c91f1ad4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
 (Assignee)

Updated

•
a year ago
Flags: needinfo?(cnevinchen)
Summary: Experiment with tracking protection → Allow 'enable tracking protection in normal browsing' for release and beta channel.

Updated

•
a year ago
Flags: needinfo?(max)
 (Assignee)

Comment 19

•
a year ago 
Just for the record,
We won't have an experiment about TP. It'll just ride the train.The only change is on Beta and Release

Updated

•
a year ago
Whiteboard: [FNC][SPT57.3][INT]
Flags: needinfo?(jcheng)

Updated

•
a year ago
Status: RESOLVED → VERIFIED

Updated

•
a year ago
QA Contact: ioana.chiorean
You need to log in before you can comment on or make changes to this bug.