The default bug view has changed. See this FAQ.

decomtaminate GetSelected-RowCount / ColumnCount / CellCount on accessible tables

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: capella, Assigned: capella)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Same as previous work done for bug 760880, bug 760881, bug 760878, etc.
(Assignee)

Comment 1

5 years ago
Created attachment 633777 [details] [diff] [review]
Patch (v1)
(Assignee)

Updated

5 years ago
Attachment #633777 - Flags: review?(trev.saunders)
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.
Attachment #633777 - Flags: review?(trev.saunders)
(Assignee)

Comment 3

5 years ago
Created attachment 633845 [details] [diff] [review]
Patch (v2)

Good eye for details!
Attachment #633777 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 574336
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()
(Assignee)

Comment 5

5 years ago
Created attachment 634042 [details] [diff] [review]
Patch (v3)
Attachment #633845 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 765512
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.
Attachment #634042 - Flags: review+
(Assignee)

Comment 7

5 years ago
Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=4d4abea007f3
(Assignee)

Comment 8

5 years ago
Inbound push:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=01bdb326b1e4
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/01bdb326b1e4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.