Closed Bug 765512 Opened 12 years ago Closed 12 years ago

decomtaminate GetSelected (Cell / Column / Row) Indices() on accessible tables

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: capella, Assigned: capella)

References

Details

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

Attachments

(2 files, 4 obsolete files)

Refer to: Bug 765371
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #634116 - Flags: review?(trev.saunders)
Comment on attachment 634116 [details] [diff] [review]
Patch (v1)

> 
>   /**
>    * Get the set of selected column indices.
>    */
>-  virtual void SelectedColIndices(nsTArray<PRUint32>* aCols) {}
>+  virtual void SelectedCellIndices(PRUint32* aCellCount, PRInt32** aCells) {}
>+
>+  /**
>+   * Get the set of selected column indices.
>+   */
>+  virtual void SelectedColIndices(PRUint32* aCellCount, PRInt32** aCols) {}
> 
>   /**
>    * Get the set of selected row indices.
>    */
>-  virtual void SelectedRowIndices(nsTArray<PRUint32>* aRows) {}
>+  virtual void SelectedRowIndices(PRUint32* aCellCount, PRInt32** aRows) {}

what's the reason for these changes? I think we should use nsTArray's here instead of raw arrays.

Also while adding SelectedCellIndicies()  is probably reasonable it should have a correct comment.
Attachment #634116 - Flags: review?(trev.saunders)
Attached patch Patch (v2) (obsolete) — Splinter Review
Changed over to nsTArrays .... PRUint's ...
Attachment #634116 - Attachment is obsolete: true
Depends on: 765371
Comment on attachment 634660 [details] [diff] [review]
Patch (v2)

