[Webkit-unassigned] [Bug 57789] Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57789


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #88141|review+, commit-queue+      |review-
               Flag|                            |




--- Comment #8 from David Levin <levin at chromium.org>  2011-04-04 17:00:01 PST ---
(From update of attachment 88141)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.