762770 - implement IAccessibleEx

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 631624 [details] [diff] [review]
patch

>diff --git a/accessible/src/msaa/AccessibleWrap.cpp b/accessible/src/msaa/AccessibleWrap.cpp
>--- a/accessible/src/msaa/AccessibleWrap.cpp
>+++ b/accessible/src/msaa/AccessibleWrap.cpp
>@@ -1,34 +1,32 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "AccessibleWrap.h"
> 
>+#include "Accessible2_i.c"
>+#include "AccessibleRole.h"
>+#include "AccessibleStates.h"
> #include "Compatibility.h"

nit, I thought you generally tried to keep system stuff seperate?

>+    IAccessibleEx* accEx = new uiaRawElmProvider(this);
>+    HRESULT hr = accEx->QueryInterface(aIID, aInstancePtr);
>+    if (FAILED(hr))
>+      delete accEx;

kind of funny, but seems correct.

>-#define DECL_IUNKNOWN_INHERITED                                               \
>-public:                                                                       \
>-STDMETHODIMP QueryInterface(REFIID, void**);                                  \
>+#define DECL_IUNKNOWN                                                          \
>+public:                                                                        \
>+  virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID, void**);            \
>+  virtual ULONG STDMETHODCALLTYPE AddRef()                                     \
>+    {  return ++mRefCnt; }                                                     \
>+  virtual ULONG STDMETHODCALLTYPE Release()                                    \
>+  {                                                                            \
>+    ULONG r = --mRefCnt;                                                       \
>+    if (r == 0)                                                                \
>+      delete this;                                                             \
>+    return r;                                                                  \

couldn't this just be 

mRefCnt--;
if (!mRefCnt)
  return mRefCnt;

delete this;
return 0;

then we don't have to remember the order of side effects :)

>+  }                                                                            \
>+private:                                                                       \
>+  ULONG mRefCnt;                                                               \
>+public:                                                              

This macro looks like a big foot gun when you have any inheritance or the like going on. In the best case you'll waste a word per object, and it wouldn't suprise me if there was something that looks reasonable but actually acts on different mRefCnt members for inherited classes.  Marking AdRef() and Release() as final would remove atleast some of this scaryness I think.

it's also funny that this macro actually implements AddRef / Release when its called DECL          \

>+#define IMPL_IUNKNOWN_QUERY_AGGR_COND(Member, Cond)                            \

nit, it seems like the use of this is more with tearoffs than agragation

> #define IMPL_IUNKNOWN_INHERITED0(Class, Super)                                \
>   IMPL_IUNKNOWN_QUERY_HEAD(Class)                                             \
>-  IMPL_IUNKNOWN_QUERY_ENTRY(Super)                                            \
>+  IMPL_IUNKNOWN_QUERY_CLASS(Super)                                            \
>   IMPL_IUNKNOWN_QUERY_TAIL                                               

so, what is the point of this macro? it seems like you could just remove it and allow the super classes QI to be called directly.

>+#define IMPL_IUNKNOWN_INHERITED1(Class, Super, Super1)                         \
>+  IMPL_IUNKNOWN_QUERY_HEAD(Class)                                              \
>+  IMPL_IUNKNOWN_QUERY_CLASS(Super1);                                           \
>+  IMPL_IUNKNOWN_QUERY_CLASS(Super)                                             \
>+  

nit, I'd think super1 and super2 or super0 and super1 would make it clearer these are both just classes we inherit from.
>+#define IMPL_IUNKNOWN_INHERITED2(Class, Super, Super1, Super2)                 \

same

does it seem reasonable to you to consider moving all these macros to someplace other than AccessibleWrap.h?

> srcdir = @srcdir@
>-VPATH = @srcdir@
>+VPATH = \
>+  $(srcdir) \
>+  $(srcdir)/../windows/uia \
>+  $(NULL)

I thought khuey hates people using VPATH?  I'm not sure what you get by this other than putting off actually adding makefiles for a while.

>--- /dev/null
>+++ b/accessible/src/windows/uia/uiaRawElmProvider.cpp
>@@ -0,0 +1,172 @@


at one point I think you wanted to do accessible/src/blah -> accessible/blah is that still true? want to start with this one?

