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

[bugzilla-daemon at webkit.org]

David Levin <levin 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 112384: Patch
https://bugs.webkit.org/attachment.cgi?id=112384&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112384&action=review


Please fix the builds that failed and address the few issues that I had.

Thanks!

> Source/WebCore/dom/Document.h:36
> +#include "DocumentEventQueue.h"

Won't a fwd decl of DocumentEventQueue suffice (just as EventQueue did before)?


> Source/WebCore/dom/DocumentEventQueue.h:57
> +    void close();

Add virtual

> Source/WebCore/workers/WorkerEventQueue.h:51
> +    void close();

add virtual on all of these

> Source/WebCore/workers/WorkerEventQueue.h:54
> +    virtual void removeEvent(Event*);

Why is this virtual? (Who overrides it?)

Similarly why do we have a protected section? Who derives from this class?

If that happens in a later patch, perhaps add the virtual and protected then
and for now leave this as private and the method as non-virtual.