1357348 - Add performance section and the UI component for content process count setting

Status: VERIFIED FIXED

(Firefox :: Settings UI, enhancement, P1)

Description

[Evan Tseng [:evanxd]]

Add performance section and the UI component for content process count setting.

Comment 1

[Evan Tseng [:evanxd]]

Attachment: bug-1357348.patch

WIP patch.

Discussed UI issues in the patch with Tina. She will provide a consistent UI design for the performance settings.

Comment 2

[Evan Tseng [:evanxd]]

Attachment: Patch v0.2

Updated patch to add access keys.

Comment 3

[Evan Tseng [:evanxd]]

Will continue to update patch to align the spec and add tests if Jared and Mike Conley is OK with the spec (Tina will share it on the slack preference channel).

Comment 4

[Evan Tseng [:evanxd]]

Attachment: Patch v0.3

Hi Jared,

I'm Evan from Taipei, Photon team.

Could you help give feedbacks for my patch?
It's my first time to write Preferences patch.

What patch does is
- Add performance settings section, UI components, and functions to align the UI spec[1].
- According to the discussion on the Preferences slack channel[2], we won't add the "Disable page prefetching" checkbox (mentioned in the spec[1]) at this stage.

Honestly, our team would like to make this patch "reveiw+" in next week. I'll do my best to try to get this done. Thanks for your help.

[1]: https://mozilla.invisionapp.com/share/ZNBDJMADT#/228017212_5-2_Advanced_Settings
[2]: https://mozilla.slack.com/archives/C4D8GS48G/p1492687329450478

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can see a few problem with the current setup on process count:

1. Simply open the preferences page will reset my the custom value set in |dom.ipc.processCount| from about:config, if the "use recommended settings" remain checked.
2. If a process count is set through preferences page but was then changed in about:config to a value other than 1 to 7, the control will become empty.

(1) will be confusing for people given it's user data loss. (2) may be acceptable because we are dealing with user playing with about:config.

I would like to see (1) addressed before landing. Presumably by uncheck the "use recommended settings" for the user if we find the |dom.ipc.processCount| is a custom value.

Thanks!

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The other thing that was discussed offline was whether or not we want to remember the custom settings for the user when the user unchecks the checkbox before "Limit content processes to". Same thing apply to when the user checks "Use recommended performance settings".

They should still be requirements unless there are product says otherwise.

Comment 7

[Evan Tseng [:evanxd]]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5)
> I can see a few problem with the current setup on process count:
> 
> 1. Simply open the preferences page will reset my the custom value set in
> |dom.ipc.processCount| from about:config, if the "use recommended settings"
> remain checked.
I've discussed with Tina for it. Let's fix it. We will do something like, if user set `dom.ipc.processCount` from `about:config` page, we will uncheck "Use recommended performance settings" checkbox automatically.

> 2. If a process count is set through preferences page but was then changed
> in about:config to a value other than 1 to 7, the control will become empty.
> 
> (1) will be confusing for people given it's user data loss. (2) may be
> acceptable because we are dealing with user playing with about:config.
> 
> I would like to see (1) addressed before landing. Presumably by uncheck the
> "use recommended settings" for the user if we find the
> |dom.ipc.processCount| is a custom value.
>
> Thanks!

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> The other thing that was discussed offline was whether or not we want to
> remember the custom settings for the user when the user unchecks the
> checkbox before "Limit content processes to". Same thing apply to when the
> user checks "Use recommended performance settings".
> 
> They should still be requirements unless there are product says otherwise.
This is a consistency issue of design. We've already filed a follow up issue (Bug 1358952) to address that and Tina will provide their ideas there.

Comment 8

[Evan Tseng [:evanxd]]

Attachment: Patch v0.5

Hi Jared,

The design spec[1] is updated. So I updated the patch to align it.
(The original patch is at Comment 4)

Could you help give feedbacks for it?

Thanks.

[1]: https://mozilla.invisionapp.com/share/C2B97CAYH#/screens/230367024

Comment 9

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

Comment on attachment 8860894 [details] [diff] [review]
Patch v0.5

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

::: browser/app/profile/firefox.js
@@ +686,5 @@
>  pref("browser.preferences.offlineGroup.enabled", false);
>  #else
>  pref("browser.preferences.offlineGroup.enabled", true);
>  #endif
> + 

nit, trailing whitespace

