Closed
Bug 1289473
Opened 8 years ago
Closed 8 years ago
Accessibility+e10s restart prompt sometimes doesn't display
Categories
(Toolkit Graveyard :: Notifications and Alerts, defect)
Toolkit Graveyard
Notifications and Alerts
Tracking
(firefox47 unaffected, firefox48 wontfix, firefox49- verified, firefox50 verified)
VERIFIED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox47 | --- | unaffected |
firefox48 | --- | wontfix |
firefox49 | - | verified |
firefox50 | --- | verified |
People
(Reporter: jimm, Assigned: jimm)
References
Details
Attachments
(1 file, 2 obsolete files)
4.99 KB,
patch
|
jimm
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Darnit, I introduced this when I did a little code reorg in bug 1198459 (dd880f72d470). If there's a client running, getting the prompt is pretty random. Sometimes it works, sometimes it doesn't. Totally depends on how quickly the client requests a11y interfaces from us and how quickly we create the first browser window. Working on a patch that should make this reliable.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jmathies
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Assignee | ||
Comment 1•8 years ago
|
||
This patch makes a few changes - 1) Have E10SAccessibilityCheck start listening for the a11y observer events earlier. 2) If E10SAccessibilityCheck doesn't have a window to prompt with, it sets a flag indicating it needs to in the future. 3) Add a new callback on E10SAccessibilityCheck for window opened which checks the prompt flag and prompts if necessary.
Attachment #8774842 -
Flags: review?(felipc)
Comment 2•8 years ago
|
||
Comment on attachment 8774842 [details] [diff] [review] patch Review of attachment 8774842 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ +3027,5 @@ > > _showE10sAccessibilityWarning: function() { > // We don't prompt about a11y incompat if e10s is off. > + if (!Services.appinfo.browserTabsRemoteAutostart && > + !this._wantsPrompt) { why this extra check here? in which case would e10s be off but we still want to prompt users?
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to :Felipe Gomes (needinfo me!) from comment #2) > Comment on attachment 8774842 [details] [diff] [review] > patch > > Review of attachment 8774842 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/nsBrowserGlue.js > @@ +3027,5 @@ > > > > _showE10sAccessibilityWarning: function() { > > // We don't prompt about a11y incompat if e10s is off. > > + if (!Services.appinfo.browserTabsRemoteAutostart && > > + !this._wantsPrompt) { > > why this extra check here? in which case would e10s be off but we still want > to prompt users? If we get the a11y observer notification before a window is created, we will try to prompt again in the onWindowRestored handler. At that point Services.appinfo.browserTabsRemoteAutostart returns false due to the accessibility pref that gets set. To get into a state where _wantsPrompt is true, browserTabsRemoteAutostart has to be true at some earlier point since we set it down below this check. If browserTabsRemoteAutostart has always been false (e10s disabled from start) _wantsPrompt will also be false.
Assignee | ||
Comment 4•8 years ago
|
||
removing that second _wantsPrompt check since browserTabsRemoteAutostart is cached from the first call.
Attachment #8774842 -
Attachment is obsolete: true
Attachment #8774842 -
Flags: review?(felipc)
Attachment #8774917 -
Flags: review?(felipc)
Comment 5•8 years ago
|
||
Comment on attachment 8774917 [details] [diff] [review] patch v.2 Review of attachment 8774917 [details] [diff] [review]: ----------------------------------------------------------------- nit: please change the new function name from onWindowRestored to onWindowsRestored
Attachment #8774917 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 6•8 years ago
|
||
updated per comments.
Attachment #8774917 -
Attachment is obsolete: true
Attachment #8775144 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•8 years ago
|
||
[Tracking Requested - why for this release]: get this uplifted to 49 once it's baked a bit on nightly.
tracking-firefox49:
--- → ?
Updated•8 years ago
|
Component: General → Notifications and Alerts
Product: Firefox → Toolkit
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5172702dcd32 Ensure e10s+accessibility notice displays for a11y clients that constantly run. r=felipe
Keywords: checkin-needed
Pushed by jmathies@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/855b04e6a374 Accessibility+e10s prompt - follow up to add missing review nit changes. r=felipe
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Pulsebot from comment #9) > Pushed by jmathies@mozilla.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/855b04e6a374 > Accessibility+e10s prompt - follow up to add missing review nit changes. > r=felipe ^ review nits, missed a qref.
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5172702dcd32 https://hg.mozilla.org/mozilla-central/rev/855b04e6a374
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8775144 [details] [diff] [review] patch v.3 Approval Request Comment [Feature/regressing bug #]: bug 1198459 [User impact if declined]: flaky e10s+accessibility prompt [Describe test coverage new/current, TreeHerder]: On nightly and aurora for a while [Risks and why]: low, no bugs reported [String/UUID change made/needed]: none
Attachment #8775144 -
Flags: approval-mozilla-beta?
Comment 13•8 years ago
|
||
Comment on attachment 8775144 [details] [diff] [review] patch v.3 On m-c without problems, sounds like a good idea for a11y users. This should make it into beta 4 next week.
Attachment #8775144 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 14•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f3beb0f28c64
Updated•8 years ago
|
Flags: qe-verify+
Updated•8 years ago
|
Comment 15•8 years ago
|
||
Confirming this fix using a Microsoft Surface Pro 2 device running latest Windows 10 x64 (Anniversary release). Testing was performed using latest 50.0a2 Aurora (build ID 20160906004000) and Fx 49.0 RC (build ID 20160905130425).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: cornel.ionce
Updated•9 months ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•