Last Comment Bug 739882 - decomtaminate getting row and column count on accessible tables
: decomtaminate getting row and column count 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:
Blocks: dexpcoma11y 747219 747227 757440
  Show dependency treegraph
 
Reported: 2012-03-27 21:08 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-05-22 07:51 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (13.40 KB, patch)
2012-04-16 18:36 PDT, Max Li [:maxli]
surkov.alexander: feedback+
Details | Diff | Review
Patch v2 (13.38 KB, patch)
2012-04-17 04:33 PDT, Max Li [:maxli]
tbsaunde+mozbugs: review+
Details | Diff | Review
Patch v3 (13.45 KB, patch)
2012-04-19 14:35 PDT, Max Li [:maxli]
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-03-27 21:08:24 PDT
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] 2012-04-16 18:36:27 PDT
Created attachment 615584 [details] [diff] [review]
Patch v1
Comment 2 alexander :surkov 2012-04-16 23:06:25 PDT
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] 2012-04-17 04:33:38 PDT
Created attachment 615664 [details] [diff] [review]
Patch v2
Comment 4 Trevor Saunders (:tbsaunde) 2012-04-18 13:17:06 PDT
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] 2012-04-19 14:35:08 PDT
Created attachment 616750 [details] [diff] [review]
Patch v3
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-04-25 07:41:44 PDT
https://hg.mozilla.org/mozilla-central/rev/bd794853712c

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