[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





--- Comment #9 from David Grogan <dgrogan at chromium.org>  2011-04-04 18:30:14 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.

The idea was

What: Move event queue from Document to ScriptExecutionContext
Why: so that it can be accessed from workers

Though I now added that _IndexedDB_ needs to access it from workers.

>> Source/WebCore/ChangeLog:8
>> +        Change EventQueue to lazily construct m_eventQueueTimer.
> 
> This looks like it should be a function level comment.

Moved.

>> 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.

Yeah, that's awkward, I didn't realize those weren't applicable to workers though it seems obvious in hindsight.  I'll either move the scroll stuff to Document or, more likely, to a subclass of EventQueue that is instantiated in Document.  Or any other suggestion you have.

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

Didn't realize.

>> 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.

Moved.

>> Source/WebCore/dom/EventQueue.cpp:93
>> +    if (m_queuedEvents.isEmpty() && m_pendingEventTimer)
> 
> How can you have an event in the queue but no timer?

You can't.  This function could be called before any events are enqueued though.

>> Source/WebCore/dom/EventQueue.cpp:100
>> +    ASSERT(m_pendingEventTimer);
> 
> Why was this assert added? (Good to comment in ChangeLog about this.)

Because I think this function can't be called when m_pendingEventTimer is null.  Such an occurrence indicates a logic failure that I want to know about.  Granted, the next ASSERT would fail but the message produced by this one would be clearer.

>> Source/WebCore/dom/ScriptExecutionContext.h:59
>> +    class EventQueue;
> 
> This is out of order.

fixed

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