[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
bugzilla-daemon at webkit.org
Thu Apr 28 16:41:49 PDT 2011
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?
More information about the webkit-reviews
mailing list