1486462 - Add checkboxes to the Content Blocking Preferences UI

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[(no longer active)]

UX mock: https://mozilla.invisionapp.com/share/QVIITATG4JT#/screens/301705296

Comment 1

[(no longer active)]

Attachment: Add checkboxes to the Content Blocking Preferences UI

This patch adds tests for the interaction of dependent controls under each one of the categories.  I need to think about what else could use tests here, but otherwise it should be complete and ready to land.  This is a patch on top of bug 1484769.

Comment 2

[Francesco Lodolo [:flod]]

Comment on attachment 9004432 [details] [diff] [review]
Add checkboxes to the Content Blocking Preferences UI

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

No issues string-wise.

Comment 3

[(no longer active)]

Attachment: Add checkboxes to the Content Blocking Preferences UI

With an additional test for the dependent tracking protection controls...

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 9004608 [details] [diff] [review]
Add checkboxes to the Content Blocking Preferences UI

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

::: browser/components/preferences/in-content/privacy.js
@@ +213,5 @@
>            .disabled = disabled;
>        }
> +
> +      // Notify observers that the TP UI has been updated
> +      Services.obs.notifyObservers(window, "privacy-pane-tp-ui-updated");

Like we chatted about, I understand that this is probably the least painful solution right now, but please add a comment that this is for tests :)

@@ +636,5 @@
>     */
>    trackingProtectionReadPrefs() {
>      let enabledPref = Preferences.get("privacy.trackingprotection.enabled");
>      let pbmPref = Preferences.get("privacy.trackingprotection.pbmode.enabled");
> +    let tpControl, tpCheckbox;

nit: please declare each of these on its own line

@@ +715,5 @@
>     */
>    trackingProtectionWritePrefs() {
>      let enabledPref = Preferences.get("privacy.trackingprotection.enabled");
>      let pbmPref = Preferences.get("privacy.trackingprotection.pbmode.enabled");
> +    let tpControl, tpCheckbox;

nit: please on two lines

@@ +1215,5 @@
> +    let bcControl = document.getElementById("blockCookiesCB");
> +
> +    switch (pref.value) {
> +      case Ci.nsICookieService.BEHAVIOR_ACCEPT:
> +        this.enableThirdPartyCookiesUI();

This is just duplicating efforts with readBlockCookiesCB, can we remove it from there or merge these two functions somehow?

::: browser/components/preferences/in-content/tests/browser_contentblocking.js
@@ +146,5 @@
>      ok(!Services.prefs.prefHasUserValue(pref), `reset the pref ${pref}`);
>    }
>  
>    ok(Services.prefs.prefHasUserValue(TP_PREF), "did not reset the TP pref");
> +  Services.prefs.clearUserPref(TP_PREF);

This should already be handled by extension.unload, no?

@@ +280,5 @@
> +    "#blockCookiesCB, #blockCookiesCB > radio",
> +  ];
> +  let alwaysDisabledControls = [
> +      "#trackingProtectionMenu",
> +      "[control=trackingProtectionMenu]",

nit: indendation

@@ +361,5 @@
>      Ci.nsICookieService.BEHAVIOR_REJECT_FOREIGN, // Block All Third-Party Cookies
>      Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER, // Block Cookies from third-party trackers
>    ];
>    for (let value of prefValuesToTest) {
> +    SpecialPowers.pushPrefEnv({set: [

why did you stop awaiting SpecialPowers.pushPrefEnv in most of this test?

Comment 5

[(no longer active)]

(In reply to Johann Hofmann [:johannh] from comment #4)
> @@ +1215,5 @@
> > +    let bcControl = document.getElementById("blockCookiesCB");
> > +
> > +    switch (pref.value) {
> > +      case Ci.nsICookieService.BEHAVIOR_ACCEPT:
> > +        this.enableThirdPartyCookiesUI();
> 
> This is just duplicating efforts with readBlockCookiesCB, can we remove it
> from there or merge these two functions somehow?

Indeed, these calls can now be removed.  I still prefer to keep the functions separate since they do different work especially after the removal of these calls.

> :::
> browser/components/preferences/in-content/tests/browser_contentblocking.js
> @@ +146,5 @@
> >      ok(!Services.prefs.prefHasUserValue(pref), `reset the pref ${pref}`);
> >    }
> >  
> >    ok(Services.prefs.prefHasUserValue(TP_PREF), "did not reset the TP pref");
> > +  Services.prefs.clearUserPref(TP_PREF);
> 
> This should already be handled by extension.unload, no?

Oh, you're right.

> @@ +361,5 @@
> >      Ci.nsICookieService.BEHAVIOR_REJECT_FOREIGN, // Block All Third-Party Cookies
> >      Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER, // Block Cookies from third-party trackers
> >    ];
> >    for (let value of prefValuesToTest) {
> > +    SpecialPowers.pushPrefEnv({set: [
> 
> why did you stop awaiting SpecialPowers.pushPrefEnv in most of this test?

Oops, those were some debugging changes as I was powering through the test failures while working on this, I meant to have reverted them before posting the patch...  Will fix before landing.

Comment 6

[(no longer active)]

Thanks a lot for the reviews!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dba07112ba9
Add checkboxes to the Content Blocking Preferences UI; r=johannh,flod

Comment 8

[Cosmin Sabou [:CosminS]]

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

Comment 9

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the checkboxes were implemented, I verified using Fx 63.0b11, on Windows 10 x64, mac OS 10.13.6 and Ubuntu 14.04 LTS.