986978 - Add Data Choices Tab to preferences, with GUI option to enable/disable Crash Reporter

Status: RESOLVED FIXED

(Thunderbird :: Preferences, enhancement)

Description

[Spampot]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

Currently the crash reporter must be enabled/disabled by manually editing the application.ini file (as was the case in earlier Firefox versions).

However, Firefox has been equipped with an explicit GUI option to enable/disable crash reports for quite a long time, and by default ignores its application.ini file unless explicitly forced to use it by according command line options.

I suggest that Thunderbird does just the same: Ignore application.ini by default, and ad a GUI option to enable/disable crash reports.

Pros:
+ A GUI option is easy to find (compared to manually editing a textfile that can typically only be accessed by admins).
+ More important: Each time Thunderbird is updated (or auto-updated), the application.ini file is replaced and your customizations, including the crash report settings, are lost.

Comment 1

[rsx11m]

That was introduced for Firefox with Toolkit bug 540532 and for SeaMonkey with bug 697453, nothing in the Thunderbird prefpanes. I don't see this filed yet, thus confirming as RFE.

Comment 2

[Hessam Salehi]

Attachment: 986978-v1.patch

is there any extra work needed ?

Comment 3

[Spampot]

If the solution
(a) reliably IGNORES (or better: REMOVES+REPLACES) the application.ini setting, and
(b) the setting "survives" Thunderbird upgrades without being reset,
then is is exactly what I was looking for.

Comment 4

[rsx11m]

Comment on attachment 8402404 [details] [diff] [review]
986978-v1.patch

>+    let enableCrashReport = Components.classes["@mozilla.org/toolkit/crash-reporter;1"]
>+                                      .getService(Components.interfaces.nsICrashReporter)
>+                                      .submitReports;

The SeaMonkey version wraps this into

>+  if ("nsICrashReporter" in Components.interfaces)

and hides the checkbox if the crash reporter interface is not available.
Any reason to assume that nsICrashReporter always exists?

Comment 5

[Magnus Melin [:mkmelin]]

If you're up to it, we should port over all of the "Data Choices" tab to thunderbird.

Comment 6

[rsx11m]

Comment on attachment 8402404 [details] [diff] [review]
986978-v1.patch

Ok, I've applied the patch to a current 31.0a1 trunk build but unfortunately don't see the desired behavior. The box is by default unchecked even though it should be checked, and it only sticks on normal completion but is cleared after a crash again. More importantly, it doesn't make any difference for me whether or not that box is checked, the Crash Reporter shows up either way...

I was testing on a 64-bit Linux build (sending SIGSEGV to the process), and iirc there was a recent bug with the Crash Reporter, thus either may contribute to the behavior I see if you are working on a different platform or build.

On the layout, the location of the checkbox sure is where it's supposed to be. Specifically on Windows though, is there sufficient vertical space? If not, maybe telemetryInfoLink can move on the same line as the checkbox (which in turn may cause localization issues if the labels need more space in a language).

Comment 7

[Hessam Salehi]

Attachment: 986978-v2.diff

