By using a different opt-out pref each time we enable e10s on Nightly, we can re-enable e10s for people who opted out of the last time we enabled e10s.

Chris and I just discussed this on IRC. We worry that this might be overly complex and not that much needed. Instead, when we turn on tabs.remote.autostart on nightly, we can do so in a #ifdef in the prefs js file, changing the default value of the pref, so that people manually setting that pref to false would override that and get no e10s. It seems that's all we need i.e. nothing more needs to be done here. Correct?

+1

This bug should be fixed for the e10s M2 milestone.

If we just change the default pref, we have to worry about the opposite problem: users who are currently opted-in to remote.autostart will lose their user-set pref when we set that pref to true by default, and then when we set it to false again they will revert to having e10s off. We don't really want that. I think we should switch the code to check for remote.autostart || remote.autostart.flip1, then set flip1 to true when we want to enable by default. Then we disable by removing the flip1 default pref, and change the code to check flip2 and set flip2=true the next time we want to toggle it on again. That way we can keep flipping on/off without affecting existing e10s users, and without people being able to opt-out permanently too easily.

To make this change, the checkbox in the prefs dialog becomes a little tricky, because we need to decide which pref the user is trying to control by checking/unchecking it. Here's what I think the behavior should be: A) If checkbox gets checked => It means that the user is manually opting in to e10s, so we set .remote.autostart => true B) If checkbox gets unchecked, and .remote.autostart was false => It means that the user was put in e10s by remote.autostart.1, so we set .remote.autostart => false C) If checkbox gets unchecked, and .remote.autostart was true => It means that the user had previously opted in to e10s, but wants to opt out now. We need to set .remote.autostart => false, and also .remote.autostart.1 => false, in case it is in a period where we are doing the temporary testing. Note that case B and C both end with both prefs as false, so we can simplify and merge them in the code. This is what I implemented with this patch.

Case B, obviously I mean: we set .remote.autostart.1 => false

Actually it's better not to blindly set remote.autostart.1 to false unless it was true, to not prematurely opt-out users of trial #1.

This goes against convention of matching the id to the preference name, but if we make the temp pref's <preference> ID be just "e10sTempPref" or some such, then we have fewer lines to patch when changing the temp pref to .2, .3, etc. At a conference so I'm not sure I'll be able to review this quickly, perhaps mconley can help?

(The logic described in comment 6/comment 7/comment 8 makes sense to me!)

Good idea, updated with the suggestion in comment 9

Comment on attachment 8489476 [details] [diff] [review] E10s opt out, v3 >diff --git a/browser/components/preferences/in-content/main.js b/browser/components/preferences/in-content/main.js >- e10sCheckbox.checked = e10sPref.value; >+ let e10sTempPref = document.getElementById("e10sTempPref"); >+ e10sCheckbox.checked = e10sPref.value || e10sTempPref.value; It would be cleaner to expose BrowserTabsRemoteAutostart to JS (if it isn't already?) and just use that here, I suppose. But this works for now. > enableE10SChange: function () >+ let prefsToChange; >+ // e10sCheckbox.checked means we're entering this function in the >+ // transition unchecked -> checked. Not a big deal but I'd find this easier to understand with just "// enabling e10s autostart" and "// disabling e10s autostart" comments in each block. > if (shouldProceed) { >- e10sPref.value = e10sCheckbox.checked; >+ for (prefToChange of prefsToChange) { Missing a "let"? >- Services.startup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); >+ // Services.startup.quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart); Oops Looks good otherwise, but I haven't tested it extensively.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #12) > >- e10sCheckbox.checked = e10sPref.value; > >+ let e10sTempPref = document.getElementById("e10sTempPref"); > >+ e10sCheckbox.checked = e10sPref.value || e10sTempPref.value; > > It would be cleaner to expose BrowserTabsRemoteAutostart to JS (if it isn't > already?) and just use that here, I suppose. But this works for now. It is exposed (through Services.appinfo.browserTabsRemoteAutostart), but it includes more checks (like if we are in safe mode), and we need the checkbox to only reflect the state of the prefs, without those extra checks (so if you enter in Safe mode you can still properly uncheck this pref if e10s is causing problems)

Marco, can you add this to the current iteration?

Added to IT 35.2 (In reply to :Felipe Gomes from comment #15) > Marco, can you add this to the current iteration?