Closed
Bug 765371
Opened 12 years ago
Closed 12 years ago
decomtaminate GetSelected-RowCount / ColumnCount / CellCount on accessible tables
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: capella, Assigned: capella)
References
Details
(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])
Attachments
(1 file, 2 obsolete files)
21.15 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
Same as previous work done for bug 760880, bug 760881, bug 760878, etc.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #633777 -
Flags: review?(trev.saunders)
Comment 2•12 years ago
|
||
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•12 years ago
|
||
Good eye for details!
Attachment #633777 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Blocks: dexpcoma11y
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Attachment #633845 -
Attachment is obsolete: true
Comment 6•12 years ago
|
||
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•12 years ago
|
||
Push to TRY: https://tbpl.mozilla.org/?tree=Try&rev=4d4abea007f3
Assignee | ||
Comment 8•12 years ago
|
||
Inbound push: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=01bdb326b1e4
Target Milestone: --- → mozilla16
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/01bdb326b1e4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•