385573 - implement IAccessibleTable::getSelectedXXX methods

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Evan Yan]

Comment on attachment 269521 [details] [diff] [review]
patch

>Index: accessible/src/atk/nsXULTreeAccessibleWrap.cpp
>
>+NS_IMETHODIMP
>+nsXULTreeAccessibleWrap::GetSelectedCellsCount(PRUint32* aCount)
>+{
>+  return NS_ERROR_NOT_IMPLEMENTED;
>+}
>+

Shouldn't we put these methods in base/nsXULTreeAccessible.cpp?
And is there a reason we don't implement these for xul tree table?

>+NS_IMETHODIMP
>+nsHTMLTableAccessible::GetSelectedCells(PRUint32 *aNumCells,
>+                                        PRInt32 **aCells)
[...]
>+  for (rowIndex = 0; rowIndex < rowsCount; rowIndex++) {
>+    PRInt32 columnIndex;
>+    for (columnIndex = 0; columnIndex < columnsCount; columnIndex++) {
>+      PRInt32 index = rowIndex * columnIndex + columnIndex;

I guess it should be (rowIndex * columnsCount + columnIndex), in both of the two loops.

>+  for (rowIndex = 0; rowIndex < rowsCount; rowIndex++) {
>+    PRInt32 columnIndex;
>+    for (columnIndex = 0; columnIndex < columnsCount; columnIndex++) {
>+      PRInt32 index = rowIndex * columnIndex + columnIndex;

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> Shouldn't we put these methods in base/nsXULTreeAccessible.cpp?
> And is there a reason we don't implement these for xul tree table?

No until nsXULTreeAccessible implements nsIAccessibleTable. Iirc we have bug for this.

> I guess it should be (rowIndex * columnsCount + columnIndex), in both of the
> two loops.

right, I'll fix it

Comment 4

[Evan Yan]

Comment on attachment 269521 [details] [diff] [review]
patch

r=me for non-msaa part

Aaron, could you review msaa part?

Comment 5

[Aaron Leventhal]

Comment on attachment 269521 [details] [diff] [review]
patch

r+ for MSAA part. Using C style arrays is a little dangerous, so I'm going to request it gets sr'd as well.

Comment 6

[Aaron Leventhal]

When I say dangerous to use C arrays, I mean it's always a concern to avoid buffer overruns, not just for stability but also for security.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 269521 [details] [diff] [review])
> r+ for MSAA part. Using C style arrays is a little dangerous, so I'm going to
> request it gets sr'd as well.
> 

Ok, asked Neil for sr.

Comment 8

[[e-mail]]

Comment on attachment 269521 [details] [diff] [review]
patch

>+  PRBool *states = new PRBool[cellsCount];
Consider using an nsAutoArrayPtr here.

>+      PRInt32 index = rowIndex * columnIndex + columnIndex;
This calculation is wrong. You'll find it easier to set index to 0 before the loop and increment it at the end of each inner loop.

>+HRESULT
>+CAccessibleTable::GetSelectedItems(long aMaxItems, long **aItems,
>+                                   long *aItemsCount, eItemsType aType)
>+{
>+  *aItems = NULL;
My COM-fu isn't that good but I think aItems is an in parameter preallocated of size aMaxItems.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 10

[[e-mail]]

Comment on attachment 271818 [details] [diff] [review]
patch2

>+  nsAutoArrayPtr<PRInt32> cellsArray(new PRInt32[*aNumCells]);
I don't mind this but it's less useful than for states.

>+HRESULT
>+CAccessibleTable::GetSelectedItems(long aMaxItems, long **aItems,
>+                                   long *aItemsCount, eItemsType aType)
>+{
>+  *aItems = NULL;
You removed the other assignment to *aItems but you forgot this one...

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch3

with Neil's comments. Thank you for reviews.

Comment 12

[alexander :surkov (:asurkov)]

checked in

Checking in accessible/public/nsIAccessibleTable.idl;
/cvsroot/mozilla/accessible/public/nsIAccessibleTable.idl,v  <--  nsIAccessibleTable.idl
new revision: 1.8; previous revision: 1.7
done
Checking in accessible/src/atk/nsXULTreeAccessibleWrap.cpp;
/cvsroot/mozilla/accessible/src/atk/nsXULTreeAccessibleWrap.cpp,v  <--  nsXULTreeAccessibleWrap.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in accessible/src/html/nsHTMLTableAccessible.cpp;
/cvsroot/mozilla/accessible/src/html/nsHTMLTableAccessible.cpp,v  <--  nsHTMLTableAccessible.cpp
new revision: 1.53; previous revision: 1.52
done
Checking in accessible/src/msaa/CAccessibleTable.cpp;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleTable.cpp,v  <--  CAccessibleTable.cpp
new revision: 1.4; previous revision: 1.3
done
Checking in accessible/src/msaa/CAccessibleTable.h;
/cvsroot/mozilla/accessible/src/msaa/CAccessibleTable.h,v  <--  CAccessibleTable.h
new revision: 1.2; previous revision: 1.1
done