"# of content process" option is exposed to non-e10s users in Preferences

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

(Depends on 1 bug)

Trunk
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-preference])

Attachments

(2 attachments, 1 obsolete attachment)

I am not entirely sure if this is a problem to be fixed (or what the valid fix would be), but I think this deserve a bug to discuss.

The newly-added performance section is not entirely applicable to non-e10s users. A decision is needed here to decide what we want to do with them.
hi Tina,

need your comment on this one.

thank you very much
Flags: needinfo?(thsieh)
Hi Tim,
Thanks for raising this issue. We have a similar issue happens on hardware acceleration. The current copy string is "Use hardware acceleration when available" but it doesn't tell users 1) why it's not available and 2) How to make it available when it's failed. There is a Bug 1267516 discussing this issue for a while, but it hasn't been fixed yet. 

I believe that we need to tell non-e10s users the reason why content process isn't applicable on the UI instead of simply grey it out or hide it. Consider the tight schedule of launching performance options, I suggest us to work on it step by step:

Step 1. Refine the copy to tell users that the number of content processes will only be applicable when e10s is available.
Step 2. Grey out the entire content processes option and provide the info of why and how to enable it.


Hi Michelle, 
Content processes isn't applicable when the e10s is disabled. The telemetry data (https://mzl.la/2qVLCO5) shows that over 50% of users on v53 in the past month are using Firefox with disabled e10s. Could you give us some recommendations for the copy?
Flags: needinfo?(thsieh) → needinfo?(mheubusch)
Jean, could you help me reach Michelle so I can lock down the string needed for Firefox 55? Thanks!
Flags: needinfo?(jgong)

Comment 4

