Closed Bug 747219 Opened 12 years ago Closed 12 years ago

decomtaminate GetCellAt() on accessible tables

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

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)

Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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 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+
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
(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?
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.
(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?
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.
Attached patch Patch (v2) (obsolete) — Splinter Review
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)
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+
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?
Attached patch Patch (v3)Splinter Review
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
(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.
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.

Attachment

General

Created:
Updated:
Size: