[webkit-reviews] review denied: [Bug 60790] Make EventQueue spawn a Task for each asynchronous event : [Attachment 94003] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 18 17:11:30 PDT 2011
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.)
More information about the webkit-reviews
mailing list