[webkit-reviews] review denied: [Bug 57789] Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers : [Attachment 91416] Patch

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied David Grogan
<dgrogan at chromium.org>'s request for review:
Bug 57789: Move eventqueue from Document to ScriptExecutionContext so that it
can be accessed from workers
https://bugs.webkit.org/show_bug.cgi?id=57789

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

------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91416&action=review

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

If this is purely a refactoring with no visible/testable functional changes,
then please add a line in the ChangeLog saying that, instead of leaving this
in.

> Source/WebCore/dom/Document.cpp:1708
> +//	 m_eventQueue->cancelQueuedEvents();

Please don't leave in commented out code (assuming it's necessary to remove
this line - why?)

> Source/WebCore/dom/EventQueue.cpp:30
> +#include "CrossThreadTask.h"

What do you use from this #include?

> Source/WebCore/dom/EventQueue.cpp:67
> +    void dispatchEvent(ScriptExecutionContext*, PassRefPtr<Event> event)

Add an empty newline between methods.

> Source/WebCore/dom/EventQueue.cpp:84
> +	 , m_scrollEvent(scrollEvent)

Do you actually need this, or can you just check if the type is scroll event?

> Source/WebCore/dom/EventQueue.cpp:102
> +    m_scriptExecutionContext->postTask(EventDispatcherTask::create(event,
isScrollEvent, this));

Does this have the same semantics as DOMTimer? Enqueued events in a document
should not fire while modal dialogs are up (which DOMTimer gets as a side
effect of being an ActiveDOMObject).
fast/events/scroll-event-during-modal-dialog.html is a layout test that checks
for this behavior.

> Source/WebCore/dom/EventQueue.h:55
> +    void removeScrollEvent(PassRefPtr<Event>);

This should be private.

> Source/WebCore/dom/EventQueue.h:68
> +    friend class EventQueueTimer;

This is no longer necessary, you're removed EventQueueTimer.

> Source/WebCore/dom/EventQueue.h:69
> +    class EventDispatcherTask;

Why is this forward declaration necessary?