757504 - decomtaminate GetColumnExtentAt/GetRowExtentAt on accessible tables

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

see bug 747219 for idea.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

First attempt ... (rough patch) ... asking mentor for feedback ...

TRY push in progress ...
https://tbpl.mozilla.org/?tree=Try&rev=56cac934240c

Comment 2

[Trevor Saunders (:tbsaunde)]

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.

Comment 3

[Mark Capella [:capella]]

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?

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v2)

FYI Current version with nits addressed ...

Comment 5

[Trevor Saunders (:tbsaunde)]

(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.

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v3)

Ah, I see ! Thanks for the explanation :)

Here's the patch with that changed also.

Comment 7

[alexander :surkov (:asurkov)]

(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.

Comment 8

[Trevor Saunders (:tbsaunde)]

(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 9

[Mark Capella [:capella]]

Comment on attachment 627441 [details] [diff] [review]
Patch (v3)

Forgot to add review flag to this ...

Comment 10

[alexander :surkov (:asurkov)]

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

Comment 11

[Mark Capella [:capella]]

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?

Comment 12

[alexander :surkov (:asurkov)]

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 :)

Comment 13

[Mark Capella [:capella]]

Attachment: Patch (v4)

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.

Comment 14

[Trevor Saunders (:tbsaunde)]

Mark, would you mind attaching a patch that doesn't have a bunch of random whitespace changes in it?

Comment 15

[Mark Capella [:capella]]

Trevor, please elaborate. You have an issue where you'd like me to replace whitespaces that we normally remove?

Comment 16

[Trevor Saunders (:tbsaunde)]

(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.

Comment 17

[Mark Capella [:capella]]

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.

Comment 18

[alexander :surkov (:asurkov)]

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).

Comment 19

[Trevor Saunders (:tbsaunde)]

> 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.

Comment 20

[Trevor Saunders (:tbsaunde)]

(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.

Comment 21

[alexander :surkov (:asurkov)]

I think the diff has an option to exclude whitespace changes from the patch

Comment 22

[Trevor Saunders (:tbsaunde)]

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

Comment 23

[Trevor Saunders (:tbsaunde)]

> ::: 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?

Comment 24

[alexander :surkov (:asurkov)]

(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

Comment 25

[Mark Capella [:capella]]

Attachment: Patch final

As pushed to TRY, after fixups for nsAccessible

https://tbpl.mozilla.org/?tree=Try&rev=53656adc5e7a

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/05bd3d79b022