530081 - Clean up our tree walker

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

We've abused our tree walker code a bit with our 192 crash fixing. This is a reminder to tidy it up.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 424725 [details] [diff] [review]
patch

>       // Gets the name from input accessibles
>       // Note: if input have label elements then the name isn't calculated
>       // from them.
>       testName("btn_labelledby_mixed_input",
>-               "input button Submit Query Reset input image");
>+               "input button Submit Query Reset Submit Query");

Where does this change come from exactly? is it a desired change? In other word, might the patch actually regress something, or is this a bug fix?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)

> >       testName("btn_labelledby_mixed_input",
> >-               "input button Submit Query Reset input image");
> >+               "input button Submit Query Reset Submit Query");
> 
> Where does this change come from exactly? is it a desired change? In other
> word, might the patch actually regress something, or is this a bug fix?

This is bug fix. Now the submit button (input@type="image") hasn't accessible name, however it has a "Submit Query" label. This patch fix its.

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 424725 [details] [diff] [review]
patch

Cool! r=me

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 424725 [details] [diff] [review]
patch

Nice! I'm very happy with this direction (along with the new API patch).

>-  nsAccessibleTreeWalker.cpp \
>+  nsAccTreeWalker.cpp \

Why the name change? I like *accessible* because it is easier to query our crash database for our issues ;)


>+  PRUint32 length = 0;
>+  if (mState->childList)
>+    mState->childList->GetLength(&length);
>+
>+  for (; mState->childIdx < static_cast<PRInt32>(length); mState->childIdx++) {

Is that cast really necessary?

>+    nsIContent* childNode = mState->childList->GetNodeAt(mState->childIdx);
>+
>+    PRBool isHidden = PR_FALSE;
>+    nsCOMPtr<nsIAccessible> accessible;
>+    nsCOMPtr<nsIDOMNode> childDOMNode(do_QueryInterface(childNode));
>+    GetAccService()->GetAccessible(childDOMNode, presShell, mWeakShell, nsnull,
>+                                   &isHidden, getter_AddRefs(accessible));
>+
>+    if (accessible)
>+      return accessible.forget();
>+
>+    // Walk down into subtree to find accessibles.
>+    if (!isHidden) {
>+      if (!PushState(childNode))
>+        break;

Note: with nested ifs I prefer explicit {} -- but do as you will :)

>+
>+      accessible = GetNextAccessibleInternal(PR_TRUE);
>+      if (accessible)
>+        return accessible.forget();

Especially here so that it is super clear what the } is for.

>     }

>--- a/accessible/src/base/nsAccessibleTreeWalker.h
>+++ b/accessible/src/base/nsAccTreeWalker.h

>   /**
>-   * Moves current state to point to the next child accessible.
>+   * Return the next child accessible.
>    */
>-  NS_IMETHOD GetNextSibling();
>+  already_AddRefed<nsIAccessible> GetNextAccessible()
>+  {
>+    return GetNextAccessibleInternal(PR_FALSE);
>+  }

I realize this naming is similar to what existed but I wonder if it is worth putting "child" in the name.

GetNextAccChild and GetNextAccChildAux?  (too long?)


Very nice to see test_gencontent.html.

Almost an r+ but I want to go over it again...

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 424725 [details] [diff] [review])
> Nice! I'm very happy with this direction (along with the new API patch).
> 
> >-  nsAccessibleTreeWalker.cpp \
> >+  nsAccTreeWalker.cpp \
> 
> Why the name change? I like *accessible* because it is easier to query our
> crash database for our issues ;)

Our util classes use "acc" prefix. Your querying approach shouldn't be changed because of nsAccUtils and etc.

> >+  for (; mState->childIdx < static_cast<PRInt32>(length); mState->childIdx++) {
> 
> Is that cast really necessary?

yes, a warning because of different types comparison.

> Note: with nested ifs I prefer explicit {} -- but do as you will :)

