This bug is closely related to the work made on: https://bugzilla.mozilla.org/show_bug.cgi?id=506961 https://bugzilla.mozilla.org/show_bug.cgi?id=448602 It would be great if nsIEventListenerService supports also registering callbacks that are called when a new event listener is added or an existing removed. This would allow to keep the event panel (displaying list of event listeners) up to date without necessity to entirely refresh the view (which can be time consuming). Honza

i'm pretty sure i wanted this while i was @cenzic.

bgrins, would this be good enough? https://treeherder.mozilla.org/#/jobs?repo=try&revision=a03d7bfc38d8

Comment on attachment 8597034 [details] [diff] [review] v1 Review of attachment 8597034 [details] [diff] [review]: ----------------------------------------------------------------- This works very well! I have put together a quick patch in Bug 1157469 using this API and have confirmed it works when adding and removing event listeners on elements in the inspector

Comment on attachment 8597034 [details] [diff] [review] v1 Hmm, if you think EventListenerService should work as singleton, you should: * hide its constructor (i.e., define it in private or protected) like the destructor * make nsLayoutModule.cpp use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR * drop NS_NewEventListenerService() in EventListenerService.cpp Additionally, don't we need to check script blocker? And how about if the instance is destroyed by shutting down XPCOM during the loop in NotifyPendingChanges()?

Why would we need to check script blockers? The API calls the callback async. >make nsLayoutModule.cpp use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR Well, then ctor needs to be public.

Comment on attachment 8597034 [details] [diff] [review] v1 I could use the macro, and not NS_NewEventListenerService(), but that is really not about this bug.

(In reply to Olli Pettay [:smaug] from comment #5) > Why would we need to check script blockers? The API calls the callback async. Ah, I got it. > >make nsLayoutModule.cpp use NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR > Well, then ctor needs to be public. I don't think so, only EventUtils::GetInstance() should be public. (In reply to Olli Pettay [:smaug] from comment #6) > I could use the macro, and not NS_NewEventListenerService(), but > that is really not about this bug. Okay, then, you shouldn't use MOZ_ASSERT() at checking the instance in the destructor.

Comment on attachment 8597034 [details] [diff] [review] v1 >+EventListenerService* >+EventListenerService::sInstance = nullptr; >+ >+EventListenerService::EventListenerService() >+{ >+ MOZ_ASSERT(!sInstance); >+ sInstance = this; >+} >+ >+EventListenerService::~EventListenerService() >+{ >+ MOZ_ASSERT(sInstance == this); So, please use NS_ASSERTION() until we make this class completely singleton. >+NS_IMETHODIMP >+EventListenerService::AddListenerChangeListener(nsIListenerChangeListener* aListener) >+{ >+ if (!mChangeListeners.Contains(aListener)) { >+ mChangeListeners.AppendElement(aListener); >+ } >+ return NS_OK; >+}; >+ >+ nit: why don't you insert just one empty line? >+void >+EventListenerService::NotifyAboutMainThreadListenerChangeInternal(dom::EventTarget* aTarget) >+{ >+ MOZ_ASSERT(NS_IsMainThread()); >+ if (!mChangeListeners.IsEmpty()) { if(mChangeListeners.IsEmpty()) { return; } can reduce the following indent. >+ if (!mPendingListenerChanges) { >+ mPendingListenerChanges = nsArrayBase::Create(); >+ nsCOMPtr<nsIRunnable> runnable = NS_NewRunnableMethod(this, >+ &EventListenerService::NotifyPendingChanges); >+ NS_DispatchToCurrentThread(runnable); >+ } >+ >+ if (!mPendingListenerChangesSet.Get(aTarget)) { >+ mPendingListenerChanges->AppendElement(aTarget, false); This looks a bit tricky... but it's okay because I have no idea to mPendingListenerChanges becomes nullptr again even if others will insert something between |mPendingListenerChanges = nsArrayBase::Create()| and this line. >+ mPendingListenerChangesSet.Put(aTarget, true); >+ } >+ } >+} >+ >+void >+EventListenerService::NotifyPendingChanges() >+{ >+ nsCOMPtr<nsIMutableArray> changes; >+ mPendingListenerChanges.swap(changes); >+ mPendingListenerChangesSet.Clear(); >+ >+ nsTObserverArray<nsCOMPtr<nsIListenerChangeListener>>::EndLimitedIterator >+ iter(mChangeListeners); >+ while (iter.HasMore()) { >+ nsCOMPtr<nsIListenerChangeListener> listener = iter.GetNext(); >+ listener->ListenersChanged(changes); Doesn't a call of this cause XPCOM shutdown? If so, you may need to quite from this loop, isn't it? >+ void addListenerChangeListener(in nsIListenerChangeListener aListener); >+ void removeListenerChangeListener(in nsIListenerChangeListener aListener); ListenerChangeListener sounds odd to me (I know it's not wrong name), but I have no better idea. If my some concerns are wrong or fixed before landing, r=masayuki And please file a follow up bug to make the class guaranteed as singleton class.

> >+EventListenerService::~EventListenerService() > >+{ > >+ MOZ_ASSERT(sInstance == this); > > So, please use NS_ASSERTION() until we make this class completely singleton. It is really MOZ_ASSERT style issue if someone else creates another instance > >+ while (iter.HasMore()) { > >+ nsCOMPtr<nsIListenerChangeListener> listener = iter.GetNext(); > >+ listener->ListenersChanged(changes); > > Doesn't a call of this cause XPCOM shutdown? If so, you may need to quite > from this loop, isn't it? Why? We don't have such checks for other callbacks. > ListenerChangeListener sounds odd to me Yeah, it is a bit odd, but couldn't figure out anything better

(In reply to Olli Pettay [:smaug] from comment #9) > > >+ while (iter.HasMore()) { > > >+ nsCOMPtr<nsIListenerChangeListener> listener = iter.GetNext(); > > >+ listener->ListenersChanged(changes); > > > > Doesn't a call of this cause XPCOM shutdown? If so, you may need to quite > > from this loop, isn't it? > Why? We don't have such checks for other callbacks. E.g., isn't it possible some listeners try to kill the process, like "Exit" menu? Then, the loop will keep working without crash? (Of course, it's not usual scenario, but I worry that if it causes a security hole.)

Why wouldn't it work like any other callback? I'm not worried about that case.

I worry about the others too... This sounds similar to checking if widget is destroyed before dispatching an event. But if you're sure it's safe different from widget case, my worry is just wrong.