Closed Bug 1364094 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Preferences, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: evanxd, Assigned: evanxd)

References

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

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.
Flags: qe-verify+
Whiteboard: [photon-preference]
Attachment #8866827 - Flags: review?(jaws)
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.
Hi Jared,

Could you help review the patch?

Thanks.
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?
Attachment #8866827 - Flags: review?(jaws) → review-
> 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.
If the patch at Comment 7 is good, I'll update the tests.
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!
Attachment #8866827 - Flags: review?(jaws) → review+
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
The try is good. Let's land the patch.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/535111590aba
Make defaultPerformancePref become true after changing processCountPref and accelerationPref at one time. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/535111590aba
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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
QA Whiteboard: [bugday-20170607]
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]
As this bug is verified as fixed in both linux (comment 15) and windows (comment 16), I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.