747227 - decomtaminate GetCellIndexAt() on accessible tables

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

similar to bug 747219 look at bugs 739882 739883 739885 for a good idea of what to change where.

Comment 1

[Max Li [:maxli]]

Attachment: Patch v1

Comment 2

[Trevor Saunders (:tbsaunde)]

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

since 3 of the implementations are the same I'd suggest changing the impl in TableAccessible to be reuturn ColCount() * aRowIdx + aColIdx; 

>+ARIAGridAccessible::CellIndexAt(PRUint32 aRowIdx, PRUint32 aColIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aCellIndex);
>-  *aCellIndex = -1;
>+  if (aRowIdx < 0 || aColIdx < 0)
>+    return -1;  

trivially always true

>+  PRInt32 rowCount = RowCount();
>+  if (aRowIdx >= rowCount)
>+    return -1;
>+  
>+  PRInt32 colsCount = ColCount();
>+  if (aColIdx >= colsCount)
>+    return -1;

only check these conditions in callers that actually need it.

it would be nice if you updated the use in atk/ as well.

Comment 3

[Max Li [:maxli]]

Attachment: Patch v2

Comment 4

[Trevor Saunders (:tbsaunde)]

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

>+  TableAccessible* table = accWrap->AsTable();
>+  NS_ENSURE_TRUE(table, nsnull);

nit, -1 not nsnull since you don't return a pointer

> 
>-    PRInt32 index;
>-    nsresult rv = accTable->GetCellIndexAt(aRow, aColumn, &index);
>-    NS_ENSURE_SUCCESS(rv, -1);
>-
>-    return static_cast<gint>(index);
>+  PRInt32 index = table->CellIndexAt(aRow, aColumn);
>+  return static_cast<gint>(index);

nit, no need for the local variable

>+xpcAccessibleTable::GetCellIndexAt(PRInt32 rowIndex, PRInt32 columnIndex, PRInt32 *aCellIndex)

nit, please don't go over 80 chars, here and possibly below.

>+{

Comment 5

[Max Li [:maxli]]

Attachment: Patch v4

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 622133 [details] [diff] [review]
Patch v4

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

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +905,4 @@
>  {
> +  nsITableLayout* tableLayout = GetTableLayout();
> +  PRInt32 index = -1;
> +  

nit: empty line contains whitespaces, I'll fix this before landing

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +48,5 @@
>  }
>  
>  nsresult
> +xpcAccessibleTable::GetCellIndexAt(PRInt32 rowIndex, PRInt32 columnIndex,
> +                                   PRInt32* aCellIndex)

aRowIndex, aColIndex

::: accessible/src/xpcom/xpcAccessibleTable.h
@@ +25,5 @@
>    nsresult GetCaption(nsIAccessible** aCaption);
>    nsresult GetSummary(nsAString& aSummary);
>    nsresult GetColumnCount(PRInt32* aColumnCount);
>    nsresult GetRowCount(PRInt32* aRowCount);
> +  nsresult GetCellIndexAt(PRInt32 rowIndex, PRInt32 columnIndex, 

nit: space at the end (please configure your editor to avoid it)

@@ +26,5 @@
>    nsresult GetSummary(nsAString& aSummary);
>    nsresult GetColumnCount(PRInt32* aColumnCount);
>    nsresult GetRowCount(PRInt32* aRowCount);
> +  nsresult GetCellIndexAt(PRInt32 rowIndex, PRInt32 columnIndex, 
> +                          PRInt32* aCellIndex);

same

Comment 7

[alexander :surkov (:asurkov)]

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

Comment 8

[Joe Drew (not getting mail)]

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