Bug 78840 – Abstract ImageEventSender into a general purpose EventSender

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

We should abstract the functionality of ImageEventSender into a general purpose class, called EventSender. This general purpose class may be useful towards fixing bug #38995.

[Daniel Bates]

Created attachment 127438 [details]
Patch

The name "EventSender" is a bit vague. The purpose of this class is to queue up senders of a specific DOM event, say a Load event, and to call each sender back when they should dispatch such an event. Maybe EventSenderScheduler or EventSenderQueue would be a more descriptive name?

[Daniel Bates]

Comment on attachment 127438 [details]
Patch

Clearing review flag as I need to add EventSender.h to all build systems.

[Daniel Bates]

Created attachment 127464 [details]
Patch

[Adam Barth]

Comment on attachment 127464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127464&action=review

> Source/WebCore/dom/EventSender.h:88
> +    if (m_dispatchSoonList.isEmpty())
> +        m_timer.stop();

Do you mean to check whether the list is empty or whether it contains all zeros?  I'm not sure how this function could result in m_dispatchSoonList being empty if it wasn't already empty.

> Source/WebCore/loader/ImageLoader.cpp:-385
> -    if (m_dispatchSoonList.isEmpty())
> -        m_timer.stop();

I see this was here already.  We might want to ask the original author why they intended here.

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 127464 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127464&action=review
> 
> > Source/WebCore/dom/EventSender.h:88
> > +    if (m_dispatchSoonList.isEmpty())
> > +        m_timer.stop();
> 
> Do you mean to check whether the list is empty or whether it contains all zeros?  I'm not sure how this function could result in m_dispatchSoonList being empty if it wasn't already empty.

As far as I can tell this code isn't necessary anymore and is an artifact of using a QPtrList data structure for this functionality when it was originally in DocumentImpl::removeImage() (see <http://trac.webkit.org/browser/trunk/WebCore/khtml/xml/DocumentImpl.cpp?rev=12515#L2302>). In particular, DocumentImpl::removeImage() actually removed items from its QPtrList m_imageLoadEventDispatchSoonList (as opposed to zeroing out its items as we do today) ;=> m_imageLoadEventDispatchSoonList could become empty.

I'll remove this code before landing.

CC'ing Darin Adler since he may be interested to see this go by given that he kept this "if (m_dispatchSoonList.isEmpty())" code through both <http://trac.webkit.org/changeset/39296> (changed functionality to zero out list entries) and <http://trac.webkit.org/changeset/41766> (moved functionality out of Document.cpp).

[Daniel Bates]

Committed r108561: <http://trac.webkit.org/changeset/108561>

[Yong Li]

This EventSender should be SuspendableTimer or at least ActiveDOMObject?