739882 - decomtaminate getting row and column count on accessible tables

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

similar to what was done with  IsProbablyForLayout and Caption in bug 648265.
implement the methods RowCount() and ColCount() from TableAccessible on each of nsARIAGridAccessible nsHTMLTableAccessible nsXULListboxAccessible nsXULTreeGridAccessible
the same way GetRowCount() and GetColumnCount() currently are then remove GGetRowCount() and GetColumnCount()
implement GetRowCount() and and GetColumnCount() on xpcAccessibleTable using the TableAccessible pointer member
finally adjust the NS_DECL_OR_FORWARD_WITH_XPCACCESSIBLETABLE macro in accessible/src/xpcom/xpcAccessibleTable.h to forward GetRowCount() and GetColumnCount() to xpcAccessibleTable.

Comment 1

[Max Li [:maxli]]

Attachment: Patch v1

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 615584 [details] [diff] [review]
Patch v1

Review of attachment 615584 [details] [diff] [review]:
-----------------------------------------------------------------

please attach new patch and get review from Trevor

::: accessible/src/generic/ARIAGridAccessible.cpp
@@ +90,2 @@
>    if (IsDefunct())
> +    return 0;

you don't need IsDefunct check, now it's internal method

@@ +110,3 @@
>  {
> +  if (IsDefunct())
> +    return 0;

same

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +569,2 @@
>    if (IsDefunct())
> +    return 0;

same

@@ +573,2 @@
>  
> +  PRInt32 rows, columns;

initialize them please

@@ +573,4 @@
>  
> +  PRInt32 rows, columns;
> +  tableLayout->GetTableSize(rows, columns);
> +  return columns;

wonder if you get any warnings here. btw, I filed bug 746060 for the GetTableSize issue.

@@ +582,2 @@
>    if (IsDefunct())
> +    return 0;

no defuct pls

@@ +586,2 @@
>  
> +  PRInt32 rows, columns;

initialize

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +21,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +xpcAccessibleTable::GetColumnCount(PRInt32 *aColumnCount)

type* name

@@ +33,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +xpcAccessibleTable::GetRowCount(PRInt32 *aRowCount)

type* name

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +23,5 @@
>    xpcAccessibleTable(mozilla::a11y::TableAccessible* aTable) : mTable(aTable) { }
>  
>    nsresult GetCaption(nsIAccessible** aCaption);
> +  nsresult GetColumnCount(PRInt32 *aColumnCount);
> +  nsresult GetRowCount(PRInt32 *aRowCount);

same

@@ +39,5 @@
>      { return xpcAccessibleTable::GetSummary(aSummary); } \
> +  NS_SCRIPTABLE NS_IMETHOD GetColumnCount(PRInt32 *aColumnCount) \
> +    { return xpcAccessibleTable::GetColumnCount(aColumnCount); } \
> +  NS_SCRIPTABLE NS_IMETHOD GetRowCount(PRInt32 *aRowCount) \
> +    { return xpcAccessibleTable::GetRowCount(aRowCount); } \

same

::: accessible/src/xul/nsXULListboxAccessible.cpp
@@ +247,2 @@
>    if (IsDefunct())
> +    return 0;

no defunct

@@ +276,2 @@
>    if (IsDefunct())
> +    return 0;

same

::: accessible/src/xul/nsXULTreeGridAccessible.cpp
@@ +77,3 @@
>  {
> +  if (IsDefunct())
> +    return 0;

no defunct

@@ +86,3 @@
>  {
> +  if (IsDefunct() || !mTreeView)
> +    return 0;

no defunct check

@@ +89,2 @@
>  
> +  PRInt32 rowCount;

initialize it

@@ +89,4 @@
>  
> +  PRInt32 rowCount;
> +  mTreeView->GetRowCount(&rowCount);
> +  return rowCount;

I filed bug 746063 about nsITreeView issue, but probably you should make sure that rowCount >= 0 before you return it. Otherwise you end up with not desired results I think.

Comment 3

[Max Li [:maxli]]

Attachment: Patch v2

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 615664 [details] [diff] [review]
Patch v2

>   AccIterator cellIter(row, filters::GetCell);
>   nsAccessible *cell = nsnull;
> 
>+  PRUint32 columnCount = 0;

nit, colCount

>-  PRInt32 rows;
>-  return tableLayout->GetTableSize(rows, *acolumnCount);
>+  PRInt32 rows = 0, columns = 0;
>+  tableLayout->GetTableSize(rows, columns);

nit, rowCount colCount

>-  PRInt32 columns;
>-  return tableLayout->GetTableSize(*arowCount, columns);
>+  PRInt32 rows = 0, columns = 0;

same

>+xpcAccessibleTable::GetColumnCount(PRInt32* aColumnCount)
>+{
>+  NS_ENSURE_ARG_POINTER(aColumnCount);
>+  *aColumnCount = 0;
>+  if (!mTable)

nit, blank line before if

>+xpcAccessibleTable::GetRowCount(PRInt32* aRowCount)
>+{
>+  NS_ENSURE_ARG_POINTER(aRowCount);
>+  *aRowCount = 0;
>+  if (!mTable)

same

>   nsCOMPtr<nsIDOMXULSelectControlElement> element(do_QueryInterface(mContent));
>-  NS_ENSURE_STATE(element);
> 
>   PRUint32 itemCount = 0;
>-  nsresult rv = element->GetItemCount(&itemCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  element->GetItemCount(&itemCount);

you probably shouldn't rely on the QueryInterface succeding.

general comment you shtype* name not type *name.

r=me with those fixed

btw you should change the methods that call GetRowCount() / GetColumnCount() to call the new methods, but doing that as other methods are converted or in follow ups is probably fine.

sorry about the delay in reviewing this, was busy with school and life.

Comment 5

[Max Li [:maxli]]

Attachment: Patch v3

Comment 6

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd794853712c

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/bd794853712c