Closed Bug 1399014 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox for Android :: Settings and Preferences, enhancement)

Product:
Firefox for Android
Component:
Settings and Preferences
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: cnevinchen, Assigned: cnevinchen)

References

Details

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

Attachments

(1 file, 1 obsolete file)

Bug 1399014 - Allow 'enable tracking protection in normal browsing' for release and beta channel.
59 bytes, text/x-review-board-request
maliu
: review+
Details 
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
Hi Joe
Please help decide the experiment we want.
Flags: needinfo?(jcheng)
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
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)
(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.
Assignee: nobody → cnevinchen
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 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+
Attachment #8907162 - Attachment is obsolete: true
hg error in cmd: hg pull gecko -r 0b79b57ec3038e13cec4e88c20479ef2049af405: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: HTTP Error 504: Gateway Timeout
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
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
https://hg.mozilla.org/mozilla-central/rev/0d59c91f1ad4
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Flags: needinfo?(cnevinchen)
Summary: Experiment with tracking protection → Allow 'enable tracking protection in normal browsing' for release and beta channel.
Flags: needinfo?(max)
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
Whiteboard: [FNC][SPT57.3][INT]
Flags: needinfo?(jcheng)
See Also: → 1409192
Status: RESOLVED → VERIFIED
QA Contact: ioana.chiorean
You need to log in before you can comment on or make changes to this bug.