1094409 - clarify prefs UI when e10s is disabled for whatever reason

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

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.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Screenshot: https://cloudup.com/cdjwrVoLpue

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Will add an #ifdef NIGHTLY_BUILD around the nsXULAppInfo::Observe implementation.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

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 5

[:Felipe Gomes (needinfo for replies!)]

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

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Alternate patch

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.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: third patch

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.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/789381d7270b

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/789381d7270b