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

[Connor McAdams]

On Fri, Aug 27, 2021 at 09:26:09AM +0100, Huw Davies wrote:
> 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?

So, according to MSDN, get_accChild is supposed to return 'S_FALSE' if
the child ID that is passed in represents a 'simple element', which is
an element that has its data retrieved from its parent IAccessible. If
the child ID represents a full IAccessible 'child', it returns S_OK and
an IDispatch interface. However, I've found through testing, that even
if get_accChild returns a failure code, AccessibleObjectFromEvent
doesn't fail, it just behaves as though it went through the simple
element path (S_FALSE) path. A FIXME may not be the best way to signify
this, but I feel like it's a little bit useful to have some indication
if this method returned a failure code for logging. Maybe a WARN or a
TRACE or something might be more appropriate. There is a test for this
in my current tests.
> > -
> > - 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.

Thanks for the review, and I'll get to work on cleaning things up for a
v2.