[webkit-reviews] review denied: [Bug 45631] Scroll event should be fired asynchronously : [Attachment 73934] 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 73934: Patch
https://bugs.webkit.org/attachment.cgi?id=73934&action=review

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

> WebCore/dom/Document.h:1021
> +    void enqueueEvent(PassRefPtr<Event> event, PassRefPtr<Node>
eventTarget);

I suggest having the caller call setTarget on the event before calling
enqueueEvent. Then there would be no need to pass the target separately.

I don’t think it’s a good idea to have functions with two different purposes on
the same object. Having enqueueEvent in Node isn’t really all that helpful, and
creates ambiguity.

The argument name “event” should not be written here since the type already
makes it clear.

> WebCore/dom/Document.h:1023
> +    void enqueueEventIfNotAlreadyEnqueued(PassRefPtr<Event> event,
PassRefPtr<Node> eventTarget);

This long awkward name is not even accurate. This enqueues an event if another
of the same type targeted at the same target is not already enqueued. That’s
not the same as *this* event already being enqueued.

I don’t think that creating an event and then later discovering that it is
already enqueued is a suitable API. Instead, I suggest that the function pass
only the type of the event and the target and actual event creation be done
inside the function if it’s needed. The only tricky part is making sure we pass
the right values for canBubble and cancelable, but I think we can start with
those hardcoded at first, and add arguments if needed later.

And we can come up with a more accurate name. Maybe enqueueCoalescedEvent(const
String& type, PassRefPtr<EventTarget>).

> WebCore/dom/EventQueue.cpp:36
> +struct EnqueuedEvent : Noncopyable {

Why Noncopyable?

> WebCore/dom/EventQueue.cpp:38
> +    RefPtr<Event> m_event;
> +    RefPtr<Node> m_eventTarget;

Normally we do not use the "m_" prefix for the members of a struct.

> WebCore/dom/EventQueue.cpp:55
> +void EventQueue::enqueueEvent(PassRefPtr<Event> event, PassRefPtr<Node>
eventTarget)

The targets should be stored in the events, not separately. We don’t need the
EnqueuedEvent struct at all.

> WebCore/dom/EventQueue.cpp:96
> +    Node* eventTarget = event->m_eventTarget.get();
> +    if (!eventTarget->inDocument())
> +	   return;

What about if the entire document is no longer being displayed, say if you
navigated away from it or destroyed it. We need to test that case.

> WebCore/page/EventHandler.cpp:2760
> -	  
m_frame->document()->dispatchEvent(Event::create(eventNames().scrollEvent,
true, false));
> +	  
m_frame->document()->enqueueEventIfNotAlreadyEnqueued(Event::create(eventNames(
).scrollEvent, true, false));

I noticed that the multiple places emitting scrollEvent don’t agree on the
setting of canBubble. This call site passes true.

> WebCore/rendering/RenderLayer.cpp:1401
> -    if (view) {
> -	   if (FrameView* frameView = view->frameView())
> -	       frameView->scheduleEvent(Event::create(eventNames().scrollEvent,
false, false), renderer()->node());
> -    }
> +    if (view && view->frameView())
> +	  
renderer()->node()->enqueueEventIfNotAlreadyEnqueued(Event::create(eventNames()
.scrollEvent, false, false));

This call site passes false for canBubble.

It does not make sense to check view and view->frameView() for 0 any more here.
That was only done so we could call the scheduleEvent function.

The pauseScheduledEvents and resumeScheduledEvents function will no longer
affect the dispatching of this event. Why is that OK?

> WebCore/rendering/RenderListBox.cpp:543
> -	   node()->dispatchEvent(Event::create(eventNames().scrollEvent, false,
false));
> +	  
node()->enqueueEventIfNotAlreadyEnqueued(Event::create(eventNames().scrollEvent
, false, false));

This call site passes false for canBubble.