Re: [PATCH v2] oleacc: Add support for retrieving an HWND from accNavigate to WindowFromAccessibleObject.

[Piotr Caban]

Hi Connor,

On 5/11/22 22:56, Connor McAdams wrote:
> @@ -402,20 +402,36 @@ HRESULT WINAPI WindowFromAccessibleObject(IAccessible *acc, HWND *phwnd)
>   {
>       IDispatch *parent;
>       IOleWindow *ow;
> -
>
> VARIANT v, cid;
>  HRESULT hres;
>
> TRACE("%p %p\n", acc, phwnd);
>
> IAccessible_AddRef(acc);

There's no need to increase acc refcount at this point. It can be done 
before the while loop.
> - while(1) {
> -
>    hres = IAccessible_QueryInterface(acc, &IID_IOleWindow, (void**)&ow);
> -
>    if(SUCCEEDED(hres)) {
> -
>        hres = IOleWindow_GetWindow(ow, phwnd);
> -
>        IOleWindow_Release(ow);
> -
> - ow = NULL;

I don't like setting ow to NULL here. It will be probably better to 
restructure the code so it's not needed. It leads to some errors in 
further code.
> - hres = IAccessible_QueryInterface(acc, &IID_IOleWindow, (void**)&ow);
> - if(SUCCEEDED(hres)) {
> -
>    hres = IOleWindow_GetWindow(ow, phwnd);
> -
>    IOleWindow_Release(ow);
> -
>    if(*phwnd) {

You should not base anything on *phwnd value at this point. It may be 
uninitialized if IOleWindow_GetWindow returned error.
>           IAccessible_Release(acc);
>           return hres;
>       }
> - }
> -
> - VariantInit(&v);
> - variant_init_i4(&cid, CHILDID_SELF);
> - hres = IAccessible_accNavigate(acc, 10, cid, &v);

How about defining something like NAVDIR_INTERNAL_HWND and using it 
instead of a magic constant?
> - if(SUCCEEDED(hres) && V_VT(&v) == VT_I4)
> -
>    *phwnd = IntToPtr(V_I4(&v));
> -
> - if(ow) {
> -
>    IAccessible_Release(acc);
> -
>    return S_OK;
> - }

This looks very suspicious. It depends on phwnd to be set by GetWindow 
or accNavigate call. It will e.g. not work when both GetWindow and 
accNavigate returns error. The code is hard to follow while it's written 
this way.
> - while(1) {
>      hres = IAccessible_get_accParent(acc, &parent);
>      IAccessible_Release(acc);
>      if(FAILED(hres))
>
> @@ -429,6 +445,14 @@ HRESULT WINAPI WindowFromAccessibleObject(IAccessible *acc, HWND *phwnd)
>           IDispatch_Release(parent);
>           if(FAILED(hres))
>               return hres;
> -
> -
>    hres = IAccessible_QueryInterface(acc, &IID_IOleWindow, (void**)&ow);
> -
>    if(SUCCEEDED(hres)) {
> -
>        hres = IOleWindow_GetWindow(ow, phwnd);
> -
>        IOleWindow_Release(ow);
> -
>        IAccessible_Release(acc);
> -
>        return hres;
> -
>    }
> }
> }
>
> diff --git a/dlls/oleacc/tests/main.c b/dlls/oleacc/tests/main.c
> index 7854764ec08..43dfa039f34 100644
> --- a/dlls/oleacc/tests/main.c
> +++ b/dlls/oleacc/tests/main.c
> @@ -59,8 +59,10 @@ DEFINE_EXPECT(Accessible_get_accChildCount);
>   DEFINE_EXPECT(Accessible_get_accChild);
>   DEFINE_EXPECT(Accessible_get_accName);
>   DEFINE_EXPECT(Accessible_get_accParent);
> +DEFINE_EXPECT(Accessible_accNavigate);
>   DEFINE_EXPECT(Accessible_child_get_accName);
>   DEFINE_EXPECT(Accessible_child_get_accParent);
> +DEFINE_EXPECT(Accessible_child_accNavigate);
>
> static HANDLE (WINAPI *pGetProcessHandleFromHwnd)(HWND);
>
> @@ -91,7 +93,11 @@ static BOOL iface_cmp(IUnknown *iface1, IUnknown *iface2)
>       return unk1 == unk2;
>   }
>
> +static IAccessible Accessible;
>   static IAccessible Accessible_child;
> +static IOleWindow OleWindow;
> +static HWND Accessible_accnav_hwnd = NULL;
> +static HWND OleWindow_hwnd = NULL;

static variables are already zeroed.

Thanks,
Piotr