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

Status: VERIFIED FIXED

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

Description

[Nevin Chen(Not active on Bugzilla)]

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

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Hi Joe
Please help decide the experiment we want.

Comment 2

[Nevin Chen(Not active on Bugzilla)]

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

Comment 3

[Nevin Chen(Not active on Bugzilla)]

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

Comment 4

[Nevin Chen(Not active on Bugzilla)]

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

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1399014 - Allow 'enable tracking protection in normal browsing' for release and beta channel.

Review commit: https://reviewboard.mozilla.org/r/178842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178842/

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1399014 - Remove old tracking protection pref UI

Removed Android pref "privacy.trackingprotection.pbmode.enabled" here only uses in Java UI.
It's different with "privacy.trackingprotection.pbmode.enabled" in Javascript world so we can remove it directly.

Review commit: https://reviewboard.mozilla.org/r/178844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178844/

Comment 7

[Nevin Chen(Not active on Bugzilla)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178842/diff/1-2/

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8907162 [details]
Bug 1399014 - Remove old tracking protection pref UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178844/diff/1-2/

Comment 9

[Max Liu [:maliu]]

Comment on attachment 8907162 [details]
Bug 1399014 - Remove old tracking protection pref UI

https://reviewboard.mozilla.org/r/178844/#review184792

Comment 10

[Max Liu [:maliu]]

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?

Comment 11

[Nevin Chen(Not active on Bugzilla)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178842/diff/2-3/

Comment 12

[Nevin Chen(Not active on Bugzilla)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178842/diff/3-4/

Comment 13

[Mozilla Autoland]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bc5e32dd5f6
Alow 'enable tracking protection in normal browsing' for release and beta channel. r=maliu

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing android lint:

https://hg.mozilla.org/integration/autoland/rev/7807000c75138d299410a996db5f95eef9775e1d

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1bc5e32dd5f69ddaca634f577df765a43b6ec451&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=130991163&repo=autoland

Comment 16

[Nevin Chen(Not active on Bugzilla)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178842/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d59c91f1ad4
Allow 'enable tracking protection in normal browsing' for release and beta channel. r=maliu

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0d59c91f1ad4

Comment 19

[Nevin Chen(Not active on Bugzilla)]

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