1364094 - The default performance settings checkbox is not checked after changing hardware acceleration and content process limit settings at one time

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Evan Tseng [:evanxd]]

The default performance settings checkbox is not checked after updating hardware acceleration and content process limit settings at one time.

STR:
1. Uncheck default performance settings checkbox
2. Changing hardware acceleration and content process limit settings.
3. Check default performance settings checkbox

Expected:
The performance settings checkbox is checked.

Actual:
The performance settings checkbox is not checked.

Comment 1

[Evan Tseng [:evanxd]]

Attachment: Bug 1364094 - Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time.

Review commit: https://reviewboard.mozilla.org/r/138420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138420/

Comment 2

[Evan Tseng [:evanxd]]

Comment on attachment 8866827 [details]
Bug 1364094 - Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time.

https://reviewboard.mozilla.org/r/138420/#review141638

::: browser/components/preferences/in-content/main.js:842
(Diff revision 1)
>        let accelerationPref = document.getElementById("layers.acceleration.disabled");
>        processCountPref.value = processCountPref.defaultValue;
>        accelerationPref.value = accelerationPref.defaultValue;
> +      // Changing `processCountPref` and `accelerationPref` here
> +      // will make `defaultPerformancePref` become to `false`.
> +      defaultPerformancePref.value = true;

Will move this change to the old organization after Bug 1364070 is landed and we approve this change.

Comment 3

[Evan Tseng [:evanxd]]

Hi Jared,

Could you help review the patch?

Thanks.

Comment 4

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

Comment on attachment 8866827 [details]
Bug 1364094 - Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time.

https://reviewboard.mozilla.org/r/138420/#review144728

::: browser/components/preferences/in-content/main.js:838
(Diff revision 1)
>        processCountPref.value = processCountPref.defaultValue;
>        accelerationPref.value = accelerationPref.defaultValue;
> +      // Changing `processCountPref` and `accelerationPref` here
> +      // will make `defaultPerformancePref` become to `false`.
> +      defaultPerformancePref.value = true;

This is quite confusing. Have you tried adding an else clause to `updateDefaultPerformanceSettingsPref` that sets defaultPerformancePref.value to true? Or can we change `updateDefaultPerformanceSettingsPref` to not set defaultPerformancePref.value to false?

Comment 5

[Evan Tseng [:evanxd]]

Comment on attachment 8866827 [details]
Bug 1364094 - Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138420/diff/1-2/

Comment 6

[Evan Tseng [:evanxd]]

Comment on attachment 8866827 [details]
Bug 1364094 - Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138420/diff/2-3/

Comment 7

[Evan Tseng [:evanxd]]

> This is quite confusing. Have you tried adding an else clause to
> `updateDefaultPerformanceSettingsPref` that sets
> defaultPerformancePref.value to true? 
If we do this, the performance settings section will be hidden automatically after changing the settings to default values. So I would say, we should not do this.

> Or can we change
> `updateDefaultPerformanceSettingsPref` to not set
> defaultPerformancePref.value to false?
How about not updating `defaultPerformancePref.value` after changing `dom.ipc.processCount` and `layers.acceleration.disabled` prefs? It almost likes not setting `defaultPerformancePref.value` to false in the `updateDefaultPerformanceSettingsPref` method.

I've updated the patch. Could you review it again? Thanks.

Comment 8

[Evan Tseng [:evanxd]]

If the patch at Comment 7 is good, I'll update the tests.

Comment 9

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

Comment on attachment 8866827 [details]
Bug 1364094 - Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time.

https://reviewboard.mozilla.org/r/138420/#review145196

Yes, I think this is a better fix. Thanks!

Comment 10

[Evan Tseng [:evanxd]]

Comment on attachment 8866827 [details]
Bug 1364094 - Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138420/diff/3-4/

Comment 11

[Evan Tseng [:evanxd]]

Thanks for reviewing, Jared.

Let's land the patch after the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e831eb304160

Comment 12

[Evan Tseng [:evanxd]]

The try is good. Let's land the patch.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/535111590aba
Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time. r=jaws

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/535111590aba

Comment 15

[Sayed [:LiquidO2]]

I have reproduced this issue following the STR from comment 0 with nightly 55.0a1 (2017-05-11) (64-bit) in 64bit Linux

I can verify that this issue is now fixed in Nightly 55.0a1

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

Comment 16

[Tanvir Rahman]

I have reproduced this Bug on Nightly 55.0a1 (2017-05-11) on Windows 10, 64 Bit!

The bug's fix is now verified on latest Beta 55.0b4

Build ID 	20170622104007
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170621]

Comment 17

[Sayed [:LiquidO2]]

As this bug is verified as fixed in both linux (comment 15) and windows (comment 16), I am marking this bug as verified fixed.