809094 - Preference pane options for Firefox Health Report

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Gregory Szorc [:gps]]

Firefox Health Report will require some preference pane work.

This may or may not involve moving some things from Advanced -> General -> System Defaults to the Privacy tab.

Comment 1

[Gregory Szorc [:gps]]

I believe mconnor is on the hook to get UX designs for this.

Comment 2

[Gregory Szorc [:gps]]

I'm taking this. I'm going off the UX designs previously talked about with Privacy.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Part 1: Move things to Privacy pane, v1

Per discussions with the Privacy Team, it is desired to have all the "Allow Firefox to phone home" checkboxes on the Privacy pane rather than buried in Advanced. This patch moves "Submit crash reports" and "Submit performance data" to the Privacy pane. "Always check to see if Firefox is your default browser" is the lone option remaining under Advanced -> System Defaults.

I have no clue what to make the sub-heading in the Privacy pane. I figure Tom can provide something good here.

gavin: I normally don't muck around in /browser. I'm sure I'm doing several things wrong, especially on the DTD front. Also, I haven't verified tests still pass. I'm hoping something fails because of this. Given the open issues, requesting feedback instead of full review. But please give full review scrutiny.

Comment 4

[Gregory Szorc [:gps]]

All the tests for regular and in-content prefs pass with this patch. Yay missing test coverage!

Not.

Comment 5

[Gregory Szorc [:gps]]

Attachment: Part 2: Add health report checkbox to privacy pane, v1

I added a health report checkbox to the privacy pane. Only asking for feedback because part 1 is still outstanding and because I need to add tests. Time to learn how to write mochitests!

Comment 6

[Gregory Szorc [:gps]]

Attachment: Screen shot of new Privacy pane

For evaluation purposes.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 680197 [details] [diff] [review]
Part 1: Move things to Privacy pane, v1

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

::: browser/components/preferences/in-content/privacy.js
@@ +438,5 @@
> +
> +  /**
> +   * Initialize the crash reports checkbox.
> +   */
> +  initSubmitCrashes: function initSubmitCrashes() {

nit: no need to name these functions anymore since bug 433529 got fixed. See http://javascript-reverse.tumblr.com/post/34328529526/javascript-function-name-inference-aka-stop-function for more information.

@@ +459,5 @@
> +      let cr = Components.classes["@mozilla.org/toolkit/crash-reporter;1"]
> +                                 .getService(Components.interfaces.nsICrashReporter);
> +      cr.submitReports = checkbox.checked;
> +    } catch (e) { }
> +  },

no comma on the trailing curly brace here.

Please also make these changes to the non-in-content privacy.js file.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +79,5 @@
>  <!ENTITY  clearOnCloseSettings.label     "Settings…">
>  <!ENTITY  clearOnCloseSettings.accesskey "t">
> +
> +<!-- TODO proper naming -->
> +<!ENTITY  reporting.label    "E.T. Phone Home">

"Data sent to &vendorShortName;"

@@ +82,5 @@
> +<!-- TODO proper naming -->
> +<!ENTITY  reporting.label    "E.T. Phone Home">
> +
> +<!-- LOCALIZATION NOTE (submitCrashes.label): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY submitCrashes.label             "Submit crash reports">

Let's take this opportunity to remove redundancy and change these strings to "Crash reports"

@@ +84,5 @@
> +
> +<!-- LOCALIZATION NOTE (submitCrashes.label): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY submitCrashes.label             "Submit crash reports">
> +<!-- LOCALIZATION NOTE (submitCrashes.accesskey): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY submitCrashes.accesskey         "C">

This value will conflict with acceptThirdParty.accesskey. Also, when an entity value changes, the entity name must change as well so localizers will notice the change.

@@ +87,5 @@
> +<!-- LOCALIZATION NOTE (submitCrashes.accesskey): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY submitCrashes.accesskey         "C">
> +
> +<!-- LOCALIZATION NOTE (submitTelemetry.label): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY submitTelemetry.label           "Submit performance data">

