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

RESOLVED FIXED in Firefox 40

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: davidb, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
All
Windows
Points:
---

Firefox Tracking Flags

(e10s+, firefox40 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

3 years ago
(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

Updated

3 years ago
tracking-e10s: --- → ?
tracking-e10s: ? → +
(Assignee)

Comment 2

3 years ago
(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.
(Assignee)

Comment 3

3 years ago
Created attachment 8588540 [details] [diff] [review]
never create fake HWND in child processes
Attachment #8588540 - Flags: review?(dbolter)
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.
Attachment #8588540 - Flags: review?(dbolter)
Attachment #8588540 - Flags: review+
Attachment #8588540 - Flags: feedback?(surkov.alexander)

Comment 5

3 years ago
(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

Updated

3 years ago
Attachment #8588540 - Flags: feedback?(surkov.alexander) → feedback-
(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...
then/they (sorry for spam)
(Assignee)

Comment 8

3 years ago
Created attachment 8588578 [details] [diff] [review]
never create fake HWND in child processes
Attachment #8588540 - Attachment is obsolete: true
Attachment #8588578 - Flags: review?(surkov.alexander)

Comment 9

3 years ago
(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

3 years ago
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.
Attachment #8588578 - Flags: review?(surkov.alexander) → review?(dbolter)
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?
Attachment #8588578 - Flags: feedback?(surkov.alexander)

Comment 12

3 years ago
(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 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.
Attachment #8588578 - Flags: review?(dbolter) → review+

Updated

3 years ago
Attachment #8588578 - Flags: feedback?(surkov.alexander)
https://hg.mozilla.org/mozilla-central/rev/20fab9c2a8ac
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.