[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 <cmcadams at codeweavers.com>
> > ---
> >  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.