[webkit-reviews] review denied: [Bug 60790] Make EventQueue spawn a Task for each asynchronous event : [Attachment 93497] 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 60790: Make EventQueue spawn a Task for each asynchronous event
https://bugs.webkit.org/show_bug.cgi?id=60790

Attachment 93497: Patch
https://bugs.webkit.org/attachment.cgi?id=93497&action=review

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

> Source/WebCore/dom/EventQueue.cpp:51
> +    : m_scriptExecutionContext(context)

How do you know that EventQueue won't outlive context?

> Source/WebCore/dom/EventQueue.cpp:90
> +	 : m_event(event)

4 space indent

> Source/WebCore/dom/EventQueue.cpp:94
> +    }

Add blank line.

> Source/WebCore/dom/EventQueue.cpp:96
> +    EventQueue* m_eventQueue;

Should this be a ref ptr? How do you know that m_eventQueue will still be alive
when this runs?

> Source/WebCore/dom/EventQueue.cpp:108
> +	   m_eventTaskMap.remove(event);

You should be able to call remove alone and not even do contains.

> Source/WebCore/dom/EventQueue.cpp:139
> +    bool found = m_eventTaskMap.contains(event);

Why not just use get directly and compare the result to 0?

Or you could use "take" and then just avoid calling "removeEvent" when the
event is cancelled.

> Source/WebCore/dom/EventQueue.cpp:140
> +    if (found) {

Perfer return early.

If (!found)
    return false;

> Source/WebCore/dom/EventQueue.cpp:149
> +    m_eventTaskMap.clear();

Don't you need to iterate through this map and call cancel on each task?