1149772 - Make nsWinUtils::MaybeStartWindowEmulation and related code match our new plan for e10s a11y.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

We have a remnant from our old e10s work that created a window per tab in the content process.

  if (Compatibility::IsJAWS() || Compatibility::IsWE() ||
      Compatibility::IsDolphin() ||
      XRE_GetProcessType() == GeckoProcessType_Content)

The latter check is likely complicating any testing we do on Windows. We should probably remove and look for the related code and remove that too.

Note eventually if content is properly sandboxed as planned, we won't be able to create windows in the content process at all.

Comment 1

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #0)
> We have a remnant from our old e10s work that created a window per tab in
> the content process.
> 
>   if (Compatibility::IsJAWS() || Compatibility::IsWE() ||
>       Compatibility::IsDolphin() ||
>       XRE_GetProcessType() == GeckoProcessType_Content)
> 
> The latter check is likely complicating any testing we do on Windows.

it can be put under the pref

> We
> should probably remove and look for the related code and remove that too.

I'd say no, at least until we didn't settle down with the e10s approach

> Note eventually if content is properly sandboxed as planned, we won't be
> able to create windows in the content process at all.

I wouldn't burn all bridges now

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #1)
> (In reply to David Bolter [:davidb] from comment #0)
> > We have a remnant from our old e10s work that created a window per tab in
> > the content process.
> > 
> >   if (Compatibility::IsJAWS() || Compatibility::IsWE() ||
> >       Compatibility::IsDolphin() ||
> >       XRE_GetProcessType() == GeckoProcessType_Content)
> > 
> > The latter check is likely complicating any testing we do on Windows.
> 
> it can be put under the pref

what pref would be appropriate? I guess we could add one, but that seems like a waste of time.

> > Note eventually if content is properly sandboxed as planned, we won't be
> > able to create windows in the content process at all.
> 
> I wouldn't burn all bridges now

It needs to happen, but I don't think there's anything to remove until we stop supporting non e10s gecko.


(In reply to David Bolter [:davidb] from comment #0)
> We have a remnant from our old e10s work that created a window per tab in
> the content process.
> 
>   if (Compatibility::IsJAWS() || Compatibility::IsWE() ||
>       Compatibility::IsDolphin() ||
>       XRE_GetProcessType() == GeckoProcessType_Content)
> 
> The latter check is likely complicating any testing we do on Windows. We

not sure about that, but I think its responsible for bug 1146797

> should probably remove and look for the related code and remove that too.

I think everything else is also needed to support window emulation in non e10s world.  Then maybe we'll need to implement window emulation that works with e10s, but hopefully we can drop it.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: never create fake HWND in child processes

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8588540 [details] [diff] [review]
never create fake HWND in child processes

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

Looks surgical enough to me.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> (In reply to alexander :surkov from comment #1)
> > (In reply to David Bolter [:davidb] from comment #0)
> > > We have a remnant from our old e10s work that created a window per tab in
> > > the content process.
> > > 
> > >   if (Compatibility::IsJAWS() || Compatibility::IsWE() ||
> > >       Compatibility::IsDolphin() ||
> > >       XRE_GetProcessType() == GeckoProcessType_Content)
> > > 
> > > The latter check is likely complicating any testing we do on Windows.
> > 
> > it can be put under the pref
> 
> what pref would be appropriate?

accessibility.ipc_architecture.enabled

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > (In reply to alexander :surkov from comment #1)
> > > (In reply to David Bolter [:davidb] from comment #0)
> > > > We have a remnant from our old e10s work that created a window per tab in
> > > > the content process.
> > > > 
> > > >   if (Compatibility::IsJAWS() || Compatibility::IsWE() ||
> > > >       Compatibility::IsDolphin() ||
> > > >       XRE_GetProcessType() == GeckoProcessType_Content)
> > > > 
> > > > The latter check is likely complicating any testing we do on Windows.
> > > 
> > > it can be put under the pref
> > 
> > what pref would be appropriate?
> 
> accessibility.ipc_architecture.enabled

I'm fine with a pref, my one concern is a vendor forgetting then set it and testing the wrong thing later on...

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

then/they (sorry for spam)

Comment 8

[Trevor Saunders (:tbsaunde)]

Attachment: never create fake HWND in child processes

Comment 9

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #6)

> > accessibility.ipc_architecture.enabled
> 
> I'm fine with a pref, my one concern is a vendor forgetting then set it and
> testing the wrong thing later on...

Pref should live till we maintain two approaches. If you are saying that the cache-approach is the only possible solution and now it's good time to burn the bridges then the pref and all related code has to be removed. Also you have to ask our partners to stop looking into multi-window approach, because I believe some of them still do.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 8588578 [details] [diff] [review]
never create fake HWND in child processes

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

::: accessible/windows/msaa/nsWinUtils.cpp
@@ +59,5 @@
>  {
>    // Register window class that'll be used for document accessibles associated
>    // with tabs.
> +  if (IPCAccessibilityActive())
> +    return false;

that will make JAWS and others not compatible with this e10s architecture, not sure whether this is on demand so redirecting review to David.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8588578 [details] [diff] [review]
never create fake HWND in child processes

Hanging an f? for surkov and vendor outreach. We want to wait right?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #11)
> Comment on attachment 8588578 [details] [diff] [review]
> never create fake HWND in child processes
> 
> Hanging an f? for surkov and vendor outreach. We want to wait right?

you may want to take the patch that doesn't touch windows emulation for AT that relies on it

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 8588578 [details] [diff] [review]
never create fake HWND in child processes

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

I think I'd take this while we wait since it would be good to know if it is causing bug 1146797 (HT Trev).

We can always follow up for testing the e10s escape hatch configuration.

Comment 14

[Trevor Saunders (:tbsaunde)]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=20fab9c2a8ac

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20fab9c2a8ac