The default bug view has changed. See this FAQ.

decomtaminate GetCellIndexAt() on accessible tables

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: maxli)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
similar to bug 747219 look at bugs 739882 739883 739885 for a good idea of what to change where.
(Assignee)

Updated

5 years ago
Assignee: nobody → maxli
(Assignee)

Comment 1

5 years ago
Created attachment 621471 [details] [diff] [review]
Patch v1
Attachment #621471 - Flags: review?(trev.saunders)
(Reporter)

Comment 2

5 years ago
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.
Attachment #621471 - Flags: review?(trev.saunders)
(Assignee)

Comment 3

5 years ago
Created attachment 621776 [details] [diff] [review]
Patch v2
Attachment #621471 - Attachment is obsolete: true
Attachment #621776 - Flags: review?(trev.saunders)
(Reporter)

Comment 4

5 years ago
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.

>+{
Attachment #621776 - Flags: review?(trev.saunders)
Attachment #621776 - Flags: review+
Attachment #621776 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 5

5 years ago
Created attachment 622133 [details] [diff] [review]
Patch v4
Attachment #621776 - Attachment is obsolete: true
Attachment #621776 - Flags: feedback?(surkov.alexander)
Attachment #622133 - Flags: feedback?(surkov.alexander)

Comment 6

5 years ago
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
Attachment #622133 - Flags: feedback?(surkov.alexander) → feedback+

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f11717aa3a10
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/f11717aa3a10
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.