> Especially here so that it is super clear what the } is for.

I would say the code is shorter and nicer when single if isn't wrapped by braces. I would save this.

> I realize this naming is similar to what existed but I wonder if it is worth
> putting "child" in the name.
> 
> GetNextAccChild and GetNextAccChildAux?  (too long?)


I think 'acc' prefix is excess since it's inside the a11y class. Possibly GetNextChild() and GetNextChildInternal or Aux if you prefer? Aux is shorter but now we user prefixes 'Internal' in nsAccessible class and 'Impl' in nsCoreUtils. We should think carefully before we introduce the 'Aux' :)

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

OK yeah stick with internal for now. We can leave it or change them all later.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 424725 [details] [diff] [review] [details])
> > Nice! I'm very happy with this direction (along with the new API patch).
> > 
> > >-  nsAccessibleTreeWalker.cpp \
> > >+  nsAccTreeWalker.cpp \
> > 
> > Why the name change? I like *accessible* because it is easier to query our
> > crash database for our issues ;)
> 
> Our util classes use "acc" prefix. Your querying approach shouldn't be changed
> because of nsAccUtils and etc.

If we stick to acc for utils that works for me.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 424725 [details] [diff] [review]
patch

r=me

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

updated to trunk

Comment 11

[alexander :surkov (:asurkov)]

try server build - https://build.mozilla.org/tryserver-builds/[e-mail]-try-7f38346f3c47/

Comment 12

[Marco Zehe (:MarcoZ)]

I've been running with this try-server build all morning and couldn't find anything wrong with either NVDA or JAWS. From my perspective this looks fine.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: page.html

Comment 14

[alexander :surkov (:asurkov)]

Attachment: start.html

save page.htm and start.html locally into the same directory, load start.htm file and click start button to test performance

Comment 15

[alexander :surkov (:asurkov)]

I think we are in good shape.

Results on the trunk build:

1: 21437 ms
2: 14459 ms
3: 14616 ms
4: 14620 ms
5: 14240 ms
6: 14540 ms
7: 15614 ms
8: 16584 ms
9: 17415 ms
10: 20246 ms
average: 16377.1 ms

Results of the patched build:

1: 22956 ms
2: 14501 ms
3: 14528 ms
4: 14449 ms
5: 14300 ms
6: 14907 ms
7: 15606 ms
8: 15538 ms
9: 15914 ms
10: 16483 ms
average: 15918.2 ms

I think there is no performance win but there is no performance lost. However theoretically GetChildren() approach makes the DOM traversal slower than existing one since it creates additional content list but it doesn't affect on results. Probably because the patched nsAccTreeWalker is quicker.

Comment 16

[[e-mail]]

Comment on attachment 426522 [details] [diff] [review]
patch2

>+  WalkState(nsIContent *aNode) :
>+    node(aNode), childIdx(-1), prevState(nsnull) {}
> 
>+  nsCOMPtr<nsIContent> node;
Nit: might want to stick to content/aContent for nsIContent types throughout (I saw another one somewhere else in the patch, I forget where.)

I'm not sure why you wanted my sr, as I didn't spot anything unusual, but, just for the record, here's a possibility that you might want to consider.

>+  PRInt32 childIdx;
If you design the walker so that childIdx points at the next index then you can make it a PRUint32 and you don't have to cast the comparison. You would of course initialise it to 0 instead of -1.

>+  mState->childIdx++;
You then wouldn't increment it here.

>+  for (; mState->childIdx < static_cast<PRInt32>(length); mState->childIdx++) {
And you would turn this into a while loop.

>+    nsIContent* childNode = mState->childList->GetNodeAt(mState->childIdx);
And increment after here instead.

Comment 17

[alexander :surkov (:asurkov)]

Thank you. I'll fix these. Mainly the reason of sr was to ensure the algorithm of changed tree walker is correct and robust.

Comment 18

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/d804f02df155