Re: [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