:Felipe Gomes (needinfo for replies!) Reporter
	Description • 8 years ago

With the reorganized Preferences tab, the Reports section (aka fhr, telemetry & crashes) are under the Updates section, which doesn't seem the best place for it.

The spec confirms that it should be under Privacy: https://mozilla.invisionapp.com/share/5RA0R4HAE#/screens/214906112

Also, of note: on a first run, there's an infobar that shows informing the use about Telemetry.. And it has a button that says "Choose What I Share". That button is currently taking to the Updates section and will need to be updated too.

	Florian Quèze [:florian]
	Comment 5 • 8 years ago
mozreview-review

Comment on attachment 8857513 [details]
Bug 1355522 - Move Telemetry and Health Report prefs to Privacy & Security section.

https://reviewboard.mozilla.org/r/129472/#review133024

::: browser/components/preferences/in-content/privacy.js:1343
(Diff revision 1)
> +  /**
> +   * Set the status of the telemetry controls based on the input argument.
> +   * @param {Boolean} aEnabled False disables the controls, true enables them.
> +   */
> +  setTelemetrySectionEnabled(aEnabled) {
> +    if (AppConstants.MOZ_TELEMETRY_REPORTING) {

If this was new code, I would suggest an early return, to avoid indenting the whole method.

I don't expect you to do this change, because I know this is just code being moved around. But if you decide to do it, please do it for all the methods currently following the same pattern.

::: browser/components/preferences/in-content/privacy.js:1385
(Diff revision 1)
> +      Services.prefs.setBoolPref(PREF_UPLOAD_ENABLED, checkbox.checked);
> +      this.setTelemetrySectionEnabled(checkbox.checked);
> +    }
> +  },
> +
> -  // Methods for Offline Apps (AppCache)
> +   // Methods for Offline Apps (AppCache)

nit: I don't think you intended to add one space before this comment.

	Florian Quèze [:florian]
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8857514 [details]
Bug 1355522 - Move scripts for spell-checking, hardware acceleration, and on-screen keyboards to main.js since the prefs moved to the main category.

https://reviewboard.mozilla.org/r/129474/#review133026

::: commit-message-4b36b:1
(Diff revision 1)
> +Bug 1355522 - Move scripts for spell-checking, hardware acceleration, and on-screen keyboardsto main.js since the prefs moved to the main category. r?mconley

"keyboardsto" please fix the typo in the commit message.

::: browser/components/preferences/in-content/tests/browser_checkspelling.js:15
(Diff revision 1)
> +  let checkbox = doc.querySelector("#checkSpelling");
> +  is(checkbox.checked,
> +     Services.prefs.getIntPref("layout.spellcheckDefault") == 2,
> +     "checkbox should represent pref value before clicking on checkbox");
> +
> +  checkbox.click();

Should the test ensure that the value of checkbox.checked is different after calling checkbox.click()?

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 9 • 8 years ago
mozreview-review-reply

Comment on attachment 8857513 [details]
Bug 1355522 - Move Telemetry and Health Report prefs to Privacy & Security section.

https://reviewboard.mozilla.org/r/129472/#review133024

> If this was new code, I would suggest an early return, to avoid indenting the whole method.
> 
> I don't expect you to do this change, because I know this is just code being moved around. But if you decide to do it, please do it for all the methods currently following the same pattern.

I've fixed this in the patch here and other places within this file.

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 10 • 8 years ago
mozreview-review-reply

Comment on attachment 8857514 [details]
Bug 1355522 - Move scripts for spell-checking, hardware acceleration, and on-screen keyboards to main.js since the prefs moved to the main category.

https://reviewboard.mozilla.org/r/129474/#review133026

> Should the test ensure that the value of checkbox.checked is different after calling checkbox.click()?

Yes, I added a check for the value of checkbox.checked to the test.

	Pulsebot
	Comment 11 • 8 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f142622c5a2b
Move Telemetry and Health Report prefs to Privacy & Security section. r=florian
https://hg.mozilla.org/integration/autoland/rev/0700969e50a4
Move scripts for spell-checking, hardware acceleration, and on-screen keyboards to main.js since the prefs moved to the main category. r=florian

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 12 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f142622c5a2b
https://hg.mozilla.org/mozilla-central/rev/0700969e50a4

	Mohammad Maruf Rahman
	Comment 13 • 8 years ago

I have reproduced this bug with Nightly 55.0a1 (2017-04-11) (64-bit) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly! 


Build ID   : 20170419100228
User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170419]