325070 - Allow accessibility to build without ATL/MFC

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Benjamin Smedberg]

Getting accessibility to build without ATL/MFC would make it a lot easier to use the MS free compilers (which do not come with ATL/MFC headers or libs). Patch forthcoming, but I don't have any clue how to test it, so Aaron you're probably going to have to apply it and make sure it does what I think it should be doing.

Comment 1

[Benjamin Smedberg]

Attachment: Use custom class to replace CComEnum, rev. 1

Comment 2

[Benjamin Smedberg]

Attachment: Use custom class to replace CComEnum, rev. 1.1

Needed a change of linker flags also.

Comment 3

[Aaron Leventhal]

Comment on attachment 210000 [details] [diff] [review]
Use custom class to replace CComEnum, rev. 1.1

We're close, but it's not working because for some reason there's a refcounting problem on pEnum. Note that we're mixing COM and XPCOM together here, which might be causing it.

For some reason adding an additional addref in get_accSelection() does make this work.
    + pvarChildren->vt = VT_UNKNOWN;    // this must be VT_UNKNOWN for an IEnumVARIANT
    + NS_ADDREF(pvarChildren->punkVal = pEnum);
   ++ NS_ADDREF(pvarChildren->punkVal);  // Temporary to show that the error is in the refcounting somewhere

Also I want to change this, because we were calling the wrong QI and getting null back :/
-  nsCOMPtr<nsIAccessibleSelectable> select;
-  nsAccessNode::QueryInterface(NS_GET_IID(nsIAccessibleSelectable), getter_AddRefs(select));
+  nsCOMPtr<nsIAccessibleSelectable> select(do_QueryInterface(NS_STATIC_CAST(nsIAccessible*, this)));

Comment 4

[Eli Friedman]

