570532 - make events coalescence smart

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-cd117348be1f/tryserver-win32/

Comment 3

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

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

I wonder if mFlushingEventsCount is the way to go or if we should have two queues.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (From update of attachment 449679 [details] [diff] [review])
> I wonder if mFlushingEventsCount is the way to go or if we should have two
> queues.

Two queues might be a bit quicker since we don't need to move elements while front elements are removed. But I don't think it helps a lot since new events can be appended because of AT actions only and I'm not sure whether they do this at all. Feel free to file bug blocking cleana11y bug.

Comment 5

[Marco Zehe (:MarcoZ)]

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

Based on my results from https://wiki.mozilla.org/Accessibility/EventCoalescing#Results_of_tests, I give this an r+. Note that I didn't look at the code for this patch.

Comment 6

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

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

accessible/src/base/nsCoreUtils.cpp
>-PRBool
>-nsCoreUtils::AreSiblings(nsINode *aNode1, nsINode *aNode2)

I would prefer you keep this method (suggest inline and define in header). I would hope the compiler does this optimization for us.

Comment 7

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

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

> void
> nsAccEventQueue::CoalesceEvents()
> {

>-        if (thisEvent->mEventRule == nsAccEvent::eAllowDupes ||
>-            thisEvent->mEventRule == nsAccEvent::eDoNotEmit)
>-          continue; //  Do not need to check
>+        // Skip event for application accessible since no coalescence for it
>+        // is supported. Ignore events unattached from DOM and events from
>+        // different documents since we can't coalesce them.
>+        if (!thisEvent->mNode || !thisEvent->mNode->IsInDoc() ||
>+            thisEvent->mNode->GetOwnerDoc() != tailEvent->mNode->GetOwnerDoc())
>+          continue;

Good.

> 
>-        if (thisEvent->mNode == tailEvent->mNode) {
>-          if (thisEvent->mEventType == nsIAccessibleEvent::EVENT_REORDER) {
>-            CoalesceReorderEventsFromSameSource(thisEvent, tailEvent);
>-            continue;
>+        // If event queue contains an event of the same type and having target
>+        // that is sibling of target of newly appended event then apply its
>+        // event rule to the newly appended event.
>+        if (thisEvent->mNode->GetNodeParent() ==
>+            tailEvent->mNode->GetNodeParent()) {
>+          tailEvent->mEventRule = thisEvent->mEventRule;
>+          return;
>+        }

Good, please access and store the tailEvent->mNode->GetNodeParent() one time? (Outside the loop)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 449679 [details] [diff] [review])
> accessible/src/base/nsCoreUtils.cpp
> >-PRBool
> >-nsCoreUtils::AreSiblings(nsINode *aNode1, nsINode *aNode2)
> 
> I would prefer you keep this method (suggest inline and define in header). I
> would hope the compiler does this optimization for us.

Really, why? We should be more friendly to content methods. It nearly doesn't make sense to keep a method if things that it performs are really simple and the method doesn't bring us up to new level of code organization.

For example, I think it's evident:
node->getParent() == node->getParent() means they are sibling
The same time it's not evident
role == ROLE_TEXT || role == STATIC_TEXT means this is text accessible

So that we don't need a util for the first case, but we would like to have it for the second case. Do you agree?

Comment 9

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

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 449679 [details] [diff] [review] [details])
> > accessible/src/base/nsCoreUtils.cpp
> > >-PRBool
> > >-nsCoreUtils::AreSiblings(nsINode *aNode1, nsINode *aNode2)
> > 
> > I would prefer you keep this method (suggest inline and define in header). I
> > would hope the compiler does this optimization for us.
> 
> Really, why? We should be more friendly to content methods. It nearly doesn't
> make sense to keep a method if things that it performs are really simple and
> the method doesn't bring us up to new level of code organization.
> 
> For example, I think it's evident:
> node->getParent() == node->getParent() means they are sibling
> The same time it's not evident
> role == ROLE_TEXT || role == STATIC_TEXT means this is text accessible
> 
> So that we don't need a util for the first case, but we would like to have it
> for the second case. Do you agree?

Generally I agree but if the method doesn't slow us down I like it for profiling reasons (the sibling method shows up in my profiling).

Comment 10

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

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

>+        // Specifies if this event target can be descendant of tail node.
>+        PRBool thisCanBeDescendantOfTail = PR_FALSE;

I'd like to know this case/checking is necessary empirically.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> Generally I agree but if the method doesn't slow us down I like it for
> profiling reasons (the sibling method shows up in my profiling).

It sounds we have different preferences for profiling :) More methods means less easy-to-read stack (it doesn't make sense to profile AreSiblings() separately).

Comment 12

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

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

>+        if (tailEvent->mEventType != nsIAccessibleEvent::EVENT_HIDE &&
>+            nsCoreUtils::IsAncestorOf(thisEvent->mNode, tailEvent->mNode)) {
> 
>+          if (thisEvent->mEventType == nsIAccessibleEvent::EVENT_REORDER) {
>+            CoalesceReorderEventsFromSameTree(thisEvent, tailEvent);
>+            if (tailEvent->mEventRule != nsAccEvent::eDoNotEmit)
>+              continue;
>+            return;
>+          }

I would find this easier to read with more {}'s (pretty please) (in other places too).

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #12)

> >+            if (tailEvent->mEventRule != nsAccEvent::eDoNotEmit)
> >+              continue;
> >+            return;
> >+          }
> 
> I would find this easier to read with more {}'s (pretty please) (in other
> places too).

