Closed Bug 1486462 Opened Last year Closed Last year

Add checkboxes to the Content Blocking Preferences UI

Categories

(Firefox :: Preferences, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

Blocks: 1484769
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.
Attachment #9004432 - Flags: review?(jhofmann)
Attachment #9004432 - Flags: review?(francesco.lodolo)
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.
Attachment #9004432 - Flags: review?(francesco.lodolo) → review+
With an additional test for the dependent tracking protection controls...
Attachment #9004608 - Flags: review?(jhofmann)
Attachment #9004432 - Attachment is obsolete: true
Attachment #9004432 - Flags: review?(jhofmann)
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?
Attachment #9004608 - Flags: review?(jhofmann) → review+
(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.
Thanks a lot for the reviews!
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dba07112ba9
Add checkboxes to the Content Blocking Preferences UI; r=johannh,flod
https://hg.mozilla.org/mozilla-central/rev/0dba07112ba9
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.