750794 - Add pref UI to enable do-not-track (DNT) in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: Preferences, enhancement)

Description

[Sid Stamm [:geekboy or :sstamm]]

We should add a pref so users can enable the do-not-track header (DNT) on all outgoing HTTP requests caused by thunderbird.

I'll have a first whack at a patch to create a checkbox in options->Security->Web Content.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: proposed patch

proposed patch.  Adds the check box right below the cookie UI in the security/Web Content pref panel.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: screenshot

screenshot for patch in attachment 620446 [details] [diff] [review].

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 620446 [details] [diff] [review]
proposed patch

Maybe needs some polish.  Blake, can you take a peek?  Screenshot in attachment 620448 [details].

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 620446 [details] [diff] [review]
proposed patch

I'm gonna redirect this to mconley, cause his review queue is shorter than mine.  ;)

I'm also requesting review from him, since it's a pretty short patch, and he'll be looking at the code anyways.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 620446 [details] [diff] [review]
proposed patch

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

Appearance-wise, looks good across each platform.  Code-wise, looks good, and operates as advertised.

Sid - can I convince you to write a quick Mozmill test for this to ensure that the pref is being set properly?

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Mike Conley (:mconley) from comment #5)
> Sid - can I convince you to write a quick Mozmill test for this to ensure
> that the pref is being set properly?

Yes you could!  I've not written one before, though, so it will take me a while.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to Sid Stamm [:geekboy] from comment #6)
> (In reply to Mike Conley (:mconley) from comment #5)
> > Sid - can I convince you to write a quick Mozmill test for this to ensure
> > that the pref is being set properly?
> 
> Yes you could!  I've not written one before, though, so it will take me a
> while.

Cool - I can help you through it.

Our tests are under mail/test/mozmill - and you'll probably want to add a test to the pref-window subdirectory.

You can copy test-attachments-pane.js for the boilerplate (everything up to the test_persist_tabs function), create a new test_foo function, and use the open_pref_window helper function spawn the preference window.

Again, the test-attachments-pane.js has some good examples in it.

Feel free to ping me on IRC or send me mail if you have any questions.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: first whack at tests

First whack at tests.  I am getting an unexpected failure, but not sure why.  (This upload is basically me saving work.)

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 620473 [details] [diff] [review]
first whack at tests

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

Sid:

This is good stuff - thanks for writing the test.

A couple of suggestions below.  Thanks!

-Mike

::: mail/test/mozmill/pref-window/test-donottrack-prefs.js
@@ +5,5 @@
> +/**
> + * Tests the manager for attachment storage services
> + */
> +
> +let Cu = Components.utils;

We don't need Cc / Ci / Cu in this test.

@@ +9,5 @@
> +let Cu = Components.utils;
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;
> +
> +let DNT_PREF_NAME = 'privacy.donottrackheader.enabled';

Let's use const for these instead of let.

@@ +16,5 @@
> +
> +let RELATIVE_ROOT = '../shared-modules';
> +let MODULE_REQUIRES = ['folder-display-helpers',
> +                       'pref-window-helpers',
> +                       'window-helpers'];

You don't use window-helpers in your test, so you can remove it.  Same with lines 29 - 30.

@@ +19,5 @@
> +                       'pref-window-helpers',
> +                       'window-helpers'];
> +
> +function setupModule(module) {
> +  let fdh = collector.getModule('folder-display-helpers');

These can be shortened to:

collector.getModule('folder-display-helpers').installInto(module);

Same with lines 26-27.

@@ +27,5 @@
> +  pwh.installInto(module);
> +
> +  let wh = collector.getModule('window-helpers');
> +  wh.installInto(module);
> +};

No semi-colon after the function definition.