>+
>+IMPL_IUNKNOWN2(uiaRawElmProvider,
>+               IAccessibleEx,
>+               IRawElementProviderSimple)

kind of funny you can't get back to IAccessible etc, I think tht breaks a COM rule, but I'm not sure if anyone will care

>+STDMETHODIMP
>+uiaRawElmProvider::GetObjectForChild(long aIdChild,
>+                                     __RPC__deref_out_opt IAccessibleEx** aAccEx)
>+{
>+  A11Y_TRYBLOCK_BEGIN
>+
>+  if (!aAccEx)
>+    return E_INVALIDARG;
>+  *aAccEx = NULL;

nit, blank line after if statement please.

>+uiaRawElmProvider::GetIAccessiblePair(__RPC__deref_out_opt IAccessible** aAcc,
>+                                      __RPC__out long* aIdChild)
>+{
>+  A11Y_TRYBLOCK_BEGIN
>+
>+  if (!aAcc || !aIdChild)
>+    return E_INVALIDARG;
>+
>+  *aAcc = NULL;
>+  *aIdChild = 0;
>+
>+  if (mAcc->IsDefunct())
>+    return CO_E_OBJNOTCONNECTED;
>+
>+  void* instancePtr = NULL;
>+  HRESULT hr = static_cast<IAccessible*>(mAcc)->QueryInterface(IID_IAccessible,
>+                                                               &instancePtr);
>+  if (SUCCEEDED(hr)) {
>+    *aIdChild = CHILDID_SELF;
>+    *aAcc = static_cast<IAccessible*>(instancePtr);
>+  }

I'm not clear why all this is needed, I think you should be able to do

*aAcc = static_cast<IAccessible*>(mAcc);
mAcc->AddRef();

>+  int ids[] = { UiaAppendRuntimeId, reinterpret_cast<int>(mAcc->UniqueID()) };

why not use uintptr_t[] and avoid the possible issue with win64?

>+  *aRuntimeIds = SafeArrayCreateVector(VT_I4, 0, 2);
>+  if (!*aRuntimeIds)
>+    return E_OUTOFMEMORY;
>+
>+  for (LONG i = 0; i < 2; i++) {
>+    SafeArrayPutElement(*aRuntimeIds, &i, (void*)&(ids[i]));
>+  }

honestly I think I'd just replace all of this with SafeArrayPut() twice in a row unless we expect to add more stuff here which I'm not sure of yet, I haven't started looking at the spec bits of this yet.

>+  virtual HRESULT STDMETHODCALLTYPE GetObjectForChild(
>+    /* [in] */ long aIdChild,
>+    /* [retval][out] */ __RPC__deref_out_opt IAccessibleEx** aAccEx);

I always found those comments to be far more anoying than helpful, but if they're autogenerated or somehow else make it easier to maintain I guess we should add them.

>+protected:
>+  nsRefPtr<AccessibleWrap> mAcc;

protected in a final class seems kind of silly but whatever

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> > #include "AccessibleWrap.h"
> > 
> >+#include "Accessible2_i.c"
> >+#include "AccessibleRole.h"
> >+#include "AccessibleStates.h"
> > #include "Compatibility.h"
> 
> nit, I thought you generally tried to keep system stuff seperate?

ok, pushed them into the end where oleacc.h is

> couldn't this just be 
> then we don't have to remember the order of side effects :)

ok

> >+  }                                                                            \
> >+private:                                                                       \
> >+  ULONG mRefCnt;                                                               \
> >+public:                                                              
> 
> This macro looks like a big foot gun when you have any inheritance or the
> like going on. In the best case you'll waste a word per object, and it
> wouldn't suprise me if there was something that looks reasonable but
> actually acts on different mRefCnt members for inherited classes.

I agree but iirc, it's nsisupports approach

>  Marking
> AdRef() and Release() as final would remove atleast some of this scaryness I
> think.

it makes sense

> it's also funny that this macro actually implements AddRef / Release when
> its called DECL          \

yes, but I hope it's ok? :)

> 
> >+#define IMPL_IUNKNOWN_QUERY_AGGR_COND(Member, Cond)                            \
> 
> nit, it seems like the use of this is more with tearoffs than agragation

ok

> > #define IMPL_IUNKNOWN_INHERITED0(Class, Super)                                \
> >   IMPL_IUNKNOWN_QUERY_HEAD(Class)                                             \
> >-  IMPL_IUNKNOWN_QUERY_ENTRY(Super)                                            \
> >+  IMPL_IUNKNOWN_QUERY_CLASS(Super)                                            \
> >   IMPL_IUNKNOWN_QUERY_TAIL                                               
> 
> so, what is the point of this macro? it seems like you could just remove it
> and allow the super classes QI to be called directly.

there's no usecase but it could be useful for objects implementing IUnknown and nsISupports the same time.

> nit, I'd think super1 and super2 or super0 and super1 would make it clearer
> these are both just classes we inherit from.

ok

> does it seem reasonable to you to consider moving all these macros to
> someplace other than AccessibleWrap.h?

yes, I just didn't end up with proper place so continue to keep them here

> > srcdir = @srcdir@
> >-VPATH = @srcdir@
> >+VPATH = \
> >+  $(srcdir) \
> >+  $(srcdir)/../windows/uia \
> >+  $(NULL)
> 
> I thought khuey hates people using VPATH?  

I don't know :)

> I'm not sure what you get by this
> other than putting off actually adding makefiles for a while.

each platform folder exposes toolkit.lib so having multiple folders and makefiles I dind't find nice solution to deal with those libs so I ended up with VPATH approach which is seems to be nice in our case. But I don't know about Makefiles wildlife :)

