[webkit-reviews] review denied: [Bug 45631] Scroll event should be fired asynchronously : [Attachment 73948] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 45631: Scroll event should be fired asynchronously
https://bugs.webkit.org/show_bug.cgi?id=45631

Attachment 73948: Patch
https://bugs.webkit.org/attachment.cgi?id=73948&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73948&action=review

Better, but still needs some work.

> WebCore/dom/Document.cpp:403
> +    , m_eventQueue(new EventQueue)

This should have an adoptPtr.

> WebCore/dom/Document.cpp:3443
> +    event->setTarget(this);

It’s unclear that this function can only queue events that are intended for the
document. Is there some way to make this more clear? I worry that if you meant
to write document()->eventQueue()->enqueueEvent() but instead just wrote
document()->enqueueEvent() you’d end up incorrectly changing the event target
to the document.

> WebCore/dom/Document.cpp:3449
> +    m_eventQueue->enqueueScrollEvent(this, true);

In WebKit code we frown on boolean arguments that get boolean constants passed
to them. Either you can define the function so it doesn’t need the argument or
you can use an enum instead of true/false. There’s no way for someone reading
“true” here to know what the “true” means.

> WebCore/dom/EventQueue.cpp:33
> +#include "Node.h"

No need to include Node.h if we are also including Document.h.

> WebCore/dom/EventQueue.cpp:46
> +    m_enqueuedEvents.clear();

No reason to do this.

> WebCore/dom/EventQueue.cpp:63
> +    if (m_nodesWithEnqueuedScrollEvents.contains(target.get()))
> +	   return;
> +
> +    m_nodesWithEnqueuedScrollEvents.add(target.get());

This does two hash lookups, and you only need to do one. Like this:

    if (!m_nodesWithEnqueuedScrollEvents.add(target.get()).second)
	return;

That does the same thing with just one hash lookup.

> WebCore/dom/EventQueue.cpp:66
> +    enqueueEvent(scrollEvent);

Should be scrollEvent.release().

> WebCore/dom/EventQueue.cpp:77
> +    EventIterator end = enqueuedEvents.end();
> +    for (EventIterator it = enqueuedEvents.begin(); it != end; ++it)
> +	   dispatchEvent(*it);

There’s no need to use iterators to walk through a vector. A for look using
size_t and indices works just fine and reads more clearly. Iterators are fine
if it’s an abstract algorithm, but no need for an iterator just for a vector.

Instead of *it this would be better if it was it->release() or if you fix the
iteration enqueuedEvents[i].release().

> WebCore/dom/EventQueue.cpp:85
> +    if (eventNames().scrollEvent == event->type())
> +	   m_nodesWithEnqueuedScrollEvents.remove(eventTarget);

No need to check the type. We could just always call this remove function.

> WebCore/dom/EventQueue.cpp:91
> +    if (eventTarget->isDocumentNode())
> +	   eventTarget->document()->dispatchWindowEvent(event);

Why? Are all event targeted at the document window events? Can’t a document
ever get a non-window event?

> WebCore/dom/EventQueue.h:39
> +struct EnqueuedEvent;

This doesn’t belong here.

> WebCore/dom/EventQueue.h:47
> +    void enqueueScrollEvent(PassRefPtr<Node> target, bool canBubble);

Your patch always passes false for canBubble, so you should remove the argument
from this function. It’s always false. You also need a test case to show that
it’s correct to have the scroll event not bubble in the one case you’re
changing, the event queued in EventHandler::sendScrollEvent.

> WebCore/dom/EventQueue.h:54
> +    Vector<RefPtr<Event> > m_enqueuedEvents;

This should be named m_queuedEvents, not m_enqueuedEvents.

> WebCore/dom/EventQueue.h:55
> +    HashSet<Node*> m_nodesWithEnqueuedScrollEvents;

This should be named m_nodesWithQueuedScrollEvents, not
m_nodesWithEnqueuedScrollEvents.

> WebCore/dom/Node.h:520
> +    void enqueueScrollEvent();

Do we really need this helper? Can’t the client callers with the event queue
directly? I’d prefer not to touch the Node class at all; it’s not good to build
the scroll event in like this.