Jim Mathies [:jimm] Assignee
	Description • 9 years ago

We will likely roll e10s out in advance of having good a11y support. Due to this we will need a blacklist of clients we disable e10s for. This bug covers this work.

	Jim Mathies [:jimm] Assignee
	Comment 1 • 9 years ago

Hey David,

I need some help here deciding what we prompt to disable for. With this patch I prompt for every client we can detect UIAUTOMATION and of course UNKNOWN. Both of which we spoke about the other day.

On Mac and linux I'm not sure what to do. With this patch if a11y is found to be active we prompt to disable.

Does this seem right?

	Jim Mathies [:jimm] Assignee
	Comment 2 • 9 years ago

> I need some help here deciding what we prompt to disable for. With this
> patch I prompt for every client we can detect UIAUTOMATION and of course
> UNKNOWN. Both of which we spoke about the other day.


to clarify - I prompt to disable e10s for every client we can detect except UIAUTOMATION and of course UNKNOWN.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 3 • 9 years ago

I think this plan makes sense.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 4 • 9 years ago

Trevor am I correct we need a bit more work to make things like focus events work etc before removing the opt-out for OTHER?

	Jim Mathies [:jimm] Assignee
	Comment 5 • 9 years ago

Comment on attachment 8599483 [details] [diff] [review]
wip

might as well kick this off.

	Jim Mathies [:jimm] Assignee
	Comment 6 • 9 years ago

I tested this with JAWS and with Inspect.exe. For the former we prompted, for the latter we did not.

	Trevor Saunders (:tbsaunde)
	Comment 7 • 9 years ago

Comment on attachment 8599483 [details] [diff] [review]
wip

>+bool
>+IsAccBlacklistedForE10S()
>+{

given Compatability.h is exported this seems like a rather silly layer of  indirection.

>+#if defined(XP_WIN)
>+  return Compatibility::IsBlacklistedForE10S();
>+#else
>+  return true;
>+#endif

I think we could always return false on linux at this point.  It doesn't work nearly well enough to release, but you can certainly use chrome and see content some.

>+  static bool IsBlacklistedForE10S() {

{ on next line please

>+    // We currently blacklist everything except UNKNOWN and UIAUTOMATION
>+    return !!(sConsumers &
>+              (NVDA     |
>+               JAWS     |
>+               OLDJAWS  |
>+               WE       |
>+               DOLPHIN  |
>+               SEROTEK  |
>+               COBRA    |
>+               ZOOMTEXT |
>+               KAZAGURU |
>+               YOUDAO));

I'm tempted to not black list the last two, but I guess we can do that later.

	Jim Mathies [:jimm] Assignee
	Comment 8 • 9 years ago

will update and repost, thanks.

	Jim Mathies [:jimm] Assignee
	Comment 9 • 9 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> Comment on attachment 8599483 [details] [diff] [review]
> wip
> 
> >+bool
> >+IsAccBlacklistedForE10S()
> >+{
> 
> given Compatability.h is exported this seems like a rather silly layer of 
> indirection.

I was anticipating platform specific code for each platform. I guess that's not going to be the case so I'll take this out and just include windows Compatability.h in nsAppRunner if needed.


> >+#if defined(XP_WIN)
> >+  return Compatibility::IsBlacklistedForE10S();
> >+#else
> >+  return true;
> >+#endif
> 
> I think we could always return false on linux at this point.  It doesn't
> work nearly well enough to release, but you can certainly use chrome and see
> content some.

will do.

> >+               ZOOMTEXT |
> >+               KAZAGURU |
> >+               YOUDAO));
> 
> I'm tempted to not black list the last two, but I guess we can do that later.

Easy to add this now if you want.

	Trevor Saunders (:tbsaunde)
	Comment 10 • 9 years ago

(In reply to Jim Mathies [:jimm] from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > Comment on attachment 8599483 [details] [diff] [review]
> > wip
> > 
> > >+bool
> > >+IsAccBlacklistedForE10S()
> > >+{
> > 
> > given Compatability.h is exported this seems like a rather silly layer of 
> > indirection.
> 
> I was anticipating platform specific code for each platform. I guess that's
> not going to be the case so I'll take this out and just include windows
> Compatability.h in nsAppRunner if needed.

I think windows is the only place we can have a finer grained thing than on or off.

> > >+               ZOOMTEXT |
> > >+               KAZAGURU |
> > >+               YOUDAO));
> > 
> > I'm tempted to not black list the last two, but I guess we can do that later.
> 
> Easy to add this now if you want.

lets play it safe and see how more exposure for UIA and linux goes.

	Jim Mathies [:jimm] Assignee
	Comment 13 • 9 years ago

try builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c18a130dc152

	Trevor Saunders (:tbsaunde)
	Comment 14 • 9 years ago

Comment on attachment 8600002 [details] [diff] [review]
patch v.2

>+  /**
>+   * Return true if we should disable e10s due to a detected
>+   * accessibility client.
>+   */
>+  static bool IsBlacklistedForE10S() {

you forgot to fix the formating here '{' belongs on next line.

	Jim Mathies [:jimm] Assignee
	Comment 15 • 9 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> Comment on attachment 8600002 [details] [diff] [review]
> patch v.2
> 
> >+  /**
> >+   * Return true if we should disable e10s due to a detected
> >+   * accessibility client.
> >+   */
> >+  static bool IsBlacklistedForE10S() {
> 
> you forgot to fix the formating here '{' belongs on next line.

fixed.

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/31297813dd26

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 9 years ago

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d22cead404e

https://treeherder.mozilla.org/logviewer.html#?job_id=9456333&repo=mozilla-inbound

	Jim Mathies [:jimm] Assignee
	Comment 18 • 9 years ago

need an ifdef around that include. relanding here in a sec.

	Pulsebot
	Comment 19 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/796c77a8118b

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/796c77a8118b

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 22 • 9 years ago

Jim, any rumblings from nightly users? (How do we check/Can I help find out?)

	Jim Mathies [:jimm] Assignee
	Comment 23 • 9 years ago

(In reply to David Bolter [:davidb] from comment #22)
> Jim, any rumblings from nightly users? (How do we check/Can I help find out?)

haven't heard any complaints, no bugs filed either afaik. The disabled pref was reset as well so, things look good so far.