[webkit-reviews] review granted: [Bug 46015] Implement shadow DOM-aware event targeting and introduce EventContext to track the context of each event dispatch. : [Attachment 69665] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 46015: Implement shadow DOM-aware event targeting and introduce
EventContext to track the context of each event dispatch.
https://bugs.webkit.org/show_bug.cgi?id=46015

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=69665&action=review

Design looks good. Are you sure you have enough test coverage?

> WebCore/dom/EventContext.cpp:52
> +Node* EventContext::node() const
> +{
> +    return m_node.get();
> +}
> +
> +EventTarget* EventContext::target() const
> +{
> +    return m_target.get();
> +}

I’d like to see these inlined.

> WebCore/dom/EventContext.cpp:61
> +WindowEventContext::WindowEventContext(Event* event, Node* node, const
Vector<EventContext>& ancestors)

This class should go into a separate source file.

It’s a little strange to pass in a vector simply so we can extract the last
element of it.

> WebCore/dom/EventContext.cpp:89
> +DOMWindow* WindowEventContext::window() const
> +{
> +    return m_window.get();
> +}
> +
> +EventTarget* WindowEventContext::target() const
> +{
> +    return m_target.get();
> +}

I’d like to see these inlined.

> WebCore/dom/EventContext.h:35
> +class ContainerNode;

Not used in this header.

> WebCore/dom/Node.cpp:2553
> +    bool skip = false;
> +    do {

I think that shouldSkipNextXXX (whatever XXX is) is a better boolean local
variable name than “skip”. Since skip is a verb it’s hard to know what it
means.

do while (true) is not the usual idiom for an infinite loop. I’d prefer that
you use while (true), which is the more usual one in this project at least.