Closed
Bug 747219
Opened 12 years ago
Closed 12 years ago
decomtaminate GetCellAt() on accessible tables
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: tbsaunde, Assigned: capella)
References
Details
(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])
Attachments
(1 file, 2 obsolete files)
13.10 KB,
patch
|
Details | Diff | Splinter Review |
same as bug 739882, bug 739883, and bug 739885
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
Asking mentor for initial feedback ... this builds and passes all mochitest-a11y's locally ... let me know what you find, and I'll fix it -- mark
Attachment #624143 -
Flags: feedback?(trev.saunders)
Comment 2•12 years ago
|
||
Comment on attachment 624143 [details] [diff] [review] Patch (v1) Review of attachment 624143 [details] [diff] [review]: ----------------------------------------------------------------- stealing feedback request from Trevor, asking him for review ::: accessible/src/html/nsHTMLTableAccessible.cpp @@ +882,3 @@ > nsCOMPtr<nsIDOMElement> cellElement; > + GetCellAt(aRowIndex, aColumnIndex, *getter_AddRefs(cellElement)); > + NS_ENSURE_TRUE(cellElement, nsnull); no warning, just return null @@ +886,5 @@ > nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement)); > + if (!cellContent) > + return nsnull; > + > + NS_ENSURE_TRUE(mDoc, nsnull); it shouldn't be null, so do not add it ::: accessible/src/xpcom/xpcAccessibleTable.cpp @@ +60,5 @@ > + if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() || > + aColumnIndex < 0 || aColumnIndex >= mTable->ColCount()) > + return NS_ERROR_INVALID_ARG; > + > + NS_IF_ADDREF(*aCell = mTable->CellAt(aRowIndex, aColumnIndex)); I think I'd check *aCell instead of row and column indices and it's null then return invalid_arg ::: accessible/src/xul/nsXULListboxAccessible.cpp @@ +289,4 @@ > > nsCOMPtr<nsIDOMXULSelectControlItemElement> item; > + control->GetItemAtIndex(aRowIndex, getter_AddRefs(item)); > + NS_ENSURE_TRUE(item, nsnull); no warning pls @@ +297,2 @@ > > + NS_ENSURE_TRUE(mDoc, nsnull); no check ::: accessible/src/xul/nsXULTreeGridAccessible.cpp @@ +333,5 @@ > > +nsAccessible* > +nsXULTreeGridAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex) > +{ > + nsAccessible* rowAccessible = GetTreeItemAccessible(aRowIndex); rowAccessible -> row please @@ +345,4 @@ > > nsRefPtr<nsXULTreeItemAccessibleBase> rowAcc = do_QueryObject(rowAccessible); > + if (!rowAcc) > + return nsnull; would you mind to file a bug to add a downcasting to nsXULTreeItemAccessibleBase (or alternatively to make GetTreeItemAccessible to return nsXULTreeItemAccessibleBase*)
Attachment #624143 -
Flags: review?(trev.saunders)
Attachment #624143 -
Flags: feedback?(trev.saunders)
Attachment #624143 -
Flags: feedback+
Assignee | ||
Comment 3•12 years ago
|
||
FYI, I had to leave the row and column indices check in the xpcAccessibleTable.cpp member function GetCellAt(), that I'd copied from the existing GetCellIndexAt() member. Without the check, we fail getCellAt() at line number 79 with NS_ERROR_ILLEGAL_VALUE ... http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/table/test_table_2.html?force=1#77
Comment 4•12 years ago
|
||
(In reply to Mark Capella [:capella] from comment #3) > Without the check, we fail getCellAt() at line number 79 with > NS_ERROR_ILLEGAL_VALUE ... > http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/ > table/test_table_2.html?force=1#77 I'm curious what returns this value?
Assignee | ||
Comment 5•12 years ago
|
||
I had changed all the nits above as requested, and found that one item to fail out. Putting it back causes the test to pass. Now there is an additional issue where test_general.html hangs ... I'm putting things back one at a time and testing to see what's going on.
Comment 6•12 years ago
|
||
(In reply to Mark Capella [:capella] from comment #5) > I had changed all the nits above as requested, and found that one item to > fail out. Putting it back causes the test to pass. Yes, I get this but I don't understand the cause. I didn't see where you can return this error value. > Now there is an additional issue where test_general.html hangs ... I'm > putting things back one at a time and testing to see what's going on. it shouldn't be related, btw, which test_general.html?
Assignee | ||
Comment 7•12 years ago
|
||
My mistake, the hanging test is treeupdate/test_imagemap.html ... if I pull all my patches out of the queue and re-build and re-test, the problem is still there so my patch can't be responsible. fwiw, if while the test is hanging, I click on a background window (shift the focus?) the test then continues, and runs to completion successfully.
Assignee | ||
Comment 8•12 years ago
|
||
Next version of the patch with nits addressed, asking tbsaunde for review?. FYI, a fresh pull, and a non-debug / clobber build resolved all test issues :)
Attachment #624143 -
Attachment is obsolete: true
Attachment #624143 -
Flags: review?(trev.saunders)
Attachment #624336 -
Flags: review?(trev.saunders)
Reporter | ||
Comment 9•12 years ago
|
||
Comment on attachment 624336 [details] [diff] [review] Patch (v2) >+nsHTMLTableAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex) >+{ > nsCOMPtr<nsIDOMElement> cellElement; >- nsresult rv = GetCellAt(aRow, aColumn, *getter_AddRefs(cellElement)); >- NS_ENSURE_SUCCESS(rv, rv); >+ GetCellAt(aRowIndex, aColumnIndex, *getter_AddRefs(cellElement)); >+ if (!cellElement) >+ return nsnull; > > nsCOMPtr<nsIContent> cellContent(do_QueryInterface(cellElement)); >+ if (!cellContent) >+ return nsnull; >+ > nsAccessible* cell = mDoc->GetAccessible(cellContent); >+ if (!cell) >+ return nsnull; you never derefrence it after this so no need for a check. > >- if (!cell) { >- return NS_ERROR_INVALID_ARG; >+ if (cell == this) { >+ // XXX bug 576838: crazy tables (like table6 in tables/test_table2.html) may >+ // return itself as a cell what makes Orca hang. >+ return nsnull; > } nit, be nice to do // XXX blah blah // blahh if (cell == this) return nsnull; return cell; >+ if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() || >+ aColumnIndex < 0 || aColumnIndex >= mTable->ColCount()) you need to case aRowIdx / aColIdx to PRUint32 before comparing to RowCount() / ColCount() to keep gcc happy >+nsXULListboxAccessible::CellAt(PRUint32 aRowIndex, PRUint32 aColumnIndex) >+{ > nsCOMPtr<nsIDOMXULSelectControlElement> control = > do_QueryInterface(mContent); >+ if (!control) >+ return nsnull; you probably should warn here since mContent should have the right type given we created this type of accessible.
Attachment #624336 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Re: >+ if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() || >+ aColumnIndex < 0 || aColumnIndex >= mTable->ColCount()) you need to case aRowIdx / aColIdx to PRUint32 before comparing to RowCount() / ColCount() to keep gcc happy This code was cloned from existing member function GetCellIndexAt() http://mxr.mozilla.org/mozilla-central/source/accessible/src/xpcom/xpcAccessibleTable.cpp#51 I don't see why gcc would complain about one and not the other, but I'm a WIN guy so it may not be obvious to me. Can you elaborate?
Assignee | ||
Comment 11•12 years ago
|
||
FYI, we've got feedback+ and review+... this would be the final version unless Trev has further details for the gcc comments.
Attachment #624336 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
Push to inbound TRY https://tbpl.mozilla.org/?tree=Try&rev=eea7a9606f4e
Assignee | ||
Comment 13•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c
Target Milestone: --- → mozilla15
Comment 14•12 years ago
|
||
(In reply to Mark Capella [:capella] from comment #13) > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a6feee6c13c https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6feee6c13c
Reporter | ||
Comment 15•12 years ago
|
||
(In reply to Mark Capella [:capella] from comment #10) > Re: > >+ if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() || > >+ aColumnIndex < 0 || aColumnIndex >= mTable->ColCount()) > you need to case aRowIdx / aColIdx to PRUint32 before comparing to > RowCount() / ColCount() to keep gcc happy > > This code was cloned from existing member function GetCellIndexAt() > http://mxr.mozilla.org/mozilla-central/source/accessible/src/xpcom/ > xpcAccessibleTable.cpp#51 > > I don't see why gcc would complain about one and not the other, but I'm a > WIN guy so it may not be obvious to me. it does complain about the other, but its only a warning.
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7a6feee6c13c
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
•