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 alexander :surkov 2012-05-22 09:59:20 PDT
see bug 747219 for idea.
Comment 1 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 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 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 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 Xi Yang 2012-05-26 18:16:49 PDT
How to check that a aCellIndex is within the boundary?
Comment 6 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 Xi Yang 2012-05-27 09:44:46 PDT
Created attachment 627567 [details] [diff] [review]
Patch(v1)
Comment 8 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 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 Xi Yang 2012-05-28 06:03:38 PDT
Created attachment 627679 [details] [diff] [review]
Patch(v2)
Comment 11 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 Xi Yang 2012-06-13 04:22:17 PDT
Created attachment 632634 [details] [diff] [review]
Patch(v3)
Comment 13 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 Xi Yang 2012-06-16 22:38:37 PDT
Created attachment 633867 [details] [diff] [review]
Patch(v4)
Comment 15 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 Xi Yang 2012-06-18 05:05:59 PDT
Created attachment 633994 [details] [diff] [review]
Patch(v5)
Comment 17 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 Xi Yang 2012-06-18 06:51:47 PDT
Created attachment 634010 [details] [diff] [review]
Patch(v6)
Comment 19 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 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 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 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 Xi Yang 2012-06-22 01:58:37 PDT
Status : NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
Comment 24 Xi Yang 2012-06-22 01:59:41 PDT
Status: NEW → ASSIGNED
Assignee: nobody@mozilla.orgvinceyang15@gmail.com
Comment 25 Xi Yang 2012-06-22 02:21:41 PDT
Hi Trevor, just wondering how to assign this bug to me?
Comment 26 Mark Capella [:capella] 2012-06-22 02:25:15 PDT
i don't think he'll mind if I help you ...
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-06-22 17:09:32 PDT
This doesn't apply cleanly. Please rebase and re-attach.
Comment 28 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 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 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 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 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 Mark Capella [:capella] 2012-06-22 23:55:19 PDT
I'm in #introduction as username: capella
Comment 34 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 Mark Capella [:capella] 2012-06-23 15:45:59 PDT
Push to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3845dd694369
Comment 36 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.