prompt Nightly users to enable e10s permanently for testing

RESOLVED FIXED in Firefox 35

Status

()

Firefox
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 35
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite -
qe-verify -

Firefox Tracking Flags

(e10sm2+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Goal: get more testing of E10S

Proposal: prompt users on Nightly to enable e10s completely (all windows are e10s all the time) with a doorhanger. I'm thinking it should look something like:

"Help us test e10s! _Learn More_ [Enable and Restart]"

Under the covers, this would just set the autostart pref to true, and restart.

We should find somewhere to point the "Learn More" link to.
Bug 1064886 provides a way for users who've accepted this prompt to easily disable it later. We could mention that on the "Learn more" page, and maybe even notify users who've enabled it this way that they can undo the opt-in.
In bug 1063842 comment 3, Dolske makes a point for using the global notification instead of a doorhanger. I think that's also something to consider for this one.
Created attachment 8486432 [details] [diff] [review]
patch

This doesn't implement the "Learn more" link, and the string/styling leave a little to be desired, but it's most of what I'd want. This prompts Nightly users on startup with a doorhanger. There's a limit of 5 prompts before we give up showing it.

Philipp, with those caveats in mind, what do you think? Any concerns about the idea in general? Would you prefer something not-a-doorhanger?

Screenshot: https://cloudup.com/cm-qhh8gFU1
Attachment #8486432 - Flags: ui-review?(philipp)
Attachment #8486432 - Flags: feedback?(dtownsend+bugmail)
(In reply to :Felipe Gomes from comment #2)
> In bug 1063842 comment 3, Dolske makes a point for using the global
> notification instead of a doorhanger. I think that's also something to
> consider for this one.

It's a good point, but I think notification bars are not noticeable enough. That's a separate broader problem to fix, certainly, but probably something we need to workaround in the short term here somehow.
Comment on attachment 8486432 [details] [diff] [review]
patch

Review of attachment 8486432 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me. Showing this on every startup might be a bit much, maybe we should show every day?

Once e10s has been enabled should we ramp the prompt counter pref up to 10 so if they choose to disable they don't start getting prompted immediately?

::: browser/components/nsBrowserGlue.js
@@ +810,5 @@
> +          Services.prefs.setIntPref("browser.displayedE10SPrompt", e10sPromptShownCount + 1);
> +        } catch (ex) {
> +          Cu.reportError("Failed to show e10s prompt: " + ex);
> +        }
> +        

Whitespace nit
Attachment #8486432 - Flags: feedback?(dtownsend+bugmail) → feedback+
Created attachment 8486576 [details]
e10s icons

Two issues here:

- I think even among Nightly users »e10s« is jargony. How about changing the message to:
»Would you like to help test multiprocess Nightly?«

- Can we use an icon in that doorhanger just to make things look balanced? I re-purposed one of shorlanders icons in the attachment.
Unverified, but I imagine that this will need to be skipped during tests, otherwise this doorhanger showing by default on b-c might cause tests to fail.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #4)
> (In reply to :Felipe Gomes from comment #2)
> > In bug 1063842 comment 3, Dolske makes a point for using the global
> > notification instead of a doorhanger. I think that's also something to
> > consider for this one.
> 
> It's a good point, but I think notification bars are not noticeable enough.
> That's a separate broader problem to fix, certainly, but probably something
> we need to workaround in the short term here somehow.

OTOH, doorhangers are only attached to one tab and dismiss easily. So even if you suffer from a bit of initial notification bar blindness, you've got a chance to eventually notice. It's also the proper UI for a global browser notification (vs doorhangers, which are site-specific). Though I'll admit the latter isn't a major issue if this is only a short-term hack for Nightly.
Chris: I'm thinking of making the "Learn More" link link to https://wiki.mozilla.org/Electrolysis, since it covers most of what we want. We might want to make a few tweaks to that page:
- adjust the "how to enable" text given that I will land this with bug 1064886
- maybe elaborate on "why we need your help" in a prominent location
- I wonder whether it might be better to move https://etherpad.mozilla.org/e10s-known-issues off an etherpad and link it more prominently, to make it easier to avoid vandalism

Can you help update the page to take those into account?
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Flags: needinfo?(cpeterson)
(In reply to Justin Dolske [:Dolske] from comment #8)
> OTOH, doorhangers are only attached to one tab and dismiss easily. So even
> if you suffer from a bit of initial notification bar blindness, you've got a
> chance to eventually notice. It's also the proper UI for a global browser
> notification (vs doorhangers, which are site-specific). Though I'll admit
> the latter isn't a major issue if this is only a short-term hack for Nightly.

Fair point re: persistence. Doorhangers are easier to set up (icon and "learn more" link built-in), and I already wrote this patch, so let's go with this for now and we can iterate if needed. This is definitely a short-term hack for Nightly.
(In reply to Philipp Sackl [:phlsa] from comment #6)
> - I think even among Nightly users »e10s« is jargony. How about changing the
> message to:
> »Would you like to help test multiprocess Nightly?«

I went with:
"Would you like to help us test multiprocess Nightly (e10s)? You can also enable e10s in Nightly preferences."

(the latter in case the user clicks the "Learn More" link, which dismisses the doorhanger, and also to explain how to undo it afterwards)

> - Can we use an icon in that doorhanger just to make things look balanced? I
> re-purposed one of shorlanders icons in the attachment.

Yes! Thanks.

(In reply to :Felipe Gomes from comment #7)
> Unverified, but I imagine that this will need to be skipped during tests,

Good catch! I will add the relevant pref to the test profile.
(In reply to Dave Townsend [:mossop] from comment #5)
> Looks ok to me. Showing this on every startup might be a bit much, maybe we
> should show every day?

I'm OK with being slightly annoying :) I will add a "No thanks" secondary action that sets the pref to 5, though.

> Once e10s has been enabled should we ramp the prompt counter pref up to 10
> so if they choose to disable they don't start getting prompted immediately?

Good idea, though a reminder to re-enable later could be useful as well. I with the "No thanks" the annoyingness is manageable, so I'm not going to do this.
Created attachment 8487191 [details] [diff] [review]
updated patch

In addition to the comments above, I also added "persistWhileVisible" to the notification options, to avoid the doorhanger being dismissed by the first navigation in the new window (if any).

https://tbpl.mozilla.org/?tree=Try&rev=24217f336b79
Attachment #8486432 - Attachment is obsolete: true
Attachment #8486432 - Flags: ui-review?(philipp)
Created attachment 8487196 [details] [diff] [review]
updated patch

I forgot to wrap things in the E10S_TESTING_ONLY ifdef, did that now. Felipe, can you offer final r+ here, given that Dave is out?
Attachment #8486576 - Attachment is obsolete: true
Attachment #8487191 - Attachment is obsolete: true
Attachment #8487196 - Flags: review?(felipc)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #9)
> https://wiki.mozilla.org/Electrolysis, since it covers most of what we want.
> We might want to make a few tweaks to that page:
> - adjust the "how to enable" text given that I will land this with bug
> 1064886
> - maybe elaborate on "why we need your help" in a prominent location
> - I wonder whether it might be better to move
> https://etherpad.mozilla.org/e10s-known-issues off an etherpad and link it
> more prominently, to make it easier to avoid vandalism

Some other things to add:
- how to report a bug/search for dupes?
Another thing I forgot in that patch was to define E10S_TESTING_ONLY in each of the 3 theme moz.build files - fixed that locally.
Attachment #8487196 - Flags: review?(felipc) → review+
* I think the prompt should tell users how they can *disable* e10s (in Nightly preferences).

* I will clean up the wiki page pointed to by the "Learn More" link.
Flags: needinfo?(cpeterson)
Gavin, we might want to skip the e10s prompt if the browser.tabs.remote pref is false. This will allow tests (like mozmill bug 1065436) to set the pref so they can opt-out of the opt-in prompt (to avoid test problems).
Created attachment 8487549 [details] [diff] [review]
Inside E10SUINotification

Since we are adding similar code going in the same place (from bug 1063842), I took the liberty to do a small code move to put everything inside one #ifdef E10S_TESTING_ONLY block. I also included here the part that you sent me on pastebin that includes the moz.build changes. I tested this new patch now that bug 1063842 has landed and it worked properly. But I did not address comment 18.

Your original patch should still apply easily, so feel free to go with the former if you prefer
(In reply to Chris Peterson (:cpeterson) from comment #17)
> * I think the prompt should tell users how they can *disable* e10s (in
> Nightly preferences).

I think that's implied by "you can enable in preferences", but I will rephrase to "You can enable/disable in Nightly preferences."

> * I will clean up the wiki page pointed to by the "Learn More" link.

Thanks!
(In reply to Chris Peterson (:cpeterson) from comment #18)
> Gavin, we might want to skip the e10s prompt if the browser.tabs.remote pref
> is false. This will allow tests (like mozmill bug 1065436) to set the pref
> so they can opt-out of the opt-in prompt (to avoid test problems).

Does mozmill not use prefs_general.js? I've already disabled the prompting entirely there in my patch.
Flags: needinfo?(hskupin)
https://hg.mozilla.org/integration/fx-team/rev/15204230df1d
Iteration: --- → 35.1
Points: --- → 3
tracking-e10s: --- → ?
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
Target Milestone: --- → Firefox 35
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #21)
> Does mozmill not use prefs_general.js? I've already disabled the prompting
> entirely there in my patch.

No, it doesn't use this pref file given that it is located outside of the tree. For now we have set all the prefs we really need via mozrunner / mozprofile. There is an ongoing initiative to sync up the mentioned prefs_general.js with Mozmill, but we would like to not add too much of custom prefs, which will put us further away from what users actually seeing. Bug 905400 was not a high priority for us so far, but maybe we should re-prioritize it.

On the other side please note that Mozmill does NOT support e10s yet! Because of that we have to disable e10s via bug 1065436 for testruns with Mozmill. In the next couple of weeks we will put our full energy in adding e10s support (bug 695026). That means when we are going to enable e10s permanently we should be ready, hopefully even earlier.
Flags: needinfo?(hskupin)
FWIW, we're likely to enable e10s on trunk temporarily sooner than "a couple of weeks".
That's why bug 1065436 exists so that we do not allow e10s to be enabled until Mozmill has support for it. Only that way we will be able to continue our testing for Nightly builds.
https://hg.mozilla.org/mozilla-central/rev/15204230df1d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
tracking-e10s: ? → m2+
Depends on: 1066573
Depends on: 1066588
Blocks: 997462

Updated

3 years ago
QA Contact: jbecerra
This bug has already gone on Nightly and various users reported that it showed up for them.. I don't think it needs QE verification
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.