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)
Core
Disability Access APIs
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)
38.76 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
38.87 KB,
patch
|
Details | Diff | Splinter Review |
Refer to: Bug 765371
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #634116 -
Flags: review?(trev.saunders)
Comment 2•12 years ago
|
||
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)
Assignee | ||
Comment 3•12 years ago
|
||
Changed over to nsTArrays .... PRUint's ...
Attachment #634116 -
Attachment is obsolete: true
Comment 4•12 years ago
|
||
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
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #634660 -
Attachment is obsolete: true
Attachment #635249 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 6•12 years ago
|
||
(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);
Comment 7•12 years ago
|
||
> > >+ 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.
Assignee | ||
Comment 8•12 years ago
|
||
FYI, WIN7, VC++ 2010 Express, produces the attached when trying to drop the default {} on the function declarations ...
Assignee | ||
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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+
Assignee | ||
Comment 11•12 years ago
|
||
Final version for landing.
Assignee | ||
Comment 12•12 years ago
|
||
push to TRY https://tbpl.mozilla.org/?tree=Try&rev=9c2604e5791e
Assignee | ||
Comment 13•12 years ago
|
||
Push to inbound: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3845dd694369
Comment 14•12 years ago
|
||
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.
Description
•