Last Comment Bug 747227 - decomtaminate GetCellIndexAt() on accessible tables
: decomtaminate GetCellIndexAt() on accessible tables
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Max Li [:maxli]
:
Mentors:
Depends on: 739882
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-04-19 15:53 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-05-10 18:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (12.20 KB, patch)
2012-05-06 16:11 PDT, Max Li [:maxli]
no flags Details | Diff | Review
Patch v2 (11.74 KB, patch)
2012-05-07 16:17 PDT, Max Li [:maxli]
tbsaunde+mozbugs: review+
Details | Diff | Review
Patch v4 (11.77 KB, patch)
2012-05-08 13:53 PDT, Max Li [:maxli]
surkov.alexander: feedback+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-04-19 15:53:59 PDT
similar to bug 747219 look at bugs 739882 739883 739885 for a good idea of what to change where.
Comment 1 Max Li [:maxli] 2012-05-06 16:11:38 PDT
Created attachment 621471 [details] [diff] [review]
Patch v1
Comment 2 Trevor Saunders (:tbsaunde) 2012-05-06 23:04:30 PDT
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] 2012-05-07 16:17:26 PDT
Created attachment 621776 [details] [diff] [review]
Patch v2
Comment 4 Trevor Saunders (:tbsaunde) 2012-05-08 08:58:05 PDT
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] 2012-05-08 13:53:58 PDT
Created attachment 622133 [details] [diff] [review]
Patch v4
Comment 6 alexander :surkov 2012-05-10 04:34:52 PDT
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 8 Joe Drew (not getting mail) 2012-05-10 18:41:48 PDT
https://hg.mozilla.org/mozilla-central/rev/f11717aa3a10

Note You need to log in before you can comment on or make changes to this bug.