Last Comment Bug 757503 - decomtaminate GetColumnIndexAt/GetRowIndexAt/GetRowAndColumnIndicesAt on accessible tables
: decomtaminate GetColumnIndexAt/GetRowIndexAt/GetRowAndColumnIndicesAt on acce...
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)
: mozilla16
Assigned To: Xi Yang
:
: alexander :surkov
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-05-22 09:59 PDT by alexander :surkov
Modified: 2012-06-23 19:56 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch(v1) (19.70 KB, patch)
2012-05-27 09:44 PDT, Xi Yang
no flags Details | Diff | Splinter Review
Patch(v2) (16.99 KB, patch)
2012-05-28 06:03 PDT, Xi Yang
no flags Details | Diff | Splinter Review
Patch(v3) (17.01 KB, patch)
2012-06-13 04:22 PDT, Xi Yang
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch(v4) (16.93 KB, patch)
2012-06-16 22:38 PDT, Xi Yang
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch(v5) (17.06 KB, patch)
2012-06-18 05:05 PDT, Xi Yang
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Patch(v6) (17.06 KB, patch)
2012-06-18 06:51 PDT, Xi Yang
no flags Details | Diff | Splinter Review
Patch - Re-based for push (17.33 KB, patch)
2012-06-22 22:47 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description User image alexander :surkov 2012-05-22 09:59:20 PDT
see bug 747219 for idea.
Comment 1 User image Xi Yang 2012-05-24 15:41:56 PDT
Does it mean we have to decomtaminate GetCellIndexAt() first, and then we can finish the rest of it?
Comment 2 User image alexander :surkov 2012-05-25 00:35:01 PDT
(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.
Comment 3 User image Xi Yang 2012-05-25 04:57:58 PDT
In ARIAGridAccessible::GetRowIndexAt
What happens if the statement
NS_ENSURE_ARG(aCellIndex < rowCount * colsCount);
fails?
Comment 4 User image alexander :surkov 2012-05-25 08:13:20 PDT
(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
Comment 5 User image Xi Yang 2012-05-26 18:16:49 PDT
How to check that a aCellIndex is within the boundary?
Comment 6 User image Trevor Saunders (:tbsaunde) 2012-05-26 18:41:51 PDT
(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.
Comment 7 User image Xi Yang 2012-05-27 09:44:46 PDT
Created attachment 627567 [details] [diff] [review]
Patch(v1)
Comment 8 User image Trevor Saunders (:tbsaunde) 2012-05-27 18:55:59 PDT
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 9 User image alexander :surkov 2012-05-27 22:34:42 PDT
Comment on attachment 627567 [details] [diff] [review]
Patch(v1)

canceling until Trevor's comments are fixed
Comment 10 User image Xi Yang 2012-05-28 06:03:38 PDT
Created attachment 627679 [details] [diff] [review]
Patch(v2)
Comment 11 User image Trevor Saunders (:tbsaunde) 2012-05-28 13:00:55 PDT
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.
Comment 12 User image Xi Yang 2012-06-13 04:22:17 PDT
Created attachment 632634 [details] [diff] [review]
Patch(v3)
Comment 13 User image Trevor Saunders (:tbsaunde) 2012-06-13 16:22:46 PDT
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
Comment 14 User image Xi Yang 2012-06-16 22:38:37 PDT
Created attachment 633867 [details] [diff] [review]
Patch(v4)
Comment 15 User image Trevor Saunders (:tbsaunde) 2012-06-18 03:29:45 PDT
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.
Comment 16 User image Xi Yang 2012-06-18 05:05:59 PDT
Created attachment 633994 [details] [diff] [review]
Patch(v5)
Comment 17 User image Trevor Saunders (:tbsaunde) 2012-06-18 06:22:21 PDT
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 !=
Comment 18 User image Xi Yang 2012-06-18 06:51:47 PDT
Created attachment 634010 [details] [diff] [review]
Patch(v6)
Comment 19 User image Trevor Saunders (:tbsaunde) 2012-06-18 08:01:25 PDT
Comment on attachment 634010 [details] [diff] [review]
Patch(v6)

no need to rerequest review for that change
Comment 20 User image Xi Yang 2012-06-18 22:52:06 PDT
(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?
Comment 21 User image Trevor Saunders (:tbsaunde) 2012-06-19 06:03:38 PDT
(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
Comment 22 User image Xi Yang 2012-06-19 17:35:57 PDT
(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
Comment 23 User image Xi Yang 2012-06-22 01:58:37 PDT
Status : NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
Comment 24 User image Xi Yang 2012-06-22 01:59:41 PDT
Status: NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
Comment 25 User image Xi Yang 2012-06-22 02:21:41 PDT
Hi Trevor, just wondering how to assign this bug to me?
Comment 26 User image Mark Capella [:capella] 2012-06-22 02:25:15 PDT
i don't think he'll mind if I help you ...
Comment 27 User image Ryan VanderMeulen [:RyanVM] 2012-06-22 17:09:32 PDT
This doesn't apply cleanly. Please rebase and re-attach.
Comment 28 User image Mark Capella [:capella] 2012-06-22 19:21:53 PDT
I have a patch going in this weekend involving the same area of code. I'll land this for him.
Comment 29 User image Mark Capella [:capella] 2012-06-22 22:47:08 PDT
Created attachment 636030 [details] [diff] [review]
Patch - Re-based for push

Re-based and tested locally ... will push to TRY when the tree opens
Comment 30 User image Xi Yang 2012-06-22 23:45:26 PDT
(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?
Comment 31 User image Mark Capella [:capella] 2012-06-22 23:47:12 PDT
I'll do what I can ... can you talk on IRC? I'd be faster ..
Comment 32 User image Xi Yang 2012-06-22 23:53:20 PDT
(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?
Comment 33 User image Mark Capella [:capella] 2012-06-22 23:55:19 PDT
I'm in #introduction as username: capella
Comment 34 User image Mark Capella [:capella] 2012-06-23 00:16:03 PDT
FYI to all, TRY test status:
https://tbpl.mozilla.org/?tree=Try&rev=9c2604e5791e
Comment 35 User image Mark Capella [:capella] 2012-06-23 15:45:59 PDT
Push to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3845dd694369
Comment 36 User image Ryan VanderMeulen [:RyanVM] 2012-06-23 19:56:20 PDT
https://hg.mozilla.org/mozilla-central/rev/3845dd694369

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