Closed
Bug 1094409
Opened 10 years ago
Closed 10 years ago
clarify prefs UI when e10s is disabled for whatever reason
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 36
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: Gavin, Assigned: Gavin)
Details
Attachments
(1 file, 2 obsolete files)
7.44 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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 | ||
Comment 2•10 years ago
|
||
Screenshot: https://cloudup.com/cdjwrVoLpue
Assignee | ||
Comment 3•10 years ago
|
||
Will add an #ifdef NIGHTLY_BUILD around the nsXULAppInfo::Observe implementation.
Comment 4•10 years ago
|
||
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•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
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•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8517872 -
Flags: review?(wmccloskey)
Attachment #8517872 -
Flags: review?(felipc)
Attachment #8517872 -
Flags: review+
Comment 10•10 years ago
|
||
Updated•10 years ago
|
tracking-e10s:
--- → +
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in
before you can comment on or make changes to this bug.
Description
•