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

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.

	alexander :surkov (:asurkov)
	Comment 1 • 9 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

	Trevor Saunders (:tbsaunde) Assignee
	Comment 2 • 9 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.

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

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.

	alexander :surkov (:asurkov)
	Comment 5 • 9 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

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

(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...

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

then/they (sorry for spam)

	alexander :surkov (:asurkov)
	Comment 9 • 9 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.

	alexander :surkov (:asurkov)
	Comment 10 • 9 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.

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

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?

	alexander :surkov (:asurkov)
	Comment 12 • 9 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

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

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.

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

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

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

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