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

This is for some follow up from discussion on bug 1149772. Vendors are asking.

Shall we make the emulation work in e10s mode? Ultimately this is probably the smoothest option for our users...

	alexander :surkov (:asurkov)
	Comment 1 • 9 years ago

to make the user experience, either Firefox has to be running in single process mode or emulation has to work in mutli process. Transitional period may be quite long, so perhaps the emulation is something we should do. Is it resource heavy to implement it?

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

I think we all agree. Changing summary.

<@tbsaunde>	davidb: I expect we have to
<~davidb>	tbsaunde, yeah I think you're right

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

Yura is good to take this.

	alexander :surkov (:asurkov)
	Comment 5 • 8 years ago

Comment on attachment 8792538 [details] [diff] [review]
1206711 part 1

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

r=me sorry it got that long

::: accessible/windows/msaa/DocAccessibleWrap.cpp
@@ -148,5 @@
> -      if (tabChild)
> -        tabChild->SendGetWidgetNativeData(&nativeData);
> -      else
> -        nativeData = reinterpret_cast<mozilla::WindowsHandle>(
> -          rootDocument->GetNativeWindow());

ideally this should be landed separately

::: accessible/windows/msaa/nsWinUtils.h
@@ +14,5 @@
>  namespace a11y {
>  class DocAccessible;
>  const LPCWSTR kClassNameRoot = L"MozillaUIWindowClass";
>  const LPCWSTR kClassNameTabContent = L"MozillaContentWindowClass";
> +const LPCWSTR kPropNameDocAcc = L"DocAccessible";

perhaps we should give it a Moz prefix, to avoid possibly conflicts with somebody trying to set up same property on our window

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 6 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be4241bf0b2

	Pulsebot
	Comment 7 • 8 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fca6bd5a5b5b
remove unused ipc bits from DocAccessibleWrap::DoInitialUpdate. r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed4462900a9b
store doc accessible in emulated HWND itself. r=surkov

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

As per IRC this is now our main blocker for e10s + a11y testing on Jaws/WE/Dolphin.

	Carsten Book [:Tomcat]
	Comment 9 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fca6bd5a5b5b
https://hg.mozilla.org/mozilla-central/rev/ed4462900a9b

	Trevor Saunders (:tbsaunde)
	Comment 11 • 8 years ago

Comment on attachment 8793883 [details] [diff] [review]
1206711 part 2

I haven't thought about this in depth yet.

>diff --git a/accessible/base/Platform.h b/accessible/base/Platform.h
>index 2520456..ef9ad03 100644
>--- a/accessible/base/Platform.h
>+++ b/accessible/base/Platform.h

I'm really not a fan of sticking all these functions specific to one thing in a generic spot and then stubbing them for all the other platforms.


> /**
>+ * Determine if a given native window is emulated.
>+ */
>+bool IsWindowEmulated(void* aWindowHandle);

I don't know what you need this for, but it seems like it could be kept in windows/ somehow.

>+
>+/**
>+ * Depending on the platform and if window emulation is required, create a
>+ * native window for a tab accessible document.
>+ */
>+void* MaybeCreateNativeWindow(DocAccessibleParent* aDocument);

it seems like you could do this in ProxyCreated() if the proxy is a top level document.

>+
>+/**
>+ * Depending on the platform and if window emulation is enabled, destroy a
>+ * native window for a tab accessible document.
>+ */
>+void* MaybeDestroyNativeWindow(void* aWindowHandle);

same with ProxyDestroyed() and I don't know what the return value is about.

it seems like you could split out the creation and destruction out, and get that in as soon as you do it in the existing proxyCreated/Destroyed.

>+
>+/**
>+ * Update native window parent for emulated window if it is different.
>+ */
>+void MaybeUpdateNativeWindowParent(void* aWindowHandle,
>+                                   void* aNewParentWindowHandle);

I'm not sure what the best way to handle this is, but probably a static function on AccessibleWrap or maybe just stick the code in TabParent by the code to send down a new window.

>index 6f4527a7..4ec9824 100644
>--- a/accessible/other/Platform.cpp
>+++ b/accessible/other/Platform.cpp

if you add things here you also need to add them for mac and atk.

>+bool
>+a11y::IsWindowEmulated(void*)
>+{

you should return something here, or release assert

>+}
>+
>+void*
>+a11y::MaybeCreateNativeWindow(DocAccessibleParent*)
>+{
>+}

same

>+
>+void*
>+a11y::MaybeDestroyNativeWindow(void*)
>+{
>+}

and heree

>+
>+void
>+a11y::MaybeUpdateNativeWindowParent(void*, void*)
>+{
>+}

same

> AccessibleWrap::GetHWNDFor(Accessible* aAccessible)
> {
>   if (!aAccessible) {
>     return nullptr;
>   }
>
>-  if (XRE_IsContentProcess()) {
>-    DocAccessible* doc = aAccessible->Document();
>-    if (!doc) {
>-      return nullptr;
>-    }
>-
>-    DocAccessibleChild* ipcDoc = doc->IPCDoc();
>-    if (!ipcDoc) {
>-      return nullptr;
>-    }
>-
>-    auto tab = static_cast<dom::TabChild*>(ipcDoc->Manager());
>-    MOZ_ASSERT(tab);
>-    return reinterpret_cast<HWND>(tab->GetNativeWindowHandle());
>-  }

this doesn't seem right.

> DocAccessibleWrap::GetNativeWindow() const
> {
>-  return mHWND ? mHWND : DocAccessible::GetNativeWindow();
>+  if (mHWND) {
>+    return mHWND;
>+  }
>+  if (XRE_IsContentProcess()) {
>+    DocAccessibleChild* ipcDoc = IPCDoc();
>+    if (!ipcDoc) {

I don't think that should ever be false.

>+      return nullptr;
>+    }
>+    auto tab = static_cast<dom::TabChild*>(ipcDoc->Manager());
>+    MOZ_ASSERT(tab);
>+    return reinterpret_cast<HWND>(tab->GetNativeWindowHandle());

static_cast should be fine.

>+extern const wchar_t* kPropNameTabContent;

why is this extern? and why don't you define it here?

>+bool
>+a11y::IsWindowEmulated(void* aWindowHandle)
>+{
>+  MOZ_ASSERT(aWindowHandle);
>+
>+  if (!aWindowHandle) {
>+    return false;
>+  }
>+  HWND hwnd = reinterpret_cast<HWND>(aWindowHandle);
>+  return nsWinUtils::IsWindowEmulationStarted() &&
>+         ::GetPropW(hwnd, kPropNameTabContent);

then this is it emulated for a remote tab.

> const wchar_t* kPropNameTabContent = L"AccessibleTabWindow";
>@@ -148,28 +150,37 @@ WindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
>
>   switch (msg) {
>     case WM_GETOBJECT:
>     {
>       // Do explicit casting to make it working on 64bit systems (see bug 649236
>       // for details).
>       int32_t objId = static_cast<DWORD>(lParam);
>       if (objId == OBJID_CLIENT) {
>+        IAccessible* msaaAccessible = nullptr;
>         DocAccessible* document =
>           reinterpret_cast<DocAccessible*>(::GetPropW(hWnd, kPropNameDocAcc));
>         if (document) {
>-          IAccessible* msaaAccessible = nullptr;
>           document->GetNativeInterface((void**)&msaaAccessible); // does an addref
>-          if (msaaAccessible) {
>-            LRESULT result = ::LresultFromObject(IID_IAccessible, wParam,
>-                                                 msaaAccessible); // does an addref
>-            msaaAccessible->Release(); // release extra addref
>-            return result;
>+        } else {
>+          DocAccessibleParent* docParent =
>+            reinterpret_cast<DocAccessibleParent*>(

static_cast

>+              ::GetPropW(hWnd, kPropNameDocAccParent));

you could use one attribute if you steal a bit from the pointer to store if its remote, but I don't know that it matters.

>+          if (docParent) {
>+            auto wrapper =
>+              reinterpret_cast<DocProxyAccessibleWrap*>(WrapperFor(docParent));

static_cast

> #include "mozilla/dom/indexedDB/ActorsParent.h"
> #include "mozilla/plugins/PluginWidgetParent.h"
>@@ -297,16 +300,19 @@ TabParent::TabParent(nsIContentParent* aManager,
>   , mTabSetsCursor(false)
>   , mHasContentOpener(false)
> #ifdef DEBUG
>   , mActiveSupressDisplayportCount(0)
> #endif
>   , mLayerTreeEpoch(0)
>   , mPreserveLayers(false)
>   , mFirstActivate(true)
>+#if defined(XP_WIN) && defined(ACCESSIBILITY)
>+  , mHwnd(nullptr)

this should have a name that makes it clear what its about.

>-    Unused << SendUpdateNativeWindowHandle(newWindowHandle);
>+    if (mHwnd && newWindowHandle) {
>+      a11y::MaybeUpdateNativeWindowParent(
>+        mHwnd, reinterpret_cast<HWND>(newWindowHandle));
>+    } else {
>+      Unused << SendUpdateNativeWindowHandle(newWindowHandle);

probably better to always send the update even if nothing currently uses it.

> #ifdef ACCESSIBILITY
>-  delete static_cast<a11y::DocAccessibleParent*>(aParent);
>+  auto doc = static_cast<a11y::DocAccessibleParent*>(aParent);
>+#ifdef XP_WIN
>+  if (doc->IsTopLevel() && mHwnd) {
>+    mHwnd = reinterpret_cast<HWND>(a11y::MaybeDestroyNativeWindow(mHwnd));
>+    if (!mIsDestroyed) {
>+      uintptr_t newWindowHandle = 0;
>+      if (nsCOMPtr<nsIWidget> widget = GetWidget()) {
>+        newWindowHandle =
>+          reinterpret_cast<uintptr_t>(widget->GetNativeData(NS_NATIVE_WINDOW));
>+      }
>+      Unused << SendUpdateNativeWindowHandle(newWindowHandle);

this seems dangerous, this function can be called while the tab it is in is being torn down.

>     doc->SetTopLevel();
>     a11y::DocManager::RemoteDocAdded(doc);
>+#ifdef XP_WIN
>+    if (mHwnd) {
>+      mHwnd = reinterpret_cast<HWND>(a11y::MaybeDestroyNativeWindow(mHwnd));

and this #ifdefery makes the Platform.h changes make less sense.  your basically pretending that its platform independent when basically none of it actually is.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 12 • 8 years ago

This should updates most of the comments. I changed reinterpret cast to static were possible (there was at least 1 place that you mentioned where static cast was not allowed).

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 13 • 8 years ago

FYI this is still crashing but I managed to get 2 different stack traces before VS debugger crashed:

https://pastebin.mozilla.org/8913299
https://pastebin.mozilla.org/8913295

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

Comment on attachment 8795011 [details] [diff] [review]
1206711 part 2 v2

>+  void SetEmulatedWindowHandle(uintptr_t aWindowHandle)
>+  {
>+    if (aWindowHandle) {
>+      SendEmulatedWindowHandleCreated(aWindowHandle);
>+    } else if (mEmulatedWindowHandle) {
>+      HWND hWnd = reinterpret_cast<HWND>(mEmulatedWindowHandle);
>+      ::DestroyWindow(hWnd);
>+    }
>+    mEmulatedWindowHandle = aWindowHandle;
>+  }
>+
>+  void UpdateEmulatedWindowHandleParent(uintptr_t aWindowHandle) {
>+    if (mEmulatedWindowHandle && aWindowHandle) {
>+      HWND hWnd = reinterpret_cast<HWND>(mEmulatedWindowHandle);
>+      HWND parentHwnd = reinterpret_cast<HWND>(aWindowHandle);
>+      if (parentHwnd != ::GetParent(hWnd)) {
>+        ::SetParent(hWnd, parentHwnd);
>+      }
>+    }
>+  }

these are kind of big to put inline, though each caller is doing a destinct thing, so I'd be tempted to just have an API to get / set the handle and do whatever you need in the callers.

>+  virtual bool RecvEmulatedWindowHandleCreated(const uintptr_t& aWindowHandle) override;
Sorry, what was I thinking when I suggested this?  You can just stick this in the SendCOMProxy message which the child blocks on before continueing.  So if you setup the emulated window in the content process when that returns you know that it can't possibly matter what the window is yet, and that you will have setup the window handle before it can matter.

>+   * Update the child with the tab's current emulated window handle.
>+   *
>+   * @param aNewHandle The emulated window handle for the tab.

given above you can nix this message.

> AccessibleWrap::GetHWNDFor(Accessible* aAccessible)
> {
>   if (!aAccessible) {
>     return nullptr;
>   }
>
>-  if (XRE_IsContentProcess()) {
>-    DocAccessible* doc = aAccessible->Document();
>-    if (!doc) {
>-      return nullptr;
>-    }
>-
>-    DocAccessibleChild* ipcDoc = doc->IPCDoc();
>-    if (!ipcDoc) {
>-      return nullptr;
>-    }
>-
>-    auto tab = static_cast<dom::TabChild*>(ipcDoc->Manager());
>-    MOZ_ASSERT(tab);
>-    return reinterpret_cast<HWND>(tab->GetNativeWindowHandle());
>-  }

This doesn't immediately seem right, I expect you good break out some parts of this that can go in on there own, and are easy to make ok.

> AccessibleWrap::UpdateSystemCaretFor(Accessible* aAccessible)
> {
>-  // Move the system caret so that Windows Tablet Edition and tradional ATs with
>+  // Move the system caret so that Windows Tablet Edition and tradional ATs with

please put whitespace / spelling in separate commits, and really I don't see a reason that sort of thing needs to be reviewed.

> DocAccessibleWrap::GetNativeWindow() const
> {
>-  return mHWND ? mHWND : DocAccessible::GetNativeWindow();
>+  if (XRE_IsContentProcess()) {
>+    DocAccessibleChild* ipcDoc = IPCDoc();
>+    HWND hWnd = reinterpret_cast<HWND>(ipcDoc->GetEmulatedWindowHandle());
>+    if (hWnd) {
>+      return hWnd;
>+    }
>+
>+    auto tab = static_cast<dom::TabChild*>(ipcDoc->Manager());
>+    MOZ_ASSERT(tab);
>+    return reinterpret_cast<HWND>(tab->GetNativeWindowHandle());
>+  } else if (mHWND) {
>+    return mHWND;
>+  }
>+  return DocAccessible::GetNativeWindow();

off hand it seems like this will only change the window the document is in, but I haven't looked closely, sorry trying to get something done here.

that's it for now, I'll finish off later.

	Trevor Saunders (:tbsaunde)
	Comment 15 • 8 years ago

> >-  if (XRE_IsContentProcess()) {
> >-    DocAccessible* doc = aAccessible->Document();
> >-    if (!doc) {
> >-      return nullptr;
> >-    }
> >-
> >-    DocAccessibleChild* ipcDoc = doc->IPCDoc();
> >-    if (!ipcDoc) {
> >-      return nullptr;
> >-    }
> >-
> >-    auto tab = static_cast<dom::TabChild*>(ipcDoc->Manager());
> >-    MOZ_ASSERT(tab);
> >-    return reinterpret_cast<HWND>(tab->GetNativeWindowHandle());
> >-  }
> 
> This doesn't immediately seem right, I expect you good break out some parts
> of this that can go in on there own, and are easy to make ok.

> > DocAccessibleWrap::GetNativeWindow() const
> > {
> >-  return mHWND ? mHWND : DocAccessible::GetNativeWindow();
> >+  if (XRE_IsContentProcess()) {
> >+    DocAccessibleChild* ipcDoc = IPCDoc();
> >+    HWND hWnd = reinterpret_cast<HWND>(ipcDoc->GetEmulatedWindowHandle());
> >+    if (hWnd) {
> >+      return hWnd;
> >+    }
> >+
> >+    auto tab = static_cast<dom::TabChild*>(ipcDoc->Manager());
> >+    MOZ_ASSERT(tab);
> >+    return reinterpret_cast<HWND>(tab->GetNativeWindowHandle());
> >+  } else if (mHWND) {
> >+    return mHWND;
> >+  }
> >+  return DocAccessible::GetNativeWindow();
> 
> off hand it seems like this will only change the window the document is in,
> but I haven't looked closely, sorry trying to get something done here.

oh, I see this is just reorganizing, but I don't see any reason you need to do that, and it makes the patch harder to understand, so please undo it.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 16 • 8 years ago

This is a separated out bit that causes some confusion. GetHWND method in accessible wrap would still call document(Wrap)->GetNativeWindow so there will not be only one way of getting a native window.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 17 • 8 years ago

Haven't actually tested it yet as trunk build seems to now just crash in my VM but I addressed (I think) most of the things you mentioned.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 18 • 8 years ago

Comment on attachment 8796296 [details] [diff] [review]
1206711 part 2

Aaron, since you added the original bit for content process in GetHwnd, just wanted to make sure that this looks good to you.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 19 • 8 years ago

Comment on attachment 8796296 [details] [diff] [review]
1206711 part 2

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

::: accessible/windows/msaa/AccessibleWrap.cpp
@@ -1301,5 @@
>  {
>    if (!aAccessible) {
>      return nullptr;
>    }
> -  if (XRE_IsContentProcess()) {

It is unclear to me why this code was removed; we need to be able to resolve the HWND for any Accessible in content.

::: accessible/windows/msaa/DocAccessibleWrap.cpp
@@ +119,5 @@
>  // DocAccessible public
>  void*
>  DocAccessibleWrap::GetNativeWindow() const
>  {
> +  if (XRE_IsContentProcess()) {

I would prefer if we just delegated to AccessibleWrap::GetHWNDFor in this case.

	Trevor Saunders (:tbsaunde)
	Comment 20 • 8 years ago

I was meaning to say more or less the same thing, but since yzen is out it wasn't terribly high priority.

(In reply to Aaron Klotz [:aklotz] from comment #19)
> Comment on attachment 8796296 [details] [diff] [review]
> 1206711 part 2
> 
> Review of attachment 8796296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/windows/msaa/AccessibleWrap.cpp
> @@ -1301,5 @@
> >  {
> >    if (!aAccessible) {
> >      return nullptr;
> >    }
> > -  if (XRE_IsContentProcess()) {
> 
> It is unclear to me why this code was removed; we need to be able to resolve
> the HWND for any Accessible in content.

I believe it is correctly added in the below hunk, but see below.

> ::: accessible/windows/msaa/DocAccessibleWrap.cpp
> @@ +119,5 @@
> >  // DocAccessible public
> >  void*
> >  DocAccessibleWrap::GetNativeWindow() const
> >  {
> > +  if (XRE_IsContentProcess()) {
> 
> I would prefer if we just delegated to AccessibleWrap::GetHWNDFor in this
> case.

I'm not sure if I'm *opposed* to doing this, but without more justification I don't see a reason to do it.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 21 • 8 years ago

yzen is back \o/

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 22 • 8 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #20)
> I was meaning to say more or less the same thing, but since yzen is out it
> wasn't terribly high priority.
> > ::: accessible/windows/msaa/DocAccessibleWrap.cpp
> > @@ +119,5 @@
> > >  // DocAccessible public
> > >  void*
> > >  DocAccessibleWrap::GetNativeWindow() const
> > >  {
> > > +  if (XRE_IsContentProcess()) {
> > 
> > I would prefer if we just delegated to AccessibleWrap::GetHWNDFor in this
> > case.
> 
> I'm not sure if I'm *opposed* to doing this, but without more justification
> I don't see a reason to do it.

There are other places where GetNativeWindow is used to get a native window other than GetHWNDFor. Since GetHWNDFor also delegates to doc accessible's GetNativeWindow when the argument is not a proxy I was hoping to handle the content process case in GetNativeWindow so it resolves correctly everywhere.

	(No longer employed by Mozilla) Aaron Klotz
	Comment 23 • 8 years ago

Fair enough. Thanks for the clarification.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 24 • 8 years ago

See comment 22 why I moved things into doc accessible wrap.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 26 • 8 years ago

Also part 3 depends on bug 1314707 landing first

	Trevor Saunders (:tbsaunde)
	Comment 27 • 8 years ago

(In reply to Yura Zenevich [:yzen] from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #20)
> > I was meaning to say more or less the same thing, but since yzen is out it
> > wasn't terribly high priority.
> > > ::: accessible/windows/msaa/DocAccessibleWrap.cpp
> > > @@ +119,5 @@
> > > >  // DocAccessible public
> > > >  void*
> > > >  DocAccessibleWrap::GetNativeWindow() const
> > > >  {
> > > > +  if (XRE_IsContentProcess()) {
> > > 
> > > I would prefer if we just delegated to AccessibleWrap::GetHWNDFor in this
> > > case.
> > 
> > I'm not sure if I'm *opposed* to doing this, but without more justification
> > I don't see a reason to do it.
> 
> There are other places where GetNativeWindow is used to get a native window
> other than GetHWNDFor. Since GetHWNDFor also delegates to doc accessible's
> GetNativeWindow when the argument is not a proxy I was hoping to handle the
> content process case in GetNativeWindow so it resolves correctly everywhere.

It certainly seems like a mistake that AccessibleWrap::GetHWND() and DocAccessible::GetNativeWindow() are different, but I'd tend to say the better thing to do is make everything use GetHWND() and remove GetNativeWindow().  In any case it looks like all the users of GetNativeWindow() either don't need to care about the e10s business, or need to be handled differently anyway.

However I guess this isn't a big deal, and this has waited too long (sorry!), so whatever I guess its fine.

	Trevor Saunders (:tbsaunde)
	Comment 28 • 8 years ago

Comment on attachment 8814193 [details] [diff] [review]
1206711 part 2

not a fan, but whatever.

	Trevor Saunders (:tbsaunde)
	Comment 29 • 8 years ago

Comment on attachment 8814194 [details] [diff] [review]
1206711 part 3

> DocAccessibleChild::DocAccessibleChild(DocAccessible* aDoc)
>   : DocAccessibleChildBase(aDoc)
>+  , mEmulatedWindowHandle(0)

',' on previous line

>+    a11y::DocAccessibleParent* doc = GetTopLevelDocAccessible();
>+    HWND hWnd;
>+    if (doc &&
>+        (hWnd = reinterpret_cast<HWND>(doc->GetEmulatedWindowHandle()))) {

move the assignment out of the if, just add another if if you must, but I think you can work the check into the next one.

>     }
> 
>     auto parentDoc = static_cast<a11y::DocAccessibleParent*>(aParentDoc);
>     bool added = parentDoc->AddChildDoc(doc, aParentID);
> #ifdef XP_WIN
>     MOZ_ASSERT(aDocCOMProxy.IsNull());
>     if (added) {
>       a11y::WrapperFor(doc)->SetID(aMsaaID);
>+      if (a11y::nsWinUtils::IsWindowEmulationStarted()) {
>+        doc->SetEmulatedWindowHandle(parentDoc->GetEmulatedWindowHandle());

I'm not sure we get much from the parent side subdocuments knowing about the fake window, but whatever.

> #ifdef XP_WIN
>     a11y::WrapperFor(doc)->SetID(aMsaaID);
>     MOZ_ASSERT(!aDocCOMProxy.IsNull());
>     RefPtr<IAccessible> proxy(aDocCOMProxy.Get());
>     doc->SetCOMProxy(proxy);

I'm pretty sure the rest of this needs to happen *before* you call that and send a message to the parent with the doc's fake window.

>+    if (a11y::nsWinUtils::IsWindowEmulationStarted()) {
>+      a11y::Accessible* outerDoc = doc->OuterDocOfRemoteBrowser();
>+      a11y::RootAccessible* rootDocument = outerDoc->RootAccessible();
>+      MOZ_ASSERT(rootDocument);
>+
>+      bool isActive = true;
>+      nsIntRect rect(CW_USEDEFAULT, CW_USEDEFAULT, 0, 0);
>+      if (a11y::Compatibility::IsDolphin()) {
>+        rect = doc->Bounds();

That calls ProxyAccessible::Bounds() which afaik just isn't defined on windows at this point.  I guess you could define it using COM, but that's definitely suboptimal perf wise, and may risk deadlocks I'm not sure what happens if the main process COM proxying tries to deadlock with the child process sending the parent a sync ipdl message.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 30 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67074f9d2633593e48ac2d969f4192b19231830d

	Pulsebot
	Comment 31 • 8 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfba17b7eb3f
always use DocAccessibleWrap to get native window. r=tbsaunde

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 32 • 8 years ago

Moving emulation stuff from TabParent to DocAccessibleParent

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cfba17b7eb3f

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

Comment on attachment 8820057 [details] [diff] [review]
1206711 part 3

># HG changeset patch
># User Yura Zenevich <yzenevich@mozilla.com>
>
>Bug 1206711 - adding support for HWND emulation for content top level docs on Windows.

s/adding/add/ but really that's not exactly what you are doing here, you stick the hwnd in all docs not just the top level ones.

>+DocAccessibleParent::SetEmulatedWindowHandle(uintptr_t aWindowHandle)

its ifdefed, you might as well make the type HWND

>+{
>+  if (!aWindowHandle && mEmulatedWindowHandle && IsTopLevel()) {

I'm not sure there's a good reason to set the member in non top level docs in the first place, but whatever.

>+  void SetEmulatedWindowHandle(uintptr_t aWindowHandle);
>+  uintptr_t GetEmulatedWindowHandle() const { return mEmulatedWindowHandle; }

might as well make the type HWND here too.

>   nsTArray<DocAccessibleParent*> mChildDocs;
>   DocAccessibleParent* mParentDoc;
>
>+#if defined(XP_WIN)
>+  // The handle associated with the emulated window that contains this document
>+  uintptr_t mEmulatedWindowHandle;

same

>   mParentProxy.reset(const_cast<IAccessibleHolder&>(aParentCOMProxy).Release());
>   SetConstructedInParentProcess();
>
>   for (uint32_t i = 0, l = mDeferredEvents.Length(); i < l; ++i) {
>     mDeferredEvents[i]->Dispatch();
>   }
>
>   mDeferredEvents.Clear();
>+  mEmulatedWindowHandle = aEmulatedWindowHandle;

that should probably come before dispatching the events.

>+  uintptr_t GetEmulatedWindowHandle() const { return mEmulatedWindowHandle; }

HWND

>+  uintptr_t mEmulatedWindowHandle;

same

>@@ -80,16 +80,20 @@ void
> a11y::ProxyDestroyed(ProxyAccessible* aProxy)
> {
>   AccessibleWrap* wrapper =
>     reinterpret_cast<AccessibleWrap*>(aProxy->GetWrapper());
>   MOZ_ASSERT(wrapper);
>   if (!wrapper)
>     return;
>
>+  if (aProxy->IsDoc() && nsWinUtils::IsWindowEmulationStarted()) {
>+    aProxy->AsDoc()->SetEmulatedWindowHandle(0);
>+  }

seems pointless, the ctor just set it to 0 didn't it?

>+        } else {
>+          DocAccessibleParent* docParent = static_cast<DocAccessibleParent*>(
>+            ::GetPropW(hWnd, kPropNameDocAccParent));
>+          if (docParent) {
>+            auto wrapper =
>+              static_cast<DocProxyAccessibleWrap*>(WrapperFor(docParent));

AccessibleWrap* should be fine, drop the cast.

r=me with comments fixed.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 35 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18b487a88f457855c85518e7e5cc6f5eda18940c

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 36 • 7 years ago

Hi Marco, so here are the builds from the try run from Comment 35:

debug: https://archive.mozilla.org/pub/firefox/try-builds/yura.zenevich@gmail.com-18b487a88f457855c85518e7e5cc6f5eda18940c/try-win64-debug/
opt: https://archive.mozilla.org/pub/firefox/try-builds/yura.zenevich@gmail.com-18b487a88f457855c85518e7e5cc6f5eda18940c/try-win64/

Note that I suspect virtual buffer will not work since Brett mentioned the focus is not set on the emulated window for the tab. If so I think I will have to explicitly do that when new window is created for an active top doc and when active top doc is updated.

	Marco Zehe (:MarcoZ)
	Comment 37 • 7 years ago

I still cannot get the virtual buffer to work at all with this build, but again, I'm using JAWS 17, need to get JAWS 18 to work and re-test later. There may be stuff in there they fixed that will make it work with 18. But this is an important issue because not everybody will be on the latest release of JAWS due to upgrade costs.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 38 • 7 years ago

Hi Marco, not sure if you will be able to manage to test with JAWS but this try run includes changes that set focus on active emulated window. if you have a moment, could you see if it makes things better? Note WE still isnt recognized.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f912c672661d3ec5863e57f6820bb77faee2a0e
https://archive.mozilla.org/pub/firefox/try-builds/yura.zenevich@gmail.com-4f912c672661d3ec5863e57f6820bb77faee2a0e/

	Marco Zehe (:MarcoZ)
	Comment 39 • 7 years ago

This didn't make any difference to me unfortunately. :-(

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 40 • 7 years ago

Carrying over r+

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 42 • 7 years ago

Marco, would you take a look at this build, i think this one might be it :)

https://archive.mozilla.org/pub/firefox/try-builds/yura.zenevich@gmail.com-56606f67ed8f03fe7053326f4186286d43746a99/

	Marco Zehe (:MarcoZ)
	Comment 43 • 7 years ago

(In reply to Yura Zenevich [:yzen] from comment #42)
> Marco, would you take a look at this build, i think this one might be it :)
> 
> https://archive.mozilla.org/pub/firefox/try-builds/yura.zenevich@gmail.com-
> 56606f67ed8f03fe7053326f4186286d43746a99/

Unfortunately that try build hasn't finished. So I couldn't try anything. :-(

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 44 • 7 years ago

Ah sorry the link didn't paste correctly: 
https://archive.mozilla.org/pub/firefox/try-builds/yura.zenevich@gmail.com-56606f67ed8f03fe7053326f4186286d43746a99/

	Marco Zehe (:MarcoZ)
	Comment 45 • 7 years ago

Nope, the link is the one I visited, but the sub directory is empty except for a gzipped log file or so.
https://archive.mozilla.org/pub/firefox/try-builds/yura.zenevich@gmail.com-56606f67ed8f03fe7053326f4186286d43746a99/try-win64/

	Pulsebot
	Comment 46 • 7 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd70938318f8
add support for HWND emulation for content top level docs on Windows. r=tbsaunde

	Phil Ringnalda (:philor)
	Comment 47 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dd70938318f8

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

Comment on attachment 8829721 [details] [diff] [review]
1206711 part 4

>-  // Accessibles in child processes are said to have the HWND of the window
>-  // their tab is within.  Popups are always in the parent process, and so
>-  // never proxied, which means this is basically correct.
>   if (aAccessible->IsProxy()) {
>     ProxyAccessible* proxy = aAccessible->Proxy();
>     if (!proxy) {
>       return nullptr;
>     }

That we use this code like this is really round about and silly, but we can fix it some other day.

> 
>+    // If window emulation is enabled, retrieve emulated native window from a
>+    // containing document proxy.

retrieve the emulated window from the containing document ...

	Pulsebot
	Comment 49 • 7 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6589d2f22f6a
ensure that emulation is taken into account when resolving HWND for a proxy. r=tbsaunde

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 50 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6589d2f22f6a

	(No longer employed by Mozilla) Aaron Klotz
	Comment 51 • 7 years ago

Is there more work to be done in this bug, or should it be resolved?

	Pulsebot
	Comment 53 • 7 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/771f1590f03e
pass emulated window proxy to doc accessible child to be used as parent IAccessible for top level documents. r=tbsaunde

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 54 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/771f1590f03e

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

As per today's meeting, Yura will close this fixed and open compat specific bugs for remaining issues (and add aes+ as appropriate to whiteboard).

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 56 • 7 years ago

Closing and will open bugs that are specific to the issues uncovered.