@@ +34,5 @@
> + * Test that selecting the checkbox for the do not track feature actually sets
> + * the preference.
> + */
> +function test_donottrack_checkbox() {
> +  open_pref_window("paneSecurity", function(w) {

According to MDN, on some platforms, the preferences are only written once the dialog is accepted (see https://developer.mozilla.org/en/XUL/prefwindow).

So you'll need to check the pref, run open_pref_window once, accept the dialog, check the pref, then re-run open_pref_window again to check that the pref has been reset.

@@ +43,5 @@
> +
> +    let checkbox = w.e("privacyDoNotTrackPref");
> +
> +    // tick the DNT box.
> +    checkbox.checked = true;

Here's where things get a little annoying - it looks like the prefWindow binding doesn't notice preference changes on checkboxes that are set programmatically like this.

This is because the binding is monitoring checkboxes for "onchange" events, which aren't fired unless the user has set focus on something, the value changes, and then focus is blurred.

You can get around this by clicking on the checkbox instead, with:

w.click(w.eid("privacyDoNotTrackPref"));

@@ +58,5 @@
> +    // make sure all is reset.
> +    assert_false(checkbox.checked);
> +    assert_false(Services.prefs.getBoolPref(DNT_PREF_NAME));
> +
> +    close_window(w);

Because you want to save the preferences, you'll need to accept the dialog instead of closing it with the window helper.

You can do that with:

w.e("MailPreferences").acceptDialog();

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: tests

(In reply to Mike Conley (:mconley) from comment #9)
> ::: mail/test/mozmill/pref-window/test-donottrack-prefs.js
> @@ +5,5 @@
> We don't need Cc / Ci / Cu in this test.
> 
> @@ +9,5 @@
> Let's use const for these instead of let.
> 
> @@ +19,5 @@
> These can be shortened to:
> collector.getModule('folder-display-helpers').installInto(module);
> Same with lines 26-27.
> 
> @@ +27,5 @@
> No semi-colon after the function definition.

FYI, all of these notes above are artifacts of stuff I copied from test-attachments-pane.js ... we might want to fix these in that file too.

> @@ +34,5 @@
> According to MDN, on some platforms, the preferences are only written once
> the dialog is accepted (see https://developer.mozilla.org/en/XUL/prefwindow).

Fixed.

> @@ +43,5 @@
> You can get around this by clicking on the checkbox instead, with:
> w.click(w.eid("privacyDoNotTrackPref"));

Done and done.

> @@ +58,5 @@
> Because you want to save the preferences, you'll need to accept the dialog
> instead of closing it with the window helper.
> You can do that with:
> w.e("MailPreferences").acceptDialog();

Done and done.

Thanks for the comments.  This version of the tests works.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 620809 [details] [diff] [review]
tests

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

This looks good to me.

Just one nit.  With that fixed, r=me.

Great job, and thanks for your work!

::: mail/test/mozmill/pref-window/test-donottrack-prefs.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Tests the manager for attachment storage services

This comment needs to be updated to say that you're testing the Do Not Track toggle.

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Attachment: patch

Fixed nit, folded tests into main patch, ready to land.  Carrying over r=mconley and ui-r=mconley.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/8c4f6f448f5a

Comment 14

[Jb Piacentino]

Have we had a discussion on whether should DNT be tunred on of off by default ? Is there a reason why we would not want it on by default ?

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jb Piacentino from comment #14)
> Have we had a discussion on whether should DNT be tunred on of off by
> default ? Is there a reason why we would not want it on by default ?

I believe the reasoning is the same as with Firefox - an opt-in to DNT sends a way more powerful message than just switching it on for everyone.  The opt-in means the user has actively *decided* to not be tracked, which (I believe) is more likely to be honoured by content providers.

I think that's the general argument.  Do I have that right, Sid?

-Mike

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Yep, that's right Mike.  The presence of "DNT: 1" on the wire is supposed to reflect the user's individual choice.  We can't make that choice for them without changing what DNT means, so it should remain off by default.

Comment 17

[Tom Lowenthal]

Our blog posts include a little more detail on this:
<https://blog.mozilla.org/privacy/2011/11/09/dnt-cannot-be-default/>
<https://blog.mozilla.org/privacy/2011/11/15/deeper-discussion-of-our-decision-on-dnt-defaults/>