Closed Bug 757503 Opened 12 years ago Closed 12 years ago

decomtaminate GetColumnIndexAt/GetRowIndexAt/GetRowAndColumnIndicesAt on accessible tables

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: surkov, Assigned: vinceyang15)

References

Details

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

Attachments

(2 files, 5 obsolete files)

see bug 747219 for idea.
Summary: decomtaminate GetCellIndexAt() on accessible tables → decomtaminate GetColumnIndexAt/GetRowIndexAt/GetRowAndColumnIndicesAt on accessible tables
Does it mean we have to decomtaminate GetCellIndexAt() first, and then we can finish the rest of it?
(In reply to Xi Yang from comment #1)
> Does it mean we have to decomtaminate GetCellIndexAt() first, and then we
> can finish the rest of it?

it can be done in either way.
In ARIAGridAccessible::GetRowIndexAt
What happens if the statement
NS_ENSURE_ARG(aCellIndex < rowCount * colsCount);
fails?
(In reply to Xi Yang from comment #3)
> In ARIAGridAccessible::GetRowIndexAt
> What happens if the statement
> NS_ENSURE_ARG(aCellIndex < rowCount * colsCount);
> fails?

don't check that there (refer to bug 747227), but add these checks into xpcTableAccessible implementation
How to check that a aCellIndex is within the boundary?
(In reply to Xi Yang from comment #5)
> How to check that a aCellIndex is within the boundary?

for implementations of methods on TableAccessible you can just assume the index is valid for callers you can use RowCount() and ColCount() if they require error handling.
Attached patch Patch(v1) (obsolete) — Splinter Review
Attachment #627567 - Flags: review?(surkov.alexander)
Comment on attachment 627567 [details] [diff] [review]
Patch(v1)

>+ARIAGridAccessible::ColIndexAt(PRUint32 aCellIdx)
> {
>+  PRUint32 colsCount = ColCount();
>+  return aCellIdx % colsCount;

it seems you should change the implementation on TableAccessible to be return aCellIdx % ColCount(); and get rid of all the other implementations accept the one on nsHTMLTableAccessible which should be different.

>+PRInt32
>+ARIAGridAccessible::RowIndexAt(PRUint32 aCellIdx)

same idea.

>+ARIAGridAccessible::RowAndColIndicesAt(PRUint32 aCellIdx,
>+                                       PRInt32* aRowIdx,
>+                                       PRInt32* aColIdx)

same sort of idea, and btw nit plese try to keep arguments on one line when you can.

>+nsHTMLTableAccessible::ColIndexAt(PRUint32 aCellIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aColumn);
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>-  nsITableLayout *tableLayout = GetTableLayout();
>-  NS_ENSURE_STATE(tableLayout);
>-
>-  PRInt32 row;
>-  nsresult rv = tableLayout->GetRowAndColumnByIndex(aIndex, &row, aColumn);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  return (row == -1 || *aColumn == -1) ? NS_ERROR_INVALID_ARG : NS_OK;
>+  PRUint32 colsCount = ColCount();
>+  return aCellIdx % colsCount;

the changes for this class aren't equivelent since it used ot be we worked with indices from table layout and didn't compute our own.
Comment on attachment 627567 [details] [diff] [review]
Patch(v1)

canceling until Trevor's comments are fixed
Attachment #627567 - Flags: review?(surkov.alexander)
Attached patch Patch(v2) (obsolete) — Splinter Review
Attachment #627567 - Attachment is obsolete: true
Attachment #627679 - Flags: review?(trev.saunders)
Comment on attachment 627679 [details] [diff] [review]
Patch(v2)

>   virtual void RowAndColIndicesAt(PRUint32 aCellIdx, PRInt32* aRowIdx,
>-                                  PRInt32* aColIdx) {}
>+                                  PRInt32* aColIdx) 
>+    { 
>+      *aRowIdx = aCellIdx / ColCount();
>+      *aColIdx = aCellIdx % ColCount();

use local var here since result is used more than once and computation isn't cheap.

>+nsHTMLTableAccessible::ColIndexAt(PRUint32 aCellIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aColumn);
>+  nsITableLayout* tableLayout = GetTableLayout();

you should check tableLayout isn't null, which can happen in some cases.  Here and other methods in this class.

>+  PRInt32 row = -1;
>+  PRInt32 aColIdx = -1;

use one statement, and name the variables rowIdx and colIdx

>+  PRInt32 aRowIdx = -1;
>+  PRInt32 col = -1;
>+  tableLayout->GetRowAndColumnByIndex(aCellIdx, &aRowIdx, &col);

same

>+nsHTMLTableAccessible::RowAndColIndicesAt(PRUint32 aCellIdx, PRInt32* aRowIdx,
>+                                          PRInt32* aColIdx)

you really can't get that on two lines?

otherwise seems fine, but I think I'd like to see  one more version.
Attachment #627679 - Flags: review?(trev.saunders)
Attached patch Patch(v3) (obsolete) — Splinter Review
Attachment #627679 - Attachment is obsolete: true
Attachment #632634 - Flags: review?(trev.saunders)
Comment on attachment 632634 [details] [diff] [review]
Patch(v3)

>+  nsITableLayout* tableLayout = GetTableLayout();
>   NS_ENSURE_STATE(tableLayout);

no, that will return a nsresult which is wrong, also since it could happen in normal circumstances you don't want the warning.  just do if (!tableLayout) return -1;

>-  nsITableLayout *tableLayout = GetTableLayout();
>+  nsITableLayout* tableLayout = GetTableLayout();
>   NS_ENSURE_STATE(tableLayout);

same

>   nsITableLayout* tableLayout = GetTableLayout();
>-  if (tableLayout)
>-    tableLayout->GetRowAndColumnByIndex(aIndex, aRowIdx, aColumnIdx);
>-
>-  return (*aRowIdx == -1 || *aColumnIdx == -1) ? NS_ERROR_INVALID_ARG : NS_OK;
>+  tableLayout->GetRowAndColumnByIndex(aCellIdx, aRowIdx, aColIdx);

you shouldn't call it if tableLayout is null

>+xpcAccessibleTable::GetColumnIndexAt(PRInt32 aCellIndex,
>+                                     PRInt32* aColumnIndex)

nit, aCellIdx and aColIdx

also please always put things on one line when you can fit it in 80 chars

>+                                  PRInt32* aRowIndex)

same

>+xpcAccessibleTable::GetRowAndColumnIndicesAt(PRInt32 aCellIndex,
>+                                             PRInt32* aRowIndex,
>+                                             PRInt32* aColumnIndex)

here too
Attachment #632634 - Flags: review?(trev.saunders) → review+
Attached patch Patch(v4) (obsolete) — Splinter Review
Attachment #632634 - Attachment is obsolete: true
Attachment #633867 - Flags: review?(trev.saunders)
Comment on attachment 633867 [details] [diff] [review]
Patch(v4)

>+nsHTMLTableAccessible::ColIndexAt(PRUint32 aCellIdx)
> {
>-  NS_ENSURE_ARG_POINTER(aColumn);
>+  nsITableLayout* tableLayout = GetTableLayout();
>+  if (!tableLayout) return -1;

nit, return on own line here and below

>+  if (aCellIdx < 0 || aCellIdx >= mTable->RowCount() * mTable->ColCount())
>+    return NS_ERROR_INVALID_ARG;

nit, you need to static cast aCellIdx to PRUint32 before comparing to RowCount() * ColCount() to avoid a warning here and below.
Attachment #633867 - Flags: review?(trev.saunders) → review+
Attached patch Patch(v5) (obsolete) — Splinter Review
Attachment #633867 - Attachment is obsolete: true
Attachment #633994 - Flags: review?(trev.saunders)
Comment on attachment 633994 [details] [diff] [review]
Patch(v5)

>+  if (aCellIdx < 0 
>+      || static_cast<PRUint32>(aCellIdx) 
>+      != mTable->RowCount() * mTable->ColCount())
>+    return NS_ERROR_INVALID_ARG;

>= not !=
Attachment #633994 - Flags: review?(trev.saunders) → review+
Attached patch Patch(v6)Splinter Review
Attachment #633994 - Attachment is obsolete: true
Attachment #634010 - Flags: review?(trev.saunders)
Comment on attachment 634010 [details] [diff] [review]
Patch(v6)

no need to rerequest review for that change
Attachment #634010 - Flags: review?(trev.saunders)
(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> Comment on attachment 634010 [details] [diff] [review]
> Patch(v6)
> 
> no need to rerequest review for that change

So the bug is fixed?
(In reply to Xi Yang from comment #20)
> (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > Comment on attachment 634010 [details] [diff] [review]
> > Patch(v6)
> > 
> > no need to rerequest review for that change
> 
> So the bug is fixed?

well, you need to get the patch landed :)  if the patch cleanly applies you  can set the checkin-needed keyword and someone will take care of it for you
(In reply to Trevor Saunders (:tbsaunde) from comment #21)
> (In reply to Xi Yang from comment #20)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > > Comment on attachment 634010 [details] [diff] [review]
> > > Patch(v6)
> > > 
> > > no need to rerequest review for that change
> > 
> > So the bug is fixed?
> I'm not quite sure how it works, could you explain it for me?
> well, you need to get the patch landed :)  if the patch cleanly applies you 
> can set the checkin-needed keyword and someone will take care of it for you
Status : NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
Status: NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
Hi Trevor, just wondering how to assign this bug to me?
i don't think he'll mind if I help you ...
Assignee: nobody → vinceyang15
Keywords: checkin-needed
Status: NEW → ASSIGNED
This doesn't apply cleanly. Please rebase and re-attach.
Keywords: checkin-needed
I have a patch going in this weekend involving the same area of code. I'll land this for him.
Re-based and tested locally ... will push to TRY when the tree opens
(In reply to Mark Capella [:capella] from comment #29)
> Created attachment 636030 [details] [diff] [review]
> Patch - Re-based for push
> 
> Re-based and tested locally ... will push to TRY when the tree opens

Hi Mark, I have some problems with testing, would you please show me how to test my code?
I'll do what I can ... can you talk on IRC? I'd be faster ..
(In reply to Mark Capella [:capella] from comment #31)
> I'll do what I can ... can you talk on IRC? I'd be faster ..

I guess so, in which room?
I'm in #introduction as username: capella
https://hg.mozilla.org/mozilla-central/rev/3845dd694369
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.