(In reply to comment #3)
> (From update of attachment 210000 [details] [diff] [review] [edit])
> We're close, but it's not working because for some reason there's a refcounting
> problem on pEnum. Note that we're mixing COM and XPCOM together here, which
> might be causing it.
> 
> For some reason adding an additional addref in get_accSelection() does make
> this work.

You forgot to AddRef in your QueryInterface implementation.  Seems like that could be the problem.

Comment 5

[Eli Friedman]

Attachment: Updated patch

I'm not sure how to test it, but this should fix the issues.

Comment 6

[Aaron Leventhal]

Comment on attachment 250322 [details] [diff] [review]
Updated patch

Tested the patch with Window-Eyes 5.5, which uses IEnumVARIANT. No regressions observed.

I haven't looked at the code a lot yet. Punting actual code review to Alexander Surkov.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 250322 [details] [diff] [review]
Updated patch

Patch looks good. I have only few comments.

>+  // IEnumVARIANT
...
>+  STDMETHODIMP Skip(unsigned long celt) { mCurIndex += celt; return S_OK; }

MSDN suppose another implementation of the method.

Return Values
If the specified number of elements was skipped, the return value is S_OK.
If the end of the sequence was reached before skipping the requested number of elements, the return value is S_FALSE.

Example
The following code implements Skip for collections in the Lines sample file Enumvar.cpp.
STDMETHODIMP
CEnumVariant::Skip(ULONG cElements)
{
m_lCurrent += cElements; 
if (m_lCurrent > (long)(m_lLBound+m_cElements))
{
m_lCurrent =  m_lLBound+m_cElements;
return ResultFromScode(S_FALSE);
}
else return NOERROR;
}


>+  STDMETHODIMP Reset() { mCurIndex = 0; return S_OK; }
>+  STDMETHODIMP Clone(IEnumVARIANT FAR* FAR* ppenum) {
>+    NS_ADDREF(*ppenum = new AccessibleEnumerator(*this));
>+    return S_OK;
>+  }

You should check result of new() operator. MSDN says:

If the method succeeds, the return value is S_OK.
If there is insufficient memory to complete the operation, the return value is E_OUTOFMEMORY.


>+HRESULT
>+AccessibleEnumerator::QueryInterface(REFIID iid, void ** ppvObject)
>+{
>+  if (iid == IID_IEnumVARIANT) {
>+    *ppvObject = static_cast<IEnumVARIANT*>(this);
>+    AddRef();
>+    return S_OK;
>+  }
>+  else if (iid == IID_IUnknown) {

nit: please up 'else if' like:

} else if (iid == IID_IUnknown) {

> /**
>   * This method is called when a client wants to know which children of a node
>   *  are selected. Currently we only handle this for HTML selects, which are the
>   *  only nsIAccessible objects to implement nsIAccessibleSelectable.

It will be great if you'll change this comment too :) because actually nsIAccessibleSelectable is supported by xhtml/xul/xforms elements. Something like 'Accessible object for this node should implement
nsIAccessibleSelectable interface' instead of 'Currently ...'. Though it's up to you.

Comment 8

[Eli Friedman]

Attachment: Final patch

I guess I'll assign this bug to myself, since I've updated the patch.

Adressed review comments, plus a couple other small issues I found (dealing with a couple unlikely potential integer overflow problems, and making get_accSelection return E_OUTOFMEMORY instead of NS_ERROR_FAILURE when creating the enumerator fails.)

Do I need an sr for this patch?

Comment 9

[Aaron Leventhal]

sr= is optional in a11y module on trunk right now, but if you think it will help, please get one. [e-mail] is very thorough and responsive.

Comment 10

[[e-mail]]

Comment on attachment 251118 [details] [diff] [review]
Final patch

>+OS_LIBS = oleaut32.lib user32.lib
I'm not build expert but IIRC this should be OS_LIBS += oleaut32.lib

>-STDMETHODIMP nsAccessibleWrap::AccessibleObjectFromWindow(HWND hwnd,
>+STDMETHODIMP nsAccessibleWrap::AccessibleObjectFromWindow(HWND hwnd,                                              
Nit: spaces crept in to the end of this line.

>+  if (iid == IID_IEnumVARIANT) {
>+    *ppvObject = static_cast<IEnumVARIANT*>(this);
>+    AddRef();
>+    return S_OK;
>+  } else if (iid == IID_IUnknown) {
Don't else after return. Just write a new if statement.

>+    nsCOMPtr<nsIAccessible> accel;
>+    mArray->QueryElementAt(mCurIndex, NS_GET_IID(nsIAccessible), getter_AddRefs(accel));
nsCOMPtr<nsIAccessible> accel(do_QueryElementAt(mArray, mCurIndex));
(you may need to include nsArrayUtils.h)

>+  // Check if we can skip the requested number of elements
>+  // (the second check is an overflow check)
>+  if (mCurIndex + celt > length || mCurIndex + celt < celt) {
You can avoid that by checking if (celt > length - mCurIndex)
(because we have the invariant that mCurIndex <= length)

>+  nsCOMPtr<nsIAccessibleSelectable> 
>+    select(do_QueryInterface(NS_STATIC_CAST(nsIAccessible*, this)));
Unnecessary, we already implement this interface.
(A space crept in to the end of the first line too.)

Comment 11

[Eli Friedman]

(In reply to comment #10)
> (From update of attachment 251118 [details] [diff] [review])
> >+OS_LIBS = oleaut32.lib user32.lib
> I'm not build expert but IIRC this should be OS_LIBS += oleaut32.lib

Okay.

> >-STDMETHODIMP nsAccessibleWrap::AccessibleObjectFromWindow(HWND hwnd,
> >+STDMETHODIMP nsAccessibleWrap::AccessibleObjectFromWindow(HWND hwnd,                                              
> Nit: spaces crept in to the end of this line.

Oops.

> >+  if (iid == IID_IEnumVARIANT) {
> >+    *ppvObject = static_cast<IEnumVARIANT*>(this);
> >+    AddRef();
> >+    return S_OK;
> >+  } else if (iid == IID_IUnknown) {
> Don't else after return. Just write a new if statement.

Fine.

> >+    nsCOMPtr<nsIAccessible> accel;
> >+    mArray->QueryElementAt(mCurIndex, NS_GET_IID(nsIAccessible), getter_AddRefs(accel));
> nsCOMPtr<nsIAccessible> accel(do_QueryElementAt(mArray, mCurIndex));
> (you may need to include nsArrayUtils.h)

Ah, didn't know that existed.

> >+  // Check if we can skip the requested number of elements
> >+  // (the second check is an overflow check)
> >+  if (mCurIndex + celt > length || mCurIndex + celt < celt) {
> You can avoid that by checking if (celt > length - mCurIndex)
> (because we have the invariant that mCurIndex <= length)

Okay.

> >+  nsCOMPtr<nsIAccessibleSelectable> 
> >+    select(do_QueryInterface(NS_STATIC_CAST(nsIAccessible*, this)));
> Unnecessary, we already implement this interface.
> (A space crept in to the end of the first line too.)

We already implement it, yes, but it's not necessarily supported, depending on the object.  See nsAccessible::QueryInterface.

Comment 12

[Aaron Leventhal]

Eli, will you be attaching a new patch for that?

Comment 13

[Eli Friedman]

Attachment: Updated Patch

Comment 14

[[e-mail]]

Comment on attachment 251308 [details] [diff] [review]
Updated Patch

>+  if (celt > length - mCurIndex) {
>+    hr = S_FALSE;
>+    celt = length - mCurIndex;
>+  }
Thanks for fixing this one too. As a bonus, my understanding is that any decent optimising compiler will user a temporary for length - mCurIndex.

Comment 15

[Aaron Leventhal]

Eli, this is pretty close. Want to get it in?

Comment 16

[Eli Friedman]

Checked in.