(In reply to rsx11m from comment #6)
> Comment on attachment 8402404 [details] [diff] [review]
> 986978-v1.patch
> 
> Ok, I've applied the patch to a current 31.0a1 trunk build but unfortunately
> don't see the desired behavior. The box is by default unchecked even though
> it should be checked, and it only sticks on normal completion but is cleared
> after a crash again. More importantly, it doesn't make any difference for me
> whether or not that box is checked, the Crash Reporter shows up either way...
> 
As far as I know, according to using |.crashreports| (it's different with .enabled)
it's exactly the desired behaviour. also This implementation is exactly the way that
Firefox and Seamonkey has implemented it [1] so I think it's the common way to deal
with crash reporter.
when you uncheck "tell mozilla about this crash" in the crash reporter window it
leads to uncheck the crashreports checkbox in prefs after restart of thunderbird.

[1] http://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/advanced.js#199

> I was testing on a 64-bit Linux build (sending SIGSEGV to the process), and
> iirc there was a recent bug with the Crash Reporter, thus either may
> contribute to the behavior I see if you are working on a different platform
> or build.

My OS is ubuntu 13.10/64bit

> On the layout, the location of the checkbox sure is where it's supposed to
> be. Specifically on Windows though, is there sufficient vertical space? If
> not, maybe telemetryInfoLink can move on the same line as the checkbox
> (which in turn may cause localization issues if the labels need more space
> in a language).

as Magnus said I ported "data choices" and moved the telemetry and the crash
reporter to it.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8405922 [details] [diff] [review]
986978-v2.diff

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

I'll have to build with crashreporter enabled to test still, but some initial comments;
The layout/alignment + just "Learn More" text that firefox uses on trunk is nicer IMO, so please use that.

Nit: re the alignment of the values in the dtd, I'd just skip aligning them at all, it's futile as code changes... Now they look to be semi-aligned.

::: mail/locales/en-US/chrome/messenger/preferences/advanced.dtd
@@ +35,5 @@
>  <!ENTITY showReturnReceipts.accesskey  "R">
>  
> +<!-- Data Choices -->
> +<!ENTITY telemetrySection.label        "Telemetry">
> +<!ENTITY telemetryDesc.label           "Shares performance, usage, hardware and customization data about your E-mail client with &vendorShortName; to help us make &brandShortName; better">

I wouldn't capitalize E-mail here

@@ +42,5 @@
> +<!ENTITY telemetryInfoLink.label       "More information about Telemetry">
> +<!ENTITY enableCrashReporter.label      "Submit crash reports">
> +<!ENTITY enableCrashReporter.accesskey  "b">
> +<!ENTITY crashReporterSection.label    "Crash Reporter">
> +<!ENTITY crashReporterDesc.label       "&brandShortName; submits crash reports to help &vendorShortName; make your E-mail client more stable and secure">

And not here either.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8405922 [details] [diff] [review]
986978-v2.diff

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

Seems to work for me. 
(FTR, Firefox also do pop up the crash reporter - the option only affects if the "send report" checkbox is checked by default or not.

I'd still want the layout of the "Learn More" links and such to match what firefox trunk has.

Comment 10

[Magnus Melin [:mkmelin]]

Hessam: any progress?

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: bug986978_data_choices.patch

Mostly copy-paste from firefox... screenshot to follow

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: data-choices.png

Comment 13

[Magnus Melin [:mkmelin]]

And in the screenshot, pretend it says "e-mail client" instead of browser. Forgot to update the pic after I fixed that.

Comment 14

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

Comment on attachment 8561003 [details] [diff] [review]
bug986978_data_choices.patch

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

So, r-, based on the missing telemetryEnabledChanged function.  (If that's explained or fixed, I'm happy to change it to an r+.  :)

Thanks,
Blake.

::: mail/app/profile/all-thunderbird.js
@@ +145,5 @@
>  pref("app.releaseNotesURL", "http://live.mozillamessaging.com/%APP%/releasenotes?locale=%LOCALE%&version=%VERSION%&os=%OS%&buildid=%APPBUILDID%");
>  
> +// URL for "Learn More" for Crash Reporter.
> +pref("toolkit.crashreporter.infoURL",
> +     "http://www.mozilla.org/thunderbird/legal/privacy/#crash-reporter");");

That id doesn't exist.  Is there a bug to change that page?

::: mail/components/preferences/advanced.js
@@ -124,5 @@
> -  /**
> -   * When the user toggles telemetry, update the rejected value as well, so we
> -   * know he expressed a choice, and don't re-prompt inadvertently.
> -   */
> -  telemetryEnabledChanged: function (event)

Wait, why are we removing this?  Shouldn't it be moved down to line 555-ish?

@@ +513,5 @@
> +    }
> +  },
> +
> +  /**
> +   *

If we're not going to have a comment, let's just skip the comment block.  ;)

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Blake Winton (:bwinton) from comment #14)
> That id doesn't exist.  Is there a bug to change that page?

Bug 1133209.

> ::: mail/components/preferences/advanced.js
> @@ -124,5 @@
> > -  /**
> > -   * When the user toggles telemetry, update the rejected value as well, so we
> > -   * know he expressed a choice, and don't re-prompt inadvertently.
> > -   */
> > -  telemetryEnabledChanged: function (event)
> 
> Wait, why are we removing this?  Shouldn't it be moved down to line 555-ish?

No, none of that stuff exist anymore, since bug 829881 (and before).

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8561003 [details] [diff] [review]
bug986978_data_choices.patch

Too lazy to submit a new patch with the empty comments removed (those too just copied from firefox) :)

Comment 17

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

Comment on attachment 8561003 [details] [diff] [review]
bug986978_data_choices.patch

Okay, that sounds reasonable to me.  :)  Thanks!

Comment 18

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/dc756f0e0961 -> FIXED

Comment 19

[Spampot]

Just to make sure: Does the fix also involve IGNORING the application.ini file?

(See my original report: The application.ini file should be fully ignored unless explicitly supplied via command line parameter. That's how Firefox does it.)

This is important because otherwise the new GUI option will be overridden by the application.ini setting, with itself is reset by almost every automatic update.