Closed Bug 1357348 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Preferences, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
firefox55 --- verified

People

(Reporter: evanxd, Assigned: evanxd)

References

(Depends on 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file, 4 obsolete files)

Add performance section and the UI component for content process count setting.
Whiteboard: [photon][photon-preference]
Whiteboard: [photon][photon-preference] → [photon-preference]
Assignee: nobody → evan
Status: NEW → ASSIGNED
Attached patch bug-1357348.patch (obsolete) — Splinter Review
WIP patch.

Discussed UI issues in the patch with Tina. She will provide a consistent UI design for the performance settings.
Attached patch Patch v0.2 (obsolete) — Splinter Review
Updated patch to add access keys.
Attachment #8859525 - Attachment is obsolete: true
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).
Attached patch Patch v0.3 (obsolete) — Splinter Review
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
Attachment #8859965 - Attachment is obsolete: true
Attachment #8860364 - Flags: feedback?(jaws)
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!
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.
Target Milestone: --- → Firefox 55
(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.
Attachment #8860364 - Flags: feedback?(jaws)
Attached patch Patch v0.5 (obsolete) — Splinter Review
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
Attachment #8860894 - Flags: feedback?(jaws)
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/
Attachment #8860894 - Flags: feedback?(jaws) → feedback+
Attachment #8860364 - Attachment is obsolete: true
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.
Hi Jared,

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

Thanks.
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"
Attachment #8861389 - Flags: review?(jaws) → review-
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
Hi Francis,

What do you think of Comment 17?
Flags: needinfo?(frlee)
Hi Jared,

I update the patch for your review comments?

Could you take a look again?

Thanks.
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
Flags: needinfo?(frlee) → needinfo?(chsiang)
(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 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>.
Attachment #8861389 - Flags: review?(jaws) → review-
> 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 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.
Attachment #8861389 - Flags: review?(jaws) → review+
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.
> 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.
yes multi-processes is for 55.
Flags: needinfo?(chsiang)
QA Contact: hani.yacoub
Treeherder jobs are good. Let's land the patch.
Keywords: checkin-needed
Flags: qe-verify+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e151a646b157
Add the performance section to set the hardware acceleration and content process count prefs, r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e151a646b157
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Depends on: 1360846
Shouldn't the new performance section also include the "Enable multi-process &brandShortName;" preference?
(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.
Flags: needinfo?(thsieh)
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. :)
Flags: needinfo?(thsieh)
Filed bug 1361438 for moving the e10s checkbox to Performance section.
Priority: -- → P1
This bug was verified on Firefox Nightly 55.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.