should save us a number of Qis etc.

This makes sense however I'd say we need to rely on accessible tree instead. For that we need something like content lists that gets updated when tree mutates. We could introduce this feature for AccCollector. That doesn't mean we shouldn't fix this bug though.

Comment on attachment 597326 [details] [diff] [review] Patch > #include "nsAccUtils.h" >+#include "nsContentList.h" keep it with other non-a11y headers please. > nsHTMLRadioButtonAccessible::GetPositionAndSizeInternal(PRInt32 *aPosInSet, > PRInt32 *aSetSize) > { >- nsAutoString nsURI; >- mContent->NodeInfo()->GetNamespaceURI(nsURI); > nsAutoString tagName; > mContent->NodeInfo()->GetName(tagName); >+ PRInt32 namespaceId; I don't think you ever actually set that >+ mContent->NodeInfo()->NamespaceID(); you ignore the return value of that > > nsAutoString type; > mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type); > nsAutoString name; > mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name); I think we don't use these at all anymore > PRUint32 inputsCount = 0; >- inputs->GetLength(&inputsCount); >+ inputsCount = inputs->Length(true); make it one statement > > // Compute posinset and setsize. > PRInt32 indexOf = 0; > PRInt32 count = 0; > > for (PRUint32 index = 0; index < inputsCount; index++) { >- nsCOMPtr<nsIDOMNode> itemNode; >- inputs->Item(index, getter_AddRefs(itemNode)); >- >- nsCOMPtr<nsIContent> item(do_QueryInterface(itemNode)); >+ nsCOMPtr<nsIContent> item; >+ inputs->Item(index, item); shouldn't that be nsIContent* input = inputs->Item()?

> > nsAutoString type; > > mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::type, type); > > nsAutoString name; > > mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name); > > I think we don't use these at all anymore We using it inside loop.

Comment on attachment 598267 [details] [diff] [review] Patch Review of attachment 598267 [details] [diff] [review]: ----------------------------------------------------------------- f=me ::: accessible/src/html/nsHTMLFormControlAccessible.cpp @@ +188,2 @@ > nsAutoString tagName; > mContent->NodeInfo()->GetName(tagName); maybe we don't care but can't tagname contain a namespace prefix, in case of xhtml? @@ +188,4 @@ > nsAutoString tagName; > mContent->NodeInfo()->GetName(tagName); > + PRInt32 namespaceId; > + namespaceId = mContent->NodeInfo()->NamespaceID(); define and assign value

Comment on attachment 598787 [details] [diff] [review] Patch Review of attachment 598787 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/html/nsHTMLFormControlAccessible.cpp @@ +203,3 @@ > } else { > nsIDocument* doc = mContent->OwnerDoc(); > + inputs = NS_GetContentList(doc, namespaceId, tagName); you can use NS_GetFuncStringContentList to avoid nodes traversal below and use indexOf and count on the list

btw, I think you need to keep the content list alive (put it as member) that guarantees us it will be reused for any radio button of the same group once you queried it. It's live content list so it's kept updated when tree mutates. I just think that whole document traversal for each radio button must be highly expensive.

(In reply to alexander :surkov from comment #9) > btw, I think you need to keep the content list alive (put it as member) that > guarantees us it will be reused for any radio button of the same group once > you queried it. It's live content list so it's kept updated when tree > mutates. I just think that whole document traversal for each radio button > must be highly expensive. yeah, that seems like a good idea, but I think that was already an issue right?

(In reply to Trevor Saunders (:tbsaunde) from comment #10) > (In reply to alexander :surkov from comment #9) > yeah, that seems like a good idea, but I think that was already an issue > right? yes, are you thinking to go without fixing comment #8 and comment #9?

(In reply to alexander :surkov from comment #11) > (In reply to Trevor Saunders (:tbsaunde) from comment #10) > > (In reply to alexander :surkov from comment #9) > > > yeah, that seems like a good idea, but I think that was already an issue > > right? > > yes, are you thinking to go without fixing comment #8 and comment #9? well, I'm not sure about fixing #8, but yes I think we can leave 9 for another bug.

Comment on attachment 598787 [details] [diff] [review] Patch this is an improvement s it is, and I want it out of my queue, so f=me

1) the patch is updated to trunk 2) some variables renaming 3) changed to not flush the content (aDoFlash=false arg in Length and Item methods). If content is not flushed then accessible tree is not updated and flushing the content doesn't necessary update the accessible tree. So flushing is not useful here but makes us slower.

bug 759138 is filed for comment 8 and comment 9.