"Performance data"

@@ +89,5 @@
> +
> +<!-- LOCALIZATION NOTE (submitTelemetry.label): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY submitTelemetry.label           "Submit performance data">
> +<!-- LOCALIZATION NOTE (submitTelemetry.accesskey): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY submitTelemetry.accesskey       "T">

This accesskey will conflict with the clearOnCloseSettings accesskey.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 680254 [details] [diff] [review]
Part 2: Add health report checkbox to privacy pane, v1

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

::: browser/components/preferences/in-content/privacy.js
@@ +469,5 @@
> +  /**
> +   * Initialize the health report service reference and checkbox.
> +   */
> +  initSubmitHealthReport: function initSubmitHealthReport() {
> +    this._healthReporter =

I'd rather not have us hold a reference to the reporter since it shouldn't provide much performance gain here.

@@ +477,5 @@
> +                        .reporter;
> +
> +    let checkbox = document.getElementById("submitHealthReportBox");
> +    try {
> +      checkbox.checked = this._healthReporter.dataSubmissionPolicyAccepted;

In what cases does checking this member throw an exception?

@@ +487,5 @@
> +   */
> +  updateSubmitHealthReport: function updateSubmitHealthReport() {
> +    let checkbox = document.getElementById("submitHealthReportBox");
> +    try {
> +      let current = this._healthReporter.dataSubmissionPolicyAccepted;

s/current/accepted/

::: browser/components/preferences/in-content/privacy.xul
@@ +242,5 @@
> +  <checkbox id="submitHealthReportBox"
> +            flex="1"
> +            oncommand="gPrivacyPane.updateSubmitHealthReport();"
> +            label="&submitHealthReport.label;"
> +            accesskey="&submitHealthReport.accesskey;" />

nit: no space between quote and closing slash.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +87,5 @@
>  <!-- LOCALIZATION NOTE (submitCrashes.accesskey): Moved from chrome/browser/preferences/advanced.dtd -->
>  <!ENTITY submitCrashes.accesskey         "C">
>  
> +<!ENTITY submitHealthReport.label        "Submit daily health report">
> +<!ENTITY submitHealthReport.accesskey    "R">

This accesskey will conflict with clearOnClose.accesskey.

Comment 9

[Gregory Szorc [:gps]]

(In reply to Jared Wein [:jaws] from comment #7)
> @@ +459,5 @@
> > +      let cr = Components.classes["@mozilla.org/toolkit/crash-reporter;1"]
> > +                                 .getService(Components.interfaces.nsICrashReporter);
> > +      cr.submitReports = checkbox.checked;
> > +    } catch (e) { }
> > +  },
> 
> no comma on the trailing curly brace here.

Why? I know some seem to apply this rule. I think it is silly to leave it out.

The trailing comma, while optional, serves an important purpose: it allows insertions of new properties without having to change an existing line. You don't have to think about where you are adding something inside an array or object: you just do it and it just works.

I don't know how many times I've added new items to an array or object and have received cryptic syntax errors because I forgot to check if the item before had a trailing comma. If you always use the trailing comma, you have no extra work to do. You just insert a new property or element and things work. Along the same vein, if you refactor code and move things around, you can copy and paste whole lines without having to worry about context. The trailing comma saves time and effort. We should all use it.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #9)

> > no comma on the trailing curly brace here.
> 
> Why? I know some seem to apply this rule. I think it is silly to leave it
> out.

Trailing commas is current style, according to my reading of reviews around the project. (I know dolske wants it, and I've switched to using trailing commas, too.)

Comment 11

[Gregory Szorc [:gps]]

See bug 508637 regarding trailing commas.

Comment 12

[Gregory Szorc [:gps]]

Attachment: Part 1: Move data submission items to Privacy pane, v1

Addressed all of the initial items of feedback. I totally missed the accesskey collisions because I thought they were case sensitive.

There is still an open question on whether these strings are appropriate. I figure we can do the review dance and update the strings later if someone says they need to change.

Comment 13

[Gregory Szorc [:gps]]

(In reply to Jared Wein [:jaws] from comment #8)
> ::: browser/components/preferences/in-content/privacy.js
> @@ +469,5 @@
> > +  /**
> > +   * Initialize the health report service reference and checkbox.
> > +   */
> > +  initSubmitHealthReport: function initSubmitHealthReport() {
> > +    this._healthReporter =
> 
> I'd rather not have us hold a reference to the reporter since it shouldn't
> provide much performance gain here.

I didn't do this for the performance gain: I did it for code conciseness. I think the current approach is less evil than:

1) Writing the XPCOM boilerplate more than once
2) Writing a function to simply return said value

You are probably going to need to assign the reporter to a local in updateSubmitHealthReport(), so why not just assign it to `this` on initialization?

After saying all that. Meh.

Comment 14

[Gregory Szorc [:gps]]

Attachment: Part 2: Add health report checkbox to privacy pane, v2

I think I addressed all the points of feedback.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Gregory Szorc [:gps] from comment #12)
> There is still an open question on whether these strings are appropriate. I
> figure we can do the review dance and update the strings later if someone
> says they need to change.

You should get ui-review for these strings as well as for the new layout of the privacy pane.

Comment 16

[Gregory Szorc [:gps]]

(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to Gregory Szorc [:gps] from comment #12)
> > There is still an open question on whether these strings are appropriate. I
> > figure we can do the review dance and update the strings later if someone
> > says they need to change.
> 
> You should get ui-review for these strings as well as for the new layout of
> the privacy pane.

This is my first time submitting patches that touch UI. If I'm doing it wrong, please adjust the bug appropriately and point me to the docs telling me how to do it right.

Comment 17

[Gregory Szorc [:gps]]

Attachment: Screen shot of new Privacy pane, v2

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 680830 [details] [diff] [review]
Part 1: Move data submission items to Privacy pane, v1

UI review was performed on part 2.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 680830 [details] [diff] [review]
Part 1: Move data submission items to Privacy pane, v1

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

r=me with the following changes addressed.

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
@@ +79,5 @@
>  <!ENTITY  clearOnCloseSettings.label     "Settings…">
>  <!ENTITY  clearOnCloseSettings.accesskey "t">
> +
> +<!-- TODO proper naming -->
> +<!ENTITY  reporting.label    "Data sent to &vendorShortName;">

Hmmm... the other strings within this dialog are present-tense/future-tense, whereas this string is past-tense. The other thing that concerns me is the scariness of this text.

Let's change this to "Help Improve &brandShortName;", which gives more background while also fixing the tense and appearing less scary. With this we should prefix the other three strings with "Send ". Sorry for the dance here.

@@ +88,5 @@
> +<!ENTITY crashReports.accesskey         "o">
> +
> +<!-- LOCALIZATION NOTE (performanceData.label): Moved from chrome/browser/preferences/advanced.dtd -->
> +<!ENTITY performanceData.label          "Performance data">
> +<!-- LOCALIZATION NOTE (reportingPerformanceData.accesskey): Moved from chrome/browser/preferences/advanced.dtd -->

s/reportingPerformanceData/performanceData/

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 680840 [details] [diff] [review]
Part 2: Add health report checkbox to privacy pane, v2

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

::: browser/components/preferences/in-content/privacy.js
@@ +469,5 @@
> +  /**
> +   * Initialize the health report service reference and checkbox.
> +   */
> +  initSubmitHealthReport: function () {
> +    this._healthReporter =

In this function and in updateSubmitHealthReport, the healthReporter should be checked for null. Based on one of the comments in https://bug808219.bugzilla.mozilla.org/attachment.cgi?id=680153 this variable can be null even though MOZ_SERVICES_HEALTHREPORT is defined.

+ * One can obtain a reference to the underlying HealthReporter instance by
+ * accessing .reporter. If this property is null, the reporter isn't running
+ * yet or has been disabled.

@@ +484,5 @@
> +   * Update the health report policy acceptance with state from checkbox.
> +   */
> +  updateSubmitHealthReport: function () {
> +    let checkbox = document.getElementById("submitHealthReportBox");
> +    let accepted = this._healthReporter.dataSubmissionPolicyAccepted;

I'm pretty sure Cc and Ci are defined in this context through another file that is included (preferences.js I believe). Caching the healthReporter on |this| to remove code duplication doesn't seem worth the risk that we will extend the lifetime of the reporter longer than it is needed. Since Cc and Ci are defined in this scope, the code duplication is minimal and the two lines aren't likely to change anytime in the future.

Please change this so that healthReporter is a local variable.

Comment 21

[Gregory Szorc [:gps]]

(In reply to Jared Wein [:jaws] from comment #19)
> ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
> @@ +79,5 @@
> >  <!ENTITY  clearOnCloseSettings.label     "Settings…">
> >  <!ENTITY  clearOnCloseSettings.accesskey "t">
> > +
> > +<!-- TODO proper naming -->
> > +<!ENTITY  reporting.label    "Data sent to &vendorShortName;">
> 
> Hmmm... the other strings within this dialog are present-tense/future-tense,
> whereas this string is past-tense. The other thing that concerns me is the
> scariness of this text.
> 
> Let's change this to "Help Improve &brandShortName;", which gives more
> background while also fixing the tense and appearing less scary. With this
> we should prefix the other three strings with "Send ". Sorry for the dance
> here.

I'm waiting on the Privacy Team to provide explicit wording here. I figure we can check something into larch (so it's there and people can look at the builds) and fix the wording before the merge to m-c.

Comment 22

[Gregory Szorc [:gps]]

Attachment: Part 2: Add health report checkbox to privacy pane, v3

Checkbox is now disabled if no health reporter instance could be obtained. I'm not sure if this is appropriate or if we should just hide the element. The only time when no instance can be obtained should be when the master healthreport.serviceEnabled pref is changed to false. There is no UI for this pref outside of about:config. It only exists so there is a master kill switch for the feature. I don't anticipate this pref being used outside of corp deployments, where it is desirable to lock down applications.

I also wrote my first mochitests ever!

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 681154 [details] [diff] [review]
Part 2: Add health report checkbox to privacy pane, v3

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

Thanks for writing a mochitest for this :)

::: browser/components/preferences/in-content/privacy.js
@@ +477,5 @@
> +
> +    let checkbox = document.getElementById("submitHealthReportBox");
> +
> +    if (!reporter) {
> +      checkbox.disabled = true;

Let's hide the checkbox if the reporter is disabled. If the user does not have a straightforward way to enable the checkbox, then showing it disabled will only lead to confusion. In the case of admins wanting to permanently block the reporter, hiding this checkbox will be good confirmation.

::: browser/components/preferences/in-content/tests/privacypane_tests.js
@@ +451,5 @@
> +  let doc = win.document;
> +
> +  let checkbox = doc.getElementById("submitHealthReportBox");
> +  ok(checkbox);
> +  is(checkbox.checked, false);

Please include description strings as a third argument to ok() and is(). These are printed during the test run and help to explain expectations. For example:
is(checkbox.checked, false, "The checkbox should not be checked by default");

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Gregory Szorc [:gps] from comment #21)
> (In reply to Jared Wein [:jaws] from comment #19)
> > ::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd
> > @@ +79,5 @@
> > >  <!ENTITY  clearOnCloseSettings.label     "Settings…">
> > >  <!ENTITY  clearOnCloseSettings.accesskey "t">
> > > +
> > > +<!-- TODO proper naming -->
> > > +<!ENTITY  reporting.label    "Data sent to &vendorShortName;">
> > 
> > Hmmm... the other strings within this dialog are present-tense/future-tense,
> > whereas this string is past-tense. The other thing that concerns me is the
> > scariness of this text.
> > 
> > Let's change this to "Help Improve &brandShortName;", which gives more
> > background while also fixing the tense and appearing less scary. With this
> > we should prefix the other three strings with "Send ". Sorry for the dance
> > here.
> 
> I'm waiting on the Privacy Team to provide explicit wording here. I figure
> we can check something into larch (so it's there and people can look at the
> builds) and fix the wording before the merge to m-c.

Please make the requested string change before checking in to Larch. I don't want us to mess up the tense of our verbiage here, and I don't want something like this to accidentally slip by.

Comment 25

[Gregory Szorc [:gps]]

(In reply to Jared Wein [:jaws] from comment #24)
> Please make the requested string change before checking in to Larch. I don't
> want us to mess up the tense of our verbiage here, and I don't want
> something like this to accidentally slip by.

I'm not too worried about this slipping by: this feature is high visibility and the right people will see it in due time. People are clamoring for builds from larch to test this feature and this checkbox is the last piece blocking simple testing, so I want to get it landed ASAP.

Comment 26

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/larch/rev/2e1a01b71ae6
https://hg.mozilla.org/projects/larch/rev/cb2fbb587463

Comment 27

[Gregory Szorc [:gps]]

This made a bunch of browser chrome tests go orange apparently because the default prefs file from services/healthreport/ isn't being loaded. This is almost certainly a build config issue of some sorts.

https://tbpl.mozilla.org/php/getParsedLog.php?id=17008774&tree=Larch&full=1

Comment 28

[Gregory Szorc [:gps]]

I forgot to add the prefs file to *app/installer/package-manifest.in. Will track in another bug.

Comment 29

[Gregory Szorc [:gps]]

I fixed the package manifest and now we seem to be getting intermittent oranges in the tests for the privacy pane.

https://tbpl.mozilla.org/?tree=Larch&rev=664be25a2e2d

Not all results were in when I typed this, but all of OS X worked while Linux and Windows were failing. Most interesting.

Comment 30

[Richard Newman [:rnewman]]

Comment on attachment 680830 [details] [diff] [review]
Part 1: Move data submission items to Privacy pane, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 31

[Gregory Szorc [:gps]]

I don't believe we need to uplift this since the code in question will not be enabled on desktop Firefox and we don't want to burden l10n with the extra DTD file, especially considering the contents will change.

Comment 32

[Théo Chevalier]

Comment on attachment 680830 [details] [diff] [review]
Part 1: Move data submission items to Privacy pane, v1

I assume this patch will reach mozilla-central at some point, right?
If so, you will need to add changes landed in bug 737600 for the telemetry pref.
If not, please ignore this comment :)

Comment 33

[Larissa Co [:lco]]

Attachment: Data Choices Pref Pane

Jishnu and I worked on the text of this pane, and mconnor and I worked on where to place it in the pref pane. We know it's not perfect, but we think it's robust enough for a first release.

Regarding the location in the Advanced > Data Choices (new) tab:

With the longer text & explanation (which are necessary to make the differences among these choices more understandable to the user), we couldn't fit the checkboxes in the privacy pane. The advantage to creating a separate pane for these preferences is that we can link directly from the info bar button to this pane without showing the user other options in the privacy pane which are irrelevant to his current action. Additionally, this pane can be a starting point for organizing all the privacy choices we present to the user.

The quickest way for us to create a new pref pane (without having to come up with a new icon, name, etc.) was to add a tab in the "Advanced" Settings. This isn't the ideal placement, but hopefully we can improve on this later. 

Additional implications:

We removed the "Submit crash reports" and "Submit performance data" checkboxes from Advanced > General into this new panel to eliminate redundancy.

Comment 34

[Mike Connor [:mconnor]]

Attachment: v0.8

still need to do:

* apply same changes to in-content version of prefs
* fix URLs
* followup on SUMO to add new prefpane article

Comment 35

[Mike Connor [:mconnor]]

Attachment: v1.0

Followups needed after this patch:

* Need to add ID (crash-reporter) to crash reporter section of privacy policy
* Need to add FHR to privacy policy and ensure it's directly linkable (health-report)
* Need to add a new page to SUMO for the new tab in advanced (redirect on prefs-advanced-data-choices) and update docs to account for the relocated prefs (crash/perf stuff moving to data choices)

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 698367 [details] [diff] [review]
v1.0

Also need a browser peer in here.

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 698367 [details] [diff] [review]
v1.0

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

::: browser/components/preferences/advanced.js
@@ +235,5 @@
> +
> +    let reporter = Components.classes["@mozilla.org/healthreport/service;1"]
> +                                     .getService(Components.interfaces.nsISupports)
> +                                     .wrappedJSObject
> +                                     .reporter;

.reporter is a promise in my latest patch queue. It can resolve to null. It is never rejected.

CC[...]
  ...
  .wrappedJSObject
  .reporter
  .then(function onReporter(reporter) {
    if (!reporter) {
      return;
    }

    // Do stuff with HealthReporter instance.
  });

Sorry it had to be this way.

::: services/healthreport/healthreport-prefs.js
@@ +18,5 @@
>  pref("healthreport.policy.lastDataSubmissionFailureTime", "0");
>  pref("healthreport.policy.lastDataSubmissionRequestedTime", "0");
>  pref("healthreport.policy.lastDataSubmissionSuccessfulTime", "0");
>  pref("healthreport.policy.nextDataSubmissionTime", "0");
> +pref("healthreport.infoURL", "http://www.mozilla.org/legal/privacy/firefox.html#health-report");

How about "privacyURL"? "info" seems too generic.

Comment 38

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 698367 [details] [diff] [review]
v1.0

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

>+        <tab id="dataChoicesTab" label="&dataChoicesTab.label;" helpTopic="prefs-advanced-data-choices"/>

That "helpTopic" has a (hopefully soft) SUMO dependency, right? user-doc-needed?

>+        <!-- Data Choices -->
>+        <tabpanel id="dataChoicesPanel" orient="vertical">
>+#ifdef MOZ_TELEMETRY_REPORTING
>+#ifdef MOZ_SERVICES_HEALTHREPORT
> #ifdef MOZ_CRASHREPORTER
>         </tabpanel>

This tabpanel is empty if all of those are not defined. I dunno how likely that is to happen in practice (self-builds?). You could add a:
#ifndef MOZ_TELEMETRY_REPORTING
#ifndef MOZ_SERVICES_HEALTHREPORT
#ifndef MOZ_CRASHREPORTER
#define HIDE_DATACHOICES 1
#endif
#endif
#endif

>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd

>+<!ENTITY FHRLearnMore.label              "Learn More">
>+<!ENTITY telemetryLearnMore.label        "Learn More">
>+<!ENTITY crashReporterLearnMore.label    "Learn More">

This seems like a case where it couldn't possibly be a problem to re-use the same string.

r=me for the parts other than updateSubmitHealthReport (since I'm not familiar with how that works exactly, but I'm sure you/Greg have that covered).

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 698367 [details] [diff] [review]
v1.0

>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd

>+<!ENTITY FHRDesc.label                   "Helps you understand your browser performance and shares data with Mozilla about your browser health">
>+<!ENTITY telemetryDesc.label             "Shares performance, usage, hardware and customization data about your browser with Mozilla to help us make &brandShortName; better">
>+<!ENTITY crashReporterDesc.label         "&brandShortName; submits crash reports to help Mozilla make your browser more stable and secure">

Theoretically "Mozilla" should be "&vendorShortName;"

Comment 40

[Mike Connor [:mconnor]]

(In reply to Gavin Sharp from comment #38)

> That "helpTopic" has a (hopefully soft) SUMO dependency, right?
> user-doc-needed?

Keyword added, it's a soft dep (the redirect will fail until they add it, but it'll just 404 on SUMO).

> This tabpanel is empty if all of those are not defined. I dunno how likely
> that is to happen in practice (self-builds?). You could add a:
> #ifndef MOZ_TELEMETRY_REPORTING
> #ifndef MOZ_SERVICES_HEALTHREPORT
> #ifndef MOZ_CRASHREPORTER
> #define HIDE_DATACHOICES 1
> #endif
> #endif
> #endif

Belt and suspenders ftw.

> >diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd
> 
> >+<!ENTITY FHRLearnMore.label              "Learn More">
> >+<!ENTITY telemetryLearnMore.label        "Learn More">
> >+<!ENTITY crashReporterLearnMore.label    "Learn More">
> 
> This seems like a case where it couldn't possibly be a problem to re-use the
> same string.

My rule of thumb is "never assume on l10n" since there are all sorts of cases where it matters in like... two locales of 80+.  I mulled for a while, I would rather not chance it.

> r=me for the parts other than updateSubmitHealthReport (since I'm not
> familiar with how that works exactly, but I'm sure you/Greg have that
> covered).

Yep, gps and I have it covered.

(In reply to Gavin Sharp from comment #39)

> >+<!ENTITY FHRDesc.label                   "Helps you understand your browser performance and shares data with Mozilla about your browser health">
> >+<!ENTITY telemetryDesc.label             "Shares performance, usage, hardware and customization data about your browser with Mozilla to help us make &brandShortName; better">
> >+<!ENTITY crashReporterDesc.label         "&brandShortName; submits crash reports to help Mozilla make your browser more stable and secure">
> 
> Theoretically "Mozilla" should be "&vendorShortName;"

Not just theoretically.  Change made!

Comment 41

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/9e11714fcba2

I gave instructions in #developers that if the s-c merge turns any tests besides {xpcshell tests in /services} orange that this patch and only this patch should be backed out.

Comment 42

[Ed Morley [:emorley]]

(In reply to Gregory Szorc [:gps] from comment #41)
> https://hg.mozilla.org/mozilla-central/rev/9e11714fcba2
> 
> I gave instructions in #developers that if the s-c merge turns any tests
> besides {xpcshell tests in /services} orange that this patch and only this
> patch should be backed out.

Ms2ger, thank you for CCing, I wouldn't have seen this otherwise.

Gps, next time please can you merge m-c into s-c and wait for green, rather than merging straight into m-c when there are many new csets on m-c - otherwise we somewhat negate the point of having integration repos :-)

Comment 43

[Jim Jeffery not reading bug-mail 1/2/11]

Is this supposed to be in Windows also ?

Using latest hourly build on win7 x64 I see the new 'Data Choices' Tab in Advanced, but no box entries for the 'Health Report' as shown in the mockup png attachment.

tested using cset: https://hg.mozilla.org/mozilla-central/rev/66d595814554

Comment 44

[Ed Morley [:emorley]]

Unfortunately had to back this out for browser-chrome failures:
https://tbpl.mozilla.org/?rev=66d595814554
https://tbpl.mozilla.org/php/getParsedLog.php?id=18540792&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=18540995&tree=Firefox
https://tbpl.mozilla.org/php/getParsedLog.php?id=18540469&tree=Firefox

https://hg.mozilla.org/mozilla-central/rev/605ae260b7c8

Comment 45

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Ed Morley [:edmorley UTC+0] from comment #44)
> Unfortunately had to back this out for browser-chrome failures:
> https://tbpl.mozilla.org/?rev=66d595814554
> https://tbpl.mozilla.org/php/getParsedLog.php?id=18540792&tree=Firefox
> https://tbpl.mozilla.org/php/getParsedLog.php?id=18540995&tree=Firefox
> https://tbpl.mozilla.org/php/getParsedLog.php?id=18540469&tree=Firefox
> 
> https://hg.mozilla.org/mozilla-central/rev/605ae260b7c8

thanks Ed, but I'm confused or this bug is..  if the back-out was for test fails then the feature should have appeared in the build I was testing with should it not ?

Unless... the test fails are pointing to a total fail to paint the chrome in windows.

Comment 46

[Ed Morley [:emorley]]

Sorry, I have no idea if it's related to your issue, I'm not familiar with this bug or any of the others that were in the latest services-central merge. Gps is the person to ask.

Comment 47

[Ed Morley [:emorley]]

s/gps/mconnor or gps/

Comment 48

[Gregory Szorc [:gps]]

The Firefox Health Report build time option is disabled on m-c which is why you weren't seeing the checkbox. If the feature isn't built into the product, the pref pane is smart enough to not display the checkbox.

Comment 49

[Gregory Szorc [:gps]]

Attachment: v1.1

This is the previous patch:

+ ifdef MOZ_TELEMETRY_REPORTING guard around "learn more" set up because the XUL element is guarded by this ifdef as well.

+ basic mochitests for health report checkbox behavior. The other checkboxes remain untested (just like they were before this patch, sadly).

Comment 50

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 699573 [details] [diff] [review]
v1.1

>diff --git a/browser/components/preferences/in-content/tests/browser_healthreport.js b/browser/components/preferences/in-content/tests/browser_healthreport.js

>+function test() {

>+  open_preferences(runTest);

>+  gBrowser.removeCurrentTab();

We really should have open_preferences return the tab it creates so that we can explicitly close it, rather than removing the current tab (if something goes wrong opening it, that'll screw up other tests). But that's not your issue to fix.

>+  win.close();

Isn't this the content window loaded in the tab that was just closed? Seems redundant.

>+function resetPreferences() {
>+  Services.prefs.clearUserPref("browser.search.update");

This looks like a typo.

Otherwise the tests look fine.

Comment 51

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/2cb72579ba39

Fixed pref typo and double (be extra sure) tab close per review comments.

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 699573 [details] [diff] [review]
v1.1

[Approval Request Comment]
See comment in bug 804745.

Comment 53

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/2cb72579ba39

Comment 54

[Alex Keybl [:akeybl]]

Comment on attachment 699573 [details] [diff] [review]
v1.1

https://bugzilla.mozilla.org/show_bug.cgi?id=804745#c37

Comment 55

[Francesco Lodolo [:flod]]

Is there a specific reason for these hard-coded "Firefox" in advanced.dtd (instead of &brandShortName; as in aboutHealthReport.dtd)? For sure it will create inconsistencies in the interface.

<!ENTITY FHRSection.label                "Firefox Health Report">
<!ENTITY enableFHR.label                 "Enable Firefox Health Report">

Comment 56

[:Gavin Sharp [email: [e-mail]]]

Nope, that's a bug that should be fixed.

Comment 57

[Jared Wein [:jaws] (please needinfo? me)]

It should be fixed before the patch is uplifted to Aurora so that we don't need to have two string changes on Aurora.

Comment 58

[Mike Connor [:mconnor]]

Attachment: part 3 (sigh): fix hardcoded branding

Also needed to change the accesskey, for obvious reasons.

Comment 59

[Dão Gottwald [:dao]]

Comment on attachment 701886 [details] [diff] [review]
part 3 (sigh): fix hardcoded branding

H is reserve for Help... Maybe R?

You also might want to rename the other entities starting with "FHR" for consistency and since FHR is kind of cryptic (and refers to the Firefox brand).

Comment 60

[Mike Connor [:mconnor]]

Filed bug 830498 to track the string issue.

Comment 61

[Scoobidiver (away)]

Pushed to Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/293ee495f439