Re: [PATCH 1/2] oleacc: Implement AccessibleObjectFromEvent.

[Huw Davies]

On Tue, Aug 10, 2021 at 08:42:56PM -0400, Connor McAdams wrote:
> Signed-off-by: Connor McAdams
>
> dlls/oleacc/main.c      | 39 +++++++++++++++++++++++++++++++++++++++
>  dlls/oleacc/oleacc.spec |  2 +-
>  2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/oleacc/main.c b/dlls/oleacc/main.c
> index f6b66a8bcab..fd85518436c 100644
> --- a/dlls/oleacc/main.c
> +++ b/dlls/oleacc/main.c
> @@ -331,6 +331,45 @@ HRESULT WINAPI AccessibleObjectFromPoint( POINT ptScreen, IAccessible** ppacc, V
>      return E_NOTIMPL;
>  }
>
> +HRESULT WINAPI AccessibleObjectFromEvent( HWND hwnd, DWORD dwObjectID, DWORD dwChildID,
> -
>                         IAccessible** ppacc, VARIANT* pvarChild )

Not essential (and not done in most of this file) but we typically
drop the "dwppsz" prefix nonsense from variable names.  This function
is a little tricky as there are lots of uses of "child".  The 2nd param
could be "object_id", 3rd: "child_id", 4th: "acc" (maybe), 5th:
prehaps "child_out".  For the 1st, "hwnd" is pretty common or you
could use "win"/"wnd"/"window".
> +{
> - IDispatch *child;
> - VARIANT cid;
> - HRESULT hr;
> -
> - TRACE("%p %d %d %p %p\n", hwnd, dwObjectID, dwChildID, ppacc, pvarChild);
> -
> - hr = AccessibleObjectFromWindow(hwnd, dwObjectID, &IID_IAccessible, (void **)ppacc);
> - if (FAILED(hr))
> -
>    return hr;

I think having a separate variable for the window's IAccesible iface
makes sense here ("window_acc"/"parent_acc"?), rather than using the
supplied one.  You need a separate variable in the "if (child)" case
below anyway.
> -
> - V_VT(&cid) = VT_I4;
> - V_I4(&cid) = dwChildID;

It probably makes sense to add a helper to init these variants,
something like:
void variant_i4_init( VARIANT *v, int val );

Perhaps rename "cid" to "child_id_variant" (as I already mentioned this
function's overuse of "child" is tricky!)
> - hr = IAccessible_get_accChild(*ppacc, cid, &child);
> - if (FAILED(hr))
> -
>    FIXME("get_accChild failed with %#x!\n", hr);

It's unclear to me, having not spent any time looking at this API,
what failure here means.  Should we just return the failure (after
releasing "acc")?  The FIXME() implies it's not handled and yet
presumably that corresponds to the !child case below, so it seems like
you're trying to handle it?  Could this be tested?
> -
> - V_VT(pvarChild) = VT_I4;
> - if (child)
> - {
> -
>    IAccessible *acc;
> -
> -
>    if (SUCCEEDED(IDispatch_QueryInterface(child, &IID_IAccessible, (void **)&acc)))
> -
>    {
> -
>        IAccessible_Release(*ppacc);
> -
>        *ppacc = acc;
> -
>    }
> -
> -
>    IDispatch_Release(child);
> -
>    V_I4(pvarChild) = CHILDID_SELF;
> - }
> - else
> -
>    V_I4(pvarChild) = dwChildID;

This block would be cleaned up using the variant init helper and
"window_acc".

Huw.