Last Comment Bug 726283 - consider using NS_GetContentList() directly in nsHTMLRadioButtonAccessible::GetPositionAndSizeInternal()
: consider using NS_GetContentList() directly in nsHTMLRadioButtonAccessible::G...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jignesh Kakadiya [:jhk]
:
Mentors:
Depends on:
Blocks: cleana11y a11yperf
  Show dependency treegraph
 
Reported: 2012-02-11 02:24 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-05-31 06:18 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.92 KB, patch)
2012-02-15 00:02 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (3.75 KB, patch)
2012-02-17 09:50 PST, Jignesh Kakadiya [:jhk]
surkov.alexander: feedback+
Details | Diff | Splinter Review
Patch (3.73 KB, patch)
2012-02-20 00:21 PST, Jignesh Kakadiya [:jhk]
surkov.alexander: review+
tbsaunde+mozbugs: feedback+
Details | Diff | Splinter Review
patch to land (4.06 KB, patch)
2012-05-28 09:11 PDT, alexander :surkov
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-02-11 02:24:36 PST
should save us a number of Qis etc.
Comment 1 alexander :surkov 2012-02-12 19:15:23 PST
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 2 Jignesh Kakadiya [:jhk] 2012-02-15 00:02:31 PST
Created attachment 597326 [details] [diff] [review]
Patch
Comment 3 Trevor Saunders (:tbsaunde) 2012-02-17 07:37:47 PST
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()?
Comment 4 Jignesh Kakadiya [:jhk] 2012-02-17 08:09:47 PST
> >   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 5 Jignesh Kakadiya [:jhk] 2012-02-17 09:50:26 PST
Created attachment 598267 [details] [diff] [review]
Patch
Comment 6 alexander :surkov 2012-02-19 08:39:46 PST
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 7 Jignesh Kakadiya [:jhk] 2012-02-20 00:21:13 PST
Created attachment 598787 [details] [diff] [review]
Patch
Comment 8 alexander :surkov 2012-02-20 08:52:01 PST
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
Comment 9 alexander :surkov 2012-02-21 00:52:35 PST
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.
Comment 10 Trevor Saunders (:tbsaunde) 2012-03-30 21:58:13 PDT
(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?
Comment 11 alexander :surkov 2012-04-01 21:22:58 PDT
(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?
Comment 12 Trevor Saunders (:tbsaunde) 2012-04-02 02:37:23 PDT
(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 13 Trevor Saunders (:tbsaunde) 2012-05-24 09:48:12 PDT
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
Comment 14 alexander :surkov 2012-05-28 09:11:08 PDT
Created attachment 627725 [details] [diff] [review]
patch to land

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.
Comment 15 alexander :surkov 2012-05-28 09:43:47 PDT
bug 759138 is filed for comment 8 and comment 9.
Comment 16 Ed Morley [:emorley] 2012-05-31 06:18:01 PDT
https://hg.mozilla.org/mozilla-central/rev/3e2885d26e09

Note You need to log in before you can comment on or make changes to this bug.