2 years ago
You can only modify the number of content processes if you are running Firefox with e10s. Learn how to check if e10s is enabled (link to https://wiki.mozilla.org/Electrolysis)

Note - I will reach out to Joni Savage to see if we can put this wiki info into SUMO for a better experience.
Flags: needinfo?(mheubusch)
Flags: needinfo?(jgong)
Assignee: nobody → timdream
Status: NEW → ASSIGNED
This is what a non-e10s user will see with this patch.
Comment hidden (mozreview-request)
Patch updated per Tina's input.
Attachment #8876020 - Attachment is obsolete: true
Comment on attachment 8875992 [details]
Bug 1368943 - Disable content process count UI and switch description for non-e10s users,

:jaws, would you mind steal the review here? We would need this string change landed before beta 55.
Attachment #8875992 - Flags: review?(jaws)
Priority: -- → P1
Whiteboard: [photon-preference][ux][triage] → [photon-preference]
Comment on attachment 8875992 [details]
Bug 1368943 - Disable content process count UI and switch description for non-e10s users,

https://reviewboard.mozilla.org/r/147404/#review151746

::: browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd:140
(Diff revision 2)
>                                           "Learn more">
>  <!ENTITY limitContentProcessOption.label "Content process limit">
>  <!ENTITY limitContentProcessOption.description
>                                           "Additional content processes can improve performance when using multiple tabs, but will also use more memory.">
>  <!ENTITY limitContentProcessOption.accesskey   "L">
> +<!ENTITY limitContentProcessOption.disabledDescription

As usual, you don't really want Firefox hardcoded in strings.

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:153
(Diff revision 2)
>                                           "Learn more">
>  <!ENTITY limitContentProcessOption.label "Content process limit">
>  <!ENTITY limitContentProcessOption.description
>                                           "Additional content processes can improve performance when using multiple tabs, but will also use more memory.">
>  <!ENTITY limitContentProcessOption.accesskey   "L">
> +<!ENTITY limitContentProcessOption.disabledDescription

Same here
Comment hidden (mozreview-request)
(In reply to Francesco Lodolo [:flod] from comment #10)

Sorry about making the silly mistake :-/
Comment hidden (mozreview-request)
Attachment #8875992 - Flags: review?(jaws)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8875992 [details]
Bug 1368943 - Disable content process count UI and switch description for non-e10s users,

https://reviewboard.mozilla.org/r/147404/#review151878

r=me with the following issues fixed.

::: browser/components/preferences/in-content-new/main.js:228
(Diff revision 4)
> +    try {
> +      let e10sStatus = Components.classes["@mozilla.org/supports-PRUint64;1"]
> +                         .createInstance(Ci.nsISupportsPRUint64);
> +      let appinfo = Services.appinfo.QueryInterface(Ci.nsIObserver);
> +      appinfo.observe(e10sStatus, "getE10SBlocked", "");
> +      e10sEnabled = (e10sStatus.data < 2);

nit, no need for the parens.

::: browser/components/preferences/in-content/main.js:153
(Diff revision 4)
> +    try {
> +      let e10sStatus = Components.classes["@mozilla.org/supports-PRUint64;1"]
> +                         .createInstance(Ci.nsISupportsPRUint64);
> +      let appinfo = Services.appinfo.QueryInterface(Ci.nsIObserver);
> +      appinfo.observe(e10sStatus, "getE10SBlocked", "");
> +      e10sEnabled = (e10sStatus.data < 2);

nit, no parentheses necessary here.

::: browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd:141
(Diff revision 4)
>  <!ENTITY limitContentProcessOption.label "Content process limit">
>  <!ENTITY limitContentProcessOption.description
>                                           "Additional content processes can improve performance when using multiple tabs, but will also use more memory.">
>  <!ENTITY limitContentProcessOption.accesskey   "L">
> +<!ENTITY limitContentProcessOption.disabledDescription
> +                                         "You can only modify the number of content processes if you are running &brandShortName; with e10s.">

We don't use the "e10s" name in product. Please change "e10s" to "multiprocess" to match this string, http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/devtools/client/locales/en-US/performance.dtd#26

You can reword the phrasing to make the adjective fit better.

"Modifying the number of content processes is only possible with multiprocess &brandShortName."

::: browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd:143
(Diff revision 4)
>                                           "Additional content processes can improve performance when using multiple tabs, but will also use more memory.">
>  <!ENTITY limitContentProcessOption.accesskey   "L">
> +<!ENTITY limitContentProcessOption.disabledDescription
> +                                         "You can only modify the number of content processes if you are running &brandShortName; with e10s.">
> +<!ENTITY limitContentProcessOption.disabledDescriptionLink
> +                                         "Learn how to check if e10s is enabled">

"Learn how to check if multiprocess is enabled"

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:154
(Diff revision 4)
> +                                         "You can only modify the number of content processes if you are running &brandShortName; with e10s.">
> +<!ENTITY limitContentProcessOption.disabledDescriptionLink
> +                                         "Learn how to check if e10s is enabled">

Same comments about e10s here too.
Attachment #8875992 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)

Comment 16

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8c970380c92
Disable content process count UI and switch description for non-e10s users, r=jaws
Keywords: checkin-needed
Backed out for mass test failures, e.g. in browser/extensions/formautofill/test/browser/browser_privacyPreferences.js:

https://hg.mozilla.org/integration/autoland/rev/c11325203c125688c9d6d584572e9a509975d66a

Push with failures: https://hg.mozilla.org/integration/autoland/rev/a8c970380c927a352398865e90ba8605e56c5fac
Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=106033308&repo=autoland

[task 2017-06-10T05:50:34.431708Z] 05:50:34     INFO - TEST-START | browser/extensions/formautofill/test/browser/browser_privacyPreferences.js
[task 2017-06-10T05:50:34.607989Z] 05:50:34     INFO - TEST-INFO | started process screentopng
[task 2017-06-10T05:50:35.709756Z] 05:50:35     INFO - TEST-INFO | screentopng: exit 0
[task 2017-06-10T05:50:35.712878Z] 05:50:35     INFO - Buffered messages logged at 05:50:34
[task 2017-06-10T05:50:35.714408Z] 05:50:35     INFO - Entering test bound test_aboutPreferences
[task 2017-06-10T05:50:35.715937Z] 05:50:35     INFO - Console message: [JavaScript Error: "XML Parsing Error: error in processing external entity reference
[task 2017-06-10T05:50:35.720003Z] 05:50:35     INFO - Location: about:preferences
[task 2017-06-10T05:50:35.721525Z] 05:50:35     INFO - Line Number 81, Column 1:" {file: "about:preferences" line: 81 column: 1 source: "%advancedDTD;"}]
[task 2017-06-10T05:50:35.724220Z] 05:50:35     INFO - Buffered messages finished
[task 2017-06-10T05:50:35.726403Z] 05:50:35     INFO - TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/browser/browser_privacyPreferences.js | Uncaught exception - TypeError: content.document.querySelector(...) is null
[task 2017-06-10T05:50:35.731845Z] 05:50:35     INFO - Leaving test bound test_aboutPreferences
Flags: needinfo?(timdream)
Comment hidden (mozreview-request)
Thank you for back it out. The syntax of the string in comment 14 was broken and I didn't realize it because I didn't build the code with my home laptop...
Flags: needinfo?(timdream)
Keywords: checkin-needed
Triggered landing to see if we can get this in m-c safely before merge day.
Keywords: checkin-needed

Comment 21

2 years ago
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/65e8eacd6f7c
Disable content process count UI and switch description for non-e10s users, r=jaws
https://hg.mozilla.org/mozilla-central/rev/65e8eacd6f7c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
See Also: → 1359735
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.