[webkit-reviews] review granted: [Bug 46015] Implement shadow DOM-aware event targeting and introduce EventContext to track the context of each event dispatch. : [Attachment 73137] 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 73137: Patch
https://bugs.webkit.org/attachment.cgi?id=73137&action=review

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

> WebCore/dom/Node.cpp:2535
>  #if ENABLE(SVG)
> -	       // Skip <use> shadow tree elements.
> -	       if (ancestor->isSVGElement() && ancestor->isShadowNode())
> -		   continue;
> +	   // Skip SVGShadowTreeRootElement.
> +	   shouldSkipNextAncestor = ancestor->isSVGElement() &&
ancestor->isShadowNode();
> +	   if (shouldSkipNextAncestor)
> +	       continue;
>  #endif

Idea probably for next iteration: If we put the isSVGElement && isShadowNode
check into an inline function we could get the #if out of this function, much
as you did with the eventTargetRespectingSVGTargetRules function.

> WebCore/dom/Node.cpp:2555
> +    return ancestors.isEmpty() ? 0 : &(ancestors.last());

You don’t need those parentheses. To me they make the code harder to read,
although others may not feel the same way.

> WebCore/dom/Node.h:214
> +    // Node's parent or shadow tree host.
> +    ContainerNode* parentOrHostNode();

I’m not sure this function needs “Node” in its name.

> WebCore/dom/WindowEventContext.cpp:41
> +    // We don't dispatch load events to the window. That quirk was
originally
> +    // added because Mozilla doesn't propagate load events to the window
object.

Should we say “This quirk” rather than “That quirk”?