Closed Bug 1368943 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

(Depends on 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(2 files, 1 obsolete file)

Bug 1368943 - Disable content process count UI and switch description for non-e10s users,
59 bytes, text/x-review-board-request
jaws
: review+
Details
Screen Shot 2017-06-09 at 3.39.14 PM.png
461.21 KB, image/png
Details 
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)
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
Attached image Screen Shot 2017-06-09 at 3.03.02 PM.png (obsolete) — 
This is what a non-e10s user will see with this patch.
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
(In reply to Francesco Lodolo [:flod] from comment #10)

Sorry about making the silly mistake :-/
Attachment #8875992 - Flags: review?(jaws)
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+
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)
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
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: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
See Also: → 1359735
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: