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