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

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

Just a few last comments.

> Source/WebCore/dom/EventQueue.cpp:86
> +	   m_isCancelled = true;

You could release m_event here.

> Source/WebCore/dom/EventQueue.cpp:113
> +    m_eventTaskMap.add(event, task.get());

Ideally event.release() (since you don't need event anymore).

> Source/WebCore/dom/EventQueue.cpp:138
> +    if (task) {

Personally, I'd find

  if (!task)
     return false;
  task->cancel();
  removeEvent(event);
  return true;

to be more readable.

> Source/WebCore/dom/EventQueue.h:47
>  

If we get rid of the RefCounted nature of EventQueue, it makes it a lot clearer
that there aren't lifetime issues.

I think the reason for making it refcounted (which you pointed out was
https://bugs.webkit.org/show_bug.cgi?id=55512) is now gone.

> Source/WebCore/dom/EventQueue.h:49
> +    virtual ~EventQueue();

Why did this become virtual? (I don't see something new being derived from
EventQueue.)