Closed
Bug 1064885
Opened 10 years ago
Closed 10 years ago
prompt Nightly users to enable e10s permanently for testing
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
Tracking | Status | |
---|---|---|
e10s | m2+ | --- |
People
(Reporter: Gavin, Assigned: Gavin)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
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.
Comment 7•10 years ago
|
||
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.
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
(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?
Assignee | ||
Comment 16•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8487196 -
Flags: review?(felipc) → review+
Comment 17•10 years ago
|
||
* 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)
Comment 18•10 years ago
|
||
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).
Comment 19•10 years ago
|
||
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
Assignee | ||
Comment 20•10 years ago
|
||
(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!
Assignee | ||
Comment 21•10 years ago
|
||
(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)
Assignee | ||
Comment 22•10 years ago
|
||
Iteration: --- → 35.1
Points: --- → 3
tracking-e10s:
--- → ?
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
Target Milestone: --- → Firefox 35
Comment 23•10 years ago
|
||
(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)
Assignee | ||
Comment 24•10 years ago
|
||
FWIW, we're likely to enable e10s on trunk temporarily sooner than "a couple of weeks".
Comment 25•10 years ago
|
||
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.
Comment 26•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Blocks: old-e10s-m2
Updated•10 years ago
|
QA Contact: jbecerra
Comment 27•10 years ago
|
||
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.
Description
•