::: browser/components/preferences/in-content/main.js
@@ +93,5 @@
>  
> +    let processCountPref =
> +      document.getElementById("dom.ipc.processCount");
> +    processCountPref.addEventListener("change", () => {
> +      this.updateDefaultPerformanceSettingsPref();

I don't think this is right. From my reading of the code, it looks like if this pref is changed from "4 (default)" to "6" then back to "4 (default)", these extra options will hide because the defaultPerformanceSettings pref will become 'true'.

I don't think it's right to automatically hide the section if the user hasn't explicitly unchecked the "Use recommended performance settings" checkbox.

Please correct me if my reading of the code is wrong.

::: browser/components/preferences/in-content/main.xul
@@ +562,4 @@
>  </groupbox>
> +
> +<!-- Performance -->
> +<groupbox id="performanceGroup" data-category="paneGeneral">

This should be hidden="true" by default.

@@ +564,5 @@
> +<!-- Performance -->
> +<groupbox id="performanceGroup" data-category="paneGeneral">
> +  <caption><label>&performance.label;</label></caption>
> +
> +  <checkbox id="useRecommendedPerformanceSettings"

This is missing the Learn More link and the description text beneath it.

@@ +581,5 @@
> +        <menupopup>
> +          <menuitem label="1" value="1"/>
> +          <menuitem label="2" value="2"/>
> +          <menuitem label="3" value="3"/>
> +          <menuitem label="&defaultContentProcess.label;" value="4"/>

Would we ever want to change which ipcCount is the default? This is hard-coding it to always be 4. This should be reading the dom.ipc.processCount pref and using that as the 'default' value.

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
@@ +129,5 @@
> +<!ENTITY useRecommendedPerformanceSettings.label
> +                                         "Use recommended performance settings">
> +<!ENTITY useRecommendedPerformanceSettings.accesskey
> +                                         "U">
> +<!ENTITY limitContentProcess.label       "Limit content processes to">

This UI design will not work for all languages. The design here assumes the number of content processes will always be at the end of the sentence, though in some languages it may need to appear in the beginning or middle of the sentence.

At the least, the wording here should change so it is not structured as a sentence. Something like the following would work better:
Content process limit [4 (default)]

@@ +131,5 @@
> +<!ENTITY useRecommendedPerformanceSettings.accesskey
> +                                         "U">
> +<!ENTITY limitContentProcess.label       "Limit content processes to">
> +<!ENTITY limitContentProcess.description
> +                                         "More content processes can make Firefox more reponsive when using multiple tabs, but will also consume more memory.">

This needs to use &brandShortName; instead of hard-coding Firefox.

s/reponsive/responsive/

Comment 10

[Evan Tseng [:evanxd]]

Attachment: Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

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

Comment 11

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

https://reviewboard.mozilla.org/r/133386/#review136234

::: browser/components/preferences/in-content/main.js:106
(Diff revision 1)
> +    let accelerationPref =
> +      document.getElementById("layers.acceleration.disabled");
> +    accelerationPref.addEventListener("change", () => {
> +      this.updateDefaultPerformanceSettingsPref();
> +    });
> +    this.updateDefaultPerformanceSettingsPref();

> I don't think it's right to automatically hide the section if the user hasn't explicitly unchecked the "Use recommended performance settings" checkbox.

The `updateDefaultPerformanceSettingsPref` method won't automatically hide the section. We also have tests to test it in the patch.

Comment 12

[Evan Tseng [:evanxd]]

Hi Jared,

I updated the patch for your comments. Could you help review it?

Thanks.

Comment 13

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

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

Comment 14

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

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

Comment 15

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

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

https://reviewboard.mozilla.org/r/133386/#review136434

The Learn More link is still missing on the "Use recommended performance settings" checkbox.

This section should be hidden unless some preference is set to true, so we can delay releasing it until 57. Please also add a test that confirms that the section is hidden when the pref is set to false.

::: browser/components/preferences/in-content/main.js:848
(Diff revision 3)
> +      document.querySelector(`#contentProcessCount menupopup 
> +                              menuitem[value="${processCountPref.defaultValue}"]`);

Please use the child selector here instead of the descendent selector.

#contentProcessCount > menupopup > menuitem[value="${processCountPref.defaultValue}"]

Also, there is trailing whitespace here that should be removed.

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:132
(Diff revision 3)
> +
> +<!ENTITY performance.label               "Performance">
> +<!ENTITY useRecommendedPerformanceSettings.label
> +                                         "Use recommended performance settings">
> +<!ENTITY useRecommendedPerformanceSettings.description
> +                                         "Recommanded performance settings are tailored to your computer and internet connection speed.">

Spelling error, "Recommended" instead of "Recommanded"

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:137
(Diff revision 3)
> +                                         "Recommanded performance settings are tailored to your computer and internet connection speed.">
> +<!ENTITY useRecommendedPerformanceSettings.accesskey
> +                                         "U">
> +<!ENTITY limitContentProcess.label       "Content process limit">
> +<!ENTITY limitContentProcess.description
> +                                         "More content processes can make &brandShortName; more reponsive when using multiple tabs, but will also consume more memory.">

Spelling error, "responsive" instead of "reponsive"

Comment 16

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

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

Comment 17

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

https://reviewboard.mozilla.org/r/133386/#review136434

>The Learn More link is still missing on the "Use recommended performance settings" checkbox.

The page of the Learn More link doesn't exist yet, and the spec[1] says "Need to create a SUMO page" for it. I filed a bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1359735) to create the SUMO page, and sent needinfo to Joni. I would say let's file a follow up bug to add it. What do you think, Jared?

>This section should be hidden unless some preference is set to true, so we can delay releasing it until 57. Please also add a test that confirms that the section is hidden when the pref is set to false.

As my understand, we will deliver the performance section at release 55. So I think we don't need to add a pref to enable/disable it. Let's needinfo our project manager (Francis) to confirm that.

[1]: https://mozilla.invisionapp.com/share/C2B97CAYH#/screens/230367023

Comment 18

[Evan Tseng [:evanxd]]

Hi Francis,

What do you think of Comment 17?

Comment 19

[Evan Tseng [:evanxd]]

Hi Jared,

I update the patch for your review comments?

Could you take a look again?

Thanks.

Comment 20

[Francis Lee [:frlee]]

hi Evan and Jared,

the reason to release this performance options in 55 is basically for promoting our multi-process and performance improvement.
if there's no technical limitation, it will be great that we release it earlier than 57.
Let me need info PM Cindy, she can provide more information regarding this request from leadership.

thank you very much

Comment 21

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

(In reply to Evan Tseng [:evanxd] from comment #17)
> The page of the Learn More link doesn't exist yet, and the spec[1] says
> "Need to create a SUMO page" for it. I filed a bug
> (https://bugzilla.mozilla.org/show_bug.cgi?id=1359735) to create the SUMO
> page, and sent needinfo to Joni. I would say let's file a follow up bug to
> add it. What do you think, Jared?

Yes, this is fine, but bug 1359735 will only add the SUMO page. We will need to file another bug to get the Learn More link added once bug 1359735 is fixed.

Comment 22

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

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

https://reviewboard.mozilla.org/r/133386/#review136890

::: commit-message-27311:1
(Diff revision 4)
> +Bug 1357348 - Add the performance section to set the hardware acceleration and content proecess count prefs, r=jaws

Spelling error, "process" instead of "proecess".

::: browser/components/preferences/in-content/main.xul:582
(Diff revision 4)
> +      <label id="limitContentProcess">&limitContentProcess.label;</label>
> +      <menulist id="contentProcessCount" accesskey="&limitContentProcess.accesskey;" preference="dom.ipc.processCount">

Please move the accesskey to the <label>, and set control="contentProcessCount" on the <label>.

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

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

Comment 24

[Evan Tseng [:evanxd]]

> Yes, this is fine, but bug 1359735 will only add the SUMO page. We will need to file another bug to get the Learn More link added once bug 1359735 is fixed.

Sure, I will file a new bug to address that.

And I've updated the patch for the review comments. Please help take a look. Thanks a lot. :)

Comment 25

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

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

https://reviewboard.mozilla.org/r/133386/#review136978

r=me with the following change.

::: browser/components/preferences/in-content/tests/browser_performance.js:1
(Diff revision 5)
> +Services.prefs.setBoolPref("browser.preferences.defaultPerformanceSettings.enabled", true);
> +Services.prefs.setIntPref("dom.ipc.processCount", 4);
> +Services.prefs.setBoolPref("layers.acceleration.disabled", false);
> +
> +registerCleanupFunction(function() {
> +  Services.prefs.clearUserPref("browser.preferences.defaultPerformanceSettings.enabled");
> +  Services.prefs.clearUserPref("dom.ipc.processCount");
> +  Services.prefs.clearUserPref("layers.acceleration.disabled");
> +});

Sorry I didn't see this before. You should be using SpecialPowers.pushPrefEnv instead of setPref/clearUserPref.

Comment 26

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133386/diff/5-6/

Comment 27

[Evan Tseng [:evanxd]]

Comment on attachment 8861389 [details]
Bug 1357348 - Add the performance section to set the hardware acceleration and content process count prefs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133386/diff/6-7/

Comment 28

[Evan Tseng [:evanxd]]

Updated patch for review comments and fixed the test failures.

Discussed the "Recommended performance settings are tailored to your computer and internet connection speed." string with Tina. Our conclusion is updating it to "Recommended performance settings are tailored to your computer." because currently the performance section doesn't include the connection speed things.

Let's ensure the treeherder jobs are good before we land the patch.

Comment 29

[Evan Tseng [:evanxd]]

> Yes, this is fine, but bug 1359735 will only add the SUMO page. We will need
> to file another bug to get the Learn More link added once bug 1359735 is
> fixed.

The follow up bug is Bug 1360140.

Comment 30

[chsiang]

yes multi-processes is for 55.

Comment 31

[Evan Tseng [:evanxd]]

Treeherder jobs are good. Let's land the patch.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e151a646b157
Add the performance section to set the hardware acceleration and content process count prefs, r=jaws

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e151a646b157

Comment 34

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Shouldn't the new performance section also include the "Enable multi-process &brandShortName;" preference?

Comment 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Michal Stanke (Mozilla.cz) [:MikkCZ] (off until 05/2017, use needinfo) from comment #34)
> Shouldn't the new performance section also include the "Enable multi-process
> &brandShortName;" preference?

That's a very good question. Tina, the said pref is a Nightly-only option. Do you want to move it also? Also logically we should disable # of content process options if e10s is not enabled.

Comment 36

[Tina Hsieh[:Tina_Hsieh] UX]

AlThough the option is Nightly-only, I don't see any problems of having it in the Performance section.
Let's move it after the option of hardware acceleration. :)

Comment 37

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

Filed bug 1361438 for moving the e10s checkbox to Performance section.

Comment 38

[Hani Yacoub, Desktop QA]

This bug was verified on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.