> >--- /dev/null
> >+++ b/accessible/src/windows/uia/uiaRawElmProvider.cpp
> >@@ -0,0 +1,172 @@
> 
> 
> at one point I think you wanted to do accessible/src/blah -> accessible/blah
> is that still true? want to start with this one?

at least not this time since it's again big renamings

> kind of funny you can't get back to IAccessible etc, I think tht breaks a
> COM rule, but I'm not sure if anyone will care

UIA allows that

> I'm not clear why all this is needed, I think you should be able to do
> 
> *aAcc = static_cast<IAccessible*>(mAcc);
> mAcc->AddRef();

ok

> >+  for (LONG i = 0; i < 2; i++) {
> >+    SafeArrayPutElement(*aRuntimeIds, &i, (void*)&(ids[i]));
> >+  }
> 
> honestly I think I'd just replace all of this with SafeArrayPut() twice in a
> row unless we expect to add more stuff here which I'm not sure of yet, I
> haven't started looking at the spec bits of this yet.

ok, just copied ready pattern

> >+  virtual HRESULT STDMETHODCALLTYPE GetObjectForChild(
> >+    /* [in] */ long aIdChild,
> >+    /* [retval][out] */ __RPC__deref_out_opt IAccessibleEx** aAccEx);
> 
> I always found those comments to be far more anoying than helpful, but if
> they're autogenerated or somehow else make it easier to maintain I guess we
> should add them.

it seems it's usual practice (since they are autogenerated)

Comment 5

[Trevor Saunders (:tbsaunde)]

> > it's also funny that this macro actually implements AddRef / Release when
> > its called DECL          \
> 
> yes, but I hope it's ok? :)

I don't have a better name :)

> > does it seem reasonable to you to consider moving all these macros to
> > someplace other than AccessibleWrap.h?
> 
> yes, I just didn't end up with proper place so continue to keep them here

ok, iirc the nsISupports ones are just in a header called nsISupportsImpl.h or something.

> > I'm not sure what you get by this
> > other than putting off actually adding makefiles for a while.
> 
> each platform folder exposes toolkit.lib so having multiple folders and
> makefiles I dind't find nice solution to deal with those libs so I ended up
> with VPATH approach which is seems to be nice in our case. But I don't know
> about Makefiles wildlife :)

I'm not sure the static libraries matter all that much since it all ends up getting linked into libxul?

> > >--- /dev/null
> > >+++ b/accessible/src/windows/uia/uiaRawElmProvider.cpp
> > >@@ -0,0 +1,172 @@
> > 
> > 
> > at one point I think you wanted to do accessible/src/blah -> accessible/blah
> > is that still true? want to start with this one?
> 
> at least not this time since it's again big renamings

ok, fwiw it doesn't matter to me either way.

> 
> > kind of funny you can't get back to IAccessible etc, I think tht breaks a
> > COM rule, but I'm not sure if anyone will care
> 
> UIA allows that