We don't brace single line 'if' with single statement in a11y. Would it help for your reading if I add new line after continue?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> (From update of attachment 449679 [details] [diff] [review])
> >+        // Specifies if this event target can be descendant of tail node.
> >+        PRBool thisCanBeDescendantOfTail = PR_FALSE;
> 
> I'd like to know this case/checking is necessary empirically.

Originally it was introduced in bug 541474, Marco confirmed it improves things at 10% (bug 541474 comment #6). Is that ok for empirical testing?

Comment 15

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

(In reply to comment #13)
> (In reply to comment #12)
> 
> > >+            if (tailEvent->mEventRule != nsAccEvent::eDoNotEmit)
> > >+              continue;
> > >+            return;
> > >+          }
> > 
> > I would find this easier to read with more {}'s (pretty please) (in other
> > places too).
> 
> We don't brace single line 'if' with single statement in a11y.

Yeah, although I wish we did ;)

> Would it help
> for your reading if I add new line after continue?

Yes thanks.

(In reply to comment #14)
> (In reply to comment #10)
> > (From update of attachment 449679 [details] [diff] [review] [details])
> > >+        // Specifies if this event target can be descendant of tail node.
> > >+        PRBool thisCanBeDescendantOfTail = PR_FALSE;
> > 
> > I'd like to know this case/checking is necessary empirically.
> 
> Originally it was introduced in bug 541474, Marco confirmed it improves things
> at 10% (bug 541474 comment #6). Is that ok for empirical testing?

Yep. Thanks for checking!

Comment 16

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

(In reply to comment #11)
> (In reply to comment #9)
> 
> > Generally I agree but if the method doesn't slow us down I like it for
> > profiling reasons (the sibling method shows up in my profiling).
> 
> It sounds we have different preferences for profiling :) More methods means
> less easy-to-read stack (it doesn't make sense to profile AreSiblings()
> separately).

In can help optimization. If we know too much time is spent in AreSiblings for example.

Comment 17

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

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

Thanks for doing this work.

I am worried about the cases when we mark the newer event eDoNotEmit, instead of the older event, but I can see how it is more performant this way.

r=me if you are confident this won't mess up the overall event firing order. Also please fix any of my nits you agree with.

Note we need to get this a lot tighter but we certainly should take improvements in the meantime! I'm pretty sure we still need some config magic to solve this more drastically for the non-AT cases.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> (From update of attachment 449679 [details] [diff] [review])
> Thanks for doing this work.
> 
> I am worried about the cases when we mark the newer event eDoNotEmit, instead
> of the older event, but I can see how it is more performant this way.

I still think this is correct. We should preserve events order. In general if we get an event early and for some reasons we get similar event later then earlier one should be fired because events in the middle might assume this. At least let's keep this until we have a case showing this is wrong. 

> r=me if you are confident this won't mess up the overall event firing order.

I think they are in good shape. Let's catch regressions ;)

> Also please fix any of my nits you agree with.

I've fixed some styling, others I believe I commented.

> Note we need to get this a lot tighter but we certainly should take
> improvements in the meantime! I'm pretty sure we still need some config magic
> to solve this more drastically for the non-AT cases.

This is really nice to have. But it's hard to implement this to not touch AT. There should be lot of investigations here I think.

Comment 19

[alexander :surkov (:asurkov)]

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

Comment 20

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

Do we know why bz's testcase (the table one) bites this change?

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> Do we know why bz's testcase (the table one) bites this change?

I don't know. But I don't think it makes sense do any investigations immediately since it affects on Accessible Event Watcher tool only. It can be dealt with later.