Closed Bug 570532 Opened 10 years ago Closed 10 years ago

make events coalescence smart

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

()

Details

(Keywords: access)

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Attachment #449679 - Flags: review?(marco.zehe)
Attachment #449679 - Flags: review?(bolterbugz)
Comment on attachment 449679 [details] [diff] [review]
patch

I wonder if mFlushingEventsCount is the way to go or if we should have two queues.
(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 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.
Attachment #449679 - Flags: review?(marco.zehe) → review+
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 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)
(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?
(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 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.
(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 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).
(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?
(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?
(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!
(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 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.
Attachment #449679 - Flags: review?(bolterbugz) → review+
(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.
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/54e4b09275c6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Do we know why bz's testcase (the table one) bites this change?
(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.
You need to log in before you can comment on or make changes to this bug.