yeah, I guess it is QueryService, and you actually can get back with methods on IAccessibleEx

> > >+  virtual HRESULT STDMETHODCALLTYPE GetObjectForChild(
> > >+    /* [in] */ long aIdChild,
> > >+    /* [retval][out] */ __RPC__deref_out_opt IAccessibleEx** aAccEx);
> > 
> > I always found those comments to be far more anoying than helpful, but if
> > they're autogenerated or somehow else make it easier to maintain I guess we
> > should add them.
> 
> it seems it's usual practice (since they are autogenerated)

yeah, ok

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 631624 [details] [diff] [review]
patch

>+AccessibleWrap::QueryService(REFGUID aGuidService, REFIID aIID,
>+                             void** aInstancePtr)
>+{
>+  if (!aInstancePtr)
>+    return E_INVALIDARG;
>+  *aInstancePtr = NULL;

one other nit I don't think I got before, sorry if I did blank line after the if :)

sorry it took me a while to get back to this, I'd probably like to see another version, but could be convinced otherwise or could fix the nits if you won't have time :)

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> > yes, I just didn't end up with proper place so continue to keep them here
> 
> ok, iirc the nsISupports ones are just in a header called nsISupportsImpl.h
> or something.

we can do that

> I'm not sure the static libraries matter all that much since it all ends up
> getting linked into libxul?

I think yes but not putting them the way we do I get linker errors.

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 633866 [details] [diff] [review]
patch3


>+private:                                                                       \
>+  ULONG mRefCnt;                                                               \
>+public:                                                                        \

nit, \ on the last line

>+#define IMPL_IUNKNOWN_INHERITED1(Class, Super0, Super1)                        \

no need to change it now, but funny ist INHERITED1 and expects 2 super classes

>+#define IMPL_IUNKNOWN_INHERITED2(Class, Super0, Super1, Super2)                \

same

>+STDMETHODIMP

so, do you care about HRESULT STDCALL vs STDMETHODIMP? I don't, but it seems like the sort of consistancy thing you might.

