765371 - decomtaminate GetSelected-RowCount / ColumnCount / CellCount on accessible tables

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mark Capella [:capella]]

Same as previous work done for bug 760880, bug 760881, bug 760878, etc.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 633777 [details] [diff] [review]
Patch (v1)

>   while ((row = rowIter.Next())) {
>     if (nsAccUtils::IsARIASelected(row)) {
>-      (*aCount) += colCount;
>+      count += ColCount();
>       continue;

so, if no rows are selected this is faster, but if more than one row is selected its slower.  I'd probably stick with saving the return of ColCount() in a local var for now.

>   nsITableLayout *tableLayout = GetTableLayout();
>   NS_ENSURE_STATE(tableLayout);

you should probably just return 0 here, and certainly not return a nsresult as an int.

>   PRInt32 rowIndex;
>   for (rowIndex = 0; rowIndex < rowCount; rowIndex++) {

while you're here you could move the decl of rowIndex into the for loop, and make it rowIdx  (same in a couple other places below too)

>+  for (index = 0; index < colCount; index++) {
>     bool state = false;
>-    rv = IsColumnSelected(index, &state);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsresult rv = IsColumnSelected(index, &state);
>+    NS_ENSURE_SUCCESS(rv, 0);

you could use the non-xpcom version you just added :)

>+  for (index = 0; index < rowCount; index++) {
>     bool state = false;
>-    rv = IsRowSelected(index, &state);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsresult rv = IsRowSelected(index, &state);
>+    NS_ENSURE_SUCCESS(rv, 0);

same here

>   PRInt32 selectedrowCount = 0;
>   nsresult rv = control->GetSelectedCount(&selectedrowCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_SUCCESS(rv, 0);
> 
>-  *aCount = selectedrowCount;
>-  return NS_OK;
>+  return static_cast<PRUint32>(selectedrowCount);

you should be a little careful about that, I bet things will go bang if you give them the wrong number of rows because you got back a negative number for some reason.

return selectedRowCount >= 0 ? selectedRowCount : 0; is probably easiest.

>+PRUint32
>+XULTreeGridAccessible::SelectedCellCount()
> {
>-  NS_ENSURE_ARG_POINTER(aCount);
>-  *aCount = 0;
>-
>   PRUint32 selectedrowCount = 0;
>   nsresult rv = GetSelectedRowCount(&selectedrowCount);

you could use SelectedRowCount()

>-
>   PRInt32 selectedrowCount = 0;
>   nsresult rv = GetSelectionCount(&selectedrowCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_SUCCESS(rv, 0);
> 
>-  *aCount = selectedrowCount;
>-  return NS_OK;
>+  return static_cast<PRUint32>(selectedrowCount);

we should probably have a bug to dexpcom GetSelectionCount(), but for now return selectedRowCount >= 0 ? selectedRowCount : 0; is safer.

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v2)

Good eye for details!

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 633845 [details] [diff] [review]
Patch (v2)

>+ARIAGridAccessible::SelectedCellCount()
> {
>-  NS_ENSURE_ARG_POINTER(aCount);
>-  *aCount = 0;
>+  PRUint32 count = 0;
> 
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>-  PRInt32 colCount = 0;
>-  GetColumnCount(&colCount);
>+  PRUint32 colCount = ColCount();

nit, I'd probably put them on the same line here and below

>-  PRInt32 rowIndex;
>-  for (rowIndex = 0; rowIndex < rowCount; rowIndex++) {
>-    PRInt32 columnIndex;
>-    for (columnIndex = 0; columnIndex < columnCount; columnIndex++) {
>-      rv = tableLayout->GetCellDataAt(rowIndex, columnIndex,
>-                                      *getter_AddRefs(domElement),
>-                                      startRowIndex, startColIndex,
>-                                      rowSpan, colSpan,
>-                                      actualRowSpan, actualColSpan,
>-                                      isSelected);
>+  for (PRInt32 rowIdx = 0; rowIdx < rowCount; rowIdx++) {
>+    for (PRInt32 colIdx = 0; colIdx < colCount; colIdx++) {

it would make more sense for them to be unsigned

>-    rv = IsColumnSelected(index, &state);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+  for (PRInt32 colIdx = 0; colIdx < colCount; colIdx++)

same

>-    rv = IsRowSelected(index, &state);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+  for (PRInt32 rowIdx = 0; rowIdx < rowCount; rowIdx++)

same

>+  return selectedRowCount == RowCount() ? ColCount() : 0;

so, since selectedRowCount is signed that's not quiet correct, consider the case there greater than INT_MAX rows, but less than UINT_MAX rows, and for some reason selectedRowCount is INT_MIN + (RowCount - INT_MAX)


>+  PRInt32 selectedRowCount = 0;
>+  nsresult rv = GetSelectionCount(&selectedRowCount);
>+  NS_ENSURE_SUCCESS(rv, 0);
> 
>-  PRInt32 selectedrowCount = 0;
>-  rv = GetSelectionCount(&selectedrowCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  if (rowCount == selectedrowCount) {
>-    PRInt32 columnCount = 0;
>-    rv = GetColumnCount(&columnCount);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    *aCount = columnCount;
>-  }
>-
>-  return NS_OK;
>+  return selectedRowCount == RowCount() ? ColCount() : 0;

same, use selectedRowCount > 0 && selectedRowCount == RowCount()

Comment 5

[Mark Capella [:capella]]

Attachment: Patch (v3)

Comment 6

[Trevor Saunders (:tbsaunde)]

Comment on attachment 634042 [details] [diff] [review]
Patch (v3)

please rerequest review when people haven't yet r+d so they remember to look at  patches.

Comment 7

[Mark Capella [:capella]]

Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=4d4abea007f3

Comment 8

[Mark Capella [:capella]]

Inbound push:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=01bdb326b1e4

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/01bdb326b1e4