Trev, what was your suggestions?

I think the idea was to have event queue object that maintains queue of events, and takes care to fire them. Something like class EventQueue { public: void FirePendingEvents(); void EnqueueEvent(AccEvent* aEvent); private: /** * enqueues new reorder event if it doesn't need to be coalesced. */ void EnqueueReorderEvent(AccReorderEvent* aEvent); /** * Equeues new state change evnet or coalesces away old one. */ void EnqueueStateChangeEvent(AccStateChangeEvent* aEvent); ... for other types of events nsTArray<nsRefPtr<AccEvent> > mEvents; }; actually I wonder if it makes sense to make FireDelayedEvent() a template function so we know the type of the event at compile time, and don't need to dispatch based on it at runtime.

That's ok. My question was rather about EventQueue connection to other classes. I was suggesting NotificationController : public EventQueue that will allow to keep think close as we have now like 1) NotificationController::WillRefresh triggers events firing 2) NotificationController checks mEvents (it could be public method) to decide whether WillRefresh listener should be kept You had something else in your mind.

(In reply to Trevor Saunders (:tbsaunde) from comment #1) > actually I wonder if it makes sense to make FireDelayedEvent() a template > function so we know the type of the event at compile time, and don't need to > dispatch based on it at runtime. that might make sense

I don't see any cycle collection macros allowing inheritance for native classes. So if I use NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS in the base class then what should I use in derived class? Mike, do you have ideas or know anybody who could help with this?

(In reply to alexander :surkov from comment #4) > I don't see any cycle collection macros allowing inheritance for native > classes. > > So if I use NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS in the base class then > what should I use in derived class? > > Mike, do you have ideas or know anybody who could help with this? I'd say use NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS, and define UnlinkImpl and TraverseImpl properly. (that is, contrary to nsISupports, I don't think there are helpers to make life easier for you) Olli or Andrew would be able to tell for sure.

It doesn't really work right now, for reasons outlined in bug 802442.

(In reply to alexander :surkov from comment #4) > I don't see any cycle collection macros allowing inheritance for native > classes. > > So if I use NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS in the base class then > what should I use in derived class? yeah, the easier way to do this not do anything in the dirived classes, and have Traverse / Unlink downcast and then tell CC about the members of that class. So something like NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(AccEvent) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAcc) AccMutationEvent* mutationEvent = downCastAccEvent(tmp); if (mutationEvent) { NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mutationEvent->mParent); } AccVCChangeEvnet* vcChange = downCastAccEvent(tmp); if (vcChangeEvent) { NS_IMPL_CYCLE_COLLECTION_TRACERSE(vcChangeEvent); } ... NS_CYCLE_COLLECTION_TRAVERSE_END you might need to inline the traverse macros in the ifs I'm not absolutely sure without checking. all that said its probably not a whole lot harder to just not ref count these events in the first place.

(In reply to Trevor Saunders (:tbsaunde) from comment #7) > all that said its probably not a whole lot harder to just not ref count > these events in the first place. We have XPCOM wrappers over them. If XPCOM wrapper were independent then we could do that I think. In either way this bug is about NotificationController and EventQueue separation.

previous one was filed by mistake

updated to inbound

Comment on attachment 699519 [details] [diff] [review] patch3 >diff --git a/accessible/src/base/NotificationController.h b/accessible/src/base/EventQueue.h >copy from accessible/src/base/NotificationController.h >copy to accessible/src/base/EventQueue.h >--- a/accessible/src/base/NotificationController.h >+++ b/accessible/src/base/EventQueue.h >@@ -1,207 +1,51 @@ > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ nit, vim line too in .cpp as well. > #ifdef A11Y_LOG > #include "Logging.h" > #endif why is this needed? > /** >- * Notification interface. >+ * Used to organize and coalesce pending events. > */ >-class Notification >+class EventQueue > { >+ EventQueue(DocAccessible* aDocument) : mDocument(aDocument) { } >+ virtual ~EventQueue() { }; why do you need this? we always call delete on NotificationController and autogenerated dtor should be fine. >+ bool PushEvent(AccEvent* aEvent); I prefer QueueEvent() or EnqueueEvent() I'm also not convinced its useful for it to return bool since there's nothing the caller can do about the failure. >+ EventQueue(const EventQueue&); >+ EventQueue& operator = (const EventQueue&); MOZ_DELETE? > > /** > * The document accessible reference owning this queue. > */ > nsRefPtr<DocAccessible> mDocument; any reason we need a refptr here?

(In reply to Trevor Saunders (:tbsaunde) from comment #12) > >+class EventQueue > > { > >+ EventQueue(DocAccessible* aDocument) : mDocument(aDocument) { } > >+ virtual ~EventQueue() { }; > > why do you need this? we always call delete on NotificationController and > autogenerated dtor should be fine. I don't mind > >+ bool PushEvent(AccEvent* aEvent); > > I prefer QueueEvent() or EnqueueEvent() NotificationController has public QueueEvent. Having EnqueueEvent() wouldn't it be weird to have: void QueueEvent(AccEvent* aEvent) { if (PushEvent(aEvent)) ScheduleProcessing(); } > I'm also not convinced its useful > for it to return bool since there's nothing the caller can do about the > failure. depends on return value we schedule or do not a processing, I'd keep it as a boolean as long as it can be false > > /** > > * The document accessible reference owning this queue. > > */ > > nsRefPtr<DocAccessible> mDocument; > > any reason we need a refptr here? perhaps we have it because refreshobserver addrefs/releases it, when doc accessible goes away then it should shutdown notification controller so it shouldn't be necessary to have a strong pointer but it's safer to do it in separate bug.

> > > >+ bool PushEvent(AccEvent* aEvent); > > > > I prefer QueueEvent() or EnqueueEvent() > > NotificationController has public QueueEvent. Having EnqueueEvent() wouldn't > it be weird to have: oh, hm that's sort of anoying it would be nice to remove QueueEvent from NotificationController totally, but as keeping it as is fine for now. > > I'm also not convinced its useful > > for it to return bool since there's nothing the caller can do about the > > failure. > > depends on return value we schedule or do not a processing, I'd keep it as a > boolean as long as it can be false well, I'm pretty sure it actually can't because we don't use a fallible TArray > > > /** > > > * The document accessible reference owning this queue. > > > */ > > > nsRefPtr<DocAccessible> mDocument; > > > > any reason we need a refptr here? > > perhaps we have it because refreshobserver addrefs/releases it, when doc > accessible goes away then it should shutdown notification controller so it > shouldn't be necessary to have a strong pointer but it's safer to do it in > separate bug. sure

(In reply to Trevor Saunders (:tbsaunde) from comment #14) > oh, hm that's sort of anoying it would be nice to remove QueueEvent from > NotificationController totally, but as keeping it as is fine for now. ok, so I land it with your comments addressed expect QueueEvent methods stuff. > > depends on return value we schedule or do not a processing, I'd keep it as a > > boolean as long as it can be false > > well, I'm pretty sure it actually can't because we don't use a fallible > TArray it could be but as long as it can return false it's safer to have a check

(In reply to Trevor Saunders (:tbsaunde) from comment #14) > > > > nsRefPtr<DocAccessible> mDocument; > > > > > > any reason we need a refptr here? > > > > perhaps we have it because refreshobserver addrefs/releases it, when doc > > accessible goes away then it should shutdown notification controller so it > > shouldn't be necessary to have a strong pointer but it's safer to do it in > > separate bug. > > sure I filed bug 835752