>+ARIAGridAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
> {
>+  PRUint32 colCount = ColCount();
>+  if (!colCount)
>+    return;
> 
>-  return GetSelectedColumnsArray(aColumnCount, aColumns);

so, since only SelectedColCount() uses this method now you could simplify it, or lternatively you could update it so it works with this method

>+  AccIterator rowIter(this, filters::GetRow);
>+  Accessible* row = rowIter.Next();
>+  if (!row)
>+    return;
>+
>+  nsTArray<bool> isColSelArray(colCount);

I think you should actually be able to just use a plane C++ array here

>+  isColSelArray.AppendElements(colCount);
>+  for (PRUint32 i = 0; i < colCount; i++)
>+    isColSelArray[i] = true;

you could just use memset, or I think nsTArray might have a method to add a bunch of elements with the same value, but you'd need to check that

>+  PRUint32 colArrayCount = colCount;

I don't really like this, but I don't have a better idea off hand.

>+  do {
>+    if (nsAccUtils::IsARIASelected(row))
>+      continue;
>+
>+    AccIterator cellIter(row, filters::GetCell);
>+    Accessible* cell = nsnull;
>+    for (PRUint32 colIdx = 0; (cell = cellIter.Next()); colIdx++) {
>+      if (isColSelArray.SafeElementAt(colIdx, false) &&

you could make it
for (PRUint32 colIdx = 0; cell = cellIter.Next() && colIdx < colCount; colIdx++)
then you could replace safe elmeentAt() by [] which saves checking the bounds every time.

>+          !nsAccUtils::IsARIASelected(cell)) {
>+        isColSelArray[colIdx] = false;
>+        colArrayCount--;
>+      }
>+    }
>+  } while ((row = rowIter.Next()));
>+
>+  if (colArrayCount) {
>+    for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
>+      if (isColSelArray[colIdx])
>+        aCols->AppendElement(colIdx);

if your going to keep the number of selected cols you might as well force the array to have space before adding them

>+ARIAGridAccessible::SelectedRowIndices(nsTArray<PRUint32>* aRows)
> {
>+  PRUint32 rowCount = RowCount();
>   if (!rowCount)

I don't think this is ever used for anything useful is it?

>   /**
>+   * Get the set of selected cell indices.
>+   */
>+  virtual void SelectedCellIndices(nsTArray<PRUint32>* aCells) {}

if all implementations provide an impl I don't see a reason to have a default.

>+HTMLTableAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> {
>   nsCOMPtr<nsIDOMElement> domElement;
>   PRInt32 startRowIndex = 0, startColIndex = 0,
>     rowSpan, colSpan, actualRowSpan, actualColSpan;
>   bool isSelected = false;
> 
>-  PRInt32 cellsCount = columnCount * rowCount;
>-  nsAutoArrayPtr<bool> states(new bool[cellsCount]);
>-  NS_ENSURE_TRUE(states, NS_ERROR_OUT_OF_MEMORY);
>+  nsTArray<bool> isCellSelArray(colCount * rowCount);
>+  isCellSelArray.AppendElements(colCount * rowCount);

I don't think this array is doing anything useful anymore is it?

>+xpcAccessibleTable::GetSelectedCellIndices(PRUint32* aCellsArraySize,
>+                                           PRInt32** aCellsArray)
>+{
>+  NS_ENSURE_ARG_POINTER(aCellsArraySize);
>+  *aCellsArraySize = 0;
>+
>+  NS_ENSURE_ARG_POINTER(aCellsArray);
>+  *aCellsArray = 0;
>+
>+  if (!mTable)
>+    return NS_ERROR_FAILURE;
>+
>+  nsTArray<PRUint32> cellsArray;

its kind of tempting to use an nsAutoTArray with a fairly large internal buffer here, that has the risk of a big copy when the whole table is selected.  However big tables that are all selected will probably be slow anyway.  So I think going for it with a default size of maybe 40 might be reasonable.  Especially since the call stack on top of this shouldn't be terribly deep (we shouldn't be running script etc)

>+  mTable->SelectedCellIndices(&cellsArray);
>+
>+  *aCellsArraySize = cellsArray.Length();
>+  *aCellsArray = static_cast<PRInt32*>
>+    (NS_Alloc(*aCellsArraySize * sizeof(PRInt32)));
>+  if (!*aCellsArray)
>+    return NS_ERROR_OUT_OF_MEMORY;

use moz_xmalloc() and its infalible so don't null check

>+
>+  for (PRUint32 i = 0; i < *aCellsArraySize; ++i)
>+    (*aCellsArray)[i] = cellsArray[i];

memcpy()

>+xpcAccessibleTable::GetSelectedColumnIndices(PRUint32* aColsArraySize,
>+                                             PRInt32** aColsArray)

same with this method

>+xpcAccessibleTable::GetSelectedRowIndices(PRUint32* aRowsArraySize,
>+                                          PRInt32** aRowsArray)

same with this one too

>+XULListboxAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> {
>   nsCOMPtr<nsIDOMXULMultiSelectControlElement> control =
>     do_QueryInterface(mContent);
>   NS_ASSERTION(control,
>                "Doesn't implement nsIDOMXULMultiSelectControlElement.");
> 
>   nsCOMPtr<nsIDOMNodeList> selectedItems;
>   control->GetSelectedItems(getter_AddRefs(selectedItems));
>   if (!selectedItems)
>-    return NS_OK;
>+    return;
> 
>   PRUint32 selectedItemsCount = 0;
>   nsresult rv = selectedItems->GetLength(&selectedItemsCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "GetLength() Shouldn't fail!");
> 
>-  PRInt32 columnCount = 0;
>-  rv = GetColumnCount(&columnCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  PRUint32 colCount = ColCount();
>+  aCells->AppendElements(selectedItemsCount * colCount);

SetCapacity() please

>+  for (PRUint32 index = 0, cellsIdx = 0; index < selectedItemsCount; index++) {

nit, idx or rowIdx or itemIdx please

>     nsCOMPtr<nsIDOMNode> itemNode;
>     selectedItems->Item(index, getter_AddRefs(itemNode));
>     nsCOMPtr<nsIDOMXULSelectControlItemElement> item =
>       do_QueryInterface(itemNode);
> 
>     if (item) {
>       PRInt32 itemIdx = -1;
>       control->GetIndexOfItem(item, &itemIdx);
>       if (itemIdx != -1)
>-        rowsIdxArray[index] = itemIdx;
>+        for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
>+          aCells->ElementAt(cellsIdx++) = itemIdx * colCount + colIdx;

you lost the chekc the index of the item isn't -1 also put  cellIdx++ in its own statement, either as for (col = 0; col < colCount; col++, cellIdx++) or as a second statement in the loop

>+XULListboxAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
>+{
>+  PRUint32 colArrayCount = SelectedColCount();
>+  for (PRUint32 colIdx = 0; colIdx < colArrayCount; colIdx++)
>+    aCols->AppendElement(colIdx);

since you know ahead of time call SetCapacity() first

>+XULListboxAccessible::SelectedRowIndices(nsTArray<PRUint32>* aRows)
>+{
>+  nsCOMPtr<nsIDOMXULMultiSelectControlElement> control =
>+    do_QueryInterface(mContent);
>+  NS_ASSERTION(control,
>+               "Doesn't implement nsIDOMXULMultiSelectControlElement.");
>+
>+  nsCOMPtr<nsIDOMNodeList> selectedItems;
>+  control->GetSelectedItems(getter_AddRefs(selectedItems));
>+  if (!selectedItems)
>+    return;
>+
>+  PRUint32 rowCount = 0;
>+  nsresult rv = selectedItems->GetLength(&rowCount);

that isn't actually the row count then is it? ;)

>+  for (PRUint32 index = 0; index < rowCount; index++) {

nit, Idx while your here

>+      PRInt32 itemIdx = -1;
>+      control->GetIndexOfItem(item, &itemIdx);
>+      if (itemIdx != -1)

< 0 to be safe

>+XULTreeGridAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> {
>+  PRUint32 colCount = ColCount();
>+  PRUint32 rowCount = RowCount();
> 
>+  for (PRUint32 rowIdx = 0; rowIdx < rowCount; rowIdx++)
>+    if (IsRowSelected(rowIdx))
>+      for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
>+        aCells->AppendElement(rowIdx * colCount + colIdx);

you can use either AppendElements() or SetCapacity() here too before going through the loop and adding each thing individually

>+XULTreeGridAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
>+{
>   PRInt32 selectedrowCount = 0;
>   nsresult rv = GetSelectionCount(&selectedrowCount);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "GetSelectionCount() Shouldn't fail!");

couldn't you use SelectedRowCount() for that?

>+  PRUint32 colCount = ColCount();
>+  for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
>+    aCols->AppendElement(colIdx);

set the capacity first since you can
Attached patch Patch (v3) (obsolete) — Splinter Review
Attachment #634660 - Attachment is obsolete: true
Attachment #635249 - Flags: review?(trev.saunders)
(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> Comment on attachment 634660 [details] [diff] [review]
> Patch (v2)
> 
> >+ARIAGridAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
> > {
> >+  PRUint32 colCount = ColCount();
> >+  if (!colCount)
> >+    return;
> > 
> >-  return GetSelectedColumnsArray(aColumnCount, aColumns);
> 
> so, since only SelectedColCount() uses this method now you could simplify
> it, or lternatively you could update it so it works with this method
> 

Yes. I updated the SelectedColIndices() method.

> >+  AccIterator rowIter(this, filters::GetRow);
> >+  Accessible* row = rowIter.Next();
> >+  if (!row)
> >+    return;
> >+
> >+  nsTArray<bool> isColSelArray(colCount);
> 
> I think you should actually be able to just use a plane C++ array here

I can, but I like not having to delete the array at the end.
> 
> >+  isColSelArray.AppendElements(colCount);
> >+  for (PRUint32 i = 0; i < colCount; i++)
> >+    isColSelArray[i] = true;
> 
> you could just use memset, or I think nsTArray might have a method to add a
> bunch of elements with the same value, but you'd need to check that
> 
I dug into nsTArray and there doesn't seem to be a way to pre-set the values other than the loop I'm using here. I also briefly talked with ms2ger and he didn't think so either.

> >+  PRUint32 colArrayCount = colCount;
> 
> I don't really like this, but I don't have a better idea off hand.

Ok, left it for now.
> 
> >+  do {
> >+    if (nsAccUtils::IsARIASelected(row))
> >+      continue;
> >+
> >+    AccIterator cellIter(row, filters::GetCell);
> >+    Accessible* cell = nsnull;
> >+    for (PRUint32 colIdx = 0; (cell = cellIter.Next()); colIdx++) {
> >+      if (isColSelArray.SafeElementAt(colIdx, false) &&
> 
> you could make it
> for (PRUint32 colIdx = 0; cell = cellIter.Next() && colIdx < colCount;
> colIdx++)
> then you could replace safe elmeentAt() by [] which saves checking the
> bounds every time.
> 
Thanks! I didn't look into the SafeElementAt method, so I ran with your way to avoid it.

> >+          !nsAccUtils::IsARIASelected(cell)) {
> >+        isColSelArray[colIdx] = false;
> >+        colArrayCount--;
> >+      }
> >+    }
> >+  } while ((row = rowIter.Next()));
> >+
> >+  if (colArrayCount) {
> >+    for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
> >+      if (isColSelArray[colIdx])
> >+        aCols->AppendElement(colIdx);
> 
> if your going to keep the number of selected cols you might as well force
> the array to have space before adding them

Not sure what you mean in this example ... we have the isColSelArray[] not the number of selected cells in it.
> 
> >+ARIAGridAccessible::SelectedRowIndices(nsTArray<PRUint32>* aRows)
> > {
> >+  PRUint32 rowCount = RowCount();
> >   if (!rowCount)
> 
> I don't think this is ever used for anything useful is it?

Agreed, and removed.
> 
> >   /**
> >+   * Get the set of selected cell indices.
> >+   */
> >+  virtual void SelectedCellIndices(nsTArray<PRUint32>* aCells) {}
> 
> if all implementations provide an impl I don't see a reason to have a
> default.

I had trouble with this. Changing the line to remove the braces produces an unresolved external reference during link time ... ?
> 
> >+HTMLTableAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> > {
> >   nsCOMPtr<nsIDOMElement> domElement;
> >   PRInt32 startRowIndex = 0, startColIndex = 0,
> >     rowSpan, colSpan, actualRowSpan, actualColSpan;
> >   bool isSelected = false;
> > 
> >-  PRInt32 cellsCount = columnCount * rowCount;
> >-  nsAutoArrayPtr<bool> states(new bool[cellsCount]);
> >-  NS_ENSURE_TRUE(states, NS_ERROR_OUT_OF_MEMORY);
> >+  nsTArray<bool> isCellSelArray(colCount * rowCount);
> >+  isCellSelArray.AppendElements(colCount * rowCount);
> 
> I don't think this array is doing anything useful anymore is it?

Forgot to qrefresh ... it's gone.
> 
> >+xpcAccessibleTable::GetSelectedCellIndices(PRUint32* aCellsArraySize,
> >+                                           PRInt32** aCellsArray)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aCellsArraySize);
> >+  *aCellsArraySize = 0;
> >+
> >+  NS_ENSURE_ARG_POINTER(aCellsArray);
> >+  *aCellsArray = 0;
> >+
> >+  if (!mTable)
> >+    return NS_ERROR_FAILURE;
> >+
> >+  nsTArray<PRUint32> cellsArray;
> 
> its kind of tempting to use an nsAutoTArray with a fairly large internal
> buffer here, that has the risk of a big copy when the whole table is
> selected.  However big tables that are all selected will probably be slow
> anyway.  So I think going for it with a default size of maybe 40 might be
> reasonable.  Especially since the call stack on top of this shouldn't be
> terribly deep (we shouldn't be running script etc)

Lost me here, can you say that again?
> 
> >+  mTable->SelectedCellIndices(&cellsArray);
> >+
> >+  *aCellsArraySize = cellsArray.Length();
> >+  *aCellsArray = static_cast<PRInt32*>
> >+    (NS_Alloc(*aCellsArraySize * sizeof(PRInt32)));
> >+  if (!*aCellsArray)
> >+    return NS_ERROR_OUT_OF_MEMORY;
> 
> use moz_xmalloc() and its infalible so don't null check

Nice ... done!
> 
> >+
> >+  for (PRUint32 i = 0; i < *aCellsArraySize; ++i)
> >+    (*aCellsArray)[i] = cellsArray[i];
> 
> memcpy()

Done.
> 
> >+xpcAccessibleTable::GetSelectedColumnIndices(PRUint32* aColsArraySize,
> >+                                             PRInt32** aColsArray)
> 
> same with this method

Yep.
> 
> >+xpcAccessibleTable::GetSelectedRowIndices(PRUint32* aRowsArraySize,
> >+                                          PRInt32** aRowsArray)
> 
> same with this one too

Yep.
> 
> >+XULListboxAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> > {
> >   nsCOMPtr<nsIDOMXULMultiSelectControlElement> control =
> >     do_QueryInterface(mContent);
> >   NS_ASSERTION(control,
> >                "Doesn't implement nsIDOMXULMultiSelectControlElement.");
> > 
> >   nsCOMPtr<nsIDOMNodeList> selectedItems;
> >   control->GetSelectedItems(getter_AddRefs(selectedItems));
> >   if (!selectedItems)
> >-    return NS_OK;
> >+    return;
> > 
> >   PRUint32 selectedItemsCount = 0;
> >   nsresult rv = selectedItems->GetLength(&selectedItemsCount);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >+  NS_ASSERTION(NS_SUCCEEDED(rv), "GetLength() Shouldn't fail!");
> > 
> >-  PRInt32 columnCount = 0;
> >-  rv = GetColumnCount(&columnCount);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >+  PRUint32 colCount = ColCount();
> >+  aCells->AppendElements(selectedItemsCount * colCount);
> 
> SetCapacity() please

This one was trouble also. SetCapacity() seems to set an internal table capacity (upper bounds limit?) that doesn't affect the Length() value of the array. Mochitests fail as a result.
> 
> >+  for (PRUint32 index = 0, cellsIdx = 0; index < selectedItemsCount; index++) {
> 
> nit, idx or rowIdx or itemIdx please

Straightened these out.
> 
> >     nsCOMPtr<nsIDOMNode> itemNode;
> >     selectedItems->Item(index, getter_AddRefs(itemNode));
> >     nsCOMPtr<nsIDOMXULSelectControlItemElement> item =
> >       do_QueryInterface(itemNode);
> > 
> >     if (item) {
> >       PRInt32 itemIdx = -1;
> >       control->GetIndexOfItem(item, &itemIdx);
> >       if (itemIdx != -1)
> >-        rowsIdxArray[index] = itemIdx;
> >+        for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
> >+          aCells->ElementAt(cellsIdx++) = itemIdx * colCount + colIdx;
> 
> you lost the chekc the index of the item isn't -1 also put  cellIdx++ in its
> own statement, either as for (col = 0; col < colCount; col++, cellIdx++) or
> as a second statement in the loop

I thought the -1 check was in place still, but in either case it is now.  I also moved the cellsIdx++ into the for-loop.
> 
> >+XULListboxAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
> >+{
> >+  PRUint32 colArrayCount = SelectedColCount();
> >+  for (PRUint32 colIdx = 0; colIdx < colArrayCount; colIdx++)
> >+    aCols->AppendElement(colIdx);
> 
> since you know ahead of time call SetCapacity() first

Again, trouble.
> 
> >+XULListboxAccessible::SelectedRowIndices(nsTArray<PRUint32>* aRows)
> >+{
> >+  nsCOMPtr<nsIDOMXULMultiSelectControlElement> control =
> >+    do_QueryInterface(mContent);
> >+  NS_ASSERTION(control,
> >+               "Doesn't implement nsIDOMXULMultiSelectControlElement.");
> >+
> >+  nsCOMPtr<nsIDOMNodeList> selectedItems;
> >+  control->GetSelectedItems(getter_AddRefs(selectedItems));
> >+  if (!selectedItems)
> >+    return;
> >+
> >+  PRUint32 rowCount = 0;
> >+  nsresult rv = selectedItems->GetLength(&rowCount);
> 
> that isn't actually the row count then is it? ;)

Yah, I changed to selectedItemsCount.
> 
> >+  for (PRUint32 index = 0; index < rowCount; index++) {
> 
> nit, Idx while your here

Done.
> 
> >+      PRInt32 itemIdx = -1;
> >+      control->GetIndexOfItem(item, &itemIdx);
> >+      if (itemIdx != -1)
> 
> < 0 to be safe

Done.
> 
> >+XULTreeGridAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> > {
> >+  PRUint32 colCount = ColCount();
> >+  PRUint32 rowCount = RowCount();
> > 
> >+  for (PRUint32 rowIdx = 0; rowIdx < rowCount; rowIdx++)
> >+    if (IsRowSelected(rowIdx))
> >+      for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
> >+        aCells->AppendElement(rowIdx * colCount + colIdx);
> 
> you can use either AppendElements() or SetCapacity() here too before going
> through the loop and adding each thing individually

Here we don't know the final array size, so we're adding-as-we-go. 
> 
> >+XULTreeGridAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
> >+{
> >   PRInt32 selectedrowCount = 0;
> >   nsresult rv = GetSelectionCount(&selectedrowCount);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >+  NS_ASSERTION(NS_SUCCEEDED(rv), "GetSelectionCount() Shouldn't fail!");
> 
> couldn't you use SelectedRowCount() for that?

Yes. I changed it.
> 
> >+  PRUint32 colCount = ColCount();
> >+  for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
> >+    aCols->AppendElement(colIdx);
> 
> set the capacity first since you can

Again, trouble. btw, There is another similarly named method to SetCapacity, that is SetLength(). But it basically calls AppendElements() to do the work.

So afaict, for arrays where we know the final size, we can do either inter-changeably:

array.AppendElements(length);
for (i=0; i<length; i++)
   array.ElementAt(i)=value;

-or-

for (i=0; i<length; i++)
   array.AppendElement(value);
> > >+  AccIterator rowIter(this, filters::GetRow);
> > >+  Accessible* row = rowIter.Next();
> > >+  if (!row)
> > >+    return;
> > >+
> > >+  nsTArray<bool> isColSelArray(colCount);
> > 
> > I think you should actually be able to just use a plane C++ array here
> 
> I can, but I like not having to delete the array at the end.

well, I meant using a variable length array on the stack, but it seems I was wrong about that being in C++98, so maybe msvc doesn't support it :/

> > 
> > >+  isColSelArray.AppendElements(colCount);
> > >+  for (PRUint32 i = 0; i < colCount; i++)
> > >+    isColSelArray[i] = true;
> > 
> > you could just use memset, or I think nsTArray might have a method to add a
> > bunch of elements with the same value, but you'd need to check that
> > 
> I dug into nsTArray and there doesn't seem to be a way to pre-set the values
> other than the loop I'm using here. I also briefly talked with ms2ger and he
> didn't think so either.

yeah, that sounds right, so use memset

> > >+    if (nsAccUtils::IsARIASelected(row))
> > >+      continue;
> > >+
> > >+    AccIterator cellIter(row, filters::GetCell);
> > >+    Accessible* cell = nsnull;
> > >+    for (PRUint32 colIdx = 0; (cell = cellIter.Next()); colIdx++) {
> > >+      if (isColSelArray.SafeElementAt(colIdx, false) &&
> > 
> > you could make it
> > for (PRUint32 colIdx = 0; cell = cellIter.Next() && colIdx < colCount;
> > colIdx++)
> > then you could replace safe elmeentAt() by [] which saves checking the
> > bounds every time.
> > 
> Thanks! I didn't look into the SafeElementAt method, so I ran with your way
> to avoid it.

if the compiler is smart it probably doesn't actually matter, but if not it should save some branches.

> > >+          !nsAccUtils::IsARIASelected(cell)) {
> > >+        isColSelArray[colIdx] = false;
> > >+        colArrayCount--;
> > >+      }
> > >+    }
> > >+  } while ((row = rowIter.Next()));
> > >+
> > >+  if (colArrayCount) {
> > >+    for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
> > >+      if (isColSelArray[colIdx])
> > >+        aCols->AppendElement(colIdx);
> > 
> > if your going to keep the number of selected cols you might as well force
> > the array to have space before adding them
> 
> Not sure what you mean in this example ... we have the isColSelArray[] not
> the number of selected cells in it.

unless I'm wrong colArrayCount is the same as the number of selected cols.

> > >+   * Get the set of selected cell indices.
> > >+   */
> > >+  virtual void SelectedCellIndices(nsTArray<PRUint32>* aCells) {}
> > 
> > if all implementations provide an impl I don't see a reason to have a
> > default.
> 
> I had trouble with this. Changing the line to remove the braces produces an
> unresolved external reference during link time ... ?

that's odd what was it?

> > >+xpcAccessibleTable::GetSelectedCellIndices(PRUint32* aCellsArraySize,
> > >+                                           PRInt32** aCellsArray)
> > >+{
> > >+  NS_ENSURE_ARG_POINTER(aCellsArraySize);
> > >+  *aCellsArraySize = 0;
> > >+
> > >+  NS_ENSURE_ARG_POINTER(aCellsArray);
> > >+  *aCellsArray = 0;
> > >+
> > >+  if (!mTable)
> > >+    return NS_ERROR_FAILURE;
> > >+
> > >+  nsTArray<PRUint32> cellsArray;
> > 
> > its kind of tempting to use an nsAutoTArray with a fairly large internal
> > buffer here, that has the risk of a big copy when the whole table is
> > selected.  However big tables that are all selected will probably be slow
> > anyway.  So I think going for it with a default size of maybe 40 might be
> > reasonable.  Especially since the call stack on top of this shouldn't be
> > terribly deep (we shouldn't be running script etc)
> 
> Lost me here, can you say that again?

instead of nsTArray<PRUint32> do nsAutoTArray<PRUint32, 40>

> > >+XULListboxAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> > > {
> > >   nsCOMPtr<nsIDOMXULMultiSelectControlElement> control =
> > >     do_QueryInterface(mContent);
> > >   NS_ASSERTION(control,
> > >                "Doesn't implement nsIDOMXULMultiSelectControlElement.");
> > > 
> > >   nsCOMPtr<nsIDOMNodeList> selectedItems;
> > >   control->GetSelectedItems(getter_AddRefs(selectedItems));
> > >   if (!selectedItems)
> > >-    return NS_OK;
> > >+    return;
> > > 
> > >   PRUint32 selectedItemsCount = 0;
> > >   nsresult rv = selectedItems->GetLength(&selectedItemsCount);
> > >-  NS_ENSURE_SUCCESS(rv, rv);
> > >+  NS_ASSERTION(NS_SUCCEEDED(rv), "GetLength() Shouldn't fail!");
> > > 
> > >-  PRInt32 columnCount = 0;
> > >-  rv = GetColumnCount(&columnCount);
> > >-  NS_ENSURE_SUCCESS(rv, rv);
> > >+  PRUint32 colCount = ColCount();
> > >+  aCells->AppendElements(selectedItemsCount * colCount);
> > 
> > SetCapacity() please
> 
> This one was trouble also. SetCapacity() seems to set an internal table
> capacity (upper bounds limit?) that doesn't affect the Length() value of the
> array. Mochitests fail as a result.

yes, it only forces the buffer to be as big as you like you still have to actually add the elements.

> > > {
> > >+  PRUint32 colCount = ColCount();
> > >+  PRUint32 rowCount = RowCount();
> > > 
> > >+  for (PRUint32 rowIdx = 0; rowIdx < rowCount; rowIdx++)
> > >+    if (IsRowSelected(rowIdx))
> > >+      for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
> > >+        aCells->AppendElement(rowIdx * colCount + colIdx);
> > 
> > you can use either AppendElements() or SetCapacity() here too before going
> > through the loop and adding each thing individually
> 
> Here we don't know the final array size, so we're adding-as-we-go. 

for the outer loop yes, but for the inner loop you know how many things it will add.

> Again, trouble. btw, There is another similarly named method to SetCapacity,
> that is SetLength(). But it basically calls AppendElements() to do the work.
> 
> So afaict, for arrays where we know the final size, we can do either
> inter-changeably:
> 
> array.AppendElements(length);
> for (i=0; i<length; i++)
>    array.ElementAt(i)=value;
> 
> -or-
> 
> for (i=0; i<length; i++)
>    array.AppendElement(value);

or you can do
array.SetCapacity(x);
for (i = 0; i < x; i++)
  array.Append(i);

which is slightly uglier I admitt, but avoids multiple reallocs / copies.
Attached file Unresolved Link Errors (obsolete) —
FYI, WIN7, VC++ 2010 Express, produces the attached when trying to drop the default {} on the function declarations ...
Attached patch Patch (v4)Splinter Review
Tweaked a couple times, I think I got everything ...
Attachment #635249 - Attachment is obsolete: true
Attachment #635576 - Attachment is obsolete: true
Attachment #635249 - Flags: review?(trev.saunders)
Attachment #635649 - Flags: review?(trev.saunders)
Comment on attachment 635649 [details] [diff] [review]
Patch (v4)

>+  nsTArray<bool> isColSelArray(colCount);
>+  isColSelArray.AppendElements(colCount);
>+  for (PRUint32 i = 0; i < colCount; i++)
>+    isColSelArray[i] = true;

nit, use memset() here, I'd also say SetLength() makes more sense than AppendElements()

>+ARIAGridAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> {
>+  PRUint32 rowCount = RowCount();
>+  PRUint32 colCount = ColCount();

nit, same line

>+ARIAGridAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
> {
>-  NS_ENSURE_ARG_POINTER(aColumns);
>+  PRUint32 colCount = ColCount();
>+  if (!colCount)
>+    return;
> 
>-  return GetSelectedColumnsArray(aColumnCount, aColumns);
>+  AccIterator rowIter(this, filters::GetRow);
>+  Accessible* row = rowIter.Next();
>+  if (!row)
>+    return;
>+
>+  nsTArray<BYTE> isColSelArray(colCount);
>+  memset(isColSelArray.Elements(), true, colCount);

nit, bool not byte, and you should SetLength() to avoid assertions.

>+HTMLTableAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> {
>-  NS_ENSURE_ARG_POINTER(aNumCells);
>+  PRUint32 rowCount = RowCount();
>+  PRUint32 colCount = ColCount();

nit, same line

>+#define XPC_TABLE_DEFAULT_SIZE 40

there's no reason for this to be public.  either make it a static const member of the class, or move it to xpcAccessibleTable.cpp and make it a static constant.

>+XULListboxAccessible::SelectedCellIndices(nsTArray<PRUint32>* aCells)
> {
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  PRUint32 colCount = ColCount();
>+  aCells->SetCapacity(selectedItemsCount * colCount);
>+  aCells->AppendElements(selectedItemsCount * colCount);

nit, since you've forced the buffer to be large enough there's no point in this AppendElements() you could just append them as you go.

>+      if (itemIdx < 0)
>+        for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++, cellsIdx++)
>+          aCells->ElementAt(cellsIdx) = itemIdx * colCount + colIdx;

I'm pretty sure you want that test the other way around

>+  aRows->SetCapacity(rowCount);
>+  aRows->AppendElements(rowCount);

nit, you can just append on at a time after the capacity is set here too.

>+  for (PRUint32 rowIdx = 0; rowIdx < rowCount; rowIdx++) {

nit, these numbers make me think they refer to the total number of rows / index in all rows not just the set of selected ones.

>+      if (itemIdx < 0)
>+        aRows->ElementAt(rowIdx) = itemIdx;

pretty sure you want that the other way around too.

>+XULTreeGridAccessible::SelectedColIndices(nsTArray<PRUint32>* aCols)
> {
>+  if (RowCount() == SelectedRowCount()) {
>+    PRUint32 colCount = ColCount();
>+    aCols->SetCapacity(colCount);
>+    for (PRUint32 colIdx = 0; colIdx < colCount; colIdx++)
>+      aCols->AppendElement(colIdx);
>+  }

nit, I think I'd prefer early return here if none are selected.
Attachment #635649 - Flags: review?(trev.saunders) → review+
Final version for landing.
https://hg.mozilla.org/mozilla-central/rev/9e10ec9462e2
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.