Last Comment Bug 765371 - decomtaminate GetSelected-RowCount / ColumnCount / CellCount on accessible tables
: decomtaminate GetSelected-RowCount / ColumnCount / CellCount on accessible ta...
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on:
Blocks: dexpcoma11y 765512
  Show dependency treegraph
 
Reported: 2012-06-15 14:17 PDT by Mark Capella [:capella]
Modified: 2012-06-21 04:06 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (20.76 KB, patch)
2012-06-16 01:19 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (21.15 KB, patch)
2012-06-16 13:32 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (21.15 KB, patch)
2012-06-18 08:12 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Mark Capella [:capella] 2012-06-15 14:17:11 PDT
Same as previous work done for bug 760880, bug 760881, bug 760878, etc.
Comment 1 Mark Capella [:capella] 2012-06-16 01:19:01 PDT
Created attachment 633777 [details] [diff] [review]
Patch (v1)
Comment 2 Trevor Saunders (:tbsaunde) 2012-06-16 03:37:49 PDT
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] 2012-06-16 13:32:07 PDT
Created attachment 633845 [details] [diff] [review]
Patch (v2)

Good eye for details!
Comment 4 Trevor Saunders (:tbsaunde) 2012-06-18 07:12:33 PDT
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] 2012-06-18 08:12:28 PDT
Created attachment 634042 [details] [diff] [review]
Patch (v3)
Comment 6 Trevor Saunders (:tbsaunde) 2012-06-20 04:42:04 PDT
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] 2012-06-20 05:49:49 PDT
Push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=4d4abea007f3
Comment 8 Mark Capella [:capella] 2012-06-20 11:10:53 PDT
Inbound push:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=01bdb326b1e4
Comment 9 Ed Morley [:emorley] 2012-06-21 04:06:18 PDT
https://hg.mozilla.org/mozilla-central/rev/01bdb326b1e4

Note You need to log in before you can comment on or make changes to this bug.