Closed
Bug 1076314
Opened 10 years ago
Closed 10 years ago
Re-prompt nightly users to enable e10s
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
Tracking | Status | |
---|---|---|
e10s | m4+ | --- |
People
(Reporter: mossop, Assigned: Felipe)
Details
Attachments
(2 files)
Now that we've fixed more bugs it is time to re-prompt nightly users to try e10s again. We should probably include some new wording in the prompt to talk about some of the things we've fixed since the last time (crashers, pdfjs, ... ideas?) so users who disabled it before might be more tempted to try again.
Flags: firefox-backlog+
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Updated•10 years ago
|
Flags: qe-verify?
Comment 1•10 years ago
|
||
I still see the existing prompt every time I ./mach run (creating a new profile by default). Can we fix that when changing this new prompt? It's really annoying.
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Flags: qe-verify? → qe-verify-
Comment 2•10 years ago
|
||
I hope we fix bug 1072980 first, I tried enabling E10s and my browser entered a perma-crash situation, forcing me to manually edit prefs.js to disable E10s.
Comment 3•10 years ago
|
||
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> I hope we fix bug 1072980 first, I tried enabling E10s and my browser
> entered a perma-crash situation, forcing me to manually edit prefs.js to
> disable E10s.
ah looks like it's due to forceRTL add-on, so, likely not a real blocker.
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> I hope we fix bug 1072980 first, I tried enabling E10s and my browser
> entered a perma-crash situation, forcing me to manually edit prefs.js to
> disable E10s.
Fwiw, you can use Safe mode which disables e10s and then you can edit the pref or uncheck the checkbox in the preferences dialog.
Comment 5•10 years ago
|
||
(In reply to :Felipe Gomes from comment #4)
> Fwiw, you can use Safe mode which disables e10s and then you can edit the
> pref or uncheck the checkbox in the preferences dialog.
yeah, but I was a little bit upset and forgot about the shift+launch option :)
Comment 6•10 years ago
|
||
Hi Felipe, can you provide a point value.
Iteration: --- → 35.3
Flags: needinfo?(felipc)
Assignee | ||
Comment 7•10 years ago
|
||
What do people think of the simple approach of showing the list of enhancements in the popup?
Chris, what should we fill this list with?
feedbacking Madhava but anyone who cares to chime in, please do
Attachment #8501914 -
Flags: feedback?(madhava)
Flags: needinfo?(cpeterson)
Comment 8•10 years ago
|
||
I think it's a good idea because it will give people who opted-out a reason to try e10s again.
Flags: needinfo?(cpeterson)
Assignee | ||
Updated•10 years ago
|
Points: --- → 3
Flags: needinfo?(felipc)
Assignee | ||
Comment 9•10 years ago
|
||
This improves on the previous code as the following:
- easier to bump number for other rounds of re-prompting, and it will clean up after the previous pref
- won't display for local builds as requested in comment 1 (I couldn't use OFFICIAL_BRANDING check because that's also false for Nightly, so I used the update channel)
- added check for hardware acceleration so it's not gonna be offered for users who would be blocked from running e10s
The message param for doorhangers doesn't accept multi-line text, so we need to use the popupnotificationcontent feature to add more elements. I can't create that element dinamically because when the popup is dismissed, it would be destroyed, and not show up again when retrieved from the URLbar icon. So I had to add it to popup-notifications.inc.
Note that with this patch I'm also activating the notice added in bug 1063842. It will show up once for every user running with e10s autostart.
Attachment #8501935 -
Flags: review?(mconley)
Attachment #8501935 -
Flags: review?(dtownsend+bugmail)
Comment 10•10 years ago
|
||
Comment on attachment 8501935 [details] [diff] [review]
Re-prompt patch
Review of attachment 8501935 [details] [diff] [review]:
-----------------------------------------------------------------
This looks OK to me, but can I see a screenshot of the popup?
::: browser/components/nsBrowserGlue.js
@@ +2272,5 @@
> // Increase this number each time we want to roll out an
> // e10s testing period to Nightly users.
> + CURRENT_NOTICE_COUNT: 1,
> + CURRENT_PROMPT_PREF: "browser.displayedE10SPrompt.1",
> + PREVIOUS_PROMPT_PREF: "browser.displayedE10SPrompt",
As these will probably stack up, should this be an array of old prompt prefs?
Assignee | ||
Comment 11•10 years ago
|
||
The screenshot is attached :) attachment 8501914 [details]
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> ::: browser/components/nsBrowserGlue.js
> @@ +2272,5 @@
> > // Increase this number each time we want to roll out an
> > // e10s testing period to Nightly users.
> > + CURRENT_NOTICE_COUNT: 1,
> > + CURRENT_PROMPT_PREF: "browser.displayedE10SPrompt.1",
> > + PREVIOUS_PROMPT_PREF: "browser.displayedE10SPrompt",
>
> As these will probably stack up, should this be an array of old prompt prefs?
I was planning on just clearing the immediate previous pref, not all the past ones. We don't even need to do it, but it's just a bit nicer to not leave these prefs poluting the profile.
Comment 12•10 years ago
|
||
Comment on attachment 8501935 [details] [diff] [review]
Re-prompt patch
Ah, yes - somehow missed the screenshot there. My bad.
(In reply to :Felipe Gomes from comment #11)
> The screenshot is attached :) attachment 8501914 [details]
>
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> > ::: browser/components/nsBrowserGlue.js
> > @@ +2272,5 @@
> > > // Increase this number each time we want to roll out an
> > > // e10s testing period to Nightly users.
> > > + CURRENT_NOTICE_COUNT: 1,
> > > + CURRENT_PROMPT_PREF: "browser.displayedE10SPrompt.1",
> > > + PREVIOUS_PROMPT_PREF: "browser.displayedE10SPrompt",
> >
> > As these will probably stack up, should this be an array of old prompt prefs?
>
> I was planning on just clearing the immediate previous pref, not all the
> past ones. We don't even need to do it, but it's just a bit nicer to not
> leave these prefs poluting the profile.
Right, and I'm all for un-polluting user profiles where we can. The scenario I'm thinking of is that a user enables e10s, and then decides to change to a different channel (or doesn't use their Nightly) for some time so that we skip clearing one or more prefs. It's not a big deal, but it'd be nice to maybe clean-up where we can.
Otherwise, I thinks this looks good as a E10S_TESTING_ONLY sorta thing.
Attachment #8501935 -
Flags: review?(mconley) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8501914 [details]
Listing enhancements
I'm OK with this, but let's
- keep A, B, and C short (one line each)
- and no more than three -- possibly fewer?
And this _isn't_ a precedent for starting to do this in release.
Attachment #8501914 -
Flags: feedback?(madhava) → feedback+
Comment 14•10 years ago
|
||
Felipe: here is a list of bugs fixed since we first prompted Nightly users to test e10s on November 12:
http://is.gd/FBsTJW
Three notable fixes:
* Improved add-on compatibility
* Fixed many Developer Tools bugs
* Fixed spell checker
I don't think we should include bug numbers because these are meta-issues that include many fixes.
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #12)
> Comment on attachment 8501935 [details] [diff] [review]
> Re-prompt patch
>
> Ah, yes - somehow missed the screenshot there. My bad.
>
> (In reply to :Felipe Gomes from comment #11)
> > The screenshot is attached :) attachment 8501914 [details]
> >
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #10)
> > > ::: browser/components/nsBrowserGlue.js
> > > @@ +2272,5 @@
> > > > // Increase this number each time we want to roll out an
> > > > // e10s testing period to Nightly users.
> > > > + CURRENT_NOTICE_COUNT: 1,
> > > > + CURRENT_PROMPT_PREF: "browser.displayedE10SPrompt.1",
> > > > + PREVIOUS_PROMPT_PREF: "browser.displayedE10SPrompt",
> > >
> > > As these will probably stack up, should this be an array of old prompt prefs?
> >
> > I was planning on just clearing the immediate previous pref, not all the
> > past ones. We don't even need to do it, but it's just a bit nicer to not
> > leave these prefs poluting the profile.
>
> Right, and I'm all for un-polluting user profiles where we can. The scenario
> I'm thinking of is that a user enables e10s, and then decides to change to a
> different channel (or doesn't use their Nightly) for some time so that we
> skip clearing one or more prefs. It's not a big deal, but it'd be nice to
> maybe clean-up where we can.
Yeah, good point.. I'll postpone doing this the next time when there's a second pref to clean-up :)
Assignee | ||
Comment 16•10 years ago
|
||
I combined everyone's suggestions and landed the final text as:
Would you like to help us test multiprocess Nightly (e10s)? You can also enable e10s in Nightly preferences. Notable fixes:
Less crashing!
Improved add-on compatibility and DevTools
PDF.js, Web Console, Spellchecking, WebRTC now work
Assignee | ||
Updated•10 years ago
|
Attachment #8501935 -
Flags: review?(dtownsend+bugmail)
Assignee | ||
Comment 17•10 years ago
|
||
This merged to m-c, and then I backed this out in https://hg.mozilla.org/mozilla-central/rev/f5557f04db0b at Felipe's request so it doesn't ship in a Nightly on Friday or over the weekend.
Assignee | ||
Comment 19•10 years ago
|
||
Relanded for showing up on Monday's Nightly: https://hg.mozilla.org/integration/mozilla-inbound/rev/facebebfea4d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in
before you can comment on or make changes to this bug.
Description
•