Closed
Bug 757503
Opened 12 years ago
Closed 12 years ago
decomtaminate GetColumnIndexAt/GetRowIndexAt/GetRowAndColumnIndicesAt on accessible tables
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
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)
17.06 KB,
patch
|
Details | Diff | Splinter Review | |
17.33 KB,
patch
|
Details | Diff | Splinter Review |
see bug 747219 for idea.
Reporter | ||
Updated•12 years ago
|
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?
Reporter | ||
Comment 2•12 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.
In ARIAGridAccessible::GetRowIndexAt What happens if the statement NS_ENSURE_ARG(aCellIndex < rowCount * colsCount); fails?
Reporter | ||
Comment 4•12 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
Comment 6•12 years ago
|
||
(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.
Attachment #627567 -
Flags: review?(surkov.alexander)
Comment 8•12 years ago
|
||
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•12 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•12 years ago
|
||
Attachment #627567 -
Attachment is obsolete: true
Attachment #627679 -
Flags: review?(trev.saunders)
Comment 11•12 years ago
|
||
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•12 years ago
|
||
Attachment #627679 -
Attachment is obsolete: true
Attachment #632634 -
Flags: review?(trev.saunders)
Comment 13•12 years ago
|
||
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•12 years ago
|
||
Attachment #632634 -
Attachment is obsolete: true
Attachment #633867 -
Flags: review?(trev.saunders)
Comment 15•12 years ago
|
||
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•12 years ago
|
||
Attachment #633867 -
Attachment is obsolete: true
Attachment #633994 -
Flags: review?(trev.saunders)
Comment 17•12 years ago
|
||
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•12 years ago
|
||
Attachment #633994 -
Attachment is obsolete: true
Attachment #634010 -
Flags: review?(trev.saunders)
Comment 19•12 years ago
|
||
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•12 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?
Comment 21•12 years ago
|
||
(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•12 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•12 years ago
|
||
Status : NEW → ASSIGNED Assignee: nobody@mozilla.org → vinceyang15@gmail.com
Assignee | ||
Comment 24•12 years ago
|
||
Status: NEW → ASSIGNED Assignee: nobody@mozilla.org → vinceyang15@gmail.com
Assignee | ||
Comment 25•12 years ago
|
||
Hi Trevor, just wondering how to assign this bug to me?
Keywords: checkin-needed
Comment 27•12 years ago
|
||
This doesn't apply cleanly. Please rebase and re-attach.
Keywords: checkin-needed
Comment 28•12 years ago
|
||
I have a patch going in this weekend involving the same area of code. I'll land this for him.
Comment 29•12 years ago
|
||
Re-based and tested locally ... will push to TRY when the tree opens
Assignee | ||
Comment 30•12 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?
Comment 31•12 years ago
|
||
I'll do what I can ... can you talk on IRC? I'd be faster ..
Assignee | ||
Comment 32•12 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?
Comment 33•12 years ago
|
||
I'm in #introduction as username: capella
Comment 34•12 years ago
|
||
FYI to all, TRY test status: https://tbpl.mozilla.org/?tree=Try&rev=9c2604e5791e
Comment 35•12 years ago
|
||
Push to inbound: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3845dd694369
Comment 36•12 years ago
|
||
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.
Description
•