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

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied  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 88141: Fix typo
https://bugs.webkit.org/attachment.cgi?id=88141&action=review

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

> Source/WebCore/ChangeLog:5
> +	   Move event queue from Document to ScriptExecutionContext so that it
can be accessed from workers

This is what you are doing but a better comment answers why.

> Source/WebCore/ChangeLog:8
> +	   Change EventQueue to lazily construct m_eventQueueTimer.

This looks like it should be a function level comment.

> Source/WebCore/ChangeLog:14
> +	   No new tests, just a refactoring.

The refactoring seems odd given that EventQueue has all of this handling for
Node which aren't available in Workers.

The member variable m_nodesWithQueuedScrollEvents; and the method
enqueueScrollEvent are not applicable to Workers.

> Source/WebCore/ChangeLog:17
> +	   (WebCore::Document::Document):

function level comments are encouraged in WebKit.  See other well written
ChangeLog entries.

> Source/WebCore/dom/EventQueue.cpp:-23
> - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 

Unnecessary whitespace changes are discouraged.

> Source/WebCore/dom/EventQueue.cpp:-47
> -    EventQueue* m_eventQueue;    

Unnecessary whitespace changes are discouraged.

> Source/WebCore/dom/EventQueue.cpp:68
> +	   m_pendingEventTimer = adoptPtr(new EventQueueTimer(this,
m_scriptExecutionContext));

It looks like you are moving this from EventQueue, but calling adoptPtr
directly is a bad sign.

The constructor should be hidden and expose a static create method.

> Source/WebCore/dom/EventQueue.cpp:93
> +    if (m_queuedEvents.isEmpty() && m_pendingEventTimer)

How can you have an event in the queue but no timer?

> Source/WebCore/dom/EventQueue.cpp:100
> +    ASSERT(m_pendingEventTimer);

Why was this assert added? (Good to comment in ChangeLog about this.)

> Source/WebCore/dom/ScriptExecutionContext.h:59
> +    class EventQueue;

This is out of order.