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

[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 72360: Patch for landing for realz.
https://bugs.webkit.org/attachment.cgi?id=72360&action=review

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

> LayoutTests/ChangeLog:8
> +	   Tuned the test to better reflect its point: the even should indeed
fire (it used to be swallowed),

even -> event

> LayoutTests/ChangeLog:9
> +	   but it's target should be a non-shadow node.

it's -> its

> LayoutTests/fast/events/shadow-boundary-crossing.html:9
> +	       success = event.target.parentNode;

If the aim here is to check that this is a non-shadow node, maybe we should
check for the specific correct node, instead of allowing any node with a
parent.

> WebCore/dom/EventContext.h:40
> +    EventContext(Node*, EventTarget* currentTarget, EventTarget* target);

Our usual style is to use PassRefPtr for arguments when the object will take a
ref to them, even if the object is not the “sole owner”. I think you should
follow that here.

Can the node be a ContainerNode* instead of Node*? Do we construct these with
arbitrary nodes?

> WebCore/dom/Node.cpp:1445
> +ContainerNode* Node::parentOrHostNode()

Candidate for inlining?

> WebCore/dom/Node.cpp:2516
> -void Node::eventAncestors(Vector<RefPtr<ContainerNode> > &ancestors)
> +void Node::eventAncestors(Vector<EventContext> &ancestors, EventTarget*
originalTarget)

Misplaced "&" here. It's supposed to be next to the type, not the argument
name.

Functions like this one, that use an out argument, should typically have “get”
in their name. Not sure why this didn’t have that already.

> WebCore/dom/Node.cpp:2544
> +    };

Stray semicolon here. It should be removed.

> WebCore/dom/Node.cpp:2575
> +    RefPtr<EventTarget> originalTarget(event->target());

I prefer the "=" style syntax to the constructor style in cases like this. What
do you think?

> WebCore/dom/Node.cpp:2618
> +	       goto doneDispatching;

This could just be a “break” and probably should be. The reason we use goto
above is that it’s nested inside two loops.

> WebCore/dom/Node.h:216
>      // FIXME: Should be named getEventAncestors.
> -    void eventAncestors(Vector<RefPtr<ContainerNode> > &ancestors);
> +    void eventAncestors(Vector<EventContext> &ancestors, EventTarget*);

As I mentioned above, misplaced & here, and the name should be changed as the
FIXME says.

> WebCore/dom/WindowEventContext.cpp:50
> +	   topLevelContainer = topEventContext->node();
> +	   targetForWindowEvents = topEventContext->target();
> +    }

This might read better like this:

    EventTarget* targetForWindowEvents = topEventContext ?
topEventContext->node() : node;
    Node* topLevelContainer = topEventContext ? topEventContext->target() :
node;

Not sure. I suppose it depends on whether you are comfortable with ?: or not.

> WebCore/dom/WindowEventContext.h:43
> +    WindowEventContext(Event*, Node*, const EventContext*);

Same comment as above about PassRefPtr.