[PATCH v4 2/2] oleacc: Add tests for AccessibleObjectFromEvent.

[Piotr Caban]

Hi Connor,

On 8/30/21 5:27 PM, Connor McAdams wrote:
> +typedef struct {
> +    IAccessible IAccessible_iface;
> +    IEnumVARIANT IEnumVARIANT_iface;
> +
> +    LONG ref;
> +
> +    const acc_tree_object_info *info;
> +
> +    HWND hwnd;
> +    UINT enum_pos;
> +
> +    IAccessible *parent;
> +    IAccessible **children;
You can just define it as:
IAccessible *children[3];
and avoid allocations/frees.

> +} AccTreeObject;
> +
> +AccTreeObject *object_tree = NULL;
> +BOOL acc_query_interface_test_fail = FALSE;
static

> +static IAccessible *get_acc_tree_obj_child(IAccessible *iface, VARIANT child_id)
> +{
> +    AccTreeObject *obj = impl_from_AccTreeObject(iface);
> +    IAccessible *acc = NULL;
> +    LONG i;
> +
> +    if (V_VT(&child_id) != VT_I4)
> +        return NULL;
> +
> +    if (V_I4(&child_id) == CHILDID_SELF)
> +        return iface;
> +
> +    for (i = 0; i < obj->info->child_count; i++)
> +    {
> +        AccTreeObject *child = impl_from_AccTreeObject(obj->children[i]);
> +
> +        if (child->info->child_id == V_I4(&child_id))
> +        {
> +            acc = &child->IAccessible_iface;
Even so it's internal function it's good to update refcount:
IAccessible_AddRef(acc);
There's lot's of places that are hard to follow without it.

> +static void check_acc_tree_obj_for_child(IAccessible *acc,
> +        INT child_id, IAccessible **found_acc, VARIANT *found_vid)
> +{
> +    IDispatch *disp_child;
> +    VARIANT vid;
> +    HRESULT hr;
> +
> +    V_VT(&vid) = VT_I4;
> +    V_I4(&vid) = child_id;
> +    hr = IAccessible_get_accChild(acc, vid, &disp_child);
> +    if (SUCCEEDED(hr))
> +    {
> +        /*
> +         * If S_FALSE is returned, the childID was found, but it's a simple
> +         * element.
> +         */
> +        if (hr == S_FALSE)
> +        {
> +            *found_acc = acc;
> +            V_VT(found_vid) = VT_I4;
> +            V_I4(found_vid) = child_id;
> +        }
> +        else
> +        {
> +            IDispatch_QueryInterface(disp_child, &IID_IAccessible,
> +                    (void **)found_acc);
> +            V_VT(found_vid) = VT_I4;
> +            V_I4(found_vid) = CHILDID_SELF;
> +            IDispatch_Release(disp_child);
> +        }
> +    }
> +}
The function depends on found_acc and found_vid to be initialized to 
handle get_accChild errors. While it's a helper it's still not the best 
design. You can also avoid indentation level by returning early on 
get_accChild failure.

> +    children = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> +            sizeof(*children) * child_cnt);
> +    hr = AccessibleChildren(acc, 0, child_cnt, children, &rcv);
> +    ok(SUCCEEDED(hr), "AccessibleChildren failed with %#x!\n", hr);
It would be better to check exact hr value:
ok(hr == S_OK, ...

> +static void search_acc_tree_for_child_navigate(IAccessible *acc,
> +        INT child_id, IAccessible **found_acc, VARIANT *found_vid)
> +{
> +    VARIANT vid, res;
> +    HRESULT hr;
> +
> +    check_acc_tree_obj_for_child(acc, child_id, found_acc, found_vid);
> +    if (*found_acc)
> +        return;
> +
> +    V_VT(&vid) = VT_I4;
> +    V_I4(&vid) = CHILDID_SELF;
> +    hr = IAccessible_accNavigate(acc, NAVDIR_FIRSTCHILD, vid, &res);
> +    if (FAILED(hr))
> +        return;
> +
> +    while (SUCCEEDED(hr) && V_VT(&res) != VT_EMPTY && !(*found_acc))
> +    {
> +        switch (V_VT(&res))
> +        {
> +        case VT_I4:
> +            vid = res;
> +            hr = IAccessible_accNavigate(acc, NAVDIR_NEXT, vid, &res);
> +            break;
> +
> +        case VT_DISPATCH:
> +        {
> +            IAccessible *acc_child;
> +
> +            hr = IDispatch_QueryInterface(V_DISPATCH(&res), &IID_IAccessible, (void **)&acc_child);
> +            IDispatch_Release(V_DISPATCH(&res));
> +
> +            if (SUCCEEDED(hr))
> +            {
> +                search_acc_tree_for_child_navigate(acc_child, child_id, found_acc, found_vid);
> +
> +                if (!(*found_acc))
> +                {
> +                    V_VT(&vid) = VT_I4;
> +                    V_I4(&vid) = CHILDID_SELF;
> +                    hr = IAccessible_accNavigate(acc_child, NAVDIR_NEXT, vid, &res);
> +                }
> +
> +                if (*found_acc != acc_child)
> +                    IAccessible_Release(acc_child);
> +            }
> +            break;
> +        }
> +
> +        /*
> +         * Shouldn't ever reach here, if type isn't VT_I4 or VT_DISPATCH,
> +         * we've got a problem.
> +         */
> +        default:
> +            return;
It's better to remove the comment and add something like:
ok(0, ...

> +HRESULT WINAPI AccTreeObject_GetTypeInfoCount(
> +        IAccessible *iface, UINT *pctinfo)
> +{
> +    *pctinfo = 0;
> +
> +    return S_OK;
> +}
The function is not called. It's better to do something like:
ok(0, "unexpected call\n");
return E_NOTIMPL;
The same applies to other IDispatch methods (the ok() call may be even 
skipped).

> +HRESULT WINAPI AccTreeObject_get_accParent(
> +        IAccessible *iface, IDispatch **ppdispParent)
> +{
> +    AccTreeObject *This = impl_from_AccTreeObject(iface);
> +
> +    *ppdispParent = NULL;
> +    if (This->parent)
> +    {
> +        IAccessible_QueryInterface(This->parent, &IID_IDispatch, (void **)ppdispParent);
> +	return S_OK;
> +    }
> +
> +    return S_FALSE;
There's a '\t' before return S_OK. I would implement the function in 
following way:
if (This->parent)
     return IAccessible_QueryInterface(...);
*ppdispParent = NULL;
return S_FALSE;
It would be also nice to change the ppdispParent argument name.
>   
> +static void test_AccessibleObjectFromEvent(void)
> +{
> +    IAccessible *acc, *acc_child;
> +    const WCHAR *expected_name;
> +    BSTR obj_name;
> +    VARIANT cid;
> +    HRESULT hr;
> +    HWND hwnd;
> +
> +    hwnd = CreateWindowA("oleacc_test", "test", WS_OVERLAPPEDWINDOW,
> +            0, 0, 0, 0, NULL, NULL, NULL, NULL);
> +    ok(hwnd != NULL, "CreateWindow failed\n");
> +
> +    hr = create_acc_obj_tree(acc_from_event_obj_tree,
> +            ARRAY_SIZE(acc_from_event_obj_tree));
> +    if (FAILED(hr))
> +    {
> +        trace("Failed to create accessible object tree, hr %#x, aborting test.\n", hr);
> +        DestroyWindow(hwnd);
> +        return;
> +    }
Are you ever expecting it to fail? A simple ok(hr == S_OK, ...) should 
be enough.

Thanks,
Piotr