Chris Peterson [:cpeterson] Reporter
	Description • 10 years ago

      No description provided.

	Chris Peterson [:cpeterson] Reporter
	Comment 1 • 10 years ago

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.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 2 • 10 years ago

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?

	Chris Peterson [:cpeterson] Reporter
	Comment 3 • 10 years ago

+1

	Chris Peterson [:cpeterson] Reporter
	Comment 4 • 10 years ago

This bug should be fixed for the e10s M2 milestone.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 10 years ago

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.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 6 • 10 years ago

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.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 7 • 10 years ago

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

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 8 • 10 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 10 years ago

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?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 10 • 10 years ago

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

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 11 • 10 years ago

Good idea, updated with the suggestion in comment 9

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 12 • 10 years ago

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.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 13 • 10 years ago

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

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 14 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/bc5cc000624d

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 15 • 10 years ago

Marco, can you add this to the current iteration?

	Marco Mucci [:MarcoM]
	Comment 16 • 10 years ago

Added to IT 35.2

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/bc5cc000624d