If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

implement IAccessibleTable::getSelectedXXX methods

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

16.08 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
 
(Assignee)

Comment 1

10 years ago
Created attachment 269521 [details] [diff] [review]
patch
Attachment #269521 - Flags: review?(aaronleventhal)

Updated

10 years ago
Attachment #269521 - Flags: review?(aaronleventhal) → review?(Evan.Yan)

Comment 2

10 years ago
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;

Updated

10 years ago
OS: Windows XP → All
(Assignee)

Comment 3

10 years ago
(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

10 years ago
Comment on attachment 269521 [details] [diff] [review]
patch

r=me for non-msaa part

Aaron, could you review msaa part?
Attachment #269521 - Flags: review?(aaronleventhal)
Attachment #269521 - Flags: review?(Evan.Yan)
Attachment #269521 - Flags: review+

Comment 5

10 years ago
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.
Attachment #269521 - Flags: review?(aaronleventhal) → review+

Comment 6

10 years ago
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.
(Assignee)

Updated

10 years ago
Attachment #269521 - Flags: superreview?(neil)
(Assignee)

Comment 7

10 years ago
(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

10 years ago
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.
Attachment #269521 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 9

10 years ago
Created attachment 271818 [details] [diff] [review]
patch2
Attachment #269521 - Attachment is obsolete: true
Attachment #271818 - Flags: superreview?(neil)

Comment 10

10 years ago
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...
Attachment #271818 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 11

10 years ago
Created attachment 271846 [details] [diff] [review]
patch3

with Neil's comments. Thank you for reviews.
Attachment #271818 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.