Closed Bug 1094409 Opened 7 years ago Closed 7 years ago

clarify prefs UI when e10s is disabled for whatever reason

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
e10s + ---

People

(Reporter: Gavin, Assigned: Gavin)

Details

Attachments

(1 file, 2 obsolete files)

The prefs UI when e10s is blocked (or disabled because of tracking protection, bug 1088904) still allows you to enabled the checkbox, but this doesn't have any effect (when you restart the checkbox is checked but e10s is still disabled).

We should instead disable the checkbox in these cases, and ideally explain why it can't be enabled.
Attached patch patch (obsolete) — Splinter Review
This makes the checkbox reflect the actual state of e10s at all times, and further disables it if e10s is disabled in a way that the user can't override. It also appends a hardcoded en-US string to the checkbox label to explain why it might be disabled (in general, it doesn't provide the specific reason).

I used nsIObserver as a hacky way to expose the "e10s was blocked" bit from nsXULAppInfo, to avoid modifying nsIXULAppInfo/nsIXULRuntime just for this.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #8517689 - Flags: review?(felipc)
Will add an #ifdef NIGHTLY_BUILD around the nsXULAppInfo::Observe implementation.
I was thinking about this today! If you don't want to "pollute" the implementation of nsAppRunner, an alternative that I was thinking about is to use an ENV var that LogE10SBlockedReason can set, and main.js can check for its existence. With this you could even pass the string with the proper explanation.
Comment on attachment 8517689 [details] [diff] [review]
patch

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

But if you prefer this way, this patch is fine
Attachment #8517689 - Flags: review?(felipc) → review+
Nit: actually there's a case here that needs special handling. If e10s is enabled (through one of the prefs), but blocked because we're in safe mode, the checkbox should still be checked and not disabled. The idea is that if something is badly broken, the user should be able to enter Safe mode and uncheck the checkbox.
Attached patch Alternate patch (obsolete) — Splinter Review
Here's my alternative suggestion. See what you think of this approach.
Example img: http://i.imgur.com/4Iz6DVZ.png

One thing that I changed from yours is that I kept the checkbox reflecting the state of the pref. I think it should reflect the pref at all times to be clear. I believe that it being disabled (grayed out), plus the explanation test makes it clear that e10s is not actually on. But I can change this if you think otherwise.
Attachment #8517819 - Flags: review?(gavin.sharp)
Attached patch third patchSplinter Review
Felipe and I midaired on this patch authorship. His is simpler, but I wrote some code to avoid using an environment variable for this, so I guess we might as well use it.
Attachment #8517689 - Attachment is obsolete: true
Attachment #8517819 - Attachment is obsolete: true
Attachment #8517819 - Flags: review?(gavin.sharp)
Attachment #8517872 - Flags: review?(wmccloskey)
Attachment #8517872 - Flags: review?(felipc)
(In reply to :Felipe Gomes from comment #7)
> One thing that I changed from yours is that I kept the checkbox reflecting
> the state of the pref. I think it should reflect the pref at all times to be
> clear. I believe that it being disabled (grayed out), plus the explanation
> test makes it clear that e10s is not actually on. But I can change this if
> you think otherwise.

I think I disagree - having a checked, disabled checkbox combined with a "disabled because:" message seems confusing.
Attachment #8517872 - Flags: review?(wmccloskey)
Attachment #8517872 - Flags: review?(felipc)
Attachment #8517872 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/789381d7270b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.