Closed Bug 1164640 Opened 9 years ago Closed 9 years ago

[e10s] Reset the a11y disabled flag


(Firefox :: General, defect)

Not set



Firefox 41
Tracking Status
e10s m7+ ---
firefox40 --- fixed
firefox41 --- fixed


(Reporter: jimm, Assigned: Felipe)



(1 file)

Lets get our first prompt on aurora going. In this work we should also reset the a11y disabled flag on both mc and aurora.
The prompt was already enabled on aurora in bug 1161260 (see 3rd patch), and users should see it when aurora updates to 40 (it hasn't been published yet afaict).

So the only thing to do is to reset the a11y flag on m-c. No need to do it on Aurora because approximately no one should have seen it on aurora as e10s was not supported before.
Ah great, thanks felipe! Morphing.
Summary: [e10s][1st] Prompt to enable e10s on Aurora → [e10s] Reset the a11y disabled flag
Assignee: nobody → felipc
Attached patch New a11y flagSplinter Review
This does a few things:

- rename the flag, in practice making it reset the value for all users
- fixes a typo I introduced in bug 1161260. s/UpdateServices/Services/
- improves the handling of this flag when we decide to show the warning: Previously, we were treating both "absense of pref" and "pref set to false" as the same thing, so there was no way for someone to prevent the warning from keeping showing up if they want to remain with e10s enabled. We offer an option for "Don't disable" on the popup, so effectively we were ignoring it.

I mentioned in comment 1 that it wouldn't be necessary to uplift this, but with the typo fix and the improved handling I think this will be a good candidate for Aurora as well.
Attachment #8607163 - Flags: review?(jmathies)
Attachment #8607163 - Flags: review?(mconley)
Comment on attachment 8607163 [details] [diff] [review]
New a11y flag

Review of attachment 8607163 [details] [diff] [review]:

r=me with an optional suggestion.

::: browser/components/nsBrowserGlue.js
@@ +2817,5 @@
>        let updateChannel = UpdateChannel.get();
>        let channelAuthorized = updateChannel == "nightly" || updateChannel == "aurora";
>        skipE10sChecks = !channelAuthorized ||
> +                       Services.prefs.getBoolPref("browser.tabs.remote.disabled-for-a11y");

Yikes! Yes, let's get that uplifted.

@@ +2995,5 @@
>    _showE10sAccessibilityWarning: function() {
> +    let prefExistsAndIsFalse = false;
> +    try {
> +      prefExistsAndIsFalse = Services.prefs.getBoolPref("browser.tabs.remote.disabled-for-a11y") == false;

We only ever use prefExistsAndIsFalse up here, so can't we just keep it in the scope of the try? We also get to avoid this kinda weird composite boolean thing at the same time:

try {
  if (Services.prefs.getBoolPref("browser.tabs.remote.disabled-for-a11y")) {
    // We should have thrown if the pref did not exist
} catch(e) {
  // The pref did not exist yet, so we'll continue.
Attachment #8607163 - Flags: review?(mconley) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment on attachment 8607163 [details] [diff] [review]
New a11y flag

Review of attachment 8607163 [details] [diff] [review]:

what happens to the old pref?
Attachment #8607163 - Flags: review?(jmathies) → review+
Comment on attachment 8607163 [details] [diff] [review]
New a11y flag

Approval Request Comment
[Feature/regressing bug #]: allow aurora users with non-blacklisted a11y tools to enable e10s.
[User impact if declined]: 
[Describe test coverage new/current, TreeHerder]: Code is active on Nightly
[Risks and why]: 
[String/UUID change made/needed]: none
Attachment #8607163 - Flags: approval-mozilla-aurora?
(In reply to :Felipe Gomes from comment #8)
> [User impact if declined]: 
> [Risks and why]: 
Could you fill these two items? Thanks
Flags: needinfo?(felipc)
(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> (In reply to :Felipe Gomes from comment #8)

> > [User impact if declined]: This has a typo fix that would make us offer to turn e10s on even for users who have a blacklisted accessibility tool.

> > [Risks and why]: This is running in central, I think it is fairly safe. The risk would be contained to the offer popups of e10s.
Flags: needinfo?(felipc)
Attachment #8607163 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.