>+uiaRawElmProvider::GetIAccessiblePair(__RPC__deref_out_opt IAccessible** aAcc,
>+                                      __RPC__out long* aIdChild)
>+{
>+  A11Y_TRYBLOCK_BEGIN
>+
>+  if (!aAcc || !aIdChild)
>+    return E_INVALIDARG;
>+
>+  *aAcc = NULL;
>+  *aIdChild = 0;
>+
>+  if (mAcc->IsDefunct())
>+    return CO_E_OBJNOTCONNECTED;
>+
>+  void* instancePtr = NULL;
>+  HRESULT hr = static_cast<IAccessible*>(mAcc)->QueryInterface(IID_IAccessible,
>+                                                               &instancePtr);
>+  if (SUCCEEDED(hr)) {
>+    *aIdChild = CHILDID_SELF;
>+    *aAcc = static_cast<IAccessible*>(instancePtr);
>+  }

why is all this necessary? couldn't you just do

*aAcc = mAcc; // AccessibleWrap is a IAccessible
mAcc->AddRef(); // give the caller a ref

>+uiaRawElmProvider::GetRuntimeId(__RPC__deref_out_opt SAFEARRAY** aRuntimeIds)
>+{
>+  A11Y_TRYBLOCK_BEGIN
>+
>+  if (!aRuntimeIds)
>+    return E_INVALIDARG;
>+
>+  int ids[] = { UiaAppendRuntimeId, reinterpret_cast<int>(mAcc->UniqueID()) };
>+  *aRuntimeIds = SafeArrayCreateVector(VT_I4, 0, 2);
>+  if (!*aRuntimeIds)
>+    return E_OUTOFMEMORY;
>+
>+  for (LONG i = 0; i < 2; i++) {
>+    SafeArrayPutElement(*aRuntimeIds, &i, (void*)&(ids[i]));
>+  }

since its two iterations, and won't change for a long time I'd prefer the unrolled loop of two SafeArrayPutElement()s in a row.

I think it will also make dealing with the case of 64 bit easier.

either way we really should just get 64 bit right now, by adding a third element to the array in the case of 64 bit that is the other half of the pointer.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > > yes, I just didn't end up with proper place so continue to keep them here
> > 
> > ok, iirc the nsISupports ones are just in a header called nsISupportsImpl.h
> > or something.
> 
> we can do that
> 
> > I'm not sure the static libraries matter all that much since it all ends up
> > getting linked into libxul?
> 
> I think yes but not putting them the way we do I get linker errors.

khuey was interested what they were, and actually so am I, its a little suprising.

Comment 11

[Trevor Saunders (:tbsaunde)]

Attachment: patch

mostly just reworked the makefile stuff, and a few terribly minor nits.

r? khuey on the Make stuff.

Comment 12

[Trevor Saunders (:tbsaunde)]

> >+uiaRawElmProvider::GetRuntimeId(__RPC__deref_out_opt SAFEARRAY** aRuntimeIds)
> >+{
> >+  A11Y_TRYBLOCK_BEGIN
> >+
> >+  if (!aRuntimeIds)
> >+    return E_INVALIDARG;
> >+
> >+  int ids[] = { UiaAppendRuntimeId, reinterpret_cast<int>(mAcc->UniqueID()) };
> >+  *aRuntimeIds = SafeArrayCreateVector(VT_I4, 0, 2);
> >+  if (!*aRuntimeIds)
> >+    return E_OUTOFMEMORY;
> >+
> >+  for (LONG i = 0; i < 2; i++) {
> >+    SafeArrayPutElement(*aRuntimeIds, &i, (void*)&(ids[i]));
> >+  }
> 
> since its two iterations, and won't change for a long time I'd prefer the
> unrolled loop of two SafeArrayPutElement()s in a row.
> 
> I think it will also make dealing with the case of 64 bit easier.
> 
> either way we really should just get 64 bit right now, by adding a third
> element to the array in the case of 64 bit that is the other half of the
> pointer.

it occured to me using ArrayLnegth() could be made to work with win64 fairly easily so I went for that.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)

> > > I'm not sure the static libraries matter all that much since it all ends up
> > > getting linked into libxul?
> > 
> > I think yes but not putting them the way we do I get linker errors.
> 
> khuey was interested what they were, and actually so am I, its a little
> suprising.

I don't recall now, it wasn't able to find some obj files. But which way are you interested in?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> Created attachment 636705 [details] [diff] [review]
> patch
> 
> mostly just reworked the makefile stuff, and a few terribly minor nits.

curious why do you prefer this way?

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > Created attachment 636705 [details] [diff] [review]
> > patch
> > 
> > mostly just reworked the makefile stuff, and a few terribly minor nits.
> 
> curious why do you prefer this way?

I don't really care much but I expected it would make Kyle a bit happier.  do you have an objection?

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 636705 [details] [diff] [review]
patch

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

The build bits look fine.  Just a couple nits.

::: accessible/src/Makefile.in
@@ +13,5 @@
>  ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
>  PLATFORM_DIR = atk
>  else
>  ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> +PLATFORM_DIR = msaa \

nit:

PLATFORM_DIR = \
  msaa \
  ....

::: accessible/src/windows/uia/Makefile.in
@@ +10,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE = accessibility
> +LIBRARY_NAME = accessibility_toolkit_uia_s
> +EXPORT_LIBRARY = ..

= ..?

= 1?

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 636705 [details] [diff] [review]
patch

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

The build bits look fine.  Just a couple nits.

::: accessible/src/Makefile.in
@@ +13,5 @@
>  ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
>  PLATFORM_DIR = atk
>  else
>  ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> +PLATFORM_DIR = msaa \

nit:

PLATFORM_DIR = \
  msaa \
  ....

::: accessible/src/windows/uia/Makefile.in
@@ +10,5 @@
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE = accessibility
> +LIBRARY_NAME = accessibility_toolkit_uia_s
> +EXPORT_LIBRARY = ..

= ..?

= 1?

Comment 18

[Mark Capella [:capella]]

Pushed to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=d822c2b62161

Comment 19

[Mark Capella [:capella]]

Push to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4a898478fe06

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a898478fe06

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> > curious why do you prefer this way?
> 
> I don't really care much but I expected it would make Kyle a bit happier. 
> do you have an objection?

Nope. I don't care if it's common practice. 

Thank you for finishing this one!