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

fix some evident oversights

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?

(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.

patch 3 was wrong

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

(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.

Ok, I'll change it. Other findings?

> > > >+ 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 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.

(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.

(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

(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?

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.

(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!