807911 - whittle mutation events processing

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

it's not nice but it seems an improvement, we need a good code reorg for eventing

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch2

fix some evident oversights

Comment 2

[Trevor Saunders (:tbsaunde)]

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

>+AccReorderEvent::IsShowHideEventTarget(Accessible* aTarget) const

const Accessible* ?

>+  for (int32_t index = mDependentEvents.Length() - 1; index >= 0; index--) {

so, that doesn't work when you have more than 2^31 events which is a bit of an edge case, but why not do

for (uint32_t idx = count - 1; idx < count; idx--) ? (here and a couple places below)

>   enum EventGroup {

it'd be nice to unify these type things somehow...

>+  void ConnectTo(AccMutationEvent* aEvent)

ConnectTo() makes me think we make this event subordinate of argument, not  the reverse, how about AddSubMutationEvent() or something?

>+  nsTArray<nsRefPtr<AccMutationEvent> > mDependentEvents;

so, these evnets are all in the queue too right? so why do we need strong refs?  Shouldn't you add cycle collector goo if we need strong refs here?

>+    } break; // case eCoalesceReorder

ugh at the style of this switch.

nit, why not just get rid fo the braces for this case since you declare no locals?

>     case AccEvent::eCoalesceOfSameType:
>     {
>       // Coalesce old events by newer event.
>       for (int32_t index = tail - 1; index >= 0; index--) {
>         AccEvent* accEvent = mEvents[index];
>         if (accEvent->mEventType == tailEvent->mEventType &&
>+          accEvent->mEventRule == tailEvent->mEventRule) {
>+          accEvent->mEventRule = AccEvent::eDoNotEmit;
>+          return;
>+        }
>+      }
>+      for (int32_t index = tail - 1; index >= 0; index--) {
>+        AccEvent* accEvent = mEvents[index];
>+        if (accEvent->mEventType == tailEvent->mEventType &&
>             accEvent->mEventRule == tailEvent->mEventRule) {
>           accEvent->mEventRule = AccEvent::eDoNotEmit;
>           return;
>         }
>       }

what is this second loop for? it looks exactly the same as the one above, so I don't see why it isn't a really slow nop

>+NotificationController::CoalesceReorderEvents(AccEvent* aTailEvent)
> {
>+  for (int32_t index = mEvents.Length() - 2; index >= 0; index--) {

nit, s/index/idx/ ?
nit, for (uint32_t idx = length - 2; idx < length; idx--)

>+    // Skip event from different document since we don't coalesce them.
>+    if (thisEvent->mAccessible->Document() !=
>+        aTailEvent->mAccessible->Document())
>+      continue;

I shtis actually possible? if not why don't we just assert this never happens

>+    DocAccessible* document = thisEvent->mAccessible->Document();

nit, get rid of or move to first use?

>+        else if (eventType == nsIAccessibleEvent::EVENT_HIDE)
>+          NS_ERROR("Accessible tree was modified after it was removed! Huh?");

seems sort of dangerious to have macro like that, iirc some of those old macros are broken and don't end up being a statement in non debug builds.

>+NotificationController::ProcessEventQueue()
>+    AccEvent* event = events[idx];
>+    if (event->mEventRule != AccEvent::eDoNotEmit) {

nit, if == continue ?

>+        if (hyperText &&
>+          NS_SUCCEEDED(hyperText->GetCaretOffset(&caretOffset))) {

don't we have a decomified version of that?

>+          nsRefPtr<AccEvent> caretMoveEvent =
>+            new AccCaretMoveEvent(hyperText, caretOffset);
>+          nsEventShell::FireEvent(caretMoveEvent);
>+
>+          // There's a selection so fire selection change as well.
>+          int32_t selectionCount;
>+          hyperText->GetSelectionCount(&selectionCount);
>+          if (selectionCount)
>+            nsEventShell::FireEvent(nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED,
>+                                    hyperText);
>+        }
>+        continue;

this whole thing is rather ugly

DocAccessible::ProcessContentInserted(Accessible* aContainer,
>                                       const nsTArray<nsCOMPtr<nsIContent> >* aInsertedContent)
>+    if (presentContainer != aContainer)
>+      continue;

shouldn't you check presentContainer is not null too?

>+    if (containerNotUpdated) {
>+      containerNotUpdated = false;

why not just do this before the loop?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> >+  for (int32_t index = mDependentEvents.Length() - 1; index >= 0; index--) {
> 
> so, that doesn't work when you have more than 2^31 events which is a bit of
> an edge case, but why not do
> 
> for (uint32_t idx = count - 1; idx < count; idx--) ? (here and a couple
> places below)

It's tricky, I don't mind I guess but is it part of good coding style (for example are there mozilla precedents of its usage)?

Alternatively I could go with do/while or 64 bit int but not sure it's nicer.

> >   enum EventGroup {
> 
> it'd be nice to unify these type things somehow...

ideas?

> >+  void ConnectTo(AccMutationEvent* aEvent)
> 
> ConnectTo() makes me think we make this event subordinate of argument, not 
> the reverse, how about AddSubMutationEvent() or something?

ok, I don't mind. In future we might want to have something unified through different event classes.

> >+  nsTArray<nsRefPtr<AccMutationEvent> > mDependentEvents;
> 
> so, these evnets are all in the queue too right? so why do we need strong
> refs?  Shouldn't you add cycle collector goo if we need strong refs here?

it's safe to keep weak refs because we clean the queue after it was processed. I'll fix it.

> >+    } break; // case eCoalesceReorder
> 
> ugh at the style of this switch.

nothing to change, right?

> nit, why not just get rid fo the braces for this case since you declare no
> locals?

ok

> what is this second loop for? it looks exactly the same as the one above, so
> I don't see why it isn't a really slow nop

I guess artifacts :) Thanks for the catch.

> >+NotificationController::CoalesceReorderEvents(AccEvent* aTailEvent)
> > {
> >+  for (int32_t index = mEvents.Length() - 2; index >= 0; index--) {
> 
> nit, s/index/idx/ ?

we used to have 'index' in the whole file. I don't mind about 'index' replacement but perhaps we should do that throughoutly.

> >+    // Skip event from different document since we don't coalesce them.
> >+    if (thisEvent->mAccessible->Document() !=
> >+        aTailEvent->mAccessible->Document())
> >+      continue;
> 
> I shtis actually possible? if not why don't we just assert this never happens

It shouldn't be (just copy/paste old logic). I'll assert.

> >+    DocAccessible* document = thisEvent->mAccessible->Document();
> 
> nit, get rid of or move to first use?

ok, I'll use mDocument and will assert if queued event belongs to different document.

> >+        else if (eventType == nsIAccessibleEvent::EVENT_HIDE)
> >+          NS_ERROR("Accessible tree was modified after it was removed! Huh?");
> 
> seems sort of dangerious to have macro like that, iirc some of those old
> macros are broken and don't end up being a statement in non debug builds.

ok, which one should I use instead?

> >+NotificationController::ProcessEventQueue()
> >+    AccEvent* event = events[idx];
> >+    if (event->mEventRule != AccEvent::eDoNotEmit) {
> 
> nit, if == continue ?

no, we need to process hide event to destroy the tree (bug 759875)

> >+        if (hyperText &&
> >+          NS_SUCCEEDED(hyperText->GetCaretOffset(&caretOffset))) {
> 
> don't we have a decomified version of that?

funny but not yet.

> >+          // There's a selection so fire selection change as well.
> >+          int32_t selectionCount;
> >+          hyperText->GetSelectionCount(&selectionCount);
> >+          if (selectionCount)
> >+            nsEventShell::FireEvent(nsIAccessibleEvent::EVENT_TEXT_SELECTION_CHANGED,
> >+                                    hyperText);
> >+        }
> >+        continue;
> 
> this whole thing is rather ugly

heritage

> DocAccessible::ProcessContentInserted(Accessible* aContainer,
> >                                       const nsTArray<nsCOMPtr<nsIContent> >* aInsertedContent)
> >+    if (presentContainer != aContainer)
> >+      continue;
> 
> shouldn't you check presentContainer is not null too?

aContainer is never null

> >+    if (containerNotUpdated) {
> >+      containerNotUpdated = false;
> 
> why not just do this before the loop?

to filter out the case when nothing was changed, for example, node was inserted and then moved somewhere else.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch4

patch 3 was wrong

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch5

those document comparison stuff was about application accessible, so no AccReorderEvent for app accessible to avoid that.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > >+  for (int32_t index = mDependentEvents.Length() - 1; index >= 0; index--) {
> > 
> > so, that doesn't work when you have more than 2^31 events which is a bit of
> > an edge case, but why not do
> > 
> > for (uint32_t idx = count - 1; idx < count; idx--) ? (here and a couple
> > places below)
> 
> It's tricky, I don't mind I guess but is it part of good coding style (for
> example are there mozilla precedents of its usage)?
> 
> Alternatively I could go with do/while or 64 bit int but not sure it's nicer.

all I know of is the m.d.platform discussion a while back about signed vs unsigned, where that approach was brought up.

Comment 8

[alexander :surkov (:asurkov)]

Ok, I'll change it. Other findings?

Comment 9

[Trevor Saunders (:tbsaunde)]

> 
> > >+        else if (eventType == nsIAccessibleEvent::EVENT_HIDE)
> > >+          NS_ERROR("Accessible tree was modified after it was removed! Huh?");
> > 
> > seems sort of dangerious to have macro like that, iirc some of those old
> > macros are broken and don't end up being a statement in non debug builds.
> 
> ok, which one should I use instead?

looks its fine I think I was thinking of the MAI_foo macros

Comment 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 679995 [details] [diff] [review]
patch5

 NotificationController::QueueEvent(AccEvent* aEvent)
 {
+  NS_ASSERTION(aEvent->mAccessible && aEvent->mAccessible->IsApplication() ||
+               aEvent->GetDocAccessible() == mDocument,
+               "Queued event belongs to another document!");

&& binds tighter than || no, so that isn't really what you want is it?  I'd do

NS_ASSERT(aEvent->mAccessible, "event fired on not accessible node!");
NS_ASSERTION(aEvent->mAccessible->IsApplication() || aEvent->mAccessible->Document() == mDOcument, "event fired on wrong document!");

I guess that makes the first assert not really necessary, but maybe nice for understanding crashes faster.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> Comment on attachment 679995 [details] [diff] [review]
> patch5
> 
>  NotificationController::QueueEvent(AccEvent* aEvent)
>  {
> +  NS_ASSERTION(aEvent->mAccessible && aEvent->mAccessible->IsApplication()
> ||
> +               aEvent->GetDocAccessible() == mDocument,
> +               "Queued event belongs to another document!");
> 
> && binds tighter than || no, so that isn't really what you want is it?  I'd
> do
> 
> NS_ASSERT(aEvent->mAccessible, "event fired on not accessible node!");
> NS_ASSERTION(aEvent->mAccessible->IsApplication() ||
> aEvent->mAccessible->Document() == mDOcument, "event fired on wrong
> document!");

we have null mAccessible events (those having mNode initialized) that's why I did 

aEvent->mAccessible && aEvent->mAccessible->IsApplication() ||
aEvent->GetDocAccessible() == mDocument

i.e. if it's application accessible then don't fail since app acc doesn't have document and then (|| part) if event is fired for wrong document then assert.

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #11)
> (In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > Comment on attachment 679995 [details] [diff] [review]
> > patch5
> > 
> >  NotificationController::QueueEvent(AccEvent* aEvent)
> >  {
> > +  NS_ASSERTION(aEvent->mAccessible && aEvent->mAccessible->IsApplication()
> > ||
> > +               aEvent->GetDocAccessible() == mDocument,
> > +               "Queued event belongs to another document!");
> > 
> > && binds tighter than || no, so that isn't really what you want is it?  I'd
> > do
> > 
> > NS_ASSERT(aEvent->mAccessible, "event fired on not accessible node!");
> > NS_ASSERTION(aEvent->mAccessible->IsApplication() ||
> > aEvent->mAccessible->Document() == mDOcument, "event fired on wrong
> > document!");
> 
> we have null mAccessible events (those having mNode initialized) that's why
> I did 

hm, I thought AccEvent::GetAccessible() was called before this point to figure out what the accessible is, and we do need an accessible for the event at some point to be able to fire it.

> aEvent->mAccessible && aEvent->mAccessible->IsApplication() ||
> aEvent->GetDocAccessible() == mDocument
> 
> i.e. if it's application accessible then don't fail since app acc doesn't
> have document and then (|| part) if event is fired for wrong document then
> assert.

well, as it is that will crash when event has no accessible right? don't you want
aEvent->mAccessible && (aEvent->mAccessible->IsApplication() || aEvent->mAccessible->Document() == mDocument

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> well, as it is that will crash when event has no accessible right?

why it should crash?

if aEvent->mAccessible is null
then
aEvent->mAccessible && aEvent->mAccessible->IsApplication() is false and we check
aEvent->GetDocAccessible() == mDocument

AccEvent::GetDocAccessible() shouldn't crash

> don't you
> want
> aEvent->mAccessible && (aEvent->mAccessible->IsApplication() ||
> aEvent->mAccessible->Document() == mDocument

that will be false for all events having mNode initialized instead mAccessible, right?

Comment 14

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/integration/mozilla-inbound/rev/a2cdd1e64267

Trev, if we still have the issue with that assertion then I'll file a follow up on it. Please let me know.

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> 
> > well, as it is that will crash when event has no accessible right?
> 
> why it should crash?
> 
> if aEvent->mAccessible is null
> then
> aEvent->mAccessible && aEvent->mAccessible->IsApplication() is false and we
> check
> aEvent->GetDocAccessible() == mDocument
> 
> AccEvent::GetDocAccessible() shouldn't crash

yeah, seems I just can't read sorry!

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a2cdd1e64267