The default bug view has changed. See this FAQ.

decomtaminate GetColumnIndexAt/GetRowIndexAt/GetRowAndColumnIndicesAt on accessible tables

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: surkov, Assigned: Xi Yang)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
see bug 747219 for idea.
(Reporter)

Updated

5 years ago
Summary: decomtaminate GetCellIndexAt() on accessible tables → decomtaminate GetColumnIndexAt/GetRowIndexAt/GetRowAndColumnIndicesAt on accessible tables
(Assignee)

Comment 1

5 years ago
Does it mean we have to decomtaminate GetCellIndexAt() first, and then we can finish the rest of it?
(Reporter)

Comment 2

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

Comment 3

5 years ago
In ARIAGridAccessible::GetRowIndexAt
What happens if the statement
NS_ENSURE_ARG(aCellIndex < rowCount * colsCount);
fails?
(Reporter)

Comment 4

5 years ago
(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
(Assignee)

Comment 5

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

Comment 7

5 years ago
Created attachment 627567 [details] [diff] [review]
Patch(v1)
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.
(Reporter)

Comment 9

5 years ago
Comment on attachment 627567 [details] [diff] [review]
Patch(v1)

canceling until Trevor's comments are fixed
Attachment #627567 - Flags: review?(surkov.alexander)
(Assignee)

Comment 10

5 years ago
Created attachment 627679 [details] [diff] [review]
Patch(v2)
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)
(Assignee)

Comment 12

5 years ago
Created attachment 632634 [details] [diff] [review]
Patch(v3)
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+
(Assignee)

Comment 14

5 years ago
Created attachment 633867 [details] [diff] [review]
Patch(v4)
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+
(Assignee)

Comment 16

5 years ago
Created attachment 633994 [details] [diff] [review]
Patch(v5)
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+
(Assignee)

Comment 18

5 years ago
Created attachment 634010 [details] [diff] [review]
Patch(v6)
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)
(Assignee)

Comment 20

5 years ago
(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
(Assignee)

Comment 22

5 years ago
(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
(Assignee)

Comment 23

5 years ago
Status : NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
(Assignee)

Comment 24

5 years ago
Status: NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
(Assignee)

Comment 25

5 years ago
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
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
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.
Created attachment 636030 [details] [diff] [review]
Patch - Re-based for push

Re-based and tested locally ... will push to TRY when the tree opens
(Assignee)

Comment 30

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

Comment 32

5 years ago
(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
FYI to all, TRY test status:
https://tbpl.mozilla.org/?tree=Try&rev=9c2604e5791e
Push to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3845dd694369
https://hg.mozilla.org/mozilla-central/rev/3845dd694369
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.