Closed Bug 757504 Opened 9 years ago Closed 9 years ago

decomtaminate GetColumnExtentAt/GetRowExtentAt on accessible tables

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: surkov, Assigned: capella)

References

Details

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

Attachments

(2 files, 3 obsolete files)

see bug 747219 for idea.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
First attempt ... (rough patch) ... asking mentor for feedback ...

TRY push in progress ...
https://tbpl.mozilla.org/?tree=Try&rev=56cac934240c
Attachment #627155 - Flags: feedback?(trev.saunders)
Comment on attachment 627155 [details] [diff] [review]
Patch (v1)

>+PRUint32
>+ARIAGridAccessible::ColExtentAt(PRUint32 aRow, PRUint32 aColumn)

you can get rid of these overrides accept for nsHTMLTableAccessible since the impl on TableAccessible does the same thing.

>+nsHTMLTableAccessible::ColExtentAt(PRUint32 aRow, PRUint32 aColumn)
> {
>-  nsITableLayout *tableLayout = GetTableLayout();
>-  NS_ENSURE_STATE(tableLayout);
>+  PRInt32 columnExtent = 0;
>+
>+  nsITableLayout* tableLayout = GetTableLayout();
>+  if (!tableLayout)
>+    return columnExtent;

just return 0 then declare the local var where you actually need it.

>+  PRInt32 rowExtent = 0;
>+
>+  nsITableLayout* tableLayout = GetTableLayout();
>+  if (!tableLayout)
>+    return rowExtent;

same

>+xpcAccessibleTable::GetColumnExtentAt(PRInt32 aRow, PRInt32 aColumn,
>+                                      PRInt32* aColumnExtent)
>+{
>+  NS_ENSURE_ARG_POINTER(aColumnExtent);
>+  *aColumnExtent = -1;
>+
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
>+
>+  if (aRow < 0 || aRow >= mTable->RowCount() ||
>+      aColumn < 0 || aColumn >= mTable->ColCount())
>+    return NS_ERROR_INVALID_ARG;

nit, rename them to aRowIdx / aColIdx

you need to do stati_cast<PURint32>(aRowIdx) >= mTable->RowCount() to keep gcc from warning here and below.
Attachment #627155 - Flags: feedback?(trev.saunders)
Re:
ARIAGridAccessible::ColExtentAt(PRUint32 aRow, PRUint32 aColumn)
you can get rid of these overrides accept for nsHTMLTableAccessible since the impl on TableAccessible does the same thing.

The overriding function in ARIAGridAccessible::ColExtentAt() returns 1 : 0 conditionally based on valid Row/Col checks where TableAccessible just always returns 1 ... isn't that better to leave?
Attached patch Patch (v2) (obsolete) — Splinter Review
FYI Current version with nits addressed ...
Attachment #627155 - Attachment is obsolete: true
(In reply to Mark Capella [:capella] from comment #3)
> Re:
> ARIAGridAccessible::ColExtentAt(PRUint32 aRow, PRUint32 aColumn)
> you can get rid of these overrides accept for nsHTMLTableAccessible since
> the impl on TableAccessible does the same thing.
> 
> The overriding function in ARIAGridAccessible::ColExtentAt() returns 1 : 0
> conditionally based on valid Row/Col checks where TableAccessible just
> always returns 1 ... isn't that better to leave?

no, that function should run only when row and column are valid, so it shouldn't bother check.
Attached patch Patch (v3) (obsolete) — Splinter Review
Ah, I see ! Thanks for the explanation :)

Here's the patch with that changed also.
Attachment #627427 - Attachment is obsolete: true
(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> no, that function should run only when row and column are valid, so it
> shouldn't bother check.

I said to follow the practice we have but just general thinking: it might be reasonable to keep check if these methods aren't supposed for internal usage so we don't need to dupe arguments checking logic in xpcom/ia2/atk.
(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > no, that function should run only when row and column are valid, so it
> > shouldn't bother check.
> 
> I said to follow the practice we have but just general thinking: it might be
> reasonable to keep check if these methods aren't supposed for internal usage
> so we don't need to dupe arguments checking logic in xpcom/ia2/atk.

its worth considering, but I'm not really sure how to do it well since iirc atk has no way of reporting errors and we need slightly different return codes for xpcom and ia2.  As well it might mean we need to have a bunch of extra implementations around or something for cases where the default impl on TableAccessible works now.
Comment on attachment 627441 [details] [diff] [review]
Patch (v3)

Forgot to add review flag to this ...
Attachment #627441 - Flags: review?(surkov.alexander)
Comment on attachment 627441 [details] [diff] [review]
Patch (v3)

Review of attachment 627441 [details] [diff] [review]:
-----------------------------------------------------------------

otherwise looks ok, my f+ when all comments are addressed, please attach new patch and get review from Trevor

::: accessible/src/generic/ARIAGridAccessible.cpp
@@ -174,5 @@
> -
> -  if (IsDefunct())
> -    return NS_ERROR_FAILURE;
> -
> -  NS_ENSURE_ARG(IsValidRowNColumn(aRow, aColumn));

remove IsValidRowNColumn

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +931,5 @@
>    return (*aRowIdx == -1 || *aColumnIdx == -1) ? NS_ERROR_INVALID_ARG : NS_OK;
>  }
>  
> +PRUint32
> +nsHTMLTableAccessible::ColExtentAt(PRUint32 aRow, PRUint32 aColumn)

aRowIdx or aRowIndex please (row is reserved for row accessible)

@@ +940,4 @@
>  
>    nsCOMPtr<nsIDOMElement> domElement;
> +  PRInt32 startRowIndex, startColIndex, rowSpan, colSpan, actualRowSpan,
> +          columnExtent;

wrong indentation, you might want to keep it separately, please assign an initial value

::: accessible/src/xpcom/xpcAccessibleTable.cpp
@@ +62,3 @@
>      return NS_ERROR_INVALID_ARG;
>  
> +  NS_IF_ADDREF(*aCell = mTable->CellAt(aRowIdx, aColIdx));

valid indices shouldn't give null, if that happens then something goes wrong, you should return invalidarg I guess
the same below

@@ +96,5 @@
> +  if (aRowIdx < 0 || static_cast<PRUint32>(aRowIdx) >= mTable->RowCount() ||
> +      aColIdx < 0 || static_cast<PRUint32>(aColIdx) >= mTable->ColCount())
> +    return NS_ERROR_INVALID_ARG;
> +
> +  *aColumnExtent = mTable->ColExtentAt(aRowIdx, aColIdx);

it seems that 0 return value is reserved when something goes wrong then you should check it
Attachment #627441 - Flags: review?(surkov.alexander) → feedback+
You'd like me to do something like the below for the existing GetCellAt() / GetCellIndexAt() methods:

   NS_IF_ADDREF(*aCell = mTable->CellAt(aRowIdx, aColIdx));
-  return NS_OK;
+  return *cell == nsnull ? NS_ERROR_INVALID_ARG : NS_OK;

  *aCellIndex = mTable->CellIndexAt(aRowIdx, aColIdx);
-  return NS_OK;
+  return *aCellIndex == nsnull ? NS_ERROR_INVALID_ARG : NS_OK;

And for the two new methods GetColumnExtentAt() / GetRowExtentAt():

   *aColumnExtent = mTable->ColExtentAt(aRowIdx, aColIdx);
-  return NS_OK;
+  return *aColumnExtent == nsnull ? NS_ERROR_INVALID_ARG : NS_OK;

   *aRowExtent = mTable->RowExtentAt(aRowIdx, aColIdx);
-  return NS_OK;
+  return *aRowExtent == nsnull ? NS_ERROR_INVALID_ARG : NS_OK;

And maybe also for the other existing methods as well?
NS_IF_ADDREF(*aCell = mTable->CellAt(aRowIdx, aColIdx));
return *aCell ? NS_OK : NS_ERROR_INVALID_ARG;

or
nsAccessible* cell = mTable->CellAt(aRowIdx, aColIdx);
if (!cell)
  return NS_ERROR_INVALID_ARG;

NS_ADDREF(*aCell = cell);
return NS_OK;

btw, *aCellIndex and *aColumnExtent are integers so you should compare them with integer. Column extent should be compared to 0, cell index maybe -1 (since 0 is valid index I think).

> And maybe also for the other existing methods as well?
yes if it's reasonable :)
Attached patch Patch (v4)Splinter Review
I changed the GetCell() method, but it failed out our crazy table test at: 
http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/table/test_table_2.html?force=1#77

So, since this logic wasn't actually a part of this patch proper, I left it as-is.
Attachment #627441 - Attachment is obsolete: true
Attachment #627729 - Flags: review?(trev.saunders)
Mark, would you mind attaching a patch that doesn't have a bunch of random whitespace changes in it?
Trevor, please elaborate. You have an issue where you'd like me to replace whitespaces that we normally remove?
(In reply to Mark Capella [:capella] from comment #15)
> Trevor, please elaborate. You have an issue where you'd like me to replace
> whitespaces that we normally remove?

well, I think you should only fix up whitespace if you touch that line anyway or very near it, not just because you touch that file somewhere else.  Its anoying to read a diff and have to figure out if each change actually changes anything, or if it just adjusts whitespace.

If you really think we should fix them all now I wouldn't terribly mind fixing all the ones in accessible/ sometime in the next few days, but otherwise I'd tend to just leave them for when that line is being changed anyway.
I agree that removing lots of whitespaces can complicate the review process.

However, this is a relatively small patch that has been looked over fairly well already, and I felt that final review+ was reasonably expected. Fyi only end-of-line whitespace was removed from a few lines, vs. randomly, so it shouldn't be too bad for you to look over one last time.

Also, Fyi, I did see where your nsAccessible patch was coming up, and I plan to land this afterwards, so there should be no impact directly to you. I understand that I'll have to reconcile any changes in my patch that are preceeded by / clashed with yours.
I'd say to keep all introduced and valid but not related whitespace changes, sometimes it's complicated to do that (for example, my editor settings don't allow me to add unallowed whitespaces).
> However, this is a relatively small patch that has been looked over fairly
> well already, and I felt that final review+ was reasonably expected. Fyi

I'm not really sure how  this is  related?

> only end-of-line whitespace was removed from a few lines, vs. randomly, so
> it shouldn't be too bad for you to look over one last time.

I guess I'll take care of it this time, but please avoid these changes in future.

> Also, Fyi, I did see where your nsAccessible patch was coming up, and I plan
> to land this afterwards, so there should be no impact directly to you. I
> understand that I'll have to reconcile any changes in my patch that are
> preceeded by / clashed with yours.

ok, whatever.
(In reply to alexander :surkov from comment #18)
> I'd say to keep all introduced and valid but not related whitespace changes,
> sometimes it's complicated to do that (for example, my editor settings don't
> allow me to add unallowed whitespaces).

I'd think most reasonable editors would support you forcing them if you really want, though making it hard might be reasonable.  Anyway I'd probably use something like interdiff or something else from patchutils for something like this.
I think the diff has an option to exclude whitespace changes from the patch
Comment on attachment 627729 [details] [diff] [review]
Patch (v4)

>+xpcAccessibleTable::GetCellIndexAt(PRInt32 aRowIdx, PRInt32 aColIdx,
>                                    PRInt32* aCellIndex)

since you rename the other two rguments to this function please do the third.

> {
>   NS_ENSURE_ARG_POINTER(aCellIndex);
>   *aCellIndex = -1;
> 
>   if (!mTable)
>     return NS_ERROR_FAILURE;
> 
>-  if (aRowIndex < 0 || aRowIndex >= mTable->RowCount() ||
>-      aColumnIndex < 0 || aColumnIndex >= mTable->ColCount())
>+  if (aRowIdx < 0 || static_cast<PRUint32>(aRowIdx) >= mTable->RowCount() ||
>+      aColIdx < 0 || static_cast<PRUint32>(aColIdx) >= mTable->ColCount())
>     return NS_ERROR_INVALID_ARG;
> 
>-  *aCellIndex = mTable->CellIndexAt(aRowIndex, aColumnIndex);
>-  return NS_OK;
>+  *aCellIndex = mTable->CellIndexAt(aRowIdx, aColIdx);
>+  return *aCellIndex == -1 ? NS_ERROR_INVALID_ARG : NS_OK;

this isn't really consistant with GetCellAt(), and its not really clear that invalid arg is why it failed, I'd say either NS_ERROR_FAILURE or better don't throw.

>+}
>+
>+nsresult
>+xpcAccessibleTable::GetColumnExtentAt(PRInt32 aRowIdx, PRInt32 aColIdx,
>+                                      PRInt32* aColumnExtent)
>+{
>+  NS_ENSURE_ARG_POINTER(aColumnExtent);
>+  *aColumnExtent = -1;
>+
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
>+
>+  if (aRowIdx < 0 || static_cast<PRUint32>(aRowIdx) >= mTable->RowCount() ||
>+      aColIdx < 0 || static_cast<PRUint32>(aColIdx) >= mTable->ColCount())
>+    return NS_ERROR_INVALID_ARG;
>+
>+  *aColumnExtent = mTable->ColExtentAt(aRowIdx, aColIdx);
>+  return *aColumnExtent == 0 ? NS_ERROR_INVALID_ARG : NS_OK;

in this case nsHTMLTableAccessible::ColExtentAt() can certainly return 0 for other reason than bad arg, again probably best to fail peacefully.

>+                                   PRInt32* aRowExtent)
>+{
>+  NS_ENSURE_ARG_POINTER(aRowExtent);
>+  *aRowExtent = -1;
>+
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
>+
>+  if (aRowIdx < 0 || static_cast<PRUint32>(aRowIdx) >= mTable->RowCount() ||
>+      aColIdx < 0 || static_cast<PRUint32>(aColIdx) >= mTable->ColCount())
>+    return NS_ERROR_INVALID_ARG;
>+
>+  *aRowExtent = mTable->RowExtentAt(aRowIdx, aColIdx);
>+  return *aRowExtent == 0 ? NS_ERROR_INVALID_ARG : NS_OK;

same
Attachment #627729 - Flags: review?(trev.saunders) → review+
> ::: accessible/src/xpcom/xpcAccessibleTable.cpp
> @@ +62,3 @@
> >      return NS_ERROR_INVALID_ARG;
> >  
> > +  NS_IF_ADDREF(*aCell = mTable->CellAt(aRowIdx, aColIdx));
> 
> valid indices shouldn't give null, if that happens then something goes
> wrong, you should return invalidarg I guess
> the same below

That would be nice, but yeah the crazy table thing :/

> @@ +96,5 @@
> > +  if (aRowIdx < 0 || static_cast<PRUint32>(aRowIdx) >= mTable->RowCount() ||
> > +      aColIdx < 0 || static_cast<PRUint32>(aColIdx) >= mTable->ColCount())
> > +    return NS_ERROR_INVALID_ARG;
> > +
> > +  *aColumnExtent = mTable->ColExtentAt(aRowIdx, aColIdx);
> 
> it seems that 0 return value is reserved when something goes wrong then you
> should check it

I thought we had crashes were tableLayout was reasonable null, so then 0 isn't necessarily something going wrong, but I'm not really sure what it means, perhaps we should return 1 there?
(In reply to Trevor Saunders (:tbsaunde) from comment #23)

> I thought we had crashes were tableLayout was reasonable null, so then 0
> isn't necessarily something going wrong, but I'm not really sure what it
> means, perhaps we should return 1 there?

I don't have answer without investigation, let's preserve the behavior then
Attached patch Patch finalSplinter Review
As pushed to TRY, after fixups for nsAccessible

https://tbpl.mozilla.org/?tree=Try&rev=53656adc5e7a
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/